diff --git a/.changeset/battery-soc-max-zero.md b/.changeset/battery-soc-max-zero.md new file mode 100644 index 000000000..d29e2314f --- /dev/null +++ b/.changeset/battery-soc-max-zero.md @@ -0,0 +1,9 @@ +--- +"ftw": patch +--- + +A battery whose Max SoC was saved as 0 charges again. Settings used to save a +blank number field as 0, and since 0.157.1 that 0 was the battery's charge +ceiling, so charging stopped at any level, manual Charge included. Core now +treats a battery soc_max of 0 or less as unset and uses the planner's ceiling, +and Settings saves a cleared number field as unset. diff --git a/go/cmd/ftw/battery_soc_max_test.go b/go/cmd/ftw/battery_soc_max_test.go index 26e0723f0..b6d5243fa 100644 --- a/go/cmd/ftw/battery_soc_max_test.go +++ b/go/cmd/ftw/battery_soc_max_test.go @@ -64,7 +64,32 @@ func TestSoCMaxConfigPrecedenceAndReload(t *testing.T) { zero := 0.0 cfg.Batteries = map[string]config.Battery{"battery": {SoCMax: &zero}} ctrl.DriverLimits = driverLimitsFrom(cfg.ModbusControlDrivers(), cfg.Batteries, cfg.Planner) - check(.5, false) // An explicit zero is not an absent limit. + check(.5, false) // Validate drops a configured zero; one that bypasses it still blocks charging. +} + +// Settings saved a blank Max SoC as 0. Loading that config must not leave +// a 0 % ceiling that blocks every charge order, manual Charge included. +func TestSavedZeroSoCMaxDoesNotBlockCharge(t *testing.T) { + cfg := parseBatteryLimitConfig(t, "", " soc_min: 0\n soc_max: 0\n max_charge_w: 0\n max_discharge_w: 0\n") + if cfg.Batteries["battery"].SoCMax != nil { + t.Fatalf("soc_max = %v after load, want unset", *cfg.Batteries["battery"].SoCMax) + } + cfg.Planner = &config.Planner{Enabled: true, SoCMax: .9} + ctrl := newControlStateFromConfig(cfg) + ctrl.Mode, ctrl.SlewEnabled = control.ModeCharge, false + tel := batteryLimitStore(1000) + capacities := map[string]float64{"battery": 10000} + for _, tc := range []struct { + soc float64 + charging bool + }{{.58, true}, {.9, false}} { + soc := tc.soc + tel.Update("battery", telemetry.DerBattery, 0, &soc, nil) + got := control.ComputeDispatch(tel, ctrl, capacities, 40000) + if len(got) != 1 || (got[0].TargetW > 0) != tc.charging { + t.Fatalf("SoC %.2f: got %+v, want charging=%v under the planner's 90%%", soc, got, tc.charging) + } + } } // Dispatch keeps the higher of the battery's and the running planner's diff --git a/go/internal/config/config.go b/go/internal/config/config.go index 39b36fea9..0b8e473f0 100644 --- a/go/internal/config/config.go +++ b/go/internal/config/config.go @@ -1953,6 +1953,7 @@ func isLegacyDefaultDriverRepository(repo DriverRepositorySource) bool { // Validate ensures the config is internally consistent and safe to run with. func (c *Config) Validate() error { c.NormalizeUnits() + c.dropZeroBatteryCeilings() if err := c.validateSetup(); err != nil { return err } diff --git a/go/internal/config/soc_bounds.go b/go/internal/config/soc_bounds.go index c0da7f092..dbeb45d8d 100644 --- a/go/internal/config/soc_bounds.go +++ b/go/internal/config/soc_bounds.go @@ -1,5 +1,10 @@ package config +import ( + "log/slog" + "maps" +) + // WithBatterySoCBounds returns a copy of drivers in which every driver that has // a matching batteries[] entry gets charge_ceil_soc / discharge_floor_soc // defaulted into its free-form Config from the battery's soc_max / soc_min. @@ -51,3 +56,26 @@ func WithBatterySoCBounds(drivers []Driver, batteries map[string]Battery) []Driv } return out } + +// dropZeroBatteryCeilings treats a battery soc_max of 0 or less as unset. +// Settings saved a cleared number field as 0, and an older Settings saved +// every field on the tab. Since soc_max became the dispatch charge ceiling, +// that 0 blocked all charging, manual Charge included. A battery that must +// not charge sets max_charge_w: 0. +func (c *Config) dropZeroBatteryCeilings() { + cloned := false + for name, b := range c.Batteries { + if b.SoCMax == nil || *b.SoCMax > 0 { + continue + } + slog.Warn("config: ignoring battery soc_max of 0 or less; the planner's ceiling applies", + "battery", name, "soc_max", *b.SoCMax) + // Callers validate shallow copies of the live config; never write + // into the map they share. + if !cloned { + c.Batteries, cloned = maps.Clone(c.Batteries), true + } + b.SoCMax = nil + c.Batteries[name] = b + } +} diff --git a/go/internal/config/soc_bounds_test.go b/go/internal/config/soc_bounds_test.go index 630acf1fb..58c116cb2 100644 --- a/go/internal/config/soc_bounds_test.go +++ b/go/internal/config/soc_bounds_test.go @@ -1,6 +1,9 @@ package config -import "testing" +import ( + "encoding/json" + "testing" +) func f64(v float64) *float64 { return &v } @@ -96,3 +99,67 @@ func TestWithBatterySoCBoundsDoesNotMutateInput(t *testing.T) { t.Errorf("merged copy lost existing key foo: got %v", got) } } + +// Settings saved a blank Max SoC as 0. Only soc_max loses it: a soc_min of 0 +// and the power limits are real choices. +func TestDropZeroBatteryCeilings(t *testing.T) { + c := &Config{Batteries: map[string]Battery{ + "blank": {SoCMin: f64(0), SoCMax: f64(0), MaxChargeW: f64(7500)}, + "negative": {SoCMax: f64(-0.1)}, + "set": {SoCMax: f64(0.9)}, + "unset": {SoCMin: f64(0.1)}, + }} + + c.dropZeroBatteryCeilings() + + for name, b := range c.Batteries { + if name != "set" && b.SoCMax != nil { + t.Errorf("%s: soc_max = %v, want unset", name, *b.SoCMax) + } + } + if b := c.Batteries["set"]; b.SoCMax == nil || *b.SoCMax != 0.9 { + t.Errorf("set: soc_max = %v, want 0.9", b.SoCMax) + } + if b := c.Batteries["blank"]; b.SoCMin == nil || *b.SoCMin != 0 || b.MaxChargeW == nil || *b.MaxChargeW != 7500 { + t.Errorf("blank: other limits changed: %+v", b) + } + if out := WithBatterySoCBounds([]Driver{{Name: "blank"}}, c.Batteries); out[0].Config["charge_ceil_soc"] != nil { + t.Errorf("a dropped soc_max still reached the driver: %v", out[0].Config) + } +} + +// A box's settings live in the config database. A zero an older Core stored +// there must load as unset, and the copy being validated must not write into +// the map it shares with the live config. +func TestStoredZeroSoCMaxLoadsAsUnset(t *testing.T) { + path, _, st := importedLiveSettings(t) + doc, found, err := st.Configuration() + if err != nil || !found { + t.Fatalf("stored configuration: found=%v err=%v", found, err) + } + var stored map[string]any + if err := json.Unmarshal(doc.Document, &stored); err != nil { + t.Fatal(err) + } + stored["config"].(map[string]any)["batteries"] = map[string]any{ + "pixii": map[string]any{"soc_min": 0, "soc_max": 0, "max_charge_w": 0, "max_discharge_w": 0}, + } + raw, _ := json.Marshal(stored) + if _, err := st.SaveConfiguration(raw, doc.Revision, nil); err != nil { + t.Fatal(err) + } + got, err := Load(path) + if err != nil { + t.Fatal(err) + } + if b := got.Batteries["pixii"]; b.SoCMax != nil || b.SoCMin == nil || b.MaxChargeW == nil { + t.Fatalf("loaded %+v, want soc_max unset and the other limits kept", b) + } + + live := Config{Batteries: map[string]Battery{"pixii": {SoCMax: f64(0)}}} + next := live + next.dropZeroBatteryCeilings() + if live.Batteries["pixii"].SoCMax == nil || next.Batteries["pixii"].SoCMax != nil { + t.Fatalf("live %+v, copy %+v: want only the copy changed", live.Batteries, next.Batteries) + } +} diff --git a/web/settings-shell.test.mjs b/web/settings-shell.test.mjs index 5c625044b..136997abe 100644 --- a/web/settings-shell.test.mjs +++ b/web/settings-shell.test.mjs @@ -361,6 +361,21 @@ describe("unchanged settings fields", () => { assert.equal(rig.saved().planner.soc_max, 0.95); }); + // A blank Max SoC saved as 0 became a 0 % charge ceiling in Core. + it("saves a cleared number as unset, not 0", async () => { + const rig = await formShell({ site: { name: "Home" }, batteries: { pixii: { soc_max: 0, max_charge_w: 7.5 } } }); + const number = (path, value, unitScale) => Object.assign(stubElement(), { + dataset: unitScale ? { path, unitScale } : { path }, type: "number", defaultValue: value, value }); + const max = number("batteries.pixii.soc_max", "0"); + const charge = number("batteries.pixii.max_charge_w", "7.5", "1000"); + rig.addField(max); + rig.addField(charge); + max.value = ""; + charge.value = ""; + await rig.context.saveConfig(); + assert.deepEqual(rig.saved().batteries, { pixii: { soc_max: null, max_charge_w: null } }); + }); + // A select shows the first option when the stored value is none of them, // and an unchanged save then never stores what the form showed (#1436). it("shows a select's default when the stored value is not an option", async () => { diff --git a/web/settings.js b/web/settings.js index 55cdc16b9..5bd4972fb 100644 --- a/web/settings.js +++ b/web/settings.js @@ -362,8 +362,10 @@ if (fieldValue(input, input.defaultValue) === input.value) return; var path = input.dataset.path; var val = input.type === "number" ? parseFloat(input.value) : input.value; - if (input.type === "number" && isNaN(val)) val = 0; - if (input.type === "number" && input.dataset.unitScale) { + // A blank number is unset, so Core uses its default. Saving 0 instead + // gave a battery a 0 % charge ceiling. + if (input.type === "number" && isNaN(val)) val = null; + else if (input.type === "number" && input.dataset.unitScale) { val = val * parseFloat(input.dataset.unitScale); } // Preserve a stored password when the user hasn't typed over it.