From f9e7fbf86dc391b7c0219edc836eab72970f7a76 Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Fri, 4 Sep 2026 14:11:11 +0200 Subject: [PATCH] SAFETY-32: bms_soc_floor per pack, rule 8 per unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bms_soc_floor` describes A BATTERY, not a site. At two packs of different models the cut-outs differ, and one number for both graded the shallower pack against the deeper one's BMS - so a site with cut-outs of 10 % and 30 % under `peak_soc_floor: 20` STARTED, and the guard would command a forced discharge that pack B's BMS, not the operator's floor, is the thing that stops. That is rule 8's own failure mode, reached through rule 8. Scoped to the card's third bullet, per its PM PROOFREAD: the aggregate anti-windup (DEV-02) and per-instance `achievable_w` (CFG-04) were verified as already shipped and are untouched. * config.yaml: `bms_soc_floor: int(0,50)` REQUIRED per `inverters:` entry. Nothing is inherited; the flat key stays the single-inverter shape's own cut-out. A per-entry DEFAULT was declined - a default is one number for every pack, which is this card's defect - and the decision is recorded. * inverter.py: `Inverter.bms_soc_floor`; an entry that states none gets None, not a borrowed number and not 0.0 (the most permissive value the key has). * peakguard.py: `Pack`, `pack_floor_pct()`, `pack_permissions()`, `binding_soc_floor()`. `discharge_permitted()` is unchanged and is still the one place the question is answered. * validate.py: `unit_bms_floors_indexed()` carries the ROSTER index; rule 8 grades EVERY unit and NAMES the one it refuses. Graded with the offender SECOND and with the same two numbers in the other order. * main.py: `Controller.packs`; `decide(soc_floor=...)` is the per-pack reduction of the same option; the floor-limited notification names the pack that stopped the site. `pack_floor_soc` - the emptiest pack, and its None-on-missing behaviour - is untouched. * tools/supervisor_shape_probe.py: RE-RUN and repaired. The required key broke §C and left §D printing PASS on a refusal about a different key. §D is now an isolated counterfactual over an accepted merged baseline, one key varied, asserting the key name. `all cases passed` on 2026.08.0. Stated rather than hidden: under any config rule 8 accepts the per-pack FLOOR reduces to `peak_soc_floor` for every pack. The reduction is written for `unit_caps_indexed()`'s reason and graded on both sides - 1775 rule-8-valid pairs answer the option, the pair rule 8 refuses answers the cut-out. `peak_soc_floor` is still one number for the site, so the "deeper pack needlessly withheld" half is filed as an open question, not closed. Full 19-file suite green. Twelve source mutations through tools/mutation_run.sh: eleven red, one argued survivor. The battery also found `_flat_bms_floor()` defaulting in two places, each covering for the other. No measurement of inverter B appears anywhere: it does not exist yet. Every second-pack figure is simulated and labelled. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 62 +++ .../DECISIONS/SAFETY-32-per-pack-bms-floor.md | 215 +++++++++ goodwe_controller/DOCS.md | 2 +- goodwe_controller/app/inverter.py | 85 +++- goodwe_controller/app/main.py | 54 ++- goodwe_controller/app/peakguard.py | 127 +++++ goodwe_controller/app/validate.py | 102 +++- goodwe_controller/config.yaml | 35 +- goodwe_controller/test_inverter.py | 42 +- goodwe_controller/test_peakguard.py | 453 +++++++++++++++++- goodwe_controller/test_validate.py | 9 +- .../tools/supervisor_shape_probe.py | 70 ++- 12 files changed, 1193 insertions(+), 63 deletions(-) create mode 100644 goodwe_controller/DECISIONS/SAFETY-32-per-pack-bms-floor.md diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 8af1c83..77f4905 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -179,6 +179,68 @@ two power-moving phases on one grid connection pulling against each other. is loop and allocator territory. Pinned as a running check (`test_maintenance.py` part 12) so it cannot be forgotten. +**`bms_soc_floor` describes a BATTERY, so at two packs it stops being one number +(SAFETY-32).** The peak guard's energy backstop rests on two figures: the EMS +floor `peak_soc_floor` (the depth a money-saving discharge may never reach) and +`bms_soc_floor`, the percent **that pack's own BMS** cuts out at — register +`0x0560`, read as 10 on the reference GW5000S-BP. Rule 8 refuses an EMS floor at +or below the cut-out, because such a floor can never bind: the BMS wins and the +status page reports a backstop that does not exist. With ONE cut-out for two +packs of different models that rule graded the EMS floor against whichever +battery happened to be written into the flat key, and a site with cut-outs of +10 % and 30 % under `peak_soc_floor: 20` **started** — the guard would then +command a forced discharge that pack B's BMS, not the operator's floor, is the +thing that stops. Rule 8's own failure mode, reached through rule 8. + +* **`bms_soc_floor` becomes a per-entry roster key, and it is REQUIRED there.** + Nothing is inherited: an entry that omits it has no cut-out, does not borrow + the flat one, and is refused by name at startup. A per-entry *default* was + considered and declined — a default is one number for every pack, which is the + defect this card removes; `DECISIONS/SAFETY-32-per-pack-bms-floor.md` records + the decision and its migration cost. The flat key is unchanged and is now + documented as the single-inverter shape's own cut-out. +* **Rule 8 grades EVERY unit and NAMES the one it refuses**, through a new + `unit_bms_floors_indexed()` that carries the **roster** index the value came + off. CFG-04 had just fixed a rule 5/6 bug of exactly this shape — a caps-list + index fed to a function that indexes the roster, so the message named the wrong + inverter — and this reader skips nothing, so there is no second list for an + index to desynchronise from. Graded on a roster whose offender is the SECOND + entry *and* on the same two numbers in the other order. +* **The permission is answered PER PACK.** `Controller.pack_floor_soc` — the + emptiest measured pack — is untouched and is still what `decide()` grades; + what changed is the floor. `peakguard.pack_permissions()` answers one + `(name, reason)` per pack against that pack's own floor, and the floor-limited + notification now names the pack that stopped the site rather than "the + battery". At a roster of one the alarm is byte-for-byte what SAFETY-25 shipped. +* **An unstated cut-out withholds rather than defaults.** `None`, not `0.0` — + `discharge_permitted()` already treats an unknown floor as no permission, so + the absence costs a shave and never a pack, including on `sim/inject.py`'s path + which does not run the startup gate. +* ⚠️ **Stated rather than hidden:** under any config rule 8 accepts, the per-pack + *floor* reduces to `peak_soc_floor` for every pack — rule 8 makes it so. The + reduction is written for `unit_caps_indexed()`'s reason ("unreachable" is a + claim about rule ordering, not a property of a function) and is graded on both + sides: 1775 rule-8-valid `(peak_soc_floor, bms_soc_floor)` pairs answer the + option, and the pair rule 8 refuses answers the cut-out. `peak_soc_floor` + itself is still one number for the site, so the "deeper pack needlessly + withheld" half is filed as an open question, not closed. +* **`tools/supervisor_shape_probe.py` re-run and repaired.** MUTATION.md habit 8: + a committed transcript is a check and it decays. The required key broke §C and + left §D printing PASS on a refusal about a *different* key. §D is now an + isolated counterfactual over an accepted, merged baseline, one key varied per + case, asserting the key **name** in the refusal — `all cases passed` on + `amd64-hassio-supervisor:2026.08.0`. +* **Twelve source mutations, eleven red, one argued survivor**, through + `tools/mutation_run.sh` (bytecode purged). The battery also found a real + weakness of its own: `_flat_bms_floor()` had the default in two places and each + covered for the other, so deleting either survived — masking, in three lines of + option reading. One defaulting line now. + +⚠️ **No measurement of inverter B appears anywhere in this change.** It does not +exist yet. Every second-pack figure — in fixtures, in the probe, in +config.yaml's commented example — is **simulated and labelled**; only inverter +A's 10 % is read off hardware. + **At two inverters the sign watchdog stops comparing one unit against the whole site meter, and becomes two checks instead of one (SAFETY-30).** `disagrees(commanded_w, grid_w, ...)` compared the command **this controller put diff --git a/goodwe_controller/DECISIONS/SAFETY-32-per-pack-bms-floor.md b/goodwe_controller/DECISIONS/SAFETY-32-per-pack-bms-floor.md new file mode 100644 index 0000000..9c8d2fd --- /dev/null +++ b/goodwe_controller/DECISIONS/SAFETY-32-per-pack-bms-floor.md @@ -0,0 +1,215 @@ +# SAFETY-32 — `bms_soc_floor` per pack, and the two judgement calls it left open + +**Status:** decided and implemented on branch `SAFETY-32`. +**Card:** SAFETY-32 — *"One `bms_soc_floor` for two packs grades the shallower +pack against the deeper one's cut-out"*, scoped by the card's own PM PROOFREAD +to its **third** bullet only. Bullets 1 (aggregate anti-windup) and 2 (per-instance +`achievable_w`) were verified as already shipped by DEV-02 and CFG-04 and were +**not** touched — `main.py`'s `actual_w=self.site_batt_w` comment is the record +of why the first of those must stay site-level, and it still reads as written. + +--- + +## 1. The defect, stated in the terms the code uses + +`bms_soc_floor` **describes a battery**: it is the percent that pack's own BMS +cuts out at, register `0x0560` on the reference GW5000S-BP, read as 10. It was a +flat option because there was one pack. + +`peak_soc_floor` **describes the site's policy**: the depth a money-saving +discharge may never reach. Rule 8 refuses `peak_soc_floor <= bms_soc_floor`, +because such a floor can never bind — the BMS wins and the status page reports a +backstop that does not exist. + +At two packs of different models the two cut-outs differ, and one number for both +is wrong **in the direction that empties a pack**: with cut-outs of 10 % and 30 % +and `peak_soc_floor: 20`, the add-on **started**, and the guard would command a +forced discharge that pack B's *BMS* — not the operator's floor — is the thing +that stops. That is rule 8's own failure mode, reached through rule 8. + +--- + +## 2. DECISION 1 — required per entry, not defaulted from `PEAK_OPTION_DEFAULTS` + +The card left this open and asked for the reason to be recorded. + +**Decided: REQUIRED of every roster entry**, in `config.yaml`'s schema +(`bms_soc_floor: int(0,50)`, no trailing `?`) and in `validate.py` rule 8, which +refuses a missing one **by name**. + +The reason is that a default here is not a convenience, it is **this card's own +defect wearing a different hat**. A default is one number applied to every pack, +which is exactly what the flat key was. An add-on that answers "10 %" for a pack +nobody has told it about has invented a property of somebody's battery, and the +failure mode of the invention is the one this card exists to remove — the +shallower pack protected by a floor that belongs to the deeper one. CFG-03's +rule ("defaults live in exactly one place") points the same way: the one mirror +`PEAK_OPTION_DEFAULTS["bms_soc_floor"]` is the default for the **flat** key, i.e. +for the single-inverter shape, and reusing it per entry would make one dict the +default in two shapes with two meanings. + +It is also the treatment `max_w` already gets, for the same reason and by the +same argument (`inverter.py`'s `build_roster()` docstring): the per-unit numbers +that describe hardware are required, and the per-unit *sensors* are optional +because an absent sensor is a real state. + +**The cost is stated rather than hidden.** This is a schema change: a roster +config written before this branch is refused by the Supervisor until each entry +gains the key, and every roster fixture in the suite needed it. Nothing in +production carries a roster today — `inverters:` ships `[]` and the second +inverter is ~8 days out — so the migration is paid by fixtures and by whoever +writes the first real roster, at a startup refusal that names the entry. + +**What is NOT inherited.** An entry that omits the key does not borrow the flat +one. `Inverter.bms_soc_floor` is then `None`, `peakguard.pack_floor_pct()` +answers `None`, and `discharge_permitted()` already treats an unknown floor as +**no permission** — so even in the window before rule 8 runs (e.g. `sim/inject.py`, +which does not run the startup gate) the absence costs a shave and never a pack. +`0.0` was rejected explicitly: it is the most permissive value the key has. + +## 3. DECISION 2 — the site verdict is the MAXIMUM of the packs' own floors + +`Controller.pack_floor_soc` — the **minimum** measured SoC, the emptiest pack — +**is unchanged**, exactly as the card instructed. What changed is the floor it is +graded against. `peakguard.binding_soc_floor()` reduces the roster's per-pack +floors, each of them `max(peak_soc_floor, that pack's own cut-out)`, with a +`max()`. The two reductions pair: a minimum SoC against a maximum floor is the +conservative combination, and both move the verdict towards refusing a shave and +away from spending a pack. `min(soc) <= max(floor)` is implied by "some pack is +at or below its own floor", so the site verdict is never more permissive than +grading each pack separately would be. The converse is not true and is not +claimed — a site can be refused while every pack individually has headroom, which +costs a shave. + +## 4. ⚠️ THE HONEST LIMITATION, and it is the reason this record exists + +**Under any configuration rule 8 accepts, `binding_soc_floor()` answers +`peak_soc_floor` for every pack.** Rule 8 refuses `peak_soc_floor <= bms_soc_floor` +on *every* unit, so `max(peak_soc_floor, bms_i) == peak_soc_floor` everywhere the +add-on will actually start. The per-pack **floor** is therefore non-degenerate +only against a config rule 8 refuses. + +This is not a defect and it is not hidden. It is the same argument +`validate.unit_caps_indexed()` makes about itself, in that function's own words: + +> *"Rule 9(d) already refuses such a config so the two lists cannot desynchronise +> in shipped use — but 'unreachable' is a claim about today's rule ordering, not +> a property of this function, and `validate_startup_config()` is also +> `sim/inject.py`'s entry point."* + +CFG-04's index desynchronisation is what happened the last time that reasoning +was left implicit. So the reduction is written, and it is graded on both sides: +`test_peakguard.py` part 12c sweeps **1775** rule-8-valid `(peak_soc_floor, +bms_soc_floor)` pairs and asserts the answer is the option in every one of them +(that is AC 5, measured over the whole option space rather than at the shipped +point), and then asserts that on the pair rule 8 *refuses* the answer is the +**cut-out** — because without that second line the sweep would be satisfied by +`soc_floor` typed straight through, and the mutation that deletes the `max()` +could never bite. It does bite: M2 on the PR. + +**What IS live at a rule-8-valid roster** is the rest of the card: rule 8 now +refuses the two-model config that used to start; the permission is answered per +pack, so the alarm names the pack that stopped the site rather than "the +battery"; and an entry with no stated cut-out is withheld instead of defaulted. + +## 5. ⚠️ THE RESIDUAL — a per-pack EMS floor is NOT this card + +`peak_soc_floor` is still **one number for the site**, so the card's own +"or the deeper pack is needlessly withheld" half is not closed and this record +does not pretend it is. With cut-outs of 10 % and 18 %, rule 8 obliges the +operator to set the EMS floor above 18, and the 10 % pack is then held above its +own need. Closing that means `peak_soc_floor` becoming a per-entry key too, which +is a second option migration and a product decision about what the site-wide +number then means. The card scoped itself to `bms_soc_floor` and explicitly told +this developer not to widen it, so it is filed here as the open question it is +rather than smuggled in. + +**Also still not in this card, and named by the shipped code already:** the +charge *ceiling* is the mirror image — `pack_floor_soc` is the wrong reduction +for it, because a ceiling is the **fullest** pack and `arbitrage`'s `soc_ceiling` +takes one number. `main.py:932-936` says so, and it belongs to CFG-04/SPIKE-04's +per-pack bound. Nothing here touched it. + +## 6. What was changed, and one thing that was found on the way + +* `config.yaml` — `bms_soc_floor: int(0,50)` required per `inverters:` entry; + the flat key and its default are untouched and are documented as the + single-inverter shape's own cut-out. +* `app/inverter.py` — `Inverter.bms_soc_floor`; `_entry_bms_floor()` (`None`, not + a borrowed number and not `0.0`); `_flat_bms_floor()`. +* `app/peakguard.py` — `Pack`, `pack_floor_pct()`, `pack_permissions()`, + `binding_soc_floor()`. `discharge_permitted()` is unchanged and is still the + one place the question is answered. +* `app/validate.py` — `unit_bms_floors_indexed()`; rule 8 grades every unit and + names it via the **roster** index the value came off. +* `app/main.py` — `Controller.packs`; the `decide(soc_floor=…)` reduction; the + floor-limited notification names the pack(s) at or below their own floor, at a + roster of two or more only. +* `tools/supervisor_shape_probe.py` — **re-run, and repaired.** MUTATION.md + habit 8: a committed transcript is a check and it decays. Making the key + required broke §C outright and, worse, left §D printing **PASS** on a refusal + about the *wrong key* — its "no `max_w`" case was answered `Missing option + 'bms_soc_floor'`. §D is now an isolated counterfactual over a **merged, + accepted** baseline with exactly one key varied per case, asserting the key's + **name** in the refusal, and it carries a `bms_soc_floor` case in both + directions. Re-run against `ghcr.io/home-assistant/amd64-hassio-supervisor:2026.08.0`: + `all cases passed`. +* `app/inverter.py`, found by the mutation battery — `_flat_bms_floor()` was + written with the default in **two** places (the `.get()` fallback *and* the + blank test). Deleting either one survived, because each covered for the other: + MUTATION.md's masking section in three lines of option reading. It has one + defaulting line now, and M12 takes six checks red. + +⚠️ **No measurement of inverter B appears anywhere in this change.** It does not +physically exist. Every second-pack number in a fixture, in the probe and in +config.yaml's commented example is **simulated and labelled as such**; only the +10 % on inverter A is read off hardware. + +--- + +## 7. The probe transcript, re-run 2026-09-04 on 2026.08.0 + +Command (unchanged, in `tools/supervisor_shape_probe.py`'s own docstring): + +``` + docker run --rm -v "$PWD:/w:ro" -v "$PWD/tools:/probe:ro" \ + --entrypoint python3 \ + ghcr.io/home-assistant/amd64-hassio-supervisor:2026.08.0 \ + /probe/supervisor_shape_probe.py +``` + +It reads `config.yaml` and runs Supervisor's own `AppOptions` validator in a +throwaway container. It touches no live Supervisor, POSTs nothing and installs +nothing. + +``` +== C. AC2: a two-inverter blob an operator would POST == + PASS two inverters + [{"name": "inverter-a", "setpoint_entity": "number.goodwe_a_setpoint_w", "soc_entity": "sensor.goodwe_a_soc", "batt_entity": "sensor.goodwe_a_battery_power", "max_w": 5000, "bms_soc_floor": 10}, {"name": "inverter-b", "setpoint_entity": "number.goodwe_b_setpoint_w", "soc_entity": "sensor.goodwe_b_so + +== D. what the schema itself refuses, per entry == + PASS the per-entry BASELINE is ACCEPTED, so every refusal below has nowhere else to come from + [{"name": "inverter-b", "setpoint_entity": "number.b", "max_w": 3000, "bms_soc_floor": 10}] + PASS an entry with no name is refused, and the refusal NAMES it + Missing option 'name' in inverters in GoodWe RS485 Controller (local_goodwe_controller) + PASS an entry with no setpoint_entity is refused, and the refusal NAMES it + Missing option 'setpoint_entity' in inverters in GoodWe RS485 Controller (local_goodwe_controller) + PASS an entry with no max_w is refused, and the refusal NAMES it + Missing option 'max_w' in inverters in GoodWe RS485 Controller (local_goodwe_controller) + PASS an entry with no bms_soc_floor (SAFETY-32) is refused, and the refusal NAMES it + Missing option 'bms_soc_floor' in inverters in GoodWe RS485 Controller (local_goodwe_controller) + PASS an entry with max_w over the range is refused, and the refusal NAMES it + value must be at most 5000.0 + PASS an entry with bms_soc_floor over the range (SAFETY-32) is refused, and the refusal NAMES it + value must be at most 50.0 + PASS a JSON STRING where the list belongs is refused + Invalid list for option 'inverters' in GoodWe RS485 Controller (local_goodwe_controller) + +== E. the ROLLBACK direction, and the operator's UI == + PASS a schema WITHOUT `inverters` drops the key with a warning rather than refusing - so a downgrade is not the §B trap in reverse + ['arbitrage_enabled', 'arbitrage_soc_ceiling', 'arbitrage_soc_floor', 'auto_start', 'batt_entity', 'batt_invert'] + PASS Supervisor's UI renderer turns it into a repeatable section rather than choking - an operator can add the second inverter in the Configuration tab, which the JSON-string alternative could not offer + [{"name": "inverters", "type": "schema", "optional": true, "multiple": true, "schema": [{"name": "name", "required": true, "type": "string"}, {"name": "setpoint_entity", "required": true, "type": "string"}, {"name": "max_w", "lengthMin": 100.0, "lengthMax": 5000.0, "required": true, "type": "integer + +== all cases passed == +``` diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index a57fb5f..79a1536 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -565,7 +565,7 @@ help - see "Why the tuning is what it is". | `peak_forecast_entity` | Quarter-hour demand forecast, for capacity-tariff markets. Empty = no cap | | `peak_cap_w` | The site's capacity-tariff target | | `peak_soc_floor` (20) | **The percent below which the capacity-peak guard will not discharge the battery.** ⚠️ It bounds **the peak guard only** — it is **not a pack-wide reserve**, and ordinary self-consumption discharges straight past it (SAFETY-25, see below). Not optional and not the same thing as `maintenance_soc_floor` | -| `bms_soc_floor` (10) | The pack's **own** cut-out percent, the number the BMS enforces whatever you set above. `peak_soc_floor` must be **strictly above** it or the add-on refuses to start | +| `bms_soc_floor` (10) | The pack's **own** cut-out percent, the number the BMS enforces whatever you set above. `peak_soc_floor` must be **strictly above** it or the add-on refuses to start. ⚠️ **At a declared `inverters:` roster this key moves INTO each entry and is REQUIRED there** (SAFETY-32): it describes a battery, and two packs of different models have different cut-outs, so one number for both protects the shallower pack with the deeper pack's BMS. Nothing is inherited — an entry that omits it is refused **by name** at startup, and the flat key below is then not read at all | | `price_now_entity`, `price_avg_entity` | Dynamic tariff. Empty = never force a paid grid top-up | On a capacity-tariff site the maintenance charge is capped by the headroom left diff --git a/goodwe_controller/app/inverter.py b/goodwe_controller/app/inverter.py index 1d0babe..1a5cf73 100644 --- a/goodwe_controller/app/inverter.py +++ b/goodwe_controller/app/inverter.py @@ -15,6 +15,11 @@ The scalars, and what each one was silently asserting: SAFETY-24's pack floor and arbitrage's bounds. An average lets the emptier pack be driven below its own floor; a maximum parks the fuller one. + `bms_soc_floor` SAFETY-32. ONE cut-out percent for every pack, and it + describes A BATTERY. Two packs of different models + have different cut-outs, so the flat key graded the + shallower pack against the deeper one's BMS - wrong + in the direction that empties a pack. `batt` ONE battery power into the anti-windup saturation detector - see the SITE/UNIT note at the bottom of this docstring, because this one goes the OTHER WAY. @@ -117,6 +122,12 @@ import math from datetime import datetime from .clamp import ClampResult +# CFG-03's ONE in-code mirror of config.yaml's SAFETY-24 defaults. Imported +# rather than retyped: the flat-keys path below needs `bms_soc_floor`'s +# shipped default and a literal here would be the fourth defaults site that +# ticket exists to prevent. peakguard imports arbiter/control/tariff and none +# of those imports this module, so the direction is one-way. +from .peakguard import PEAK_OPTION_DEFAULTS from .store import unit_key from .watchdog import SignWatchdog, SignWatchdogConfig @@ -170,7 +181,7 @@ class Inverter: def __init__(self, name, *, setpoint_entity="", soc_entity="", batt_entity="", batt_invert=False, max_w=2000.0, - store=None, sign_cfg=None): + bms_soc_floor=None, store=None, sign_cfg=None): self.name = str(name) self.setpoint_entity = str(setpoint_entity or "") self.soc_entity = str(soc_entity or "") @@ -179,6 +190,19 @@ class Inverter: # The supervised cap on what may be written to THIS setpoint entity. # See the module docstring on why this is a number and not a `Tuning`. self.max_w = float(max_w) + # SAFETY-32. THIS PACK'S OWN BMS CUT-OUT, in percent, or None when the + # roster entry did not state one. + # + # ⚠️ NOT DEFAULTED HERE, AND None RATHER THAN A NUMBER. A cut-out is a + # property of the battery in the cupboard; inventing one for a pack that + # did not state it is precisely this card's defect - a pack graded + # against a floor that belongs to a different battery - rebuilt in the + # constructor. `peakguard.pack_floor_pct()` treats None as an unknown + # floor and `discharge_permitted()` treats an unknown floor as NO + # permission, so the absence costs a shave; validate.py rule 8 refuses + # the config by name long before that, at startup. + self.bms_soc_floor = (None if bms_soc_floor is None + else float(bms_soc_floor)) # --- the device's own reported range, read from its own entity ------- # None until `hass.limits()` has answered. NOT guessed at: ESPHome is @@ -339,8 +363,9 @@ def build_roster(opts: dict, store, sign_cfg=None) -> list: TWO SHAPES, AND WHICH ONE IS IN FORCE IS DECIDED BY ONE TEST: is `inverters:` empty? - EMPTY the five flat keys - `setpoint_entity`, `soc_entity`, - `batt_entity`, `batt_invert`, `max_w` - describe the ONE inverter, + EMPTY the six flat keys - `setpoint_entity`, `soc_entity`, + `batt_entity`, `batt_invert`, `max_w`, `bms_soc_floor` (SAFETY-32) + - describe the ONE inverter, exactly as the shipped scalars did. This is what every install that upgrades into this version reads, without an operator touching anything, and it is byte-identical to what DEV-02 @@ -353,8 +378,8 @@ def build_roster(opts: dict, store, sign_cfg=None) -> list: `soc_entity` has no SoC sensor; it does NOT borrow the flat one, because "this unit's number came from somewhere else" is the defect DEV-02 spent a card removing and a fallback here would - rebuild it in three lines. `name`, `setpoint_entity` and `max_w` - are required of every entry - the schema enforces it on the + rebuild it in three lines. `name`, `setpoint_entity`, `max_w` and + `bms_soc_floor` (SAFETY-32) are required of every entry - the schema enforces it on the Supervisor path, `validate.py` rule 9 enforces it for sim/inject.py and the checks, which reach this function with any dict at all. @@ -380,6 +405,12 @@ def build_roster(opts: dict, store, sign_cfg=None) -> list: batt_entity=opts.get("batt_entity", ""), batt_invert=bool(opts.get("batt_invert")), max_w=float(opts.get("max_w", 2000)), + # SAFETY-32. THE FLAT KEY, because this IS the single-inverter shape + # - the one pack the flat options describe is the one pack whose + # cut-out `bms_soc_floor` states. Defaulted from CFG-03's single + # mirror exactly as validate.py rule 8 defaults it, so an install + # that never set the key reads the same 10 % in both places. + bms_soc_floor=_flat_bms_floor(opts), store=store, sign_cfg=sign_cfg)] roster = [] @@ -401,10 +432,54 @@ def build_roster(opts: dict, store, sign_cfg=None) -> list: # ⚠️ NOT `opts["max_w"]`. See the docstring: a rating an entry did # not state is not the flat one, it is a rating rule 9 refuses. max_w=_entry_max_w(entry), + # ⚠️ NOT `opts["bms_soc_floor"]`, FOR `max_w`'S REASON AND THIS + # CARD'S. An entry that omits its cut-out has NOT got the flat one; + # it has none, and rule 8 refuses it by name. Borrowing here would be + # SAFETY-32's own defect written in three lines - the shallower pack + # graded against the deeper pack's BMS. + bms_soc_floor=_entry_bms_floor(entry), store=store, sign_cfg=sign_cfg)) return roster +def _flat_bms_floor(opts: dict): + """The single-inverter shape's own cut-out, or None if it is unparseable. + + Defaulted from `PEAK_OPTION_DEFAULTS` when the key is absent or blank, which + is `validate._number()`'s behaviour on the same key - the two have to agree + or the validator would grade a number the roster does not build. + """ + # ⚠️ ONE defaulting line, not two. The first cut wrote the fallback into the + # `.get()` AND into the blank test below it, and a mutation that deleted + # either one survived - each was covering for the other, which is + # MUTATION.md's masking section in three lines of configuration reading. + raw = opts.get("bms_soc_floor") + if raw is None or raw == "": + return float(PEAK_OPTION_DEFAULTS["bms_soc_floor"]) + try: + return float(raw) + except (TypeError, ValueError): + return None + + +def _entry_bms_floor(entry: dict): + """One entry's OWN cut-out, or None when it did not state a usable one. + + None rather than a borrowed number and rather than 0.0 - `_entry_max_w()` + can answer 0.0 because a zero rating commands nothing, but a zero FLOOR is + the most permissive value the key has and would read as "this pack may be + emptied". Rule 8 refuses the missing key by name; until it does, None + withholds. + """ + raw = entry.get("bms_soc_floor") + if raw is None or raw == "": + return None + try: + return float(raw) + except (TypeError, ValueError): + return None + + def _entry_max_w(entry: dict) -> float: """One entry's supervised cap, or 0.0 when it did not state a usable one. diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index f102b5d..c5ff410 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -1065,6 +1065,26 @@ class Controller: return None return float(min(float(v) for v in vals)) + @property + def packs(self) -> list: + """SAFETY-32. Each pack as its own THREE numbers, in roster order. + + `pack_floor_soc` above is the site's ONE verdict and stays exactly what + DEV-02 made it - the emptiest measured pack. This is the thing that + reduction cannot produce: by the time `min()` has answered, WHICH pack + the number came from and how far that pack was from ITS OWN cut-out have + both been thrown away, and two packs at 15 % against cut-outs of 10 % and + 20 % have opposite answers that are indistinguishable afterwards. + + ⚠️ EVERY unit, live or not. Same reason as `inputs_ok`: a pack whose SoC + did not answer is a pack nothing can grade, and dropping it from this + list would let the site spend the packs that DID answer while one sat + unmeasured. `peakguard.discharge_permitted()` treats a None SoC as no + permission, which is where that must be decided. + """ + return [peakguard.Pack(inv.name, inv.soc, inv.bms_soc_floor) + for inv in self.inverters] + @property def inputs_ok(self) -> bool: """The site meter, and EVERY pack's own two readings. @@ -1881,9 +1901,21 @@ class Controller: # protect. None stays None: `decide()` treats it as floor-limited, # which costs a shave rather than a pack. soc=self.pack_floor_soc, - soc_floor=float(self.o.get( + # ⚠️ SAFETY-32. THE SAME OPTION, REDUCED OVER THE PACKS' OWN + # CUT-OUTS. `peak_soc_floor` is still one site-wide EMS floor and is + # still read from that one key with CFG-03's one mirror behind it; + # what `binding_soc_floor()` adds is the conjunction with each pack's + # OWN `bms_soc_floor`, so the floor this site is graded against can + # never be below the deepest cut-out on the roster. MAXIMUM here + # pairs with the MINIMUM in `pack_floor_soc`: both reductions move + # the verdict towards refusing a shave and away from spending a pack. + # Under any config rule 8 accepts it IS the option, for every pack - + # asserted over the whole schema range in test_peakguard.py part 12 + # rather than assumed, because "another rule refuses that" is a claim + # about rule ordering and sim/inject.py does not run rule 8 at all. + soc_floor=peakguard.binding_soc_floor(self.packs, float(self.o.get( "peak_soc_floor", - peakguard.PEAK_OPTION_DEFAULTS["peak_soc_floor"])), + peakguard.PEAK_OPTION_DEFAULTS["peak_soc_floor"]))), max_hold_s=self.p1.averager.max_hold_s, # ⚠️ SAFETY-20. The device range goes in TOO, and this call site is # the one that needed it most: `standing` is `last_write`, i.e. the @@ -2596,6 +2628,22 @@ class Controller: # notification that matters into a traceback. _at_floor = ("at the floor" if g.soc_floor is None else f"at {float(g.soc_floor):.0f} %") + # ⚠️ SAFETY-32. WHICH PACK STOPPED THE SITE. `g.soc_floor` is the floor + # the decision was actually taken with, so this re-grades nothing and + # cannot disagree with the alarm it is printed inside. At a roster of one + # the sentence is omitted entirely and this notification is byte-for-byte + # what SAFETY-25 shipped - there is exactly one pack and naming it would + # be noise on every existing install. + _blocked = [name for name, why + in peakguard.pack_permissions(self.packs, g.soc_floor) + if why is not None] + _which = ("" if len(self.inverters) < 2 else + f"\n\nSAFETY-32 - WHICH PACK: this is graded per pack against " + f"that pack's own BMS cut-out (bms_soc_floor), and the " + f"pack(s) at or below their own floor right now are " + f"{', '.join(_blocked) if _blocked else '(none - see the reason above)'}" + f". The other packs on the roster are not the reason the guard " + f"stopped and raising their cut-out will not change this.") await self.hass.call("persistent_notification", "create", { "notification_id": "goodwe_peak_soc_floor", "title": "GoodWe: the capacity-peak guard has run out of battery", @@ -2615,7 +2663,7 @@ class Controller: f"{_at_floor} - ordinary self-consumption has no SoC " f"floor at all and will go on discharging past it, down to the " f"pack's own BMS cut-out. What has stopped is the guard buying " - f"you a lower bill, not the pack being spent."), + f"you a lower bill, not the pack being spent." + _which), }) async def check_circulating(self) -> None: diff --git a/goodwe_controller/app/peakguard.py b/goodwe_controller/app/peakguard.py index e8b75ed..524c4df 100644 --- a/goodwe_controller/app/peakguard.py +++ b/goodwe_controller/app/peakguard.py @@ -360,6 +360,29 @@ distinction in DOCS.md and in this comment. Revisit if a THIRD floor appears - a loop-side guard would be the third - at which point the migration buys enough to be worth its cost. +------------------------------------------------------------------------------ +SAFETY-32: ONE `bms_soc_floor` FOR TWO PACKS GRADES ONE AGAINST THE OTHER'S BMS + +`bms_soc_floor` above describes A PACK, not a site, and it was a flat option +because there was one pack. At two, two batteries of different models have +different cut-outs, and one number for both is wrong in the direction that +empties a battery: the SHALLOWER pack (the higher cut-out) is protected by a +floor that belongs to the DEEPER one. So it became a roster key - `Pack` carries +each unit's own value, `pack_floor_pct()` conjoins it with the site's EMS floor, +and validate.py rule 8 grades EVERY unit and NAMES the one it refuses. + +⚠️ `peak_soc_floor` DID NOT MOVE, AND THE RESIDUAL IS RECORDED RATHER THAN +HIDDEN. It is one number for the site, so with cut-outs of 10 % and 18 % the +operator must set it above 18 and the deeper pack is then withheld above its own +need. That half - a per-pack EMS floor - is NOT this card and must not be +smuggled in with it; DECISIONS/SAFETY-32-per-pack-bms-floor.md states it as the +open question it is. + +⚠️ AND THE MIN() OVER MEASURED SoC IS STILL RIGHT. `Controller.pack_floor_soc` +reduces to the emptiest pack and this card does not touch it. What changed is +the FLOOR it is graded against, and the addition of a per-pack answer beside the +site's one verdict - see `pack_permissions()`. + ------------------------------------------------------------------------------ AND IT READS NO PER-PHASE QUANTITY, ON PURPOSE @@ -572,6 +595,110 @@ def discharge_permitted(soc, soc_floor) -> str | None: return None +@dataclass(frozen=True) +class Pack: + """SAFETY-32. ONE pack's three numbers for the floor question, NAMED. + + `name` the roster name, because a refusal that does not say WHICH pack + stopped the site sends an operator to the wrong device - the + same defect CFG-04's `unit_caps_indexed()` was written for. + `soc` THIS pack's measured state of charge. None when its sensor did + not answer, and None is not a full battery. + `bms_floor` THIS pack's OWN cut-out percent (`bms_soc_floor`), which is a + property of the hardware in the cupboard and NOT of the site. + None when the roster entry did not state one - see + `pack_floor_pct()` for why that is withheld and not 0 %. + + ⚠️ IT CARRIES THE CUT-OUT, NOT THE EMS FLOOR. `peak_soc_floor` is one number + for the whole site (the depth a money-saving discharge may never reach) and + stays a flat option; `bms_soc_floor` describes a battery and became a roster + key here, because two packs of different models have different cut-outs and + grading both against one of them is wrong in the direction that empties the + shallower one. + """ + + name: str + soc: float | None + bms_floor: float | None + + +def pack_floor_pct(pack, soc_floor): + """The percent THIS pack may not be spent below, or None if that is unknown. + + `max(the site's EMS floor, this pack's own cut-out)` - the CONJUNCTION, and + the direction matters: the EMS floor is what the operator asked for and the + cut-out is what the hardware will do anyway, so a pack is protected by + whichever is higher. Taking the site floor alone is the shipped defect this + card removes (the shallower pack graded against the deeper one's cut-out); + taking the cut-out alone would throw SAFETY-24's margin away. + + ⚠️ AN UNSTATED CUT-OUT IS None, NOT ZERO, AND NOT THE FLAT OPTION. 0 % would + be a floor that permits everything - the most permissive value there is, + invented from an absence - and borrowing the flat `bms_soc_floor` is CFG-04's + "nothing is inherited" broken in one line, which is this card's whole subject + restated. `discharge_permitted()` already treats a None floor as NO + permission, so an entry that forgot the key costs a shave rather than a pack. + + ⚠️ AND UNDER A rule-8-VALID CONFIG THIS IS `soc_floor`, FOR EVERY PACK. + validate.py rule 8 refuses `peak_soc_floor <= bms_soc_floor` on EVERY unit, + so `max()` selects the EMS floor everywhere the add-on will actually start - + which is exactly why the reduction is written here rather than assumed at the + call site. "Another rule refuses that config" is a claim about today's rule + ordering, not a property of this function, and `validate_startup_config()` is + also `sim/inject.py`'s entry point. `unit_caps_indexed()`'s docstring records + the same argument, and CFG-04's index desynchronisation is what happened the + last time it was left implicit. + """ + if soc_floor is None or pack.bms_floor is None: + return None + return max(float(soc_floor), float(pack.bms_floor)) + + +def pack_permissions(packs, soc_floor) -> list[tuple[str, str | None]]: + """SAFETY-32. Per pack: None if it may be spent, else the reason it may not. + + ONE `(name, reason)` pair per pack, in roster order, from the SAME + `discharge_permitted()` the site verdict uses - a second transcription of + "may this pack be spent" is a second answer to it, which is the argument + that made that predicate public in the first place. + + ⚠️ THIS IS THE OBSERVABLE THE MIN() CANNOT PRODUCE. `Controller.pack_floor_soc` + reduces the measured SoC to the emptiest pack BEFORE anything is graded, and + that reduction is right for the site's one verdict - but it has thrown away + which pack it came from and how far that pack was from ITS OWN floor by the + time the answer exists. Two packs at 15 % against floors of 10 % and 20 % + have opposite answers and are indistinguishable after the `min()`. + """ + return [(p.name, discharge_permitted(p.soc, pack_floor_pct(p, soc_floor))) + for p in packs] + + +def binding_soc_floor(packs, soc_floor): + """The ONE floor `decide()` may grade `pack_floor_soc` against, at a roster. + + MAXIMUM over the packs' own floors, and it is the conservative reduction that + pairs with the MINIMUM `pack_floor_soc` reduction: `min(soc) <= max(floor)` + is implied by "some pack is at or below its own floor", so this verdict is + never more permissive than grading each pack separately would be. The other + direction is not true and is not claimed - a site can be refused here while + every pack individually has headroom, which costs a shave and never a pack. + + None when ANY pack's floor is unknown, for `Controller.site_dev_bound()`'s + reason and in the same direction: `discharge_permitted()` treats a None floor + as no permission, so an unknown floor withholds rather than defaults. + + ⚠️ AT N=1 THIS IS `soc_floor` ITSELF under any config rule 8 accepts, and + test_peakguard.py part 12 asserts that over the whole schema range rather + than assuming it. + """ + if not packs: + return None if soc_floor is None else float(soc_floor) + floors = [pack_floor_pct(p, soc_floor) for p in packs] + if any(f is None for f in floors): + return None + return max(floors) + + def decide(projection, *, batt_w, cap_w, reading, achievable_w, soc, soc_floor, max_hold_s=None, frozen: bool = False) -> PeakDecision: """The whole guard. Pure: no clock, no I/O, no state carried between calls. diff --git a/goodwe_controller/app/validate.py b/goodwe_controller/app/validate.py index 6504c9e..ba8e253 100644 --- a/goodwe_controller/app/validate.py +++ b/goodwe_controller/app/validate.py @@ -132,7 +132,7 @@ NOT READY banner already names the problem, (b) does not apply. installed base is on a fixed or static tariff. Refusing to start without a price feed would lock those sites out of the add-on entirely. - 8. `peak_soc_floor` at or below `bms_soc_floor`. [(a)] + 8. `peak_soc_floor` at or below `bms_soc_floor`, ON ANY UNIT. [(a)] SAFETY-24, and the same shape as 1, 4, 5 and 6. The peak guard can COMMAND a discharge to hold a billed quarter down (SAFETY-07), and `peak_soc_floor` is the EMS-side bound on how far down it may take the pack. The BMS has its @@ -142,6 +142,12 @@ NOT READY banner already names the problem, (b) does not apply. of binding while its status page reports one - rule 5's "a disarmed guard that advertises itself as armed", on the guard whose failure mode is a pack that is quietly never available when it is wanted. + ⚠️ SAFETY-32: `bms_soc_floor` DESCRIBES A BATTERY, so at a roster it is a + per-entry key and this rule grades EVERY unit and NAMES the one it refuses. + One number for two packs of different models refuses the wrong config and + accepts the wrong one - cut-outs of 10 % and 30 % under `peak_soc_floor: 20` + used to start. A missing per-entry cut-out is refused here rather than + defaulted: a cut-out invented for a pack IS this defect. ⚠️ `<=`, NOT `<`, and the equality case is the interesting one. At exactly equal, the EMS floor and the pack's cut-out act at the same percent, so the @@ -386,6 +392,49 @@ def unit_caps_indexed(opts: dict) -> list[tuple[int, float]]: return caps +def unit_bms_floors_indexed(opts: dict) -> list[tuple[int, object, object]]: + """SAFETY-32. Each pack's OWN `bms_soc_floor`, carrying its ROSTER INDEX. + + `(roster index, the raw value as configured, the parsed float or None)`, one + triple per unit. The raw value travels too because the refusal quotes what + the operator actually wrote, and `!r` of a re-read `opts` key is the wrong + string the moment the value came off a roster entry. + + ⚠️ THE INDEX IS THE ROSTER'S, NOT THIS LIST'S, AND THAT IS CFG-04'S BUG + WRITTEN DOWN. Rules 5 and 6 fed a CAPS-list index into `_unit_label()`, + which indexes the ROSTER, so on a roster whose first entry was malformed the + refusal named the WRONG INVERTER - an operator sent to the wrong device by + the very message that exists to stop that. `unit_caps_indexed()` exists + because of it. This function goes one step further and SKIPS NOTHING: every + entry produces a triple, including the ones with no usable floor, because a + missing cut-out is itself what rule 8 has to refuse BY NAME. There is + therefore no second list for an index to desynchronise from. + + ⚠️ AND NOTHING IS INHERITED. At a declared roster the FLAT `bms_soc_floor` is + not read at all - CFG-04's rule, and this card's subject: a pack that borrows + a sibling's cut-out is graded against a battery it is not. At the flat shape + the flat key IS this one pack's cut-out, defaulted from CFG-03's single + mirror exactly as `inverter._flat_bms_floor()` defaults it. + """ + entries = roster_entries(opts) + if not entries: + return [(0, opts.get("bms_soc_floor"), + _number(opts, "bms_soc_floor", + PEAK_OPTION_DEFAULTS["bms_soc_floor"]))] + out: list[tuple[int, object, object]] = [] + for index, entry in enumerate(entries): + if not isinstance(entry, dict): + # Rule 9(a)/(b) already refuses a non-dict entry by index, and it + # runs FIRST. Naming it twice would hand the operator two sentences + # about one line of YAML. + continue + raw = entry.get("bms_soc_floor") + out.append((index, raw, + None if raw is None or raw == "" + else _number(entry, "bms_soc_floor", None))) + return out + + def _unit_label(opts: dict, index: int) -> str: """"" at a single-inverter config, " (inverter-b)" at a roster. @@ -871,28 +920,39 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] # in-code mirror, pinned to config.yaml by test_peakguard.py, and main.py # reads the same one - exactly as maintenance.MAINT_OPTION_DEFAULTS now is # for the `maintenance_*` family. + # + # ⚠️ SAFETY-32 - GRADED ONCE PER UNIT, AND THE REFUSAL NAMES THE UNIT. One + # `bms_soc_floor` for two packs compares the EMS floor against whichever + # battery happened to be written into the flat key: a site with cut-outs of + # 10 % and 30 % and `peak_soc_floor: 20` STARTED, and the guard would then + # command a forced discharge that the second pack's BMS - not the operator's + # floor - is the thing that stops. That is the disarmed-backstop state this + # rule exists to refuse, reached through the rule itself. So the loop is over + # units, and `_unit_label()` is fed the ROSTER index the value came off (see + # `unit_bms_floors_indexed()`), because CFG-04 has just fixed the version of + # this that named the wrong inverter. soc_floor = _number(opts, "peak_soc_floor", PEAK_OPTION_DEFAULTS["peak_soc_floor"]) - bms_floor = _number(opts, "bms_soc_floor", - PEAK_OPTION_DEFAULTS["bms_soc_floor"]) - if soc_floor is None or bms_floor is None: - problems.append( - "peak_soc_floor and bms_soc_floor must both be numbers (got " - f"{opts.get('peak_soc_floor')!r} / {opts.get('bms_soc_floor')!r})") - elif soc_floor <= bms_floor: - problems.append( - f"peak_soc_floor is {soc_floor:.0f} % but the pack's own cut-out " - f"(bms_soc_floor) is {bms_floor:.0f} %, so SAFETY-07's forced " - "discharge has no EMS-side bound at all - THE BMS WINS, and it is " - "the pack's protection rather than yours. The guard would report a " - "SoC floor on its status page while being incapable of ever being " - "the thing that stops the discharge, and the symptom of that is a " - "battery repeatedly sitting at the BMS limit with no record of why " - f"- which looks exactly like the SoC drift the maintenance cycle " - f"exists to correct. Raise peak_soc_floor above {bms_floor:.0f} % " - f"(the shipped default is " - f"{PEAK_OPTION_DEFAULTS['peak_soc_floor']:.0f} %), or correct " - "bms_soc_floor if it does not match this pack.") + for _ix, _raw, bms_floor in unit_bms_floors_indexed(opts): + where = _unit_label(opts, _ix) + if soc_floor is None or bms_floor is None: + problems.append( + f"peak_soc_floor and bms_soc_floor{where} must both be numbers " + f"(got {opts.get('peak_soc_floor')!r} / {_raw!r})") + elif soc_floor <= bms_floor: + problems.append( + f"peak_soc_floor is {soc_floor:.0f} % but the pack's own cut-out " + f"(bms_soc_floor{where}) is {bms_floor:.0f} %, so SAFETY-07's forced " + "discharge has no EMS-side bound at all - THE BMS WINS, and it is " + "the pack's protection rather than yours. The guard would report a " + "SoC floor on its status page while being incapable of ever being " + "the thing that stops the discharge, and the symptom of that is a " + "battery repeatedly sitting at the BMS limit with no record of why " + f"- which looks exactly like the SoC drift the maintenance cycle " + f"exists to correct. Raise peak_soc_floor above {bms_floor:.0f} % " + f"(the shipped default is " + f"{PEAK_OPTION_DEFAULTS['peak_soc_floor']:.0f} %), or correct " + f"bms_soc_floor{where} if it does not match this pack.") # --- warnings: real, but not worth refusing over ------------------------- integ = opts.get("integrator_max_w") diff --git a/goodwe_controller/config.yaml b/goodwe_controller/config.yaml index 0e99f52..bf54354 100644 --- a/goodwe_controller/config.yaml +++ b/goodwe_controller/config.yaml @@ -56,11 +56,13 @@ options: # soc_entity: sensor.goodwe_a_soc # batt_entity: sensor.goodwe_a_battery_power # max_w: 5000 + # bms_soc_floor: 10 # - name: inverter-b # setpoint_entity: number.goodwe_b_setpoint_w # soc_entity: sensor.goodwe_b_soc # batt_entity: sensor.goodwe_b_battery_power # max_w: 4600 + # bms_soc_floor: 10 # # ⚠️ THE 4600 IS INVERTER B'S DATASHEET FIGURE, NOT A MEASUREMENT AND NOT A # PLACEHOLDER. SPIKE-04a (2026-09-04) identified B as a GoodWe GW5048D-ES, @@ -74,17 +76,38 @@ options: # what settles that once the device range is read; see goodwe/NOTES.md §1.1b, # which records both readings and picks neither. # + # ⚠️ AND THE `bms_soc_floor: 10` BESIDE IT IS FROM THE SAME PLACE - THE + # DATASHEET AND THE SERIAL, NOT A PACK ANYONE HAS READ. No second inverter + # exists on this site (SPIKE-04, ≈ 2026-09-12 for B); the cut-out must be read + # off the pack that is actually installed (register 0x0560 on the reference + # unit) before that pack is commanded. It reads 10 here because pack A reads + # 10, which makes this example DEGENERATE ON THE FLOOR AXIS: a site whose two + # packs floor at the SAME percent cannot separate `min`, `max`, first or last + # over the roster, and every SAFETY-32 fixture that DIFFERS the floors differs + # it for exactly that reason. Do not "simplify" those fixtures to match this + # example. + # # ⚠️ NOTHING IS INHERITED FROM THE FLAT KEYS BY AN ENTRY THAT OMITS IT, and # that is the whole of DEV-02 restated as a config rule. `soc`, `batt` and the # rating became per-instance precisely so that no inverter is ever graded # against a sibling's numbers; an entry silently inheriting `soc_entity` would - # rebuild that defect in YAML. `name`, `setpoint_entity` and `max_w` are - # therefore REQUIRED of every entry (the schema below enforces it, and + # rebuild that defect in YAML. `name`, `setpoint_entity`, `max_w` and + # `bms_soc_floor` are therefore REQUIRED of every entry (the schema below enforces it, and # validate.py rule 9 enforces it again for the callers the Supervisor never # sees). An omitted `soc_entity`/`batt_entity` means THIS UNIT HAS NO SUCH # SENSOR - `inputs_ok` already stops the site for that - and never means # "use the other one's". # + # ⚠️ SAFETY-32: `bms_soc_floor` IS PER ENTRY AND IS REQUIRED, NOT DEFAULTED. + # It is the percent THAT pack's own BMS cuts out at, and two batteries of + # different models do not share one. A default would be this add-on inventing + # a cut-out for a battery it has not been told about, which is the same defect + # as inheriting one: the shallower pack gets protected by a floor that belongs + # to the deeper pack, and validate.py rule 8 - which refuses a peak_soc_floor + # the BMS would win against - would be grading the wrong number. Rule 8 grades + # EVERY entry and names the one it refuses. The flat key below is unchanged and + # is the single-inverter shape's own cut-out. + # # ⚠️ AND THE FLAT `setpoint_entity` MUST THEN BE BLANK. Leaving it set while a # roster is declared is two config sites aimed at actuators, one of which # nothing writes; if it names an entity an entry also names, SAFETY-28's @@ -367,6 +390,13 @@ options: # DoD 90. It is here so validate.py can REFUSE a peak_soc_floor at or below # it: such a floor can never bind, the BMS wins, and the status page would # report an energy backstop that does not exist. + # + # ⚠️ SAFETY-32 - THIS KEY DESCRIBES ONE PACK, so it is the SINGLE-INVERTER + # shape's number and nothing more. Declare an `inverters:` roster and each + # entry states its own `bms_soc_floor`; this one is not read at all and is + # not inherited by any entry. Rule 8 then grades every unit separately, which + # is the whole of that card: one cut-out for two packs of different models + # protects the shallower pack with the deeper pack's number. bms_soc_floor: 10 # ⚠️ THE OLD PRICE PATH, AND IT IS NOT DEAD YET. These two entities are the @@ -512,6 +542,7 @@ schema: - name: str setpoint_entity: str max_w: int(100,5000) + bms_soc_floor: int(0,50) soc_entity: str? batt_entity: str? batt_invert: bool? diff --git a/goodwe_controller/test_inverter.py b/goodwe_controller/test_inverter.py index 094f8b9..1330436 100644 --- a/goodwe_controller/test_inverter.py +++ b/goodwe_controller/test_inverter.py @@ -203,6 +203,16 @@ A_ENT = {"setpoint_entity": "number.sp_a", "soc_entity": "sensor.soc_a", # realism they do not use. So: ILLUSTRATIVE, NOT B'S RATING, and not a # placeholder for it either - the placeholder era ended on 2026-09-04. B_RATING_HYPOTHESIS = 3000.0 +# SAFETY-32. Each pack's OWN BMS cut-out percent, now a REQUIRED roster key. +# ⚠️ 10 IS MEASURED (register 0x0560 on the reference GW5000S-BP and the shipped +# `bms_soc_floor` default); 15 IS SIMULATED - there is no second pack. They +# DIFFER on purpose: a fixture whose two per-unit numbers are equal cannot show +# a check that read the wrong unit's, which is MUTATION.md habit 1 and the defect +# that has now bitten four consecutive cards. Both sit below the shipped +# `peak_soc_floor` of 20, so rule 8 accepts this roster. +A_BMS_FLOOR = 10 +B_BMS_FLOOR_SIM = 15 + A_RATING = 5000.0 BASE_OPTS = dict(A_ENT, meter_source="off", meter_entity="sensor.grid", @@ -1418,13 +1428,16 @@ _TWO_OPTS.pop("batt_entity") _TWO_OPTS["inverters"] = [ dict(name="inverter-a", setpoint_entity="number.sp_a", soc_entity="sensor.soc_a", batt_entity="sensor.batt_a", - max_w=A_RATING), + max_w=A_RATING, bms_soc_floor=A_BMS_FLOOR), # ⚠️ 3000 W IS A HYPOTHESIS AND IS MARKED AS ONE WHEREVER IT APPEARS. It is # here because it DIFFERS from A's 5000 W, which is the only property any # check below uses. SPIKE-04 measures the real device. + # ⚠️ AND SO IS ITS 15 % CUT-OUT (SAFETY-32). There is no second pack to read + # register 0x0560 off; the value is SIMULATED, and it differs from A's 10 % + # for MUTATION.md habit 1's reason and no other. dict(name="inverter-b", setpoint_entity="number.sp_b", soc_entity="sensor.soc_b", batt_entity="sensor.batt_b", - max_w=B_RATING_HYPOTHESIS), + max_w=B_RATING_HYPOTHESIS, bms_soc_floor=B_BMS_FLOOR_SIM), ] _c9, _hass9 = built(_TWO_OPTS) print(f" the roster built from `inverters:` -> {[repr(i) for i in _c9.inverters]}") @@ -1500,14 +1513,19 @@ _decl9 = dict(ln.strip().lstrip("- ").split(": ", 1) for ln in _entry9.splitlines()[1:] if ": " in ln) print(f" the shipped per-entry schema: {_decl9}") check("...and the schema half declares it as a LIST OF OBJECTS whose `name`, " - "`setpoint_entity` and `max_w` are REQUIRED - no trailing `?` - so the " - "Supervisor refuses a half-written entry before the add-on sees it, and " - "an entry can never fall back on a sibling's rating because there is no " - "entry without one", - set(_decl9) == {"name", "setpoint_entity", "max_w", "soc_entity", - "batt_entity", "batt_invert"} + "`setpoint_entity`, `max_w` and `bms_soc_floor` are REQUIRED - no " + "trailing `?` - so the Supervisor refuses a half-written entry before the " + "add-on sees it, and an entry can never fall back on a sibling's rating " + "or on a sibling's BMS cut-out because there is no entry without one. " + "⚠️ SAFETY-32 ADDED THE FOURTH ONE AS REQUIRED RATHER THAN AS " + "`int(0,50)?`, and the difference is the whole of that card: an optional " + "per-pack cut-out is a DEFAULTED one, and a default is one number for " + "two batteries - the shallower pack protected by the deeper pack's BMS", + set(_decl9) == {"name", "setpoint_entity", "max_w", "bms_soc_floor", + "soc_entity", "batt_entity", "batt_invert"} and not any(_decl9[k].endswith("?") - for k in ("name", "setpoint_entity", "max_w")) + for k in ("name", "setpoint_entity", "max_w", + "bms_soc_floor")) and all(_decl9[k].endswith("?") for k in ("soc_entity", "batt_entity", "batt_invert"))) @@ -1589,9 +1607,11 @@ check("...and two inverters on two DIFFERENT entities are accepted, because " "that is the configuration this card exists for and a validator that " "fires on the legitimate case gets switched off", validate_startup_config(_opts(setpoint_entity="", inverters=[ - dict(name="inverter-a", setpoint_entity="number.sp_a", max_w=A_RATING), + dict(name="inverter-a", setpoint_entity="number.sp_a", max_w=A_RATING, + bms_soc_floor=A_BMS_FLOOR), dict(name="inverter-b", setpoint_entity="number.sp_b", - max_w=B_RATING_HYPOTHESIS)])) == []) + max_w=B_RATING_HYPOTHESIS, + bms_soc_floor=B_BMS_FLOOR_SIM)])) == []) # --- the rest of rule 9 ------------------------------------------------------- _p_flat = validate_startup_config(_opts(inverters=[ diff --git a/goodwe_controller/test_peakguard.py b/goodwe_controller/test_peakguard.py index ea80156..eaf5436 100644 --- a/goodwe_controller/test_peakguard.py +++ b/goodwe_controller/test_peakguard.py @@ -127,7 +127,7 @@ from app.arbiter import ( from app.control import Tuning, compute, peak_at_risk from app.freeze import FreezeWatchdogConfig, looks_frozen, rail_w from app import main as app_main -from app.inverter import Inverter +from app.inverter import Inverter, build_roster from app.main import Controller from app.p1 import ( PLAUSIBLE_MAX_W, P1Ingest, QUARTER_S, QuarterAverager, make_sample, @@ -136,8 +136,9 @@ from app.p1 import ( from app.peakguard import ( BLIND_COVERAGE, BLIND_FROZEN, BLIND_NO_BLOCK, CEILING_CAP, CEILING_MONTH_PEAK, FLOOR_AT_SOC_FLOOR, FLOOR_SOC_UNREADABLE, - PEAK_OPTION_DEFAULTS, PeakDecision, blindness, decide, discharge_permitted, - excess_w, quarter_ceiling_w, uncovered_s, + PEAK_OPTION_DEFAULTS, Pack, PeakDecision, binding_soc_floor, blindness, + decide, discharge_permitted, excess_w, pack_floor_pct, pack_permissions, + quarter_ceiling_w, uncovered_s, ) from app.validate import validate_startup_config from app.tariff import ( @@ -3160,6 +3161,374 @@ check("...and the roster it was restored to is the shipped one, with a healthy " c_src.units == (app_main.UNIT_A,) and c_src.live_units() == (app_main.UNIT_A,)) +# ============================================================================== +print() +print("12. SAFETY-32 - one bms_soc_floor for two packs grades one against the") +print(" other's BMS. Per-entry cut-out, per-pack permission, rule 8 per unit") + +# ⚠️ THE TWO FLOORS DIFFER, AND THAT IS THE FIRST CHECK RATHER THAN AN +# ASSUMPTION. This card's acceptance criteria say it in words because the repo +# has now shipped SIX checks whose fixture could not reach the failure it named, +# two of them - DEV-02's `_b2.max_w` and CFG-04's blank `soc_entity` - being +# EXACTLY this shape: a per-unit claim compared against a number the fixture +# happened to have twice. MUTATION.md habit 1: write the precondition as its own +# check, so a later edit that makes them equal goes RED instead of making every +# claim below quietly vacuous. +# +# ⚠️ AND ONLY ONE OF THESE NUMBERS IS MEASURED. `A_PACK_FLOOR` is config.yaml's +# shipped `bms_soc_floor` - register 0x0560 on the reference GW5000S-BP. The two +# B values are SIMULATED: there is no second pack on this site (~8 days out) and +# nothing here is a measurement of one. They are chosen for what they have to +# straddle, and that is stated on each. +A_PACK_FLOOR = BMS_FLOOR +# BELOW peak_soc_floor, so a roster carrying it is one rule 8 ACCEPTS. +B_PACK_FLOOR_SIM = SOC_FLOOR - 2.0 +# ABOVE peak_soc_floor: the shallower pack, whose own BMS would win. This is the +# configuration the flat key used to accept and this card refuses. +B_PACK_FLOOR_DEEP_SIM = SOC_FLOOR + 15.0 +# A state of charge ABOVE the site floor and BELOW the shallow pack's cut-out - +# the ONE band in which the two packs disagree. Outside it they answer the same +# and the fixture would observe nothing. +SOC_BETWEEN = SOC_FLOOR + 5.0 +print(f" floors: A {A_PACK_FLOOR:.0f} % (measured), B {B_PACK_FLOOR_SIM:.0f} % " + f"/ {B_PACK_FLOOR_DEEP_SIM:.0f} % (SIMULATED - inverter B does not exist), " + f"peak_soc_floor {SOC_FLOOR:.0f} %, probe SoC {SOC_BETWEEN:.0f} %") +check("PRECONDITION (MUTATION.md habit 1): the two per-pack floors are " + "DIFFERENT numbers and they straddle peak_soc_floor, and the probe SoC " + "sits strictly between them. Two equal floors cannot show a pack graded " + "against the wrong one, which is the defect this card is about and the " + "defect six checks in this repository have already shipped", + A_PACK_FLOOR != B_PACK_FLOOR_DEEP_SIM + and A_PACK_FLOOR < SOC_FLOOR < B_PACK_FLOOR_DEEP_SIM + and A_PACK_FLOOR < SOC_BETWEEN < B_PACK_FLOOR_DEEP_SIM + and B_PACK_FLOOR_SIM != A_PACK_FLOOR and B_PACK_FLOOR_SIM < SOC_FLOOR) + +# --- 12a. AC 1: the permission is answered PER PACK -------------------------- +# ⚠️ BOTH PACKS ARE AT THE SAME STATE OF CHARGE. That is the whole design of +# this fixture: SoC is held EQUAL so it cannot be what separates the two +# answers, and the only thing left that can is each pack's own floor. The +# shipped `min()` over SoC - which this card does NOT change - reduces these two +# readings to one number before anything is graded, so after it the two packs +# are literally indistinguishable. +_pk_a = Pack("inverter-a", SOC_BETWEEN, A_PACK_FLOOR) +_pk_b = Pack("inverter-b", SOC_BETWEEN, B_PACK_FLOOR_DEEP_SIM) +_perm = pack_permissions([_pk_a, _pk_b], SOC_FLOOR) +print(f" per-pack at the SAME {SOC_BETWEEN:.0f} %: " + + "; ".join(f"{n}: {'PERMITTED' if w is None else w}" for n, w in _perm)) +check("⚠️ AC 1. Two packs AT THE SAME STATE OF CHARGE, graded against their OWN " + "cut-outs, get OPPOSITE answers: the deeper pack is still permitted and " + "the shallower one is WITHHELD. The SoC is equal on purpose - it is the " + "floors that differ, so this line can only be green if each pack was " + "graded against its own number", + _perm == [("inverter-a", None), + ("inverter-b", FLOOR_AT_SOC_FLOOR)]) +check("...and the refusal is asserted by its CONTENT, not by the fact that one " + "happened (MUTATION.md habit 6): `not None` is satisfied by an unreadable " + "SoC, by an unconfigured floor and by the at-floor case alike, and those " + "are three different operator actions", + _perm[1][1] == FLOOR_AT_SOC_FLOOR + and _perm[1][1] != FLOOR_SOC_UNREADABLE) + +# ⚠️ THE COUNTERFACTUAL, AND IT VARIES EXACTLY ONE THING (habit 7). Same two +# packs, same SoC, same site floor - only B's cut-out moves to A's. Both are +# then permitted. Without this line, 12a would also pass for an implementation +# that refused the second entry always, or the named one, or every roster of two. +_perm_same = pack_permissions( + [_pk_a, Pack("inverter-b", SOC_BETWEEN, A_PACK_FLOOR)], SOC_FLOOR) +print(f" ...and with B's cut-out set to A's: " + + "; ".join(f"{n}: {'PERMITTED' if w is None else 'withheld'}" + for n, w in _perm_same)) +check("...and the counterfactual: give B the SAME cut-out as A and BOTH are " + "permitted. One thing varied, from an accepted baseline - so 12a's " + "difference came from the floor and not from the position, the name, or " + "the roster being two long", + [w for _, w in _perm_same] == [None, None]) + +# ⚠️ AND THE OTHER AXIS, because a per-pack grader could still read pack[0]'s +# SoC for every pack. Same floors, DIFFERENT SoC: the emptier pack is withheld. +_perm_soc = pack_permissions( + [Pack("inverter-a", SOC_FLOOR + 10.0, A_PACK_FLOOR), + Pack("inverter-b", SOC_FLOOR - 1.0, A_PACK_FLOOR)], SOC_FLOOR) +check("...and with the floors EQUAL and the two SoC readings different, it is " + "the emptier pack that is withheld - so the grader reads each pack's own " + "SoC as well as its own floor. Two mutations, two axes, and a fixture " + "that is degenerate on one of them cannot see the other", + [w for _, w in _perm_soc] == [None, FLOOR_AT_SOC_FLOOR]) + +# --- 12b. an unstated cut-out is WITHHELD, not defaulted --------------------- +# ⚠️ AT 100 % STATE OF CHARGE, which is the strongest form of the claim: a full +# battery is the one value that disarms every other rung in this file, so if the +# missing floor is what refuses THIS pack, nothing else can be doing it. +_perm_none = pack_permissions([Pack("inverter-b", 100.0, None)], SOC_FLOOR) +print(f" a pack with NO stated cut-out, at 100 %: {_perm_none[0][1]}") +check("a roster entry that states no `bms_soc_floor` is WITHHELD even at a full " + "battery - not defaulted to the flat key, not defaulted to 0 %, and not " + "quietly permitted. An invented cut-out IS this card's defect, and 0 % is " + "the most permissive value the key has", + _perm_none[0][1] is not None + and pack_floor_pct(Pack("b", 100.0, None), SOC_FLOOR) is None + and binding_soc_floor([_pk_a, Pack("b", 100.0, None)], SOC_FLOOR) is None) + +# --- 12c. AC 5: at a roster of ONE, the floor is the option, unchanged ------- +# ⚠️ SWEPT OVER THE WHOLE SCHEMA, NOT ASSERTED ON THE SHIPPED PAIR. "N=1 is +# byte-identical" is a claim about every configuration the schema accepts, and +# the shipped pair is one point in it. Every rule-8-VALID (peak, bms) pair is +# tried; the reduction must answer the option itself in all of them, or some +# install's floor moved under it. +_SW_LO, _SW_HI = int(read_schema_min("peak_soc_floor")), int(read_schema_max("peak_soc_floor")) +_BMS_HI = int(read_schema_max("bms_soc_floor")) +_n1_pairs = [(pk, bm) for pk in range(_SW_LO, _SW_HI + 1) + for bm in range(0, _BMS_HI + 1) if bm < pk] +_n1_bad = [(pk, bm) for pk, bm in _n1_pairs + if binding_soc_floor([Pack("inverter-a", 50.0, float(bm))], + float(pk)) != float(pk)] +print(f" N=1 sweep: {len(_n1_pairs)} rule-8-valid (peak_soc_floor, " + f"bms_soc_floor) pairs over the shipped schema, {len(_n1_bad)} disagree") +check("⚠️ AC 5, SWEPT. At a roster of ONE the floor the guard grades against IS " + "`peak_soc_floor`, for every pair of values the schema accepts that rule " + "8 would also accept. The reduction is new code on the path of every " + "existing install, so 'nothing changes at N=1' is measured over the whole " + "option space rather than at the shipped point", + len(_n1_pairs) > 500 and _n1_bad == []) +# ...and the case rule 8 refuses is the case the reduction is FOR, so it must +# NOT answer the option there - otherwise the max() above is dead in both +# directions and the mutation that deletes it can never bite. +_n1_deep = binding_soc_floor( + [Pack("inverter-a", 50.0, B_PACK_FLOOR_DEEP_SIM)], SOC_FLOOR) +check("...and on the pair rule 8 REFUSES - a cut-out above the EMS floor - the " + "reduction answers the CUT-OUT and not the option, which is the only " + "direction in which it does anything. Without this line the sweep above " + "would be satisfied by `soc_floor` typed straight through", + _n1_deep == B_PACK_FLOOR_DEEP_SIM and _n1_deep != SOC_FLOOR) + +# --- 12d. AC 4: pack_floor_soc's None-on-missing is untouched --------------- +_c32, _h32 = built(TMP / "s32.db") +_b32, _restore32 = second_inverter(_c32) +_c32.inverters[0].soc, _c32.inverters[1].soc = SOC_BETWEEN, SOC_FLOOR + 30.0 +check("AC 4: `pack_floor_soc` is still the MINIMUM measured SoC across the " + "roster - this card changed the FLOOR, not the reduction, and DEV-02's " + "comment is the record of why the emptiest pack is right", + _c32.pack_floor_soc == SOC_BETWEEN) +_c32.inverters[1].soc = None +check("...and it is still None - never a number - when ANY pack did not answer, " + "so a silent pack is floor-limited rather than defaulted to full. The " + "per-pack grading added beside it must not have turned an absence into a " + "reading", + _c32.pack_floor_soc is None + and pack_permissions(_c32.packs, SOC_FLOOR)[1][1] == FLOOR_SOC_UNREADABLE) +_c32.inverters[1].soc = SOC_FLOOR + 30.0 +# ⚠️ ASSERTED AGAINST LITERALS THAT DIFFER ON ALL THREE AXES (habit 2). Compared +# against `[(inv.name, inv.soc, inv.bms_soc_floor) for inv in ...]` this check +# could not tell "each unit's own three numbers" from "the first unit's three, +# twice"; the tuples below are written out, and no two of them share a value. +_c32.inverters[0].bms_soc_floor = A_PACK_FLOOR +_c32.inverters[1].bms_soc_floor = B_PACK_FLOOR_SIM +print(f" Controller.packs -> {[(p.name, p.soc, p.bms_floor) for p in _c32.packs]}") +check("...and `Controller.packs` carries EACH unit's OWN name, SoC and cut-out " + "- not the first unit's repeated, which is DEV-02's `self.inverters[0]` " + "defect and the one that left all 19 files green", + [(pk.name, pk.soc, pk.bms_floor) for pk in _c32.packs] + == [("inverter-a", SOC_BETWEEN, A_PACK_FLOOR), + ("inverter-b", SOC_FLOOR + 30.0, B_PACK_FLOOR_SIM)] + and A_PACK_FLOOR != B_PACK_FLOOR_SIM and SOC_BETWEEN != SOC_FLOOR + 30.0) +_restore32() + +# --- 12e. rule 8 grades EVERY unit and NAMES it ------------------------------ +def _roster32(a_floor, b_floor, **kw): + """A two-entry roster, otherwise-startable, differing only in the floors.""" + entries = [] + for nm, sp, fl in (("inverter-a", "number.sp_a", a_floor), + ("inverter-b", "number.sp_b", b_floor)): + e = {"name": nm, "setpoint_entity": sp, "max_w": 2000} + if fl is not None: + e["bms_soc_floor"] = fl + entries.append(e) + return opts24(setpoint_entity="", inverters=entries, **kw) + + +check("the two-entry fixture with BOTH cut-outs under the EMS floor is " + "ACCEPTED, so every refusal below has nowhere else to come from " + "(MUTATION.md habit 7)", + validate_startup_config(_roster32(A_PACK_FLOOR, B_PACK_FLOOR_SIM)) == []) +# ⚠️ THE OFFENDER IS THE SECOND ENTRY. CFG-04 has just fixed a rule 5/6 bug of +# exactly this shape - a caps-list index fed to a function that indexes the +# ROSTER, so the refusal named the WRONG INVERTER - and an index hardwired to 0, +# or a loop that grades only the first entry, passes a fixture whose offender is +# first. So it is second here, and first in the mirror below. +_r_second = rule8(validate_startup_config( + _roster32(A_PACK_FLOOR, B_PACK_FLOOR_DEEP_SIM))) +# ⚠️ INDEXED THROUGH `_msg()`, NOT `[0]`. The mutation this pair exists to catch +# - rule 8 grading only the first unit - makes this list EMPTY, and a bare `[0]` +# turns the red into an IndexError traceback that kills the file before the +# remaining checks report. A mutation battery wants NAMED reds, not a stack. +def _msg(problems, i=0): + return problems[i] if len(problems) > i else "" + + +print(f" offender SECOND -> {len(_r_second)} refusal(s): " + f"{_msg(_r_second)[:150] or '(none)'}...") +check("⚠️ AC 3. Rule 8 grades EVERY unit: a roster whose SECOND pack has a " + "cut-out above `peak_soc_floor` is refused, ONCE, and the refusal NAMES " + "inverter-b. Under the flat key this config STARTED, and the guard would " + "then have commanded a forced discharge that pack B's BMS - not the " + "operator's floor - is the thing that stops", + len(_r_second) == 1 and "(inverter-b)" in _msg(_r_second) + and "(inverter-a)" not in _msg(_r_second)) +check("...and it names BOTH numbers and the shipped default, so the operator " + "can act on it without reading the source", + f"{B_PACK_FLOOR_DEEP_SIM:.0f} %" in _msg(_r_second) + and f"{SOC_FLOOR:.0f} %" in _msg(_r_second) + and f"{PEAK_OPTION_DEFAULTS['peak_soc_floor']:.0f} %" in _msg(_r_second)) +# The mirror. A grader hardwired to index 1 - or one that reports the LAST +# offender - passes the check above and fails this one. +_r_first = rule8(validate_startup_config( + _roster32(B_PACK_FLOOR_DEEP_SIM, A_PACK_FLOOR))) +check("...and with the SAME two numbers in the OTHER ORDER it names " + "inverter-a instead. The pair of checks is what makes this an index " + "check: one of them goes red for any hardwired position", + len(_r_first) == 1 and "(inverter-a)" in _msg(_r_first) + and "(inverter-b)" not in _msg(_r_first)) +# And both at once, because a loop that `break`s on the first offender reports +# one refusal for a config with two. +_r_both = rule8(validate_startup_config( + _roster32(B_PACK_FLOOR_DEEP_SIM, B_PACK_FLOOR_DEEP_SIM + 1.0))) +check("...and a roster where BOTH packs offend is refused TWICE, once per unit " + "and each naming its own - so an operator with two wrong packs is not " + "told about one of them and left to discover the other on the next boot", + len(_r_both) == 2 and "(inverter-a)" in _msg(_r_both, 0) + and "(inverter-b)" in _msg(_r_both, 1)) +# A missing per-entry cut-out is refused BY NAME rather than defaulted. +_r_missing = rule8(validate_startup_config(_roster32(A_PACK_FLOOR, None))) +print(f" entry with no bms_soc_floor -> {_msg(_r_missing)[:120] or '(none)'}...") +check("...and an entry that states NO `bms_soc_floor` is refused by name rather " + "than defaulted. THE DECISION THIS CARD RECORDS: a per-entry default is a " + "cut-out this add-on invented for a battery it has not been told about, " + "which is the same wrong number the flat key produced. See " + "DECISIONS/SAFETY-32-per-pack-bms-floor.md", + len(_r_missing) == 1 and "(inverter-b)" in _msg(_r_missing) + and "must both be numbers" in _msg(_r_missing)) + +# --- 12f. nothing is inherited, with the flat key SET AND DIFFERENT ---------- +# ⚠️ THE FLAT KEY IS A REAL, DIFFERENT NUMBER HERE, AND THAT IS THE CHECK. +# CFG-04 wrote this exact claim against a fixture whose flat `soc_entity` was +# BLANK, so inherited and not-inherited were the same empty string and the +# mutation that restores the fallback SURVIVED. 30 % is nothing else in this +# file, so an entry that came back holding it can only have borrowed it. +_FLAT_ODD = 30.0 +_opts_inh = {"bms_soc_floor": _FLAT_ODD, "setpoint_entity": "", + "inverters": [{"name": "inverter-a", "setpoint_entity": "number.sp_a", + "max_w": 2000, "bms_soc_floor": A_PACK_FLOOR}, + {"name": "inverter-b", "setpoint_entity": "number.sp_b", + "max_w": 2000}]} +_r_inh = build_roster(_opts_inh, _Store()) +print(f" flat bms_soc_floor {_opts_inh['bms_soc_floor']:.0f} %, roster carries " + f"{[i.bms_soc_floor for i in _r_inh]}") +check("an entry that omits `bms_soc_floor` has NO cut-out and does NOT borrow " + "the flat one - which is set to a real, DIFFERENT number right here, so " + "inherited and not-inherited are visibly different answers. A pack graded " + "against a floor that belongs to another battery is this whole card", + _r_inh[1].bms_soc_floor is None + and _r_inh[0].bms_soc_floor == A_PACK_FLOOR + and _FLAT_ODD not in [i.bms_soc_floor for i in _r_inh]) +# ...and at the FLAT shape the same key IS this one pack's cut-out. +_r_flat = build_roster({"setpoint_entity": "number.sp", + "bms_soc_floor": _FLAT_ODD}, _Store()) +check("...while at the single-inverter shape the flat key IS that one pack's " + "cut-out, because there the flat keys describe the one inverter. The two " + "readings of the same key are the two shapes, not a fallback", + len(_r_flat) == 1 and _r_flat[0].bms_soc_floor == _FLAT_ODD) +_r_dflt = build_roster({"setpoint_entity": "number.sp"}, _Store()) +check("...and with the key absent entirely the one pack takes the SHIPPED " + "default out of the one mirror - so an install that never set it reads " + "the same number the validator grades it against, and CFG-03 gains no " + "fourth defaults site", + _r_dflt[0].bms_soc_floor == BMS_FLOOR + and BMS_FLOOR == PEAK_OPTION_DEFAULTS["bms_soc_floor"]) + +# --- 12g. the call site: main.py hands decide() the per-pack reduction ------- +_floor_call = ast.unparse(next(k.value for k in _call.keywords + if k.arg == "soc_floor")) +print(f" main.py's decide(soc_floor=...) is `{_floor_call}`") +check("main.py hands `decide()` the PER-PACK reduction of `peak_soc_floor`, " + "not the bare option: `binding_soc_floor(self.packs, ...)`, so the floor " + "the site is graded against can never sit below the deepest cut-out on " + "the roster. The key and the one defaults mirror are still in that " + "expression - graded three parts above - because this card moved the " + "REDUCTION and not the source", + "binding_soc_floor" in _floor_call and "self.packs" in _floor_call + and '"peak_soc_floor"' in _floor_call.replace("'", '"')) + +# --- 12h. the operator-facing half, driven through the shipped method ------- +# ⚠️ RUN, NOT GREPPED. CFG-04's review found a substring test over main.py that +# the defeating mutation walked straight through, so this drives the shipped +# `check_peak_floor_limited()` on a real two-pack Controller and reads what the +# operator's notification actually says. +async def _notify32(packs_socs, floors): + ctl, hs = built(TMP / f"s32n{len(packs_socs)}{floors[-1]:.0f}.db") + inv_b, _ = second_inverter(ctl) + for inv, so, fl in zip(ctl.inverters, packs_socs, floors): + inv.soc, inv.bms_soc_floor = so, fl + ctl.peak_guard = PeakDecision( + engaged=True, floor_w=0.0, ceiling_w=CAP_W, ceiling_from=CEILING_CAP, + projected_w=CAP_W + 500.0, excess_w=500.0, wanted_w=500.0, + floor_limited=True, soc=min(packs_socs), + soc_floor=binding_soc_floor(ctl.packs, SOC_FLOOR), + soc_floor_reason=FLOOR_AT_SOC_FLOOR, reason="PEAK GUARD FLOOR-LIMITED: x") + ctl._peak_floor_limited = False + await ctl.check_peak_floor_limited() + return hs.last_message("goodwe_peak_soc_floor") + + +_body32 = asyncio.get_event_loop_policy().new_event_loop().run_until_complete( + _notify32([SOC_FLOOR + 30.0, SOC_FLOOR - 1.0], [A_PACK_FLOOR, A_PACK_FLOOR])) +print(f" two-pack notification tail: ...{_body32[-200:]}") +check("the notification an operator reads at 19:00 NAMES THE PACK that stopped " + "the site, and names only that one. 'the battery is at its floor' on a " + "site with two batteries is an instruction to go and look at both, and " + "the wrong one is the one that is fine", + "SAFETY-32" in _body32 and "inverter-b" in _body32 + and "inverter-a" not in _body32.split("WHICH PACK")[-1]) +# The mirror again, on the SoC axis: swap which pack is empty and the name +# swaps. A hardwired `self.inverters[1].name` passes the line above. +_body32b = asyncio.get_event_loop_policy().new_event_loop().run_until_complete( + _notify32([SOC_FLOOR - 1.0, SOC_FLOOR + 30.0], [A_PACK_FLOOR, A_PACK_FLOOR])) +check("...and with the SAME two readings swapped between the packs it names " + "inverter-a instead - so the sentence follows the measurement rather " + "than a position in the roster", + "inverter-a" in _body32b + and "inverter-b" not in _body32b.split("WHICH PACK")[-1]) + + +# ⚠️ AC 5, THE OTHER HALF: THE SAME METHOD ON A ROSTER OF ONE MUST PRINT WHAT IT +# ALWAYS PRINTED. This card adds a sentence to an alarm every existing install +# already receives, so "byte-identical at N=1" is asserted against the shipped +# text rather than assumed - and against the text of the very body the two-pack +# run above produced, minus its added sentence. +async def _notify32_one(): + ctl, hs = built(TMP / "s32n1.db") + ctl.inverters[0].soc = SOC_FLOOR - 1.0 + ctl.peak_guard = PeakDecision( + engaged=True, floor_w=0.0, ceiling_w=CAP_W, ceiling_from=CEILING_CAP, + projected_w=CAP_W + 500.0, excess_w=500.0, wanted_w=500.0, + floor_limited=True, soc=SOC_FLOOR - 1.0, + soc_floor=binding_soc_floor(ctl.packs, SOC_FLOOR), + soc_floor_reason=FLOOR_AT_SOC_FLOOR, reason="PEAK GUARD FLOOR-LIMITED: x") + ctl._peak_floor_limited = False + await ctl.check_peak_floor_limited() + return hs.last_message("goodwe_peak_soc_floor") + + +_body32one = asyncio.get_event_loop_policy().new_event_loop().run_until_complete( + _notify32_one()) +check("⚠️ AC 5. At a roster of ONE the same notification carries NO per-pack " + "sentence at all - byte for byte the message SAFETY-25 shipped. There is " + "one battery, naming it is noise, and this alarm reaches every existing " + "install", + "SAFETY-32" not in _body32one and "WHICH PACK" not in _body32one + and _body32one == _body32.split("\n\nSAFETY-32")[0] + and "peak_soc_floor bounds THE PEAK GUARD ONLY" in _body32one) + + # ============================================================================== print() print("11. mutation: what defeats these guards, and which check bites") @@ -3298,6 +3667,78 @@ MUT.append(("the grid-following loop discharging past this floor", f"pins the claim-source set, so building the floor takes this " f"part RED instead of leaving this row quietly stale")) +# ---- SAFETY-32's own defeats ------------------------------------------------- +# ⚠️ RE-IMPLEMENTED IN PROCESS, exactly as SAFETY-07's B1/B2/B3 are: no source +# file is edited, so no `.pyc` is consulted and MUTATION.md's stale-bytecode +# defect cannot reach these rows. The SOURCE-EDIT battery is on the PR, run +# through `tools/mutation_run.sh`. + +# 14. `build_roster()` letting an entry inherit the flat `bms_soc_floor` - this +# card's own defect, rebuilt in three lines, and CFG-04's exact mutation. +MUT.append(("`build_roster()` inheriting the flat `bms_soc_floor` for an entry " + "that omits it", + _r_inh[1].bms_soc_floor is None and _FLAT_ODD != A_PACK_FLOOR, + "part 12f: 'an entry that omits bms_soc_floor has NO cut-out'. ⚠️ " + "The flat key is set to 30 %, a number nothing else in this file " + "has, precisely so inherited and not-inherited are different " + "answers - CFG-04 shipped this claim against a BLANK flat key and " + "watched the mutation survive")) + +# 15. the permission graded against the site's ONE floor, i.e. the shipped +# behaviour this card removes. Re-derived on 12a's own two packs. +_mut15 = [discharge_permitted(p.soc, SOC_FLOOR) for p in (_pk_a, _pk_b)] +MUT.append(("the permission graded against the ONE `peak_soc_floor` instead of " + "each pack's own cut-out - the shipped behaviour", + _mut15 != [w for _, w in _perm], + "part 12a: 'two packs AT THE SAME STATE OF CHARGE get OPPOSITE " + "answers'")) + +# 16. every pack graded against packs[0]'s floor - DEV-02's `self.inverters[0]` +# defect and CFG-04's index borrow, in this card's costume. +_mut16 = [discharge_permitted(p.soc, pack_floor_pct(_pk_a, SOC_FLOOR)) + for p in (_pk_a, _pk_b)] +MUT.append(("every pack graded against the FIRST pack's floor", + _mut16 != [w for _, w in _perm], + "part 12a, and it is the fixture that makes it visible: the two " + "floors differ, so borrowing pack A's answers a different question")) + +# 17. `binding_soc_floor()` returning the bare option - the `max()` dropped. +MUT.append(("`binding_soc_floor()` returning `soc_floor` unreduced", + _n1_deep != SOC_FLOOR and _n1_deep == B_PACK_FLOOR_DEEP_SIM, + "part 12c: 'on the pair rule 8 REFUSES the reduction answers the " + "CUT-OUT and not the option'. ⚠️ THE N=1 SWEEP ALONE CANNOT SEE " + "THIS - it is 1775 pairs on which the two agree, which is the " + "point of pairing it with a case on which they must not")) + +# 18. an unstated per-entry cut-out defaulted to 0 % rather than withheld - the +# most permissive value the key has, invented from an absence. +MUT.append(("an unstated per-entry cut-out defaulted to 0 %", + pack_floor_pct(Pack("b", 100.0, None), SOC_FLOOR) is None + and discharge_permitted(100.0, 0.0) is None, + "part 12b: 'a roster entry that states no bms_soc_floor is " + "WITHHELD even at a full battery'")) + +# 19. rule 8 grading only the first unit, i.e. the flat rule left in place. +MUT.append(("rule 8 grading only the FIRST unit", + _r_second != [] and "(inverter-b)" in _msg(_r_second), + "part 12e: 'a roster whose SECOND pack has a cut-out above " + "peak_soc_floor is refused'. ⚠️ The offender is the SECOND entry " + "for exactly this row")) + +# 20. rule 8 naming a hardwired unit - CFG-04's rule 5/6 bug, which is why +# `unit_bms_floors_indexed()` carries the roster index. +MUT.append(("rule 8's refusal naming a hardwired inverter", + "(inverter-b)" in _msg(_r_second) and "(inverter-a)" in _msg(_r_first), + "part 12e: the two orders. Either hardwired index passes one of " + "them and fails the other")) + +# 21. the notification naming a fixed roster position rather than the pack that +# is actually at its floor. +MUT.append(("the per-pack notification naming a fixed roster position", + "inverter-a" in _body32b + and "inverter-b" not in _body32b.split("WHICH PACK")[-1], + "part 12h: the same two readings swapped between the packs")) + for label, caught, where in MUT: print(f" {'caught' if caught else 'NOT CAUGHT':>10} {label}") print(f" {where}") @@ -3312,6 +3753,12 @@ check("...and the seventh is honestly reported as not caught, because it cannot "same zero into FULL freedom, which part 3 shows is why headroom_w() is " "not consumed here", not MUT[6][1] and quarter_ceiling_w(CAP_W, _zero_peak)[0] == CAP_W) +check("...and SAFETY-32's own eight defeats are each caught too - the flat " + "cut-out inherited, the site floor graded in place of the pack's, the " + "first pack's floor borrowed for all, the reduction dropped, an absent " + "cut-out defaulted to 0 %, rule 8 grading one unit, rule 8 naming a " + "hardwired one, and the alarm naming a position instead of a pack", + all(m[1] for m in MUT[13:21]) and len(MUT) == 21) check("...and so is the last one, which is not a defeat at all but the EDGE OF " "THIS CARD: the grid-following loop can still take the pack past this " "floor, because the floor bounds the peak guard's own forced discharge " diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index 8565bbf..2be9b3a 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -204,12 +204,17 @@ check("...and the refusal names the actuator and every writer aimed at it, so " _PER_INVERTER_OK = {k: v for k, v in base().items() if k != "setpoint_entity"} _PER_INVERTER_OK["inverters"] = [ {"name": "inverter-a", "setpoint_entity": "number.goodwe_setpoint", - "max_w": 2000}, + "max_w": 2000, "bms_soc_floor": 10}, # ⚠️ INVERTER B DOES NOT EXIST. 2000 W here is the SHIPPED default repeated, # not a rating measured off a second device - SPIKE-04 measures that when # there is one to measure. Nothing in this file depends on the number. + # ⚠️ SAFETY-32 made `bms_soc_floor` a REQUIRED per-entry key, and the two + # values here DIFFER (10 / 15) rather than being copied. 15 % is SIMULATED - + # there is no second pack to read a cut-out off - and the reason it is not + # simply 10 is MUTATION.md habit 1: a fixture whose two per-unit numbers are + # equal cannot show a check reading the wrong unit's. {"name": "inverter-b", "setpoint_entity": "number.goodwe_setpoint_b", - "max_w": 2000}] + "max_w": 2000, "bms_soc_floor": 15}] check("two inverters with their own entities are ACCEPTED - one writer each, " "no shared actuator, and that is CFG-04's whole purpose", validate_startup_config(_PER_INVERTER_OK, -5000, 5000) == []) diff --git a/goodwe_controller/tools/supervisor_shape_probe.py b/goodwe_controller/tools/supervisor_shape_probe.py index 713d65b..9050e8f 100644 --- a/goodwe_controller/tools/supervisor_shape_probe.py +++ b/goodwe_controller/tools/supervisor_shape_probe.py @@ -40,8 +40,15 @@ DECISIONS/CFG-04-inverter-roster.md: reconstruction happens to lack FIRST, which is an artefact of the reconstruction and NOT evidence about `inverters`. B is that evidence. C. A two-inverter blob validates, per-entry types enforced. - D. An entry missing `name`, `setpoint_entity` or `max_w` is refused BY NAME, - and a JSON string where the list belongs is refused outright. + D. An entry missing `name`, `setpoint_entity`, `max_w` or `bms_soc_floor` is + refused BY NAME - one key varied from an ACCEPTED baseline, and the key's + own name asserted in the refusal - and a JSON string where the list belongs + is refused outright. + +⚠️ RE-RUN 2026-09-04 FOR SAFETY-32, which made `bms_soc_floor` a REQUIRED +per-entry key. That change broke §C and, worse, left §D printing PASS on a +refusal about the WRONG KEY - the exact decay MUTATION.md habit 8 describes. §C +now carries the key and §D is an isolated counterfactual asserting the key name. """ import sys, json, copy sys.path.insert(0, "/usr/src/supervisor") @@ -167,13 +174,25 @@ print("\n== C. AC2: a two-inverter blob an operator would POST ==") # the 5000/3000 it replaces - fine here, where nothing computes on the numbers, # and NOT fine in the test fixtures, which is why those keep a wide illustrative # pair instead of the real one. +# +# ⚠️ AND `bms_soc_floor` IS A REQUIRED PER-ENTRY KEY SINCE SAFETY-32, so this +# blob is also what proves an operator can still POST a two-inverter config +# after that. THE TWO FLOORS DIFFER ON PURPOSE (10 and 15): equal values would +# not exercise a per-entry number at all, for the same reason the two ratings +# must stay different. +# ⚠️ NEITHER OF B'S NUMBERS IS A MEASUREMENT OF A PACK. Only inverter A's 10 % +# is read off hardware (register 0x0560 on the reference GW5000S-BP). B's 4600 W +# is its DATASHEET figure and B's 15 % is an illustrative value chosen to differ +# - there is no second pack on this site, and in production BOTH packs are +# expected to floor at 10 %, which is why the differing pair lives here and in +# the fixtures rather than in config.yaml's example. TWO = dict(LIVE, setpoint_entity="", inverters=[ {"name": "inverter-a", "setpoint_entity": "number.goodwe_a_setpoint_w", "soc_entity": "sensor.goodwe_a_soc", "batt_entity": "sensor.goodwe_a_battery_power", - "max_w": 5000}, + "max_w": 5000, "bms_soc_floor": 10}, {"name": "inverter-b", "setpoint_entity": "number.goodwe_b_setpoint_w", "soc_entity": "sensor.goodwe_b_soc", "batt_entity": "sensor.goodwe_b_battery_power", - "max_w": 4600}]) + "max_w": 4600, "bms_soc_floor": 15}]) ok, out = validate(MERGER.merge(copy.deepcopy(defaults), copy.deepcopy(TWO))) show("two inverters", ok, json.dumps(out.get("inverters")) if ok else out) # The validated blob, for anyone who wants to POST it. Optional: the probe's @@ -184,17 +203,38 @@ except OSError: pass print("\n== D. what the schema itself refuses, per entry ==") -# The rating here is filler - each entry is refused for a MISSING key, so no -# verdict below depends on the number. It carries B's datasheet 4600 W anyway, -# so that nothing in this file reads as 3000 W being inverter B's rating. -for title, entry in ( - ("no name", {"setpoint_entity": "number.b", "max_w": 4600}), - ("no setpoint_entity", {"name": "inverter-b", "max_w": 4600}), - ("no max_w", {"name": "inverter-b", "setpoint_entity": "number.b"}), - ("max_w over the range", {"name": "inverter-b", "setpoint_entity": "number.b", "max_w": 9999})): - ok, out = validate(dict(TWO, inverters=[entry])) - show(f"an entry with {title} is refused", not ok, out) -ok, out = validate(dict(TWO, inverters="[]")) +# ⚠️ ONE KEY VARIED FROM AN ACCEPTED BASELINE, AND THE REFUSAL IS ASSERTED BY +# NAME - §B's two lessons, applied here because SAFETY-32 is what broke this +# section. These entries used to omit several keys at once and assert a bare +# `not ok`; the moment `bms_soc_floor` became required, the "no max_w" case went +# on printing PASS while the refusal it printed said `Missing option +# 'bms_soc_floor'`. A refusal of unknown provenance is not a measurement of +# anything, so the baseline below is COMPLETE and each case deletes exactly one +# key - or, for the range case, moves exactly one value out of range. +D_FULL = {"name": "inverter-b", "setpoint_entity": "number.b", "max_w": 4600, + "bms_soc_floor": 10} +# MERGED with the shipped defaults, exactly as §C is and as App.options is: the +# raw LIVE blob is missing root keys unrelated to this section (`maintenance_ +# start_hour` on the run that caught it), and a baseline refused for one of THOSE +# is the provenance problem §B2 is kept as a warning about. +D_BASE = MERGER.merge(copy.deepcopy(defaults), copy.deepcopy(TWO)) +ok, out = validate(dict(D_BASE, inverters=[D_FULL])) +show("the per-entry BASELINE is ACCEPTED, so every refusal below has nowhere " + "else to come from", ok, json.dumps(out.get("inverters")) if ok else out) +for title, entry, expect in ( + ("no name", {k: v for k, v in D_FULL.items() if k != "name"}, "name"), + ("no setpoint_entity", + {k: v for k, v in D_FULL.items() if k != "setpoint_entity"}, "setpoint_entity"), + ("no max_w", {k: v for k, v in D_FULL.items() if k != "max_w"}, "max_w"), + ("no bms_soc_floor (SAFETY-32)", + {k: v for k, v in D_FULL.items() if k != "bms_soc_floor"}, "bms_soc_floor"), + ("max_w over the range", dict(D_FULL, max_w=9999), "at most 5000"), + ("bms_soc_floor over the range (SAFETY-32)", + dict(D_FULL, bms_soc_floor=99), "at most 50")): + ok, out = validate(dict(D_BASE, inverters=[entry])) + show(f"an entry with {title} is refused, and the refusal NAMES it", + (not ok) and expect in str(out), out) +ok, out = validate(dict(D_BASE, inverters="[]")) show("a JSON STRING where the list belongs is refused", not ok, out)