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)