diff --git a/goodwe_controller/DECISIONS/SAFETY-42-unit-delivery.md b/goodwe_controller/DECISIONS/SAFETY-42-unit-delivery.md index 8e00e57..6c5e23b 100644 --- a/goodwe_controller/DECISIONS/SAFETY-42-unit-delivery.md +++ b/goodwe_controller/DECISIONS/SAFETY-42-unit-delivery.md @@ -425,3 +425,81 @@ it is working. ## 8. Mutation ledger +Every run on a **freshly copied tree of its own, `__pycache__` purged and the +count printed, mounted `:ro`, `python3 -B`** — MUTATION.md's rule, followed +because the first SAFETY-30 battery reused one directory, a container mounted it +mid-copy, read a missing `config.yaml` as a failing check, and the whole run was +discarded. **The baseline is green** (23/23) or nothing below is a result. Byte +deltas measured, not estimated; the `+0 B` ones are marked, since a +size-preserving edit is what TEST-15 turned into a false survivor once already. + +| # | file | mutation | Δ | result | +|---|---|---|---|---| +| M1 | `delivery.py` | the SoC account removed — a cold pack at its floor alarms | −31 B | **RED** — `test_delivery.py`. This is the card | +| M2 | `delivery.py` | the proper-subset rule removed — a whole-site cause names every unit | −34 B | **RED** | +| **M3** | `delivery.py` | **the `len(asked) < 2` fast path relaxed to `< 1`** | **+0 B** | **SURVIVOR — and an EQUIVALENT MUTANT.** See below | +| M4 | `delivery.py` | the strict bound relaxed to `>=` — a gap the control law calls tracking is named | +1 B | **RED** | +| **M5** | `delivery.py` | `share <= 0` relaxed to `share == 0` — charge shares graded | **+0 B** | **SURVIVOR on the first battery — GAP CLOSED**, then **RED**. See below | +| M6 | `delivery.py` | `delivery_hold_s` 185 → a round 300 | **+0 B** | **RED** — the CFG-03 pin | +| M7 | `main.py` | the watts threshold no longer `Tuning.saturation_w` (×4) | +6 B | **RED** | +| M8 | `main.py` | the latch never persisted — nothing survives a restart | −35 B | **RED** | +| M9 | `main.py` | `_delivery_announced` never set — one notification per cycle | −34 B | **RED** | +| M10 | `main.py` | `peakguard.pack_floor_pct()`'s conjunction with `peak_soc_floor` used instead of the pack's own cut-out | +151 B | **RED** — the between-the-floors fixture | +| **M11** | `main.py` | **`inv.is_live(now)[0]` replaced by a literal `True`** | −15 B | **SURVIVOR at review — GAP CLOSED**, then **RED**. See below | +| M12 | `inverter.py` | the latch not restored from the store on start-up | −46 B | **RED** | + +**11 of 12 red. One survivor, argued rather than left.** + +### M5 was a real gap, and the two obvious checks did not close it + +`test_delivery.py` already asserted `short_by(-2000, 0) is None` and +`short_by(-2000, -100) is None` — and **both stayed green under the mutant**. +For a charge share that is *under*-delivered, `share − measured` is negative and +the `gap > 0` test answers `None` anyway, so those two inputs cannot separate +`share <= 0` from `share == 0`. The one input that can is a unit moving **more** +than its charge share — `-2000 W` told, `-3000 W` measured, `share − measured` = +`+1000 W` — which the mutant reports as a shortfall. It is not one; it is +something else driving that pack, which is SAFETY-30's and SAFETY-31's question. +The check now uses that input and the mutant is red. + +This is MUTATION.md's third failure mode in miniature: **the assertion ran, the +sentence was true, and the data could not reach the failure it named.** + +### M11 was a wiring gap the pure checks structurally could not see + +`account(live=False, …)` was already graded — as a **function**. What was not +graded was that `cycle()` actually *asks* `inv.is_live(now)`, and replacing that +call with a literal `True` left every check green, because **no fixture had a +unit whose writes were being rejected**. A dropped unit still has its last +*accepted* command standing on the wire (`write_setpoint` leaves `last_write` +alone on a rejected write) and still measures nothing, so it looks exactly like a +wedged one to every number this check reads — and reporting it would be the +second, louder alarm about a state already being handled that this module takes +its 30 s floor from. `test_delivery.py` part 6 now builds that unit (with +`rejected_since` back-dated past `LINK_STALE_S`, because the fixture runs in two +seconds and the timer is `test_inverter.py`'s to grade) and the mutant is red. + +### M3 is recorded as a survivor because it is one — and it is EQUIVALENT + +`len(asked) < 2` → `len(asked) < 1`. With **one** asked unit, `unaccounted` is a +subset of a one-element set, so it is either empty — `not unaccounted` returns +`{}` — or equal to `asked` — `len(unaccounted) == len(asked)` returns `{}`. +There is no third case, and with **zero** asked units both spellings return `{}` +as well. **No input separates them**, so no check can, and one written to try +would be a check about nothing. + +⚠️ **And finding it corrected the module's own docstring, which is the more +useful half.** That paragraph claimed `len(asked) < 2` was *what makes N=1 +inert*. It is not: the **proper-subset rule** is, and the line is a fast path and +a signpost. A false attribution of the mechanism in the file's own prose is +exactly what this repository keeps finding, and a mutation that survives is how +it surfaced. The line stays — it tells a reader at a glance that a one-inverter +install never reaches any of this — with the equivalence written down beside it, +the way `circulation.circulating_w()` records its own `max(0.0, …)`. + +### Not separately mutated, and said rather than left implicit + +`account()`'s `parked` branch is graded as a function (part 1) but its **wiring** +— `parked=inv.sign_wd.tripped` — is not mutated on its own. Part 6's reversed-unit +fixture exercises the path end to end and asserts the account it produces, which +is weaker than a mutation and is stated as such. diff --git a/goodwe_controller/app/delivery.py b/goodwe_controller/app/delivery.py index d9bb91d..cc9820a 100644 --- a/goodwe_controller/app/delivery.py +++ b/goodwe_controller/app/delivery.py @@ -491,11 +491,24 @@ def delinquent(units, min_w: float) -> dict: wedged at once. Reporting it anyway would be the saturation-shaped alarm `clamp.py` warns about, produced by the guard written to replace it. - ⚠️ AND THAT IS WHY N=1 IS INERT WITHOUT A ROSTER GUARD. `asked` has at most - one element there, `len(asked) < 2` short-circuits, and every input answers - `{}`. A `if n_units > 1` in the caller would be a second place the same - decision lives; the algebra is the guard, the same way - `circulation.circulating_w()` is identically zero at N<=1. + ⚠️ AND THAT IS WHY N=1 IS INERT WITHOUT A ROSTER GUARD. A `if n_units > 1` + in the caller would be a second place the same decision lives; the algebra + is the guard, the same way `circulation.circulating_w()` is identically zero + at N<=1. + + ⚠️ BUT THE ALGEBRA THAT DOES IT IS THE PROPER-SUBSET RULE, NOT THE + `len(asked) < 2` LINE - AND THIS PARAGRAPH USED TO SAY OTHERWISE. The + mutation battery found it: relaxing that line to `len(asked) < 1` is a + SURVIVOR at +0 B, and it is an EQUIVALENT MUTANT rather than a gap in the + checks. With one asked unit, `unaccounted` is a subset of a one-element set, + so it is either empty (`not unaccounted` returns `{}`) or equal to `asked` + (`len(unaccounted) == len(asked)` returns `{}`). There is no third case and + no input on which the two spellings differ. The line is a FAST PATH and a + signpost, not the mechanism - said here rather than left for the next + mutation sweep to rediscover, exactly as `circulation.circulating_w()` says + of its own `max(0.0, ...)`. Do not report its survival as a gap in the + checks, and do not delete it either: it is what tells a reader at a glance + that a one-inverter install never reaches any of this. """ asked = [u for u in units if u.share_w is not None and float(u.share_w) > 0.0] diff --git a/goodwe_controller/test_delivery.py b/goodwe_controller/test_delivery.py index ea7d9b8..496e424 100644 --- a/goodwe_controller/test_delivery.py +++ b/goodwe_controller/test_delivery.py @@ -28,7 +28,7 @@ from app.delivery import ( DELIVERY_OPTION_DEFAULTS, REASON_CODE, DeliveryConfig, DeliveryWatchdog, UnitDelivery, account, delinquent, short_by, ) -from app.inverter import Inverter +from app.inverter import LINK_STALE_S from app.main import Controller from app.peakguard import discharge_permitted from app.store import unit_key @@ -61,6 +61,19 @@ check("...a CHARGE share answers None. There is no configured charge ceiling " "in this add-on, so a pack in constant-voltage taper cannot be explained " "and arming it would fire on every maintenance charge", short_by(-2000, 0) is None and short_by(-2000, -100) is None) +# ⚠️ THE CASE THAT ACTUALLY GRADES THE `share <= 0` GUARD, AND THE MUTATION +# BATTERY IS WHAT FOUND IT MISSING. Relaxing that guard to `share == 0` left the +# two rows above GREEN - for a charge share that is UNDER-delivered, +# `share - measured` is negative and the `gap > 0` test answers None anyway, so +# the mutant is masked. The one input where the two differ is a unit moving MORE +# than its charge share: -2000 W told, -3000 W measured, `share - measured` = +# +1000 W, which the mutant reports as a SHORTFALL. It is not one - it is +# something else driving that pack, which is SAFETY-30's and SAFETY-31's +# question - and reading it as "short of its share" would name the wrong fault. +check("...and a unit moving MORE than its charge share is not short of it " + "either. This is the input that separates `share <= 0` from `share == 0`; " + "the two rows above do not, because the `gap > 0` test masks them", + short_by(-2000, -3000) is None and short_by(-2000, -2600) is None) check("...a missing reading is None, never 'the whole share'. Zero would be " "this check inventing a fault out of an absence", short_by(2000, None) is None and short_by(None, 0) is None) @@ -590,6 +603,86 @@ check("...and it fires IDENTICALLY on the benign arm, on the identical trace, " and _cf.site_batt_w == _cw.site_batt_w and _cf.target == _cw.target) +print() +print(" ...and a unit the ALLOCATOR HAS ALREADY DROPPED is not reported twice") +# ⚠️ A WIRING CHECK THE PURE ONE IN PART 1 CANNOT MAKE, AND THE MUTATION BATTERY +# IS WHAT FOUND IT MISSING. `account(live=False, ...)` is graded up there; what +# is graded HERE is that `cycle()` actually ASKS `inv.is_live(now)`. Replacing +# that call with a literal `True` left every check green, because no fixture had +# a unit whose writes were being rejected. A dropped unit still has its last +# ACCEPTED command standing on the wire (`write_setpoint` leaves `last_write` +# alone on a rejected write) and still measures nothing, so it looks exactly +# like a wedged one - and reporting it would be a second, louder alarm about a +# state the allocator has already handled and logged, which is the +# SAFETY-15/SAFETY-19 failure this module takes its 30 s floor from. + + +class _LinkDeadHass(_DeliveryHass): + """B is healthy, then its LINK dies: writes rejected and output gone. + + ⚠️ `rejected_since` IS BACK-DATED RATHER THAN WAITED OUT. `is_live()`'s + window is `LINK_STALE_S` = 30 s of wall clock and this fixture runs in about + two; waiting it out would make the check's runtime the thing it grades. What + is graded is the CALL, not the timer - `test_inverter.py` part 3 owns the + timer. + """ + + def __init__(self, dead_from=5, **kw): + super().__init__(**kw) + self.dead_from = int(dead_from) + self.cycle_n = 0 + + def measured(self): + a, b = self.ctrl.inverters + w = lambda i: float((i.last_write if i.last_write is not None + else i.target) or 0.0) + return [w(a), 0.0 if self.cycle_n >= self.dead_from else w(b)] + + async def set_number(self, entity, value): + self.writes.append((entity, float(value))) + if entity == "number.sp_b" and self.cycle_n >= self.dead_from: + return False + return True + + +async def _link_dead(): + hass = _LinkDeadHass(dead_from=5) + c = _ctrl(hass) + b = c.inverters[1] + seen = [] + for n in range(18): + hass.cycle_n = n + if n == 5: + # the moment the link dies, back-dated past the firmware's window + b.rejected_since = (datetime.now(timezone.utc) + - timedelta(seconds=LINK_STALE_S + 10)) + await c.read_inputs() + await c.cycle() + seen.append(dict(live=b.is_live(datetime.now(timezone.utc))[0], + stood=float(b.standing_w()), meas=b.batt, + named=b.delivery_wd.tripped)) + await asyncio.sleep(0.15) + return c, seen + + +_cld, _sld = asyncio.run(_link_dead()) +_bld = _cld.inverters[1] +check("the fixture REACHES the case: B is not live, is still standing at a " + "discharge command, and is measuring nothing - i.e. it looks exactly like " + "a wedged unit to every number this check reads", + not _sld[-1]["live"] and _sld[-1]["stood"] > _cld.tuning.saturation_w + and _sld[-1]["meas"] == 0.0) +check("...and it is NOT reported, because `account()` was asked and answered. " + "The allocator has already dropped it and said so in the action log; a " + "second alarm about that is the SAFETY-15/19 failure", + not _bld.delivery_wd.tripped and _bld.delivery_wd.since is None) +check("...and the answer is the LINK one specifically, not the SoC one - so a " + "reader can tell which branch withheld the alarm", + "link" in (account(live=False, parked=False, soc=_bld.soc, + bms_floor=_bld.bms_soc_floor) or "")) +print(f" B: live={_sld[-1]['live']} standing={_sld[-1]['stood']:.0f} W " + f"measured={_sld[-1]['meas']:.0f} W -> not reported") + print() print(" ...and the one case where BOTH have something to say: a unit that is") print(" REVERSED is short too, and SAFETY-30 gets there first")