hems/fnn: add failsafeConsumptionActivePowerLimit and failsafeDurationMinimum support

[thecem]

Fix #31926, refs #32132

This pull request adds a failsafe mode to the FNN dimming logic, ensuring that if a read error occurs on the W4 input, a configurable consumption limit is applied for a minimum duration. This improves system safety and resilience in case of sensor or communication failures. The changes include configuration support, core logic updates, and comprehensive tests for the new failsafe behavior.

Failsafe Mode Implementation

• Added new fields to the Fnn struct to track failsafe state, including failsafeConsumptionLimit, failsafeDurationMinimum, failsafeActive, and failsafeEnteredAt.
• Updated the runDim method to enter failsafe mode and apply the failsafe consumption limit on W4 read errors, and to remain in failsafe mode for at least the configured minimum duration.

Configuration and Initialization

• Extended NewFromConfig and NewFnn to accept and propagate the failsafe consumption limit and minimum duration parameters. [1] [2] [3]

Testing

• Added multiple tests in fnn_test.go to verify:
  • Failsafe configuration decoding from config maps.
  • Failsafe activation on W4 read errors.
  • Correct exit from failsafe after the minimum duration and a successful read.
  • Persistence of failsafe mode during the minimum duration, even if reads succeed.
  • Error propagation when failsafe is not configured. [1] [2]

Utilities

• Added a helper function errG for simulating a W4 read error in tests.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• Consider validating failsafeDurationMinimum (e.g., rejecting negative durations) at construction time, since a negative value will cause immediate failsafe exit and may hide misconfiguration.
• The tests rely on NewFnn synchronously calling runDim during construction (e.g., TestFailsafeActivatesOnReadError and TestFailsafeNotConfiguredPropagatesError); if this initialization behavior ever changes, those tests will silently stop exercising the failsafe path, so it may be worth making that dependency explicit or refactoring to invoke runDim directly in tests.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Consider validating `failsafeDurationMinimum` (e.g., rejecting negative durations) at construction time, since a negative value will cause immediate failsafe exit and may hide misconfiguration.
- The tests rely on `NewFnn` synchronously calling `runDim` during construction (e.g., `TestFailsafeActivatesOnReadError` and `TestFailsafeNotConfiguredPropagatesError`); if this initialization behavior ever changes, those tests will silently stop exercising the failsafe path, so it may be worth making that dependency explicit or refactoring to invoke `runDim` directly in tests.

## Individual Comments

### Comment 1
<location path="hems/fnn/fnn.go" line_range="202-211" />
<code_context>

 	active, err := c.w4()
 	if err != nil {
-		return err
+		if c.failsafeConsumptionLimit <= 0 {
+			return err
+		}
+		c.mu.Lock()
+		if !c.failsafeActive {
+			c.log.WARN.Println("w4 read error, entering failsafe mode")
+			c.failsafeActive = true
+			c.failsafeEnteredAt = time.Now()
+		}
+		c.mu.Unlock()
+		return c.setConsumptionLimit(c.failsafeConsumptionLimit)
+	}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** On w4 read error, consider logging the error details and ensuring setConsumptionLimit interactions are safe under concurrent use.

The WARN log in the error path omits `err`; including it would significantly improve troubleshootability. Also, `setConsumptionLimit` is invoked after `c.mu` is unlocked. Please confirm that `setConsumptionLimit` is safe to call concurrently and does not depend on the same invariants protected by `c.mu` (e.g., `failsafeActive`), or otherwise consider keeping the call under the lock or documenting the assumptions.

Suggested implementation:

```golang
	active, err := c.w4()
	if err != nil {
		if c.failsafeConsumptionLimit <= 0 {
			// No failsafe limit configured, surface the original read error.
			return err
		}

		c.mu.Lock()
		// Under lock, record entry into failsafe mode and apply the failsafe consumption limit.
		if !c.failsafeActive {
			c.log.WARN.Printf("w4 read error, entering failsafe mode: %v", err)
			c.failsafeActive = true
			c.failsafeEnteredAt = time.Now()
		}
		err = c.setConsumptionLimit(c.failsafeConsumptionLimit)
		c.mu.Unlock()

		return err
	}

```

1. Confirm that `setConsumptionLimit` does not itself acquire `c.mu`; if it does, adjust the locking strategy to avoid self-deadlock (e.g., keep `setConsumptionLimit` lock-free and rely on the caller's lock).
2. If `setConsumptionLimit` is intended to be callable concurrently without `c.mu`, instead leave the call outside the critical section and add a comment documenting that it is concurrency-safe and does not rely on `failsafeActive` / `failsafeEnteredAt` invariants.
3. Consider adding unit tests to simulate concurrent invocations of `runDim` with injected `w4` errors to verify that failsafe activation and `setConsumptionLimit` behavior remain consistent under concurrent use.
</issue_to_address>

### Comment 2
<location path="hems/fnn/fnn_test.go" line_range="135-137" />
<code_context>
+	assert.Equal(t, failsafeLimit, *fnn.MaxConsumptionPower())
+}
+
+// TestFailsafeNotConfiguredPropagatesError verifies that without a configured
+// failsafe limit, a W4 read error is returned to the caller unchanged.
+func TestFailsafeNotConfiguredPropagatesError(t *testing.T) {
+	require.NoError(t, db.NewInstance("sqlite", ":memory:"))
+
+	// no failsafe limit: construction error because runDim is called in NewFnn
+	_, err := NewFnn(&stubSite{}, 1e3, 0, nil, nil, nil, errG(), 0, 0, 0)
+	assert.Error(t, err)
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Test description suggests propagation from runDim, but the test only covers NewFnn failing

The comment for TestFailsafeNotConfiguredPropagatesError says the W4 read error is “returned to the caller unchanged”, which implies it is validating runDim’s error propagation. In reality, this test only checks that NewFnn fails during construction because its internal runDim call sees a W4 read error with no failsafe limit. Please either update the test name/comment to clearly state it covers construction-time behavior, or add coverage for a direct runDim call (if that path is used) that asserts the original error is propagated as described.

```suggestion
 // TestFailsafeNotConfiguredCausesNewFnnError verifies that without a
 // configured failsafe limit, NewFnn fails during construction because its
 // internal runDim call returns a W4 read error.
func TestFailsafeNotConfiguredCausesNewFnnError(t *testing.T) {
```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (bug_risk): On w4 read error, consider logging the error details and ensuring setConsumptionLimit interactions are safe under concurrent use.

The WARN log in the error path omits err; including it would significantly improve troubleshootability. Also, setConsumptionLimit is invoked after c.mu is unlocked. Please confirm that setConsumptionLimit is safe to call concurrently and does not depend on the same invariants protected by c.mu (e.g., failsafeActive), or otherwise consider keeping the call under the lock or documenting the assumptions.

Suggested implementation:

	active, err := c.w4()
	if err != nil {
		if c.failsafeConsumptionLimit <= 0 {
			// No failsafe limit configured, surface the original read error.
			return err
		}

		c.mu.Lock()
		// Under lock, record entry into failsafe mode and apply the failsafe consumption limit.
		if !c.failsafeActive {
			c.log.WARN.Printf("w4 read error, entering failsafe mode: %v", err)
			c.failsafeActive = true
			c.failsafeEnteredAt = time.Now()
		}
		err = c.setConsumptionLimit(c.failsafeConsumptionLimit)
		c.mu.Unlock()

		return err
	}

1. Confirm that setConsumptionLimit does not itself acquire c.mu; if it does, adjust the locking strategy to avoid self-deadlock (e.g., keep setConsumptionLimit lock-free and rely on the caller's lock).
2. If setConsumptionLimit is intended to be callable concurrently without c.mu, instead leave the call outside the critical section and add a comment documenting that it is concurrency-safe and does not rely on failsafeActive / failsafeEnteredAt invariants.
3. Consider adding unit tests to simulate concurrent invocations of runDim with injected w4 errors to verify that failsafe activation and setConsumptionLimit behavior remain consistent under concurrent use.

[sourcery-ai]

suggestion (testing): Test description suggests propagation from runDim, but the test only covers NewFnn failing

The comment for TestFailsafeNotConfiguredPropagatesError says the W4 read error is “returned to the caller unchanged”, which implies it is validating runDim’s error propagation. In reality, this test only checks that NewFnn fails during construction because its internal runDim call sees a W4 read error with no failsafe limit. Please either update the test name/comment to clearly state it covers construction-time behavior, or add coverage for a direct runDim call (if that path is used) that asserts the original error is propagated as described.

Suggested change

	// TestFailsafeNotConfiguredPropagatesError verifies that without a configured
	// failsafe limit, a W4 read error is returned to the caller unchanged.
	func TestFailsafeNotConfiguredPropagatesError(t *testing.T) {
	// TestFailsafeNotConfiguredCausesNewFnnError verifies that without a
	// configured failsafe limit, NewFnn fails during construction because its
	// internal runDim call returns a W4 read error.
	func TestFailsafeNotConfiguredCausesNewFnnError(t *testing.T) {

[andig]

If we do this it should cover relay (consumption) and production limits as well.

[andig]

@thecem please always mention the issue a pr fixes to enable Github auto-closing it

[thecem]

#31926

[andig]

I already did that on top. It has to be "Fix #31926" to work.

[andig]

If we want to align wording with EEBus we should probably also replace MaxCurtailPower with ProductionNominalMax, but that's a separate PR.

[thecem]

Will replace MaxCurtailPower with ProductionNominalMax in a next PR, when this is merged.

[andig]

See #31948

[andig]

Will replace MaxCurtailPower with ProductionNominalMax in a next PR, when this is merged.

Actually, lets leave as-is. I think it fulfills a different purpose.

[andig]

@copilot resolve the merge conflicts in this pull request

[andig]

@copilot resolve the merge conflicts in this pull request