Skip to content

chore: simplify external battery mode #20554

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andig merged 4 commits into from
Apr 13, 2025
Merged

chore: simplify external battery mode #20554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a771ba4
chore: simplify external battery mode
andig Apr 12, 2025
afb8b7b
Fix external mode applied repeatedly
andig Apr 13, 2025
de94b9f
Test actual behaviour
andig Apr 13, 2025
1c59772
Fix test
andig Apr 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions core/site.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 8 additions & 12 deletions core/site_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}()
}
}
}()
Comment on lines +367 to +375
Copy link
Copy Markdown
Contributor

@iseeberg79 iseeberg79 Apr 12, 2025

Choose a reason for hiding this comment

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

Das ist der Code, der wg. der Bedingung nur einmal ausgeführt werden wird und jetzt nutzlos ist. Die Logik wird vom requireBatteryMode-Thread übernommen, der batteryModeWatchdogExpired regelmäßig ruft und für die Zurücksetzung sorgt

Folgendes wirkt postiv auf das UI, das eine Änderung (=! unknown) sofort anzeigt - der interne Modus wird sofort umgesetzt. Das würde ich beibehalten:

		if !disable {
			site.setBatteryMode(mode)
		}

aus batteryModeWatchdogExpired - da setzt du ja jetzt den externen Modus zurück:

...
	if elapsed > time.Minute && !site.batteryModeExternalTimer.IsZero() {
		site.SetBatteryModeExternal(api.BatteryUnknown)
		return true
	}
...

Das geht natürlich, hattest du aber vorher zugunsten des go-Threads (watchdog) mal aus einem vorherigen Entwurf verworfen.

Fazit: nicht site.setBatteryMode sollte weg, sondern die parallele go-Routine - das ändert nichts am Verhalten

Copy link
Copy Markdown
Contributor

@iseeberg79 iseeberg79 Apr 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

ich dreh' mich den ganzen Tag und such' ... du hast den Reset des expiration-Timers in SetBatteryMode eingebaut?

	if site.batteryModeExternal == api.BatteryUnknown {
		site.batteryModeExternalTimer = time.Time{}
	}

Das Snippet hab ich vermisst und erklärt, warum ich obiges angenommen hab...

Copy link
Copy Markdown
Member Author

@andig andig Apr 13, 2025

Choose a reason for hiding this comment

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

Genau. Der WD ist dann schon geendet, der Timer zeigt aber an dass noch kein Reset des internen Modus passiert ist.

}
}

Expand Down
15 changes: 14 additions & 1 deletion core/site_battery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -63,7 +73,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):
Expand Down
49 changes: 37 additions & 12 deletions core/site_battery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ 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) {
func TestRequiredExternalBatteryMode(t *testing.T) {
for _, tc := range []struct {
internal, ext, new api.BatteryMode
}{
Expand All @@ -19,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)

Expand All @@ -42,8 +43,20 @@ 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, expired api.BatteryMode
internal, ext, expected api.BatteryMode
}{
{api.BatteryUnknown, api.BatteryUnknown, api.BatteryNormal},
{api.BatteryUnknown, api.BatteryNormal, api.BatteryNormal},
Expand All @@ -61,32 +74,44 @@ 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
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)

// timer started
// 2. verify external mode applied to battery
if tc.ext != api.BatteryUnknown {
assert.False(t, site.batteryModeExternalTimer.IsZero())
batCon.EXPECT().SetBatteryMode(tc.ext).Times(1)
}
site.updateBatteryMode(false, api.Rate{})
ctrl.Finish()

// expire timer
// 3. verify required external mode only applied once
site.updateBatteryMode(false, api.Rate{})
ctrl.Finish()

// 4. verify timer expiry
site.batteryModeExternalTimer = site.batteryModeExternalTimer.Add(-time.Hour)
site.batteryModeWatchdogExpired()

// mode reverted to unknown, timer still active
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)
// 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())
}
}
Loading