SAFETY-32: bms_soc_floor per pack, rule 8 per unit
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
This commit is contained in:
committed by
glenn schrooyen
glenn schrooyen
co-authored by
Claude Opus 5
Claude Opus 5
parent
3934ad7ea5
commit
041ec0a61b
@@ -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 ==
|
||||
```
|
||||
Reference in New Issue
Block a user