Skip to content
Closed
79 changes: 58 additions & 21 deletions hems/fnn/fnn.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,16 @@
// NewFromConfig creates an FNN HEMS from generic config.
func NewFromConfig(ctx context.Context, other map[string]any, site site.API) (*Fnn, error) {
cc := struct {
MaxPower float64 // TODO deprecated
MaxDimPower float64
MaxCurtailPower float64
W3 *plugin.Config
S1 *plugin.Config
S2 *plugin.Config
W4 *plugin.Config
Interval time.Duration
MaxPower float64 // TODO deprecated
MaxDimPower float64
MaxCurtailPower float64
W3 *plugin.Config
S1 *plugin.Config
S2 *plugin.Config
W4 *plugin.Config
Interval time.Duration
FailsafeConsumptionActivePowerLimit float64
FailsafeDurationMinimum time.Duration
}{
Interval: 10 * time.Second,
}
Expand Down Expand Up @@ -69,25 +71,27 @@
maxCurtailPower = cc.MaxPower
}

return NewFnn(site, math.Abs(cc.MaxDimPower), maxCurtailPower, w3G, s1G, s2G, w4G, cc.Interval)
return NewFnn(site, math.Abs(cc.MaxDimPower), maxCurtailPower, w3G, s1G, s2G, w4G, cc.Interval, math.Abs(cc.FailsafeConsumptionActivePowerLimit), cc.FailsafeDurationMinimum)
}

func NewFnn(site site.API, maxDimPower, maxCurtailPower float64, w3G, s1G, s2G, w4G func() (bool, error), interval time.Duration) (*Fnn, error) {
func NewFnn(site site.API, maxDimPower, maxCurtailPower float64, w3G, s1G, s2G, w4G func() (bool, error), interval time.Duration, failsafeConsumptionLimit float64, failsafeDurationMinimum time.Duration) (*Fnn, error) {
if w4G != nil && maxDimPower == 0 {
return nil, errors.New("cannot have w4 without power limit")
}

c := &Fnn{
log: util.NewLogger("fnn"),
site: site,
maxDimPower: maxDimPower,
maxCurtailPower: maxCurtailPower,
s1: s1G,
s2: s2G,
w3: w3G,
w4: w4G,
productionPercent: 100,
interval: interval,
log: util.NewLogger("fnn"),

Check failure on line 83 in hems/fnn/fnn.go

View workflow job for this annotation

GitHub Actions / Lint

File is not properly formatted (gci)
site: site,
maxDimPower: maxDimPower,
maxCurtailPower: maxCurtailPower,
s1: s1G,
s2: s2G,
w3: w3G,
w4: w4G,
productionPercent: 100,
interval: interval,
failsafeConsumptionLimit: failsafeConsumptionLimit,
failsafeDurationMinimum: failsafeDurationMinimum,
}

// read the relays once synchronously so limits are valid as soon as NewFnn returns
Expand Down Expand Up @@ -121,6 +125,11 @@
productionPercent int // allowed feed-in percent (0..100), 100 = uncurtailed

interval time.Duration

failsafeConsumptionLimit float64
failsafeDurationMinimum time.Duration
failsafeActive bool
failsafeEnteredAt time.Time
}

func (c *Fnn) SetUpdated(f func()) {
Expand Down Expand Up @@ -183,14 +192,42 @@

// runDim evaluates the dimming rule and applies the dim limit.
// No-op if dim input is not configured.
// On read error, enters failsafe mode and applies failsafeConsumptionLimit if configured.
// Failsafe remains active for at least failsafeDurationMinimum after the first error.
func (c *Fnn) runDim() error {
if c.w4 == nil {
return nil
}

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()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
c.mu.Unlock()
return c.setConsumptionLimit(c.failsafeConsumptionLimit)
}

c.mu.Lock()
inFailsafe := false
if c.failsafeActive {
if time.Since(c.failsafeEnteredAt) >= c.failsafeDurationMinimum {
c.log.DEBUG.Println("leaving failsafe mode")
c.failsafeActive = false
} else {
inFailsafe = true
}
}
c.mu.Unlock()

if inFailsafe {
return nil
}

limit := 0.0
Expand Down
96 changes: 94 additions & 2 deletions hems/fnn/fnn_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package fnn

import (
"errors"
"testing"
"time"

"github.com/evcc-io/evcc/core/site"
"github.com/evcc-io/evcc/hems/hems"
"github.com/evcc-io/evcc/server/db"
"github.com/evcc-io/evcc/util"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -21,12 +24,16 @@ func boolG(v bool) func() (bool, error) {
return func() (bool, error) { return v, nil }
}

func errG() func() (bool, error) {
return func() (bool, error) { return false, errors.New("read error") }
}

// TestCurtailmentNotConfigured verifies that without W3 no curtailment
// statement is made, while dimming via W4 remains available.
func TestCurtailmentNotConfigured(t *testing.T) {
require.NoError(t, db.NewInstance("sqlite", ":memory:"))

fnn, err := NewFnn(&stubSite{}, 1e3, 1e3, nil, nil, nil, boolG(true), 0)
fnn, err := NewFnn(&stubSite{}, 1e3, 1e3, nil, nil, nil, boolG(true), 0, 0, 0)
require.NoError(t, err)

assert.Nil(t, fnn.CurtailedPercent())
Expand All @@ -40,7 +47,7 @@ func TestCurtailmentNotConfigured(t *testing.T) {
// TestDimmingNotConfigured verifies that without W4 no dimming statement is
// made, while curtailment via W3 remains available.
func TestDimmingNotConfigured(t *testing.T) {
fnn, err := NewFnn(&stubSite{}, 0, 1e3, boolG(false), nil, nil, nil, 0)
fnn, err := NewFnn(&stubSite{}, 0, 1e3, boolG(false), nil, nil, nil, 0, 0, 0)
require.NoError(t, err)

assert.Nil(t, fnn.MaxConsumptionPower())
Expand All @@ -49,3 +56,88 @@ func TestDimmingNotConfigured(t *testing.T) {
assert.NotNil(t, fnn.CurtailedPercent())
assert.NotNil(t, hems.Curtailed(fnn))
}

// TestDecodeConfig verifies that failsafe config keys are accepted and decoded.
func TestDecodeConfig(t *testing.T) {
require.NoError(t, db.NewInstance("sqlite", ":memory:"))
_ = util.NewLogger("fnn")

other := map[string]any{
"maxDimPower": 4200,
"failsafeConsumptionActivePowerLimit": 4200,
"failsafeDurationMinimum": "30m",
"w4": map[string]any{
"source": "const",
"value": false,
},
}

f, err := NewFromConfig(t.Context(), other, &stubSite{})
require.NoError(t, err)
assert.Equal(t, 4200.0, f.failsafeConsumptionLimit)
assert.Equal(t, 30*time.Minute, f.failsafeDurationMinimum)
}

// TestFailsafeActivatesOnReadError verifies that a W4 read error triggers failsafe
// mode and the configured failsafe consumption limit is applied.
func TestFailsafeActivatesOnReadError(t *testing.T) {
require.NoError(t, db.NewInstance("sqlite", ":memory:"))

const failsafeLimit = 4200.0
fnn, err := NewFnn(&stubSite{}, failsafeLimit, 0, nil, nil, nil, errG(), 0, failsafeLimit, 0)
require.NoError(t, err)

// construction calls runDim once — failsafe should already be active
assert.True(t, fnn.failsafeActive)
require.NotNil(t, fnn.MaxConsumptionPower())
assert.Equal(t, failsafeLimit, *fnn.MaxConsumptionPower())
}

// TestFailsafeExitsAfterDuration verifies that failsafe is released once
// failsafeDurationMinimum has elapsed and a successful read follows.
func TestFailsafeExitsAfterDuration(t *testing.T) {
require.NoError(t, db.NewInstance("sqlite", ":memory:"))

const failsafeLimit = 4200.0
// duration of 0 means failsafe exits on next successful read
fnn, err := NewFnn(&stubSite{}, failsafeLimit, 0, nil, nil, nil, errG(), 0, failsafeLimit, 0)
require.NoError(t, err)
assert.True(t, fnn.failsafeActive)

// switch to successful read
fnn.w4 = boolG(false)
require.NoError(t, fnn.runDim())

assert.False(t, fnn.failsafeActive)
require.NotNil(t, fnn.MaxConsumptionPower())
assert.Equal(t, 0.0, *fnn.MaxConsumptionPower())
}

// TestFailsafeRemainsActiveDuringDuration verifies that failsafe stays active
// while failsafeDurationMinimum has not yet elapsed, even when reads succeed.
func TestFailsafeRemainsActiveDuringDuration(t *testing.T) {
require.NoError(t, db.NewInstance("sqlite", ":memory:"))

const failsafeLimit = 4200.0
fnn, err := NewFnn(&stubSite{}, failsafeLimit, 0, nil, nil, nil, errG(), 0, failsafeLimit, time.Hour)
require.NoError(t, err)
assert.True(t, fnn.failsafeActive)

// switch to successful read, but duration not yet elapsed
fnn.w4 = boolG(false)
require.NoError(t, fnn.runDim())

assert.True(t, fnn.failsafeActive)
require.NotNil(t, fnn.MaxConsumptionPower())
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) {
Comment on lines +144 to +146

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

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)
}
Loading