From a771ba4ab3543e5c364c134e273b889e30aa1bdc Mon Sep 17 00:00:00 2001 From: andig Date: Sat, 12 Apr 2025 12:17:50 +0200 Subject: [PATCH 1/4] chore: simplify external battery mode --- core/site_api.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/core/site_api.go b/core/site_api.go index d10cd92cb18..23c13aeac86 100644 --- a/core/site_api.go +++ b/core/site_api.go @@ -364,19 +364,15 @@ func (site *Site) SetBatteryModeExternal(mode api.BatteryMode) { site.batteryModeExternal = mode site.publish(keys.BatteryModeExternal, mode) - if !disable { - site.setBatteryMode(mode) - - // start watchdog if not running - if site.batteryModeExternalTimer.IsZero() { - go func() { - for range time.Tick(time.Second) { - if site.batteryModeWatchdogExpired() { - return - } + // start watchdog if not running + if !disable && site.batteryModeExternalTimer.IsZero() { + go func() { + for range time.Tick(time.Second) { + if site.batteryModeWatchdogExpired() { + return } - }() - } + } + }() } } From afb8b7befb08340f828b2771a0472c9f75d65fea Mon Sep 17 00:00:00 2001 From: andig Date: Sun, 13 Apr 2025 13:10:34 +0200 Subject: [PATCH 2/4] Fix external mode applied repeatedly --- core/site_battery.go | 5 ++++- core/site_battery_test.go | 43 +++++++++++++++++++++++++++++++++------ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/core/site_battery.go b/core/site_battery.go index f9d09eb7bb8..7c17f4e922d 100644 --- a/core/site_battery.go +++ b/core/site_battery.go @@ -63,7 +63,10 @@ func (site *Site) requiredBatteryMode(batteryGridChargeActive bool, rate api.Rat // require normal mode to leave external control res = api.BatteryNormal case extMode != api.BatteryUnknown: - res = extMode + // require external mode only once + if extMode != batMode { + res = extMode + } case batteryGridChargeActive: res = mapper(api.BatteryCharge) case site.dischargeControlActive(rate): diff --git a/core/site_battery_test.go b/core/site_battery_test.go index 16a1528a712..465515aeba8 100644 --- a/core/site_battery_test.go +++ b/core/site_battery_test.go @@ -43,7 +43,7 @@ func TestExternalBatteryMode(t *testing.T) { func TestExternalBatteryModeChange(t *testing.T) { for _, tc := range []struct { - internal, ext, expired api.BatteryMode + internal, ext, expected api.BatteryMode }{ {api.BatteryUnknown, api.BatteryUnknown, api.BatteryNormal}, {api.BatteryUnknown, api.BatteryNormal, api.BatteryNormal}, @@ -64,17 +64,47 @@ func TestExternalBatteryModeChange(t *testing.T) { batteryMeters: []config.Device[api.Meter]{nil}, } + // set initial state, internal mode may already be changed site.batteryMode = tc.internal - + site.batteryModeExternal = api.BatteryUnknown assert.True(t, site.batteryModeExternalTimer.IsZero()) + + // 1. set required external mode site.SetBatteryModeExternal(tc.ext) + assert.Equal(t, site.batteryModeExternal, tc.ext, "external mode expected %s got %s", tc.ext, site.batteryModeExternal) + assert.Equal(t, site.batteryMode, tc.internal, "internal mode expected unchanged %s got %s", tc.ext, site.batteryMode) + + // 2. verify required external mode is indicated (unless unknown) + mode := site.requiredBatteryMode(false, api.Rate{}) - // timer started if tc.ext != api.BatteryUnknown { + // timer started unless external mode is disabled by setting unknown assert.False(t, site.batteryModeExternalTimer.IsZero()) + + // required mode should show external + if tc.internal != tc.ext { + assert.Equal(t, tc.ext, mode, "required mode expected %s got %s", tc.ext, mode) + } else { + assert.Equal(t, api.BatteryUnknown, mode, "required mode expected %s got %s", api.BatteryUnknown, mode) + } + } else { + // required mode should be unknown unless internal mode has been changed + if tc.internal != api.BatteryCharge { + assert.Equal(t, api.BatteryUnknown, mode, "required mode expected %s got %s", api.BatteryUnknown, mode) + } } - // expire timer + // 3. apply required external mode (invoke applyBatteryMode, then SetBatteryMode if no error) + if mode != api.BatteryUnknown { + site.SetBatteryMode(mode) + assert.Equal(t, mode, site.batteryMode, "internal mode expected %s got %s", mode, site.batteryMode) + } + + // 4. required external mode should only be applied once + mode = site.requiredBatteryMode(false, api.Rate{}) + assert.Equal(t, api.BatteryUnknown, mode, "required mode should only be set once, expected %s got %s", api.BatteryUnknown, mode) + + // 5. timer expiry site.batteryModeExternalTimer = site.batteryModeExternalTimer.Add(-time.Hour) site.batteryModeWatchdogExpired() @@ -82,8 +112,9 @@ func TestExternalBatteryModeChange(t *testing.T) { assert.Equal(t, site.batteryModeExternal, api.BatteryUnknown) assert.False(t, site.batteryModeExternalTimer.IsZero()) - mode := site.requiredBatteryMode(false, api.Rate{}) - assert.Equal(t, tc.expired.String(), mode.String(), "external mode expected %s got %s", tc.expired, mode) + // switch battery back to normal mode + mode = site.requiredBatteryMode(false, api.Rate{}) + assert.Equal(t, tc.expected, mode, "external mode expected %s got %s", tc.expected, mode) // timer disabled site.SetBatteryMode(mode) From de94b9f22fe0174319ce4b5cef31798aabbdd12e Mon Sep 17 00:00:00 2001 From: andig Date: Sun, 13 Apr 2025 13:30:07 +0200 Subject: [PATCH 3/4] Test actual behaviour --- core/site.go | 9 +----- core/site_battery.go | 10 +++++++ core/site_battery_test.go | 58 ++++++++++++++++++--------------------- 3 files changed, 37 insertions(+), 40 deletions(-) diff --git a/core/site.go b/core/site.go index 37290e5674b..da95ed7bc2e 100644 --- a/core/site.go +++ b/core/site.go @@ -915,14 +915,7 @@ func (site *Site) update(lp updater) { batteryGridChargeActive := site.batteryGridChargeActive(rate) site.publish(keys.BatteryGridChargeActive, batteryGridChargeActive) - - if batteryMode := site.requiredBatteryMode(batteryGridChargeActive, rate); batteryMode != api.BatteryUnknown { - if err := site.applyBatteryMode(batteryMode); err == nil { - site.SetBatteryMode(batteryMode) - } else { - site.log.ERROR.Println("battery mode:", err) - } - } + site.updateBatteryMode(batteryGridChargeActive, rate) if sitePower, batteryBuffered, batteryStart, err := site.sitePower(totalChargePower, flexiblePower); err == nil { // ignore negative pvPower values as that means it is not an energy source but consumption diff --git a/core/site_battery.go b/core/site_battery.go index 7c17f4e922d..3ad41120d59 100644 --- a/core/site_battery.go +++ b/core/site_battery.go @@ -39,6 +39,16 @@ func (site *Site) SetBatteryMode(batMode api.BatteryMode) { } } +func (site *Site) updateBatteryMode(batteryGridChargeActive bool, rate api.Rate) { + if batteryMode := site.requiredBatteryMode(batteryGridChargeActive, rate); batteryMode != api.BatteryUnknown { + if err := site.applyBatteryMode(batteryMode); err == nil { + site.SetBatteryMode(batteryMode) + } else { + site.log.ERROR.Println("battery mode:", err) + } + } +} + // requiredBatteryMode determines required battery mode based on grid charge and rate func (site *Site) requiredBatteryMode(batteryGridChargeActive bool, rate api.Rate) api.BatteryMode { var res api.BatteryMode diff --git a/core/site_battery_test.go b/core/site_battery_test.go index 465515aeba8..87fe9bc5127 100644 --- a/core/site_battery_test.go +++ b/core/site_battery_test.go @@ -8,6 +8,7 @@ import ( "github.com/evcc-io/evcc/util" "github.com/evcc-io/evcc/util/config" "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" ) func TestExternalBatteryMode(t *testing.T) { @@ -42,6 +43,18 @@ func TestExternalBatteryMode(t *testing.T) { } func TestExternalBatteryModeChange(t *testing.T) { + ctrl := gomock.NewController(t) + + var bat api.Meter + batCon := api.NewMockBatteryController(ctrl) + + bat = &struct { + api.Meter + api.BatteryController + }{ + BatteryController: batCon, + } + for _, tc := range []struct { internal, ext, expected api.BatteryMode }{ @@ -61,7 +74,7 @@ func TestExternalBatteryModeChange(t *testing.T) { site := &Site{ log: util.NewLogger("foo"), - batteryMeters: []config.Device[api.Meter]{nil}, + batteryMeters: []config.Device[api.Meter]{config.NewStaticDevice(config.Named{}, bat)}, } // set initial state, internal mode may already be changed @@ -74,37 +87,18 @@ func TestExternalBatteryModeChange(t *testing.T) { assert.Equal(t, site.batteryModeExternal, tc.ext, "external mode expected %s got %s", tc.ext, site.batteryModeExternal) assert.Equal(t, site.batteryMode, tc.internal, "internal mode expected unchanged %s got %s", tc.ext, site.batteryMode) - // 2. verify required external mode is indicated (unless unknown) - mode := site.requiredBatteryMode(false, api.Rate{}) - + // 2. verify external mode applied to battery if tc.ext != api.BatteryUnknown { - // timer started unless external mode is disabled by setting unknown - assert.False(t, site.batteryModeExternalTimer.IsZero()) - - // required mode should show external - if tc.internal != tc.ext { - assert.Equal(t, tc.ext, mode, "required mode expected %s got %s", tc.ext, mode) - } else { - assert.Equal(t, api.BatteryUnknown, mode, "required mode expected %s got %s", api.BatteryUnknown, mode) - } - } else { - // required mode should be unknown unless internal mode has been changed - if tc.internal != api.BatteryCharge { - assert.Equal(t, api.BatteryUnknown, mode, "required mode expected %s got %s", api.BatteryUnknown, mode) - } - } - - // 3. apply required external mode (invoke applyBatteryMode, then SetBatteryMode if no error) - if mode != api.BatteryUnknown { - site.SetBatteryMode(mode) - assert.Equal(t, mode, site.batteryMode, "internal mode expected %s got %s", mode, site.batteryMode) + batCon.EXPECT().SetBatteryMode(tc.ext).Times(1) } + site.updateBatteryMode(false, api.Rate{}) + ctrl.Finish() - // 4. required external mode should only be applied once - mode = site.requiredBatteryMode(false, api.Rate{}) - assert.Equal(t, api.BatteryUnknown, mode, "required mode should only be set once, expected %s got %s", api.BatteryUnknown, mode) + // 3. verify required external mode only applied once + site.updateBatteryMode(false, api.Rate{}) + ctrl.Finish() - // 5. timer expiry + // 4. verify timer expiry site.batteryModeExternalTimer = site.batteryModeExternalTimer.Add(-time.Hour) site.batteryModeWatchdogExpired() @@ -112,12 +106,12 @@ func TestExternalBatteryModeChange(t *testing.T) { assert.Equal(t, site.batteryModeExternal, api.BatteryUnknown) assert.False(t, site.batteryModeExternalTimer.IsZero()) - // switch battery back to normal mode - mode = site.requiredBatteryMode(false, api.Rate{}) - assert.Equal(t, tc.expected, mode, "external mode expected %s got %s", tc.expected, mode) + // battery switched back to normal mode + batCon.EXPECT().SetBatteryMode(api.BatteryNormal).Times(1) + site.updateBatteryMode(false, api.Rate{}) + ctrl.Finish() // timer disabled - site.SetBatteryMode(mode) assert.True(t, site.batteryModeExternalTimer.IsZero()) } } From 1c597726726e65db566477ef40f70af08244547d Mon Sep 17 00:00:00 2001 From: andig Date: Sun, 13 Apr 2025 13:34:52 +0200 Subject: [PATCH 4/4] Fix test --- core/site_battery_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/site_battery_test.go b/core/site_battery_test.go index 87fe9bc5127..61782b2e5a0 100644 --- a/core/site_battery_test.go +++ b/core/site_battery_test.go @@ -11,7 +11,7 @@ import ( "go.uber.org/mock/gomock" ) -func TestExternalBatteryMode(t *testing.T) { +func TestRequiredExternalBatteryMode(t *testing.T) { for _, tc := range []struct { internal, ext, new api.BatteryMode }{ @@ -20,12 +20,12 @@ func TestExternalBatteryMode(t *testing.T) { {api.BatteryUnknown, api.BatteryCharge, api.BatteryCharge}, {api.BatteryNormal, api.BatteryUnknown, api.BatteryUnknown}, - {api.BatteryNormal, api.BatteryNormal, api.BatteryNormal}, + {api.BatteryNormal, api.BatteryNormal, api.BatteryUnknown}, // no change required {api.BatteryNormal, api.BatteryCharge, api.BatteryCharge}, {api.BatteryCharge, api.BatteryUnknown, api.BatteryNormal}, {api.BatteryCharge, api.BatteryNormal, api.BatteryNormal}, - {api.BatteryCharge, api.BatteryCharge, api.BatteryCharge}, + {api.BatteryCharge, api.BatteryCharge, api.BatteryUnknown}, // no change required } { t.Logf("%+v", tc)