diff --git a/HANDOVER.md b/HANDOVER.md index 9364ae3..c926f1f 100644 --- a/HANDOVER.md +++ b/HANDOVER.md @@ -85,15 +85,22 @@ rejected. add-on will not clear a trip.** That is the point of it; use the *Start / stop control* button twice, or delete the trip from `/data/state.json`. -⚠️ **DEV-02 renamed that key and the old name is now a fossil.** There is one -`SignWatchdog` per inverter, so the latch lives under -`unit//sign_tripped` — `unit/inverter-a/sign_tripped` on a -single-inverter site. The pre-DEV-02 flat `sign_tripped` is still written in -the file on purpose (so a rollback finds its latch) and is **never read again**, -so deleting it clears nothing. An install upgrading is migrated on first start, -and if the latch cannot be made to survive that migration the controller -re-arms it from the old key and says so in the log rather than coming up -disarmed. +⚠️ **DEV-02 renamed that key. There is one `SignWatchdog` per inverter, so the +LIVE latch lives under `unit//sign_tripped`** — +`unit/inverter-a/sign_tripped` on a single-inverter site. **That is the one to +delete if you are clearing a trip by hand**; deleting the flat one alone clears +nothing. + +⚠️ **The flat `sign_tripped` is still maintained, though — it is the ROLLBACK +VIEW, not a fossil.** It is written alongside the namespaced key on a trip and +cleared alongside it when you press *Start / stop control*, so an add-on you +**downgrade** past this version still finds the latch it should. It is also read +on **every** start, and the one disagreement it can produce is acted on: *the +flat key says tripped and no inverter came up latched* means the latch was +written by an older add-on while this install was rolled back — or the migration +lost it — and the controller **re-arms the latch and shouts in the log** rather +than coming up disarmed with a green banner. So if you edit `/data/state.json` +by hand, clear **both** spellings or the next start will put the trip back. ⚠️ `test_loop.py` takes about **95 seconds** (measured 93 s) and that is not a bug. It drives the real control loop, which sleeps 1 s a tick, eight times over, to prove a diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 35b09a2..98f5c1a 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -64,6 +64,70 @@ the P1 reading, the site integrator, and the tariff quarter. two-unit figure above is architecture, not a measurement of a second device; the second rating in the checks is marked as a hypothesis. +**The write path is gated on the allocation record, not on how many config keys +exist (SAFETY-28).** specs §1.5 makes *"any configuration in which two writers +can reach one actuator"* a non-goal. Until now that non-goal was enforced by +`len(validate.ACTUATOR_WRITE_KEYS) == 1` - there is exactly one option key, so a +second writer is not expressible in the schema. True, and a statement about **the +shape of the options schema rather than about the writes**: add a per-inverter +config (CFG-04) and there are two real writers with that length still 1, so the +check stays green while the property it protects is gone. `sim/inject.py`'s +`two_writers` scenario asserted on the same tuple and went green with it. + +* **The enforcement moved onto the writes.** `write_setpoint()` now refuses any + value that is not in **this tick's** allocation record from ALLOC-01: + `Controller.write_refusal()` asks `Allocation.authorises()` whether the single + allocator placed exactly this number, for this unit, on this tick. A second + writer with arithmetic of its own cannot satisfy that - its number appears in + no record - so it has to file a `Claim` and go through `resolve()`, which is + the one allocator. "Exactly one allocator per shared resource" stops being a + convention and becomes something the write path checks. +* **A refusal is a refusal, not a log line.** The unauthorised value is replaced + with **0 W and written**. Returning instead would leave the inverter latched + on its last command with the ESP32 heartbeat still being fed - holding an + unauthorised setpoint quietly, which is the outcome the gate exists to prevent. +* **A stale record cannot be replayed.** Every record now carries the `tick` it + was made for, handed *in* by the caller so `allocator.plan()` stays the pure + function `test_alloc.py` part 10 grades. The counter advances **once per + resolution, not once per loop iteration**: a new allocation makes the previous + record stale immediately, while the "inputs missing, not yet stale" hold - + which returns before the allocator runs, so one dropped meter reading does not + zero the inverter - leaves the standing authorisation intact for the heartbeat. + An **unstamped** record authorises nothing; "I cannot say when I was made" + fails closed. +* **⚠️ 0 W IS AUTHORISED UNCONDITIONALLY, SO THE FAILSAFE CANNOT DEADLOCK BEHIND + THE GATE.** `write_setpoint()` is also how layer 2 commands zero, and every + path that does so - `cycle()`'s two refusal branches, `shutdown()`, the + operator's stop button - runs while the allocator has *refused*. A gate that + asked the allocator's permission to command zero would block the safety write + behind the very component whose failure the safety write answers. The + exemption is on the **value**, not on a `failsafe=True` flag a caller asserts, + and the argument is that the hazard §1.5 names is N-times loop gain from + *broadcast*: at `S = 0` the broadcast and the correct allocation coincide - + `a_i = 1` for every unit gives every unit 0 W, which is exactly its share - so + there is no over-command to make at zero. Driven end to end through the real + refusal branch in `test_clamp.py` part 7c. +* **`unmet_w` is still not `ok`.** The gate reads `ok` and the unit's share and + never `unmet_w`, because an allocation that could not place every watt is + still safe to write; fusing them would let one saturated inverter park the + whole site. Graded on a record with 3800 W unmet. +* **`ACTUATOR_WRITE_KEYS` is replaced by `actuator_writers()`**, which counts + **writers per actuator** rather than option keys, and finds them by walking + the options tree rather than by looking for a container name CFG-04 has not + chosen yet. Two inverters with two entities are accepted - that is what the + per-inverter roster is *for*; two declarations aimed at one entity are refused, + naming the actuator and every writer pointed at it. The tuple survives only as + the enumeration a census needs, and nothing asserts on its length any more. +* **What neither check observes, said out loud.** The census sees *configured* + writers, so a second write path added in **code** is invisible to it - exactly + as it was invisible to the tuple. That half is closed at runtime by the record + gate, which does not care what the options look like or who called the method. +* Verified by breaking it: seven mutations on a purged tree via + `tools/mutation_run.sh` - removing the gate, making the zero exit an amnesty, + dropping the tick comparison, deleting the census refusal, fusing `unmet_w` + into `ok`, freezing the tick on the live path, and moving the gate below the + clamp - each turn `test_clamp.py` or `test_validate.py` red. + **The site grid target is now ALLOCATED across N inverters, sum-invariant, and never broadcast (ALLOC-01).** `ARB-03` left `allocate()` as a documented seam that passed through at N<=1 and DECLINED at N>=2; this fills it. The new diff --git a/goodwe_controller/DOCS.md b/goodwe_controller/DOCS.md index 862553e..db677ae 100644 --- a/goodwe_controller/DOCS.md +++ b/goodwe_controller/DOCS.md @@ -308,7 +308,11 @@ does. That is deliberate on both counts: driving the wrong way, three minutes later, forever. - The trip is written to `/data/state.json` — under `unit//sign_tripped`, one key per inverter since DEV-02, so a trip - caused by one inverter cannot latch another — and restored on startup. Restarting + caused by one inverter cannot latch another — and restored on startup. The + pre-DEV-02 flat `sign_tripped` is written and cleared alongside it as the + **rollback view**, so downgrading the add-on past DEV-02 still finds the + latch, and a trip taken while the install is rolled back is re-armed by the + version that comes back up rather than silently lost. Restarting the add-on is the first thing most people try when they see a red banner, and a latch that a restart clears would bring the controller back **armed, into the same fault, unattended** — and under a Supervisor that restarts a crashed add-on diff --git a/goodwe_controller/MUTATION.md b/goodwe_controller/MUTATION.md index 2722727..9a11757 100644 --- a/goodwe_controller/MUTATION.md +++ b/goodwe_controller/MUTATION.md @@ -127,6 +127,63 @@ Two rules come out of it, and both are cheap: function, that pair is the first thing to try — TEST-16's two survivors were adjacent lines in one function and neither was individually live. +## The third way a check lies: its DATA cannot reach the failure it names + +Stale bytecode manufactures survivors from a mutation that never ran. Masking +manufactures them from a second guard doing the first one's work. This one +manufactures them from a **fixture**: the check runs, the assertion is +evaluated, the sentence above it is true — and the numbers it compares are +arranged so that the failure it describes could not have changed the answer. + +It is not hypothetical and it is not rare. Three instances in one week, all +found by review rather than by a red line: + +* **ARB-03** shipped a call-site scan asserting that every `claims.append` in + `cycle()` routes through `actuator_of()`. Its own parse **saw 3 of the 8 + appends**; the other five were invisible to it and would have passed however + they were routed. +* **ALLOC-01** shipped a replay generator to hunt for envelope escapes. It found + **0**. A widened generator over the same allocator found **39,991**. +* **DEV-02** shipped `"each unit's supervised bound is ITS OWN max_w — give + inverter B a different rating and the claim filed on inverter B moves with + it"`, comparing inverter B's bound against `_b2.max_w`. The fixture defaulted + B's rating to A's, so it compared `(-5000, 5000)` against `(-5000, 5000)` — + **a number against itself**. Filing both per-unit bounds from + `self.inverters[0]` left **all 19 files green**. + +Note what these have in common with a masked rung: the mutation *runs*, and the +suite is *honestly* green. Nothing in a purge, a fingerprint or a disassembly +can see it, because the code under test really did behave identically on the +stimulus it was given. The defect is upstream of the assertion, in what the +check was handed. + +**The rule, and it is one line:** + +> **Any check asserting "X differs from Y" must be run once with X and Y +> actually made different.** + +Three cheap habits fall out of it, and all three are now used in this repository: + +1. **Write the PRECONDITION as its own check.** `test_peakguard.py` 10g asserts + that A and B have different ratings *before* it asserts that B's bound is + B's. If a later edit makes them equal again, the precondition goes red rather + than the claim going quietly vacuous. +2. **Assert against literals, not against the expression under test.** + `== [(-_b2.max_w, _b2.max_w)]` cannot distinguish "B's number" from "the + number this fixture happens to have twice"; `== (-3000.0, 3000.0)` beside a + printed `A = 5000` can. +3. **Print what the check saw.** Every one of the three above would have been + caught by a human reading one line of output — `supervised bounds by + actuator: {'inverter-a': (-5000, 5000), 'inverter-b': (-5000, 5000)}` is + self-evidently not evidence of a difference. + +And the mutation-table consequence: **a variant is not covered because a +near-miss of it is.** DEV-02's table listed the *actuator* variant of the +per-unit claim defect (`device`/`supervised` filed without `inv`), which the +static routing pin catches. The *bound* variant — right actuator, wrong +numbers — is a different defect with a different fix, and it was absent. Both +are in the table now. + ## Where each surface stands * **Local and dev-harness runs — the exposed one.** This is where FCST-03 hit diff --git a/goodwe_controller/app/allocator.py b/goodwe_controller/app/allocator.py index 78c8b9c..73913fa 100644 --- a/goodwe_controller/app/allocator.py +++ b/goodwe_controller/app/allocator.py @@ -197,12 +197,82 @@ class Allocation: quantisation_w: float = 0.0 refused: str | None = None passthrough: bool = False # N<=1: the share IS the site target, unrounded + # SAFETY-28. WHICH TICK PRODUCED THIS RECORD, as an opaque token minted by + # the caller - `Controller.alloc_tick`, incremented once per resolution. + # + # ⚠️ IT IS AN INPUT, NOT A COUNTER THIS MODULE KEEPS. `plan()` is a pure + # function and part 10 of test_alloc.py grades that by asserting the module + # holds no state at all; a serial number incremented in here would be + # exactly the cached-across-ticks thing the envelope rule forbids, wearing a + # different hat. So the tick is handed in and handed back out, and the + # freshness question is answered by whoever owns the clock. + # + # ⚠️ AND IT DEFAULTS TO None, WHICH THE WRITE GATE REFUSES. A record built + # without a tick is not "tick zero", it is a record that cannot say when it + # was made - and the gate fails closed on it. Every test in this repository + # that calls `plan()` for its arithmetic gets None and does not care; + # main.py stamps every record it puts on the write path. + tick: int | None = None @property def ok(self) -> bool: """The one boolean SAFETY-28's write gate reads.""" return self.refused is None + def authorises(self, unit: str, value: float, tick: int | None) -> str | None: + """Why `unit` may NOT be written `value` on tick `tick`, or None if it may. + + ⚠️ THIS IS THE QUESTION SAFETY-28 REPLACED A CONFIG-SHAPE PROXY WITH, and + it lives on the record rather than in main.py so that the record is the + thing that answers it. Before this, "two writers can never reach one + actuator" was enforced by `len(validate.ACTUATOR_WRITE_KEYS) == 1` - + true, but a statement about the OPTIONS SCHEMA. Add a per-inverter + config (CFG-04) and there are two real writers with that length still 1: + the check stays green while the property it protects is gone. + + Asking the record instead makes the property mechanical. A number + reaches the wire only if the single allocator placed it, on THIS tick, + for THIS unit. A second writer with its own arithmetic cannot produce + one, because it does not appear in a record; and it cannot borrow an old + one, because the tick has moved on. + + ⚠️ `unmet_w` IS NOT CONSULTED AND MUST NOT BE. An allocation that could + not place every watt is still SAFE TO WRITE - see this class's own + docstring. Fusing the two would let one saturated inverter park the + whole site, which is a worse outcome than the hole this gate closes. + + ⚠️ NOR IS 0 W ITS BUSINESS. The caller answers that one before it asks + this, and the reason is at `Controller.write_refusal()`: a gate that can + block the failsafe is worse than the hole it closes. + """ + if tick is None or self.tick is None or self.tick != tick: + return (f"the allocation record on hand was made on tick " + f"{self.tick!r} and this is tick {tick!r} - a record whose " + f"envelopes have moved cannot authorise a write") + if not self.ok: + return f"the allocator refused this tick ({self.explain()})" + if unit not in self.shares: + return (f"{unit} has no share in this tick's allocation " + f"({self.explain()})") + share = self.shares[unit] + if value != share: + return (f"{value:+.0f} W is not {unit}'s share of this tick's " + f"allocation, which is {share:+.0f} W ({self.explain()})") + # ⚠️ THE RECORD IS ASKED WHETHER IT AGREES WITH ITSELF. `within_envelope` + # exists because the half-open clamp defect once shipped a plan reading + # `inverter-a -> +500 W in [2000, +inf]` - stating the floor and a share + # below it in one breath - and `UnitPlan.explain()` renders that as + # ENVELOPE VIOLATED, DO NOT WRITE. This is the DO NOT WRITE. + # + # Unreachable at the shipped roster of one, where the pass-through's + # interval is (-inf, +inf) and every number is inside it. Graded anyway, + # on a constructed record, because the branch PR #40's reviewer replaced + # with `if False:` was unreachable too and the whole suite stayed green. + for p in self.plans: + if p.unit == unit and not p.within_envelope: + return (f"the record contradicts itself: {p.explain()}") + return None + def explain(self) -> str: """One line an operator can act on.""" if self.refused is not None: @@ -315,7 +385,7 @@ def _fill(order, weights, cap, amount): def plan(target_w, bounds, *, weights=None, live=None, scope_errors=(), - site_priority=None, site_key="site") -> Allocation: + site_priority=None, site_key="site", tick=None) -> Allocation: """Divide the site grid target `target_w` across the units in `bounds`. `bounds` unit name -> that unit's interval for THIS TICK (arbiter stage 1, @@ -333,6 +403,11 @@ def plan(target_w, bounds, *, weights=None, live=None, scope_errors=(), `site_priority` the priority of the SITE claim that chose `target_w`. It is what a unit's own winning SET is weighed against - see PINS. `site_key` the name to use when there are no units at all. + `tick` an opaque token identifying the tick this record is FOR, handed + straight back on the record. SAFETY-28's write gate compares it + against the tick it is writing on, so that a record cannot be + replayed after its envelopes have moved. It is an input rather + than something this module counts, for the reason on the field. ⚠️ N<=1 IS A PASS-THROUGH AND IS BYTE-IDENTICAL TO WHAT SHIPPED. The share IS the site target, as a float, unclamped and unrounded. That is §6.2's N=1 @@ -380,7 +455,7 @@ def plan(target_w, bounds, *, weights=None, live=None, scope_errors=(), shares={}, plans=(), dropped=(), unmet_w=int(round(float(target_w))), quantisation_w=float(int(round(float(target_w)))) - float(target_w), - refused=f"{REFUSE_SCOPE}: " + "; ".join(scope_errors)) + refused=f"{REFUSE_SCOPE}: " + "; ".join(scope_errors), tick=tick) if len(units) <= 1: only = units[0] if units else site_key @@ -390,7 +465,7 @@ def plan(target_w, bounds, *, weights=None, live=None, scope_errors=(), shares={only: share}, plans=(UnitPlan(only, int(round(share)), -inf, inf, 1.0, "N<=1: the share IS the site target"),), - passthrough=True) + passthrough=True, tick=tick) w = {u: 1.0 if weights is None else float(weights.get(u, 1.0)) for u in units} @@ -443,7 +518,7 @@ def plan(target_w, bounds, *, weights=None, live=None, scope_errors=(), return Allocation( target_w=float(target_w), sum_w=sum_w, shares={}, plans=(), dropped=tuple(dropped), unmet_w=sum_w, quantisation_w=quant, - refused=why) + refused=why, tick=tick) # --- REFUSAL 2: nobody is available ------------------------------------ if not considered: @@ -576,4 +651,4 @@ def plan(target_w, bounds, *, weights=None, live=None, scope_errors=(), return Allocation(target_w=float(target_w), sum_w=sum_w, shares=shares, plans=tuple(plans), dropped=tuple(dropped), - unmet_w=int(unmet), quantisation_w=quant) + unmet_w=int(unmet), quantisation_w=quant, tick=tick) diff --git a/goodwe_controller/app/arbiter.py b/goodwe_controller/app/arbiter.py index e814167..04aed51 100644 --- a/goodwe_controller/app/arbiter.py +++ b/goodwe_controller/app/arbiter.py @@ -416,11 +416,19 @@ def allocate(target_w: float, bounds: dict, *, weights=None, live=None, def allocate_record(target_w: float, bounds: dict, *, weights=None, live=None, - scope_errors=(), site_priority=None) -> tuple: - """`allocate()` plus the record - `(shares, allocated, allocation)`.""" + scope_errors=(), site_priority=None, tick=None) -> tuple: + """`allocate()` plus the record - `(shares, allocated, allocation)`. + + `tick` is SAFETY-28's freshness token: it identifies the tick the record is + FOR and is handed straight through to `allocator.plan()`. Left None the + record is UNSTAMPED, which the write gate refuses - so a caller that intends + the record to authorise a write must pass one. Every existing caller in this + repository wants the arithmetic and not the authority, and gets None. + """ rec = allocator.plan(target_w, bounds, weights=weights, live=live, scope_errors=scope_errors, - site_priority=site_priority, site_key=ACTUATOR_SITE) + site_priority=site_priority, site_key=ACTUATOR_SITE, + tick=tick) return rec.shares, rec.ok, rec @@ -472,7 +480,8 @@ def site_resolve(claims: list) -> Resolution: winner_priority=winner.priority) -def resolve(claims: list, *, units=None, weights=None, live=None) -> Resolution: +def resolve(claims: list, *, units=None, weights=None, live=None, + tick=None) -> Resolution: """The three stages, in order, over one cycle's claims. `units` is the site's roster of inverters. Left None it is derived from the @@ -499,6 +508,14 @@ def resolve(claims: list, *, units=None, weights=None, live=None) -> Resolution: with its own evidence removed. That mutation survived the whole suite until test_arbiter.py part 7 graded it on a SILENT unit. + ⚠️ `tick` IS SAFETY-28's FRESHNESS TOKEN AND IT IS NOT OPTIONAL ON THE LIVE + PATH. It identifies the tick this resolution is FOR, and it ends up on + `Resolution.allocation.tick`, where the write gate compares it against the + tick it is writing on. A record with no tick cannot authorise a write, which + is why main.py passes `self.alloc_tick` and why a test that only wants the + arbiter's arithmetic can leave it out and be sure it has not accidentally + minted a licence to write. + ⚠️ THE PARTITION IS BY `actuator`, AND IT IS A PARTITION, NOT A FILTER. Every claim lands in exactly one stage. That is the whole structural argument: a unit's `hi` and the site's `lo` are never both terms of the same `lo > hi` @@ -533,5 +550,6 @@ def resolve(claims: list, *, units=None, weights=None, live=None) -> Resolution: out.unit_bounds = bounds out.shares, out.allocated, out.allocation = allocate_record( out.target_w, bounds, weights=weights, live=live, - scope_errors=out.scope_errors, site_priority=out.winner_priority) + scope_errors=out.scope_errors, site_priority=out.winner_priority, + tick=tick) return out diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 6be824b..3e6b4d5 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -219,11 +219,13 @@ class Controller: # the add-on - and the controller then comes back armed, into the fault, # with nobody involved. # - # ⚠️ AND THE MIGRATION IS GRADED HERE RATHER THAN TRUSTED. A latch that - # silently failed to move into its namespace is a DISARMED SAFETY GUARD - # on an install whose state file still says `sign_tripped: true`. So the - # pre-DEV-02 flat value is compared against what unit A actually came up - # with, and a disagreement is not a log line - it re-arms the latch by + # ⚠️ AND THE LATCH IS GRADED HERE RATHER THAN TRUSTED, ON EVERY START. A + # latch that silently failed to move into its namespace - or that was + # taken by an OLDER add-on while this install was rolled back, which is + # the round trip store.py's rule 2 exists for - is a DISARMED SAFETY + # GUARD on an install whose state file still says `sign_tripped: true`. + # So the flat value is compared against what the roster actually came up + # with, and a disagreement is not a log line: it re-arms the latch by # hand and shouts, because the fail-safe direction for a latch is ON. self._grade_store_migration(store) for inv in self.inverters: @@ -361,6 +363,25 @@ class Controller: # the cached envelope this card exists to forbid, wearing a different # hat. self.allocation = None + # SAFETY-28. THE TICK THE RECORD ABOVE BELONGS TO, and the counter that + # mints it. It is incremented ONCE PER RESOLUTION - not once per loop + # iteration - and that distinction is load-bearing in both directions: + # + # * a NEW resolution makes the previous record stale IMMEDIATELY, so a + # record cannot be replayed into a later tick after the envelopes it + # describes have moved. That is the replay half of the gate. + # * a cycle that produced NO resolution does not bump it. The "inputs + # missing, not yet stale" hold above returns before the allocator + # runs precisely so that a single missed poll does not zero the + # inverter, and the heartbeat re-affirms `self.target` through the + # write path while it does. Bumping here would make that heartbeat + # unauthorised and drop the site to 0 W on the first dropped poll - + # turning a hold into the outage it exists to prevent. + # + # So the record is SUPERSEDED, never expired: it stands until the + # allocator produces its successor, and nothing else has moved in + # between that it does not still describe. + self.alloc_tick = 0 # What the last cycle's allocator dropped, so the log line above is # edge-triggered. Starts as the empty tuple - nothing has been dropped # yet and nothing has been said about it. @@ -413,25 +434,50 @@ class Controller: worse than never having persisted the latch, because now there is evidence that says it is fine. - So the pre-DEV-02 flat value is compared against what unit A ACTUALLY - came up with, and a disagreement is not reported and survived - the - latch is re-armed by hand, from the flat value, and the store is written - so the next start does not need this branch. The fail-safe direction for - a latch is ON: a spurious trip costs a 0 W hold an operator clears with - one button, and a missing one costs whatever SAFETY-02 was watching for. + So the pre-DEV-02 flat value is compared against what the roster + ACTUALLY came up with, and a disagreement is not reported and survived - + the latch is re-armed by hand, from the flat value, and the store is + written so the next start does not need this branch. The fail-safe + direction for a latch is ON: a spurious trip costs a 0 W hold an + operator clears with one button, and a missing one costs whatever + SAFETY-02 was watching for. + + ⚠️ AND IT RUNS ON EVERY START, NOT ONLY ON THE ONE THAT MIGRATES. It + used to return early on `not rep.ran`, for a real reason: while nothing + cleared the flat key, an install that had ever tripped carried + `sign_tripped: true` in its file forever, and a guard reading it would + have re-armed the failsafe on every restart for the rest of the + install's life. + + THAT GATE SWALLOWED THE EXACT ROUND TRIP store.py's RULE 2 EXISTS FOR, + and it is not hypothetical - a firmware version change with a rollback + path was performed on this site on 2026-09-02: + + 1. the install migrates -> `schema_version = 1`, + `unit/inverter-a/sign_tripped = False` + 2. the operator DOWNGRADES, and SAFETY-02 trips inside the downgrade + window. The old code knows only the flat key, so the file gains + `sign_tripped: true` and nothing namespaced + 3. the operator upgrades again. `rep.ran` is False - the store is + already at schema 1 - so BOTH the migration and this guard are + skipped, `Inverter.__init__` reads the namespaced key, and the + controller comes up with `sign_wd.tripped = False` while + /data/state.json says `sign_tripped: true` + + Which is this module's own sentence, rebuilt by the version marker: the + file still says tripped, the banner is green, and the controller resumes + driving the wrong way. + + The gate is gone because the reason for it is gone. `handle_action()` + now clears the flat keys alongside the namespaced ones when the operator + clears the latch, and the trip mirrors them, so the two spellings can + disagree in exactly one direction: FLAT SAYS TRIPPED AND NO UNIT CAME UP + TRIPPED means the flat key was written by a version that did not know + about namespaces - a downgrade-window trip - or a migration that lost + the latch. Both want the same answer, which is the one below. """ rep = self.store_migration - # ⚠️ ONLY ON THE START THAT ACTUALLY MIGRATES, AND THAT IS NOT A - # SHORTCUT. Once the store is marked migrated the NAMESPACED value is - # the live latch and the flat key is a fossil that nothing writes any - # more - so an operator who clears the failsafe leaves `sign_tripped: - # true` sitting in the file forever. A guard that went on comparing - # against it would re-arm the failsafe on every restart, for the rest of - # the install's life, on a fault that was dealt with weeks ago. That is - # rule 1 of `migrate_unit_keys()` read from the other end. - if not rep.ran: - return - if not rep.ok: + if rep.ran and not rep.ok: _LOG.error("SAFETY-02: the per-inverter store migration did not " "verify (%s). The latch is being re-derived from the " "legacy keys.", rep.explain()) @@ -439,9 +485,24 @@ class Controller: legacy_tripped = bool(data.get("sign_tripped", False)) if not legacy_tripped or not self.inverters: return - first = self.inverters[0] - if first.sign_wd.tripped: + # ⚠️ THE QUESTION IS "IS ANY NAMESPACED LATCH SET", AND IT IS ASKED OF + # THE STORE RATHER THAN OF THE ROSTER. At a roster of two a trip on B + # leaves A untripped, so re-arming A off a flat key that B's own trip + # mirrored would latch a healthy inverter on every restart - the + # flat-key defect this card removed, coming back through its own guard. + # And the STORE is the right place to ask: it holds every + # `unit/*/sign_tripped` this install has ever written, including one + # belonging to a unit that is not in TODAY's roster (a roster is + # configuration and it changes), whereas the roster only knows what is + # configured right now. The flat key carries new information only when + # nothing namespaced is latched at all. + namespaced_latched = any( + bool(v) for k, v in data.items() + if k.startswith("unit/") and k.endswith("/sign_tripped")) + if namespaced_latched or any(inv.sign_wd.tripped + for inv in self.inverters): return + first = self.inverters[0] # The one state this method exists for. reason = str(data.get("sign_reason", "") or "") first.sign_wd.tripped = True @@ -451,12 +512,14 @@ class Controller: first.store_set("sign_reason", first.sign_wd.reason) first.store_set("sign_tripped", True) _LOG.error( - "SAFETY-02 MIGRATION GUARD: this store carries a LATCHED " - "sign-disagreement failsafe under the pre-DEV-02 flat key, and %s " - "came up UNLATCHED. That is a disarmed safety guard on an install " - "whose state file says it is protected, so the latch has been " - "re-armed from the legacy value: %s. %s is held at 0 W until " - "control is started from the UI.", + "SAFETY-02 LATCH GUARD: this store carries a LATCHED " + "sign-disagreement failsafe under the pre-DEV-02 flat key and NO " + "inverter came up latched. Either the per-inverter migration lost " + "it, or it was written by an older add-on while this install was " + "rolled back. Both are a disarmed safety guard on an install whose " + "state file says it is protected, so the latch has been re-armed " + "on %s from the legacy value: %s. %s is held at 0 W until control " + "is started from the UI.", first.name, first.sign_wd.reason, first.name) self.log_event("FAILSAFE re-armed from the pre-DEV-02 store keys - " + first.sign_wd.reason) @@ -767,6 +830,77 @@ class Controller: inv.soc = await self.hass.number(inv.soc_entity) inv.batt = await self.hass.number(inv.batt_entity, inv.batt_invert) + def write_refusal(self, value: float, inv=None) -> str | None: + """Why `value` may NOT be written to `inv` right now, or None if it may. + + --- SAFETY-28: THE WRITE PATH IS GATED ON THE ALLOCATION RECORD -------- + specs §1.5 makes "any configuration in which two writers can reach one + actuator" a non-goal. Until this method that non-goal was enforced by + `len(validate.ACTUATOR_WRITE_KEYS) == 1` - there is one option key, so a + second writer is not expressible. True, and a statement about the SHAPE + OF THE OPTIONS SCHEMA rather than about the writes. Add CFG-04's + per-inverter config and there are two real writers with that length + still 1: the check stays green while the property it protects is gone. + That is this repository's named failure mode - a check that observes + something other than what it claims to observe - so the enforcement + moved here, onto the writes themselves. + + The rule is: A NUMBER REACHES THE WIRE ONLY IF THE SINGLE ALLOCATOR + PLACED IT, ON THIS TICK, FOR THIS UNIT. A second writer with arithmetic + of its own cannot satisfy that - its number is not in any record - so it + has to file a Claim and go through `resolve()`, which is the one + allocator. "Exactly one allocator per shared resource" stops being a + convention somebody can forget and becomes a thing the write path + checks. `Allocation.authorises()` holds the per-record half of the + argument. + + ⚠️ 0 W IS ALWAYS AUTHORISED, AND THAT IS THE ANSWER TO "WHAT IF THE + ALLOCATOR IS THE THING THAT FAILED". This is not a convenience exit; it + is the one value whose correctness does not depend on a split existing: + + * The hazard the non-goal names is N-times loop gain from BROADCAST - + handing every inverter the same target so each supplies it in full + (see allocator.py's opening argument, and TEST-19's measured + `N=2 gain 1.0` limit cycle). At S = 0 the broadcast and the correct + allocation COINCIDE: a_i = 1 for every unit gives every unit 0 W, + and 0 W is exactly its share. There is no over-command to make. + * It is the fail-toward-inaction direction §11.1 rule 1 already + names, and it is what layer 2 commands. `cycle()`'s two refusal + branches replace the resolution with a 0 W failsafe and then write + it through here like any other cycle; `shutdown()` and the + operator's stop button do the same. Every one of those runs while + the allocator has REFUSED - `Allocation.ok` is False and there are + no shares - so a gate that asked the allocator's permission to + command zero would deadlock the failsafe behind the very component + whose failure the failsafe exists to answer. + + A gate that can block the safety write is worse than the hole it closes, + so it cannot block it: the test for zero is first and reads nothing. + + ⚠️ AND IT IS `value == 0.0`, THE REQUESTED NUMBER - not a `failsafe=True` + argument. A flag is an assertion by the caller, which is precisely what + a second writer would pass; the value is a fact about what is going to + be written. -0.0 == 0.0 in Python, and a nan never reaches here (the + caller has already turned it into 0.0), so the predicate is total. + + ⚠️ DEV-02: THE UNIT IS AN ARGUMENT, NOT THE CONTROLLER'S N=1 VIEW OF + IT. The record holds one share PER UNIT, so "did the allocator place + this number" is a question about a NAMED actuator. Asking it with the + first inverter's name at a roster of two would authorise A's share on + B's wire and refuse B's own - the site/unit confusion this card + removes, wearing the gate's name. `_write_unit()` always passes the + unit it is about; the default is for the N=1 callers and REFUSES at + N>=2 (see `_only()`) rather than answering for the first inverter. + """ + if value == 0.0: + return None + inv = self._only() if inv is None else inv + rec = self.allocation + if rec is None: + return ("no allocation record exists yet - nothing has authorised " + f"{value:+.0f} W on {inv.name}") + return rec.authorises(inv.name, value, self.alloc_tick) + async def write_setpoint(self, value: float, *, force: bool = False, unit=None) -> None: """Put `value` on ONE inverter, or - with `unit=None` - on all of them. @@ -832,6 +966,46 @@ class Controller: "0 W and continuing to command. This is a bug upstream, " "not a limit.", value) value = 0.0 + + # --- SAFETY-28: AND IT IS REFUSED HERE, NOT LOGGED SOMEWHERE ---------- + # ⚠️ ABOVE THE CLAMP AND BELOW THE NON-FINITE COERCION, and both halves + # of that placement are decisions. + # + # Below the coercion, because a nan is loss of input rather than an + # unauthorised write: rule 1 has already turned it into 0 W, which this + # gate then authorises unconditionally, so a transport bug does not get + # a second, differently-worded refusal. + # + # Above the clamp, because the question is WHO CHOSE THIS NUMBER and the + # answer is only meaningful about the number that was asked for. The + # clamp then narrows what was authorised, always toward safety, and it + # stays the last thing between a bug and the hardware. Gating the + # CLAMPED value would be worse than useless: on every cycle the clamp + # engages, the emitted watts differ from the allocator's share by + # construction, so the gate would refuse exactly the cycles SAFETY-03 + # exists for. + # + # The refusal is a SUBSTITUTION, not a return: 0 W goes on the wire in + # place of the number nothing stands behind. Returning instead would + # leave the inverter latched on its last command with the ESP32's + # heartbeat still being fed - holding an unauthorised setpoint quietly, + # which is the outcome this gate exists to prevent. + # + # ⚠️ AND IT IS ASKED ABOUT `inv`, NOT ABOUT THE SITE. The record holds + # one share per unit, so the question "did the single allocator place + # this number" is only answerable about a named actuator; DEV-02 made + # this method per-unit, and passing the first inverter here would + # authorise A's share on B's wire. + refusal = self.write_refusal(value, inv) + if refusal is not None: + self.log_event( + f"{inv.name}: WRITE REFUSED {value:+.0f} W - commanding 0 W") + _LOG.error("SAFETY-28 REFUSED a write of %+.0f W to %s: %s. " + "Commanding 0 W instead. Every setpoint must be one the " + "allocator placed on THIS tick, for THIS unit - see " + "Controller.write_refusal().", value, inv.name, refusal) + value = 0.0 + # ⚠️ THIS UNIT'S OWN THREE NUMBERS. `inv.max_w` is the supervised cap on # what may be written to THIS setpoint entity and `inv.dev_min/dev_max` # are what THIS device reported about itself - which is the whole of "the @@ -1002,6 +1176,20 @@ class Controller: # pressed start, and one press cleared both. See store.py. inv.store_set("sign_reason", inv.sign_wd.reason) inv.store_set("sign_tripped", True) + # ⚠️ AND THE LEGACY FLAT KEYS ARE MIRRORED, WHICH IS THE OTHER + # HALF OF store.py's RULE 2. That rule keeps the flat keys "so a + # rollback still finds its latch", and it only ever covered the + # latch that existed AT MIGRATION TIME: a trip that happens + # AFTER the migration wrote nothing flat, so an operator who + # downgraded would find `sign_tripped: false` and come up + # DRIVING, into the fault. store.py used to claim the staleness + # could only fail toward a spurious latch; it could not, and + # this is what makes that sentence true. The pair is + # site-level on purpose - the version being rolled back TO + # drives exactly one actuator - and it is cleared by the same + # operator press that clears the namespaced ones. + self.store.set("sign_reason", inv.sign_wd.reason) + self.store.set("sign_tripped", True) self.log_event(f"FAILSAFE {inv.name}: " + inv.sign_wd.reason) _LOG.error("SAFETY-02 TRIPPED on %s - %s. Commanding 0 W on " "that inverter and holding it until control is " @@ -1423,7 +1611,12 @@ class Controller: # de-assert when control is stopped. self.loop_clamped = (False, "") - resolution = resolve(claims, units=self.units, live=self.live_units()) + # SAFETY-28. A new resolution is a new tick, and the previous record is + # stale from this line onwards. Incremented BEFORE resolve() so that the + # record resolve() builds is stamped with the tick it is for. + self.alloc_tick += 1 + resolution = resolve(claims, units=self.units, live=self.live_units(), + tick=self.alloc_tick) self.allocation = resolution.allocation # --- THE TWO REFUSALS, AND THEY ARE NOT THE SAME REFUSAL -------------- # Both do the same thing - REPLACE the resolution with a 0 W failsafe @@ -2589,6 +2782,32 @@ class Controller: inv.store_set("sign_reason", "") self.log_event(f"sign-disagreement failsafe cleared by " f"operator on {inv.name}") + # ⚠️ AND THE PRE-DEV-02 FLAT KEYS GO WITH THEM, UNCONDITIONALLY. + # This is what makes `flat True + namespaced False` MEAN + # something, and it is the reason `_grade_store_migration()` can + # run on every start instead of only on the start that migrates. + # + # Without it the flat key is a fossil that says `true` forever + # on any install that ever tripped, so a guard comparing against + # it would re-arm the failsafe on every restart for the rest of + # the install's life - which is why that guard used to be gated + # on `rep.ran`, and why the gate then swallowed the rollback + # ROUND TRIP: migrate, downgrade, trip during the downgrade + # window (the old code writes only the flat key), upgrade again + # -> `rep.ran` is False, the guard never runs, and the + # controller comes up UNLATCHED while /data/state.json says + # `sign_tripped: true`. That is store.py's own header sentence, + # rebuilt by the version marker. + # + # Cleared here, the two spellings can only disagree one way: the + # flat key is newer than the namespaced one, which means it was + # written by a version that did not know about namespaces - i.e. + # a trip inside a downgrade window. Unconditional rather than + # inside the loop because an operator pressing START is + # declaring the whole site clear, and the fossil belongs to no + # unit in particular. + self.store.set("sign_tripped", False) + self.store.set("sign_reason", "") if not self.auto: # ⚠️ THE WHOLE SITE, TARGETS INCLUDED - see command_site_zero(). # Zeroing `self.target` alone would leave each unit's last share diff --git a/goodwe_controller/app/store.py b/goodwe_controller/app/store.py index 20813ce..98e606f 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -43,9 +43,17 @@ _LOG = logging.getLogger("goodwe.store") # ⚠️ AND SINCE DEV-02 THE TWO SAFETY-02 KEYS ARE THE *LEGACY* SPELLING. They are # FLAT - one `sign_tripped` for a controller that now holds a LIST of inverters - # so a trip caused by inverter B would latch inverter A across restarts, and one -# operator action would clear both. They live on here as the pre-DEV-02 layout -# that `migrate_unit_keys()` reads ONCE and never reads again. Nothing in the -# add-on writes them any more; see SCHEMA_VERSION. +# operator action would clear both. The LIVE latch is per inverter, under +# `unit//...`, and that is the one the watchdog is restored from. +# +# ⚠️ THE FLAT PAIR IS NOT DEAD, THOUGH: it is the ROLLBACK VIEW. It is written +# alongside the namespaced keys on a trip and cleared alongside them on the +# operator's clear, so an add-on that is DOWNGRADED past this schema still finds +# the latch it should - and so that a trip taken while the install was rolled +# back is still visible to the version that comes back up. Rule 2 of +# `migrate_unit_keys()` and `Controller._grade_store_migration()` carry the +# argument; the short version is that a latch is only worth persisting if it +# survives the version change an operator makes while staring at the banner. DEFAULTS = { "phase": "idle", "phase_started": None, @@ -159,16 +167,31 @@ def migrate_unit_keys(store, unit: str) -> MigrationReport: THE THREE RULES, each one a case that would otherwise be a wrong answer: 1. IT NEVER OVERWRITES AN EXISTING NAMESPACED VALUE. A store that has both - spellings has been written by this version, and the namespaced one is the - live latch - the flat one is the fossil. Preferring the fossil would - resurrect a trip the operator has already cleared. - 2. THE FLAT KEYS ARE LEFT IN THE FILE. Deleting them buys tidiness and costs - a rollback: an operator who downgrades the add-on after a migration would - find no latch at all, which is the disarmed guard again in the other - direction. Left in place, a downgrade finds the trip it had. They go - stale from this moment - nothing writes them again - and the direction - that staleness fails in is a SPURIOUS latch (a 0 W hold an operator - clears), never a missing one. + spellings has been written by this version, and the namespaced one is + where the WATCHDOG is restored from. Preferring the flat one HERE would + resurrect a trip the operator has already cleared, because a migration + runs before anything can tell the two apart. Deciding between them is + main.py's job and it has the one rule that can: the flat key carries new + information only when NOTHING namespaced is latched. + 2. THE FLAT KEYS ARE LEFT IN THE FILE, AND THEY ARE KEPT LIVE. Deleting + them buys tidiness and costs a rollback: an operator who downgrades the + add-on after a migration would find no latch at all, which is the + disarmed guard again in the other direction. Left in place, a downgrade + finds the trip it had. + + ⚠️ AND "LEFT IN PLACE" IS NOT ENOUGH ON ITS OWN, WHICH IS WHAT THIS RULE + USED TO SAY. The sentence here read "they go stale from this moment - + nothing writes them again - and the direction that staleness fails in is + a SPURIOUS latch, never a missing one". That was true of the latch this + migration MOVES and false of every trip that happens AFTERWARDS: nothing + flat was written for those, so a rollback found `sign_tripped: false` and + came up DRIVING. So main.py mirrors both keys on a trip and clears both + on the operator's clear. The flat pair is the ROLLBACK VIEW of the latch, + maintained, not a fossil - which is also what lets main.py's + `_grade_store_migration()` run on every start and catch a trip that + happened while the install was rolled back (see its docstring: the flat + key saying tripped while no inverter came up tripped is the one + disagreement that can occur, and it is a DOWNGRADE-WINDOW LATCH). 3. THE VERSION MARKER IS WRITTEN LAST. A crash between the copies and the marker leaves the migration to run again, and rule 1 makes running it again a no-op. A marker written first would leave a half-migrated store diff --git a/goodwe_controller/app/validate.py b/goodwe_controller/app/validate.py index e911ce8..f76a819 100644 --- a/goodwe_controller/app/validate.py +++ b/goodwe_controller/app/validate.py @@ -229,17 +229,84 @@ _LOG = logging.getLogger("goodwe.config") # a matching integer means a matching rule. FIRMWARE_INGEST_VETO_S = 30.0 -# ⚠️ The complete set of option keys through which this add-on can reach an -# actuator. specs §1.5 makes "any configuration in which two writers can reach -# one actuator" a non-goal, and the way that non-goal is ENFORCED here is by -# construction: there is exactly one such key, so a second writer is not -# expressible in the options schema at all. sim/inject.py's `two_writers` -# scenario asserts on this tuple - if a second write path is ever added, that -# scenario goes red and somebody has to decide how the two are arbitrated -# BEFORE the config that allows both can ship. +# ⚠️ THE OPTION KEYS UNDER WHICH A WRITER CAN BE DECLARED - AN ENUMERATION, AND +# SAFETY-28 DEMOTED IT TO ONE. This tuple used to BE the guarantee. The comment +# that stood here said specs §1.5 makes "any configuration in which two writers +# can reach one actuator" a non-goal, and that the non-goal was ENFORCED by +# construction: exactly one such key, so a second writer is not expressible in +# the options schema at all. +# +# That was true, and it graded THE SHAPE OF THE OPTIONS SCHEMA rather than the +# writes. Add CFG-04's per-inverter config - a list of inverters, each with its +# own `setpoint_entity` - and there are two real writers with `len()` of this +# tuple still equal to 1. The check stays green while the property it protects +# is gone, which is this repository's named failure mode: a check that observes +# something other than what it claims to observe. +# +# So the guarantee moved to `actuator_writers()` below, which counts WRITERS PER +# ACTUATOR rather than keys, and finds them wherever in the options tree they +# are declared. This tuple survives only because a census has to know which key +# NAMES an actuator; it is not a bound on how many writers there can be, and +# nothing should ever assert on its length again. ACTUATOR_WRITE_KEYS = ("setpoint_entity",) +def _collect_writers(node, path: str, out: list) -> None: + """Walk the options tree, collecting `(where, entity)` for every writer. + + ⚠️ STRUCTURAL, NOT NAME-BASED ON THE CONTAINER, and that is the point. A + census that looked for a key called `inverters` would be guessing the name + CFG-04 has not chosen yet, and a guess that is wrong goes GREEN while two + writers exist - the same defect one level along. This walks whatever shape + the options happen to have and reports every `setpoint_entity` in it, + however deeply nested and under whatever container. + """ + if isinstance(node, dict): + for k, v in node.items(): + here = f"{path}.{k}" if path else str(k) + if k in ACTUATOR_WRITE_KEYS: + entity = str(v or "").strip() + if entity: + out.append((here, entity)) + continue + _collect_writers(v, here, out) + elif isinstance(node, (list, tuple)): + for i, v in enumerate(node): + _collect_writers(v, f"{path}[{i}]", out) + + +def actuator_writers(opts: dict) -> dict: + """actuator -> the configured writers that can reach it. INSTANCE-AWARE. + + The replacement for `len(ACTUATOR_WRITE_KEYS) == 1`. It answers HOW MANY + THINGS CAN WRITE TO EACH ACTUATOR, which is the property §1.5 actually + names, and it keeps answering it when the schema grows a per-inverter list: + two inverters with two entities are two writers on two actuators and fine, + two declarations naming ONE entity are the non-goal and are refused. + + The key is the entity id, lowercased - Home Assistant entity ids are + lowercase by construction, and folding case here means a config cannot + smuggle a second writer past the census on `Number.X` versus `number.x`. + + ⚠️ SAY WHAT THIS STILL DOES NOT OBSERVE, because the argument that demoted + the tuple applies to its replacement too. This is a census of CONFIGURED + writers. A second write path added in CODE - a `hass.set_number()` call next + to some new strategy - declares nothing and is invisible here, exactly as it + was invisible to the tuple. That hole is not closed at startup and cannot + be: it is closed at runtime, by `Controller.write_refusal()`, which refuses + any value the single allocator did not place on this tick and does not care + what the options look like. The two checks are deliberately different + kinds - one grades the config, one grades the writes - and only the second + one survives a schema this file has not seen yet. + """ + found: list = [] + _collect_writers(opts, "", found) + census: dict = {} + for where, entity in found: + census.setdefault(entity.lower(), []).append(where) + return census + + class ConfigRefused(Exception): """The configuration is wrong. Not "something broke" - see main.py's handler. @@ -265,9 +332,8 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] """ problems: list[str] = [] - # --- 1. an actuator to write to ------------------------------------------ - writers = [k for k in ACTUATOR_WRITE_KEYS - if str(opts.get(k, "") or "").strip()] + # --- 1. an actuator to write to, and AT MOST ONE WRITER PER ACTUATOR ----- + writers = actuator_writers(opts) if not writers: problems.append( "setpoint_entity is not set, so this add-on has no actuator: every " @@ -275,6 +341,29 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] "ESP32 heartbeat that holds off the layer-1 failsafe never happens. " "It would run, look healthy, and move no energy at all. Set it to " "the ESPHome number entity that carries the setpoint.") + # SAFETY-28. The §1.5 non-goal, graded on the writers rather than on the + # count of option keys. Read `actuator_writers()` for why the old check went + # green on the very config this one is here to refuse. + # + # ⚠️ AND IT REFUSES A DUPLICATE DECLARATION EVEN WHEN BOTH NAME THE SAME + # ENTITY, which is the case an author of the per-inverter config will meet + # first: keep `setpoint_entity` as inverter A's and add A to the list again + # and there are two config sites aimed at one actuator, each with its own + # neighbouring options. "They happen to be the same entity, so it is fine" + # is precisely the reasoning that makes two writers invisible. Declare each + # actuator exactly once and the refusal does not fire. + for actuator, who in sorted(writers.items()): + if len(who) > 1: + problems.append( + f"{len(who)} configured writers can reach the one actuator " + f"{actuator}: {', '.join(who)}. specs §1.5 makes any " + "configuration in which two writers can reach one actuator a " + "non-goal - two independent setpoints on one inverter is the " + "arbitration problem this add-on refuses to have, and at " + "runtime it is a meter-emulation target being written twice a " + "second by two strategies that cannot see each other. Give " + "each actuator exactly one writer, or point the second one at " + "its own entity.") # --- 2. the configured ceiling against the device's own rating ----------- max_w = _number(opts, "max_w", 2000.0) diff --git a/goodwe_controller/test_clamp.py b/goodwe_controller/test_clamp.py index 272445c..7f5c3dc 100644 --- a/goodwe_controller/test_clamp.py +++ b/goodwe_controller/test_clamp.py @@ -4,8 +4,8 @@ No framework, asserts only, same house style as test_freeze.py. -Six parts, and parts 3 and 4 are the ones that carry SAFETY-03's own -acceptance criteria; part 6 is TEST-11's: +Seven parts, and parts 3 and 4 are the ones that carry SAFETY-03's own +acceptance criteria; part 6 is TEST-11's and part 7 is SAFETY-28's: 1. THE CLAMP ARITHMETIC, on the shipped `clamp_to_rating()` - boundaries, symmetry, the device second stage, and the non-finite cases. Table driven. @@ -31,6 +31,13 @@ acceptance criteria; part 6 is TEST-11's: watchdog could see when it was consulted - so that a rename or a helper does not trip it, and with the write-path half exercised on a stimulus of its own, because parts 3-5 never produce one through cycle(). + 7. SAFETY-28. THE WRITE PATH IS GATED ON THE ALLOCATION RECORD - a value the + single allocator did not place on THIS tick does not reach the wire, a + record from a previous tick cannot authorise one, and 0 W is authorised + unconditionally so the failsafe can never deadlock behind the gate. This is + the check that replaced a config-shape proxy: `ACTUATOR_WRITE_KEYS` graded + how many option KEYS exist, which stays 1 while a per-inverter config adds + a second real writer. ------------------------------------------------------------------------------ WHY PART 4 ALSO ASSERTS ON `Decision`'s BOOLEANS AND NOT ON `reason` @@ -74,8 +81,10 @@ from app.clamp import ( CLEAR_CYCLES, REASON_CODE, REASSERT_S, ClampWatchdog, clamp_to_rating, is_commandable, ) -from app.arbiter import UnitBounds, allocate +from app.allocator import Allocation, UnitPlan, plan +from app.arbiter import P_LOOP, Claim, UnitBounds, allocate, resolve from app.control import Tuning, compute +from app.inverter import Inverter from app.main import Controller fails = [] @@ -533,16 +542,48 @@ print() print("5. no path reaches the wire around the clamp") +def arm(c, value): + """Give `c` a real allocation record authorising `value` on the CURRENT tick. + + ⚠️ SAFETY-28 MADE THIS NECESSARY, AND THE FACT THAT IT IS NECESSARY IS THE + TICKET. `write_setpoint()` now refuses any value the single allocator did + not place on this tick, so the probes below - which call it directly, the + way the heartbeat and the operator's buttons do - have to say where their + number came from. Before this they did not, and that was precisely the hole: + a second writer's number reached the wire on nothing but the fact that + somebody called the method. + + ⚠️ AND IT IS BUILT THROUGH THE SHIPPED `resolve()`, NOT BY HAND. A helper + that assembled an `Allocation` itself would be a second allocator living in + the test file, and every check below would then be grading the fixture's + idea of a record rather than the one main.py produces. The tick is bumped + the same way `cycle()` bumps it, for the same reason. + """ + c.alloc_tick += 1 + res = resolve([Claim.set("loop", P_LOOP, value, "test stimulus")], + units=c.units, live=c.units, tick=c.alloc_tick) + c.allocation = res.allocation + return res.allocation + + async def _direct(): hass = _Hass(0.0) c = Controller({"setpoint_entity": "number.sp", "max_w": 2000, "meter_source": "off"}, hass, _Store(), _Pub(), stop=asyncio.Event()) c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W - # The shape the heartbeat, shutdown() and handle_action() all use. - await c.write_setpoint(9999.0, force=True) - await c.write_setpoint(-9999.0, force=True) + # The shape the heartbeat, shutdown() and handle_action() all use - each one + # authorised by this tick's allocation, which is what those callers have + # too: the heartbeat re-affirms `self.target`, and the buttons write 0 W, + # which needs no allocator at all (see part 7). + for wanted in (9999.0, -9999.0): + arm(c, wanted) + await c.write_setpoint(wanted, force=True) + # A nan is loss of input, not an unauthorised write: rule 1 turns it into + # 0 W ABOVE the gate, and 0 W is always authorised. No arming, on purpose - + # if the gate ever moved above the coercion this line would go red. await c.write_setpoint(float("nan"), force=True) + arm(c, 1500.0) await c.write_setpoint(1500.0, force=True) return hass, c @@ -807,6 +848,11 @@ async def _write_path(moved=False): # The inverter is SMALLER than the configured ceiling - SAFETY-20's install. c.dev_min, c.dev_max = -2500.0, 2500.0 c.target = 3000.0 + # SAFETY-28: the write path only carries numbers the allocator placed, so + # this stimulus is authorised the same way cycle()'s is. It has to be: the + # clamp is what this part grades, and a gate refusal would substitute 0 W + # and there would be no clamp to see. + arm(c, c.target) if moved: await c.check_output_clamp() await c.write_setpoint(c.target) @@ -852,6 +898,321 @@ check("...and main.py:856's consumer is the line that goes quietly false", bool(ctl_w.clamp_last and ctl_w.clamp_last.engaged)) +# ============================================================================== +print() +print("7. SAFETY-28: the write path is gated on the allocation record") + +# WHY THIS PART IS IN THIS FILE. Part 5 already claims "no path reaches the wire +# around the clamp", and the clamp is the last thing between a bug and the +# hardware. SAFETY-28 adds a second thing on the same path and asks a different +# question of it: not "is this number in range" but "WHO CHOSE THIS NUMBER". +# +# THE DEFECT IT REPLACES. specs §1.5 makes two writers on one actuator a +# non-goal, and until this ticket that was enforced by +# `len(validate.ACTUATOR_WRITE_KEYS) == 1` - one option key, so a second writer +# is not expressible. True, and a statement about the OPTIONS SCHEMA rather than +# about the writes: add CFG-04's per-inverter config and there are two real +# writers with that length still 1. test_validate.py part 2 grades the config +# half; this part grades the write half, which is the one that does not care +# what the options look like. + +A_UNIT, B_HYPO = "inverter-a", "inverter-b" + + +def fresh(**over): + """A Controller with a stub HA, no cycle run, and therefore NO record.""" + hass = _Hass(0.0) + c = Controller(dict({"setpoint_entity": "number.sp", "max_w": 2000, + "meter_source": "off"}, **over), + hass, _Store(), _Pub(), stop=asyncio.Event()) + c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W + return hass, c + + +# --------------------------------------------------------------------------- # +# 7a. AC 1 - an unauthorised value is REFUSED AT THE WRITE PATH +# --------------------------------------------------------------------------- # +# The second writer, modelled as what it would actually look like in code: some +# other strategy with arithmetic of its own calling the one method that reaches +# the wire. It has a perfectly reasonable number - in range, finite, nothing the +# clamp would object to - and the only thing wrong with it is that nothing +# allocated it. +async def _second_writer(): + hass, c = fresh() + arm(c, 1500.0) # the allocator placed 1500 W this tick + await c.write_setpoint(1500.0, force=True) # the loop's own write + await c.write_setpoint(1200.0, force=True) # ...and the second writer's + return hass, c + + +hass_2w, ctl_2w = asyncio.run(_second_writer()) +print(f" allocator placed +1500 W; a second writer asked for +1200 W; " + f"the device saw {hass_2w.writes}") +check("AC 1: a value the allocation record does not contain never reaches the " + "device. 1200 W is in range and finite - the clamp has no opinion about " + "it - so the ONLY thing that stopped it is that no allocator placed it", + 1200.0 not in hass_2w.writes) +check("...and it is REFUSED, not merely logged: 0 W goes on the wire in its " + "place. Returning instead would leave the inverter latched on the last " + "command with the ESP32 heartbeat still being fed - holding an " + "unauthorised setpoint quietly, which is the outcome this gate exists " + "to prevent", + hass_2w.writes == [1500.0, 0.0]) +check("...and the action log says so in words an operator can act on", + any("WRITE REFUSED" in e for e in ctl_2w.events)) +check("...while the authorised write of the SAME shape went through " + "untouched, so the gate is not simply refusing everything", + hass_2w.writes[0] == 1500.0) + +# --------------------------------------------------------------------------- # +# 7b. AC 2 - a record from a previous tick cannot authorise a write in this one +# --------------------------------------------------------------------------- # +# ⚠️ THIS IS THE CACHED-ENVELOPE FAULT WEARING A DIFFERENT HAT. allocator.py's +# rule is that a unit's interval is read per tick and never cached, because a +# pack at its floor or a stale link moves it between one cycle and the next. +# A record is that envelope written down - so code that keeps one and replays it +# is caching the envelope through the record, and the allocator's own purity +# cannot stop it. The tick stamp is what stops it. +async def _replay(): + hass, c = fresh() + stale = arm(c, 1500.0) # tick N + await c.write_setpoint(1500.0, force=True) + arm(c, 800.0) # tick N+1: the envelopes have moved on + c.allocation = stale # ...and somebody puts the old one back + await c.write_setpoint(1500.0, force=True) + return hass, c, stale + + +hass_rp, ctl_rp, stale_rec = asyncio.run(_replay()) +print(f" a tick-{stale_rec.tick} record replayed on tick " + f"{ctl_rp.alloc_tick}: the device saw {hass_rp.writes}") +check("AC 2: replaying the previous tick's record does NOT authorise its " + "value again - the write is refused and 0 W goes out instead", + hass_rp.writes == [1500.0, 0.0]) +check("...and the refusal names both ticks, so the reason is legible rather " + "than 'computer says no'", + "tick" in (ctl_rp.write_refusal(1500.0) or "") + and str(stale_rec.tick) in (ctl_rp.write_refusal(1500.0) or "") + and str(ctl_rp.alloc_tick) in (ctl_rp.write_refusal(1500.0) or "")) +check("...and an UNSTAMPED record - one built by a caller that passed no tick " + "at all - is refused too, because 'I cannot say when I was made' must " + "fail closed and not read as tick zero", + ctl_rp.allocation is not None + and (lambda c: (setattr(c, "allocation", + plan(1500.0, {A_UNIT: UnitBounds()})) + or c.write_refusal(1500.0) is not None))(ctl_rp)) + +# ⚠️ AND THE TICK HAS TO ADVANCE ON THE LIVE PATH, WHICH IS A DIFFERENT CLAIM +# FROM ANY OF THE ABOVE. Everything so far drives `arm()`, which bumps the +# counter itself - so all of it would stay green with `self.alloc_tick += 1` +# deleted from cycle(), and the replay protection would be gone on the only path +# that matters. This drives REAL cycles and asks the same question of them. +async def _live_staleness(): + hass = _Hass(6000.0) + c = Controller({"setpoint_entity": "number.sp", "meter_entity": "sensor.grid", + "soc_entity": "sensor.soc", "batt_entity": "sensor.batt", + "meter_source": "off", "max_w": LIVE_MAX_W, "gain": 0.6, + "slew_w": 1000, "step_w": 10, "stale_input_s": 15}, + hass, _Store(), _Pub(), stop=asyncio.Event()) + c.auto = True + c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W + await c.read_inputs() + await c.cycle() + first, first_v = c.allocation, c.target + await c.read_inputs() + await c.cycle() + # ...and now the HOLD: one missed poll, which must NOT age the record, or a + # single dropped meter reading would drop the site to 0 W. + held_before = c.alloc_tick + c.grid = None + c.inputs_bad_since = datetime.now(timezone.utc) + await c.cycle() + hass.writes.clear() + await c.write_setpoint(c.target, force=True) # the heartbeat's own call + return c, first, first_v, held_before, hass + + +ctl_ls, rec1, val1, held_before, hass_ls = asyncio.run(_live_staleness()) +print(f" two real cycles: tick {rec1.tick} -> {ctl_ls.allocation.tick}; " + f"a held cycle left it at {ctl_ls.alloc_tick}") +check("PRECONDITION: two real cycles produced two records with two different " + "ticks and a value worth replaying", + rec1.tick != ctl_ls.allocation.tick and val1 != 0.0) +check("the tick advances on the LIVE path, so cycle 1's record does not " + "authorise a write on cycle 2. Everything above this line drives the " + "test's own `arm()` and would stay green with the increment deleted from " + "cycle() - this is the check that would not", + (lambda c: (setattr(c, "allocation", rec1) + or c.write_refusal(val1) is not None))(ctl_ls)) +check("⚠️ AND A CYCLE THAT PRODUCED NO RESOLUTION DOES NOT AGE IT. The " + "'inputs missing, not yet stale' hold returns before the allocator runs, " + "precisely so one dropped meter reading does not zero the inverter, and " + "the heartbeat re-affirms self.target through this same gate while it " + "does. A tick incremented per LOOP ITERATION rather than per RESOLUTION " + "would refuse that heartbeat and turn the hold into the outage it exists " + "to prevent", + ctl_ls.alloc_tick == held_before and hass_ls.writes == [ctl_ls.target] + and ctl_ls.target != 0.0) + +# --------------------------------------------------------------------------- # +# 7c. THE FAILSAFE CANNOT DEADLOCK BEHIND THE GATE +# --------------------------------------------------------------------------- # +# ⚠️ THE HAZARD THIS PART EXISTS FOR. `write_setpoint()` is also how layer 2 +# commands 0 W: cycle()'s two refusal branches substitute a 0 W failsafe and +# write it like any other cycle, and shutdown() and the operator's stop button +# do the same. Every one of those runs while the allocator has REFUSED - `ok` is +# False and there are no shares - so a gate that asked the allocator's +# permission to command zero would block the safety write behind the very +# component whose failure the safety write answers. A gate that can do that is +# worse than the hole it closes. +# +# The resolution: 0 W is authorised unconditionally, and it is authorised on the +# VALUE rather than on a `failsafe=True` flag the caller asserts. The argument is +# at Controller.write_refusal(): the hazard §1.5 names is N-times loop gain from +# broadcasting one target to N inverters, and at S = 0 the broadcast and the +# correct allocation COINCIDE - a_i = 1 for every unit gives every unit 0 W, +# which is exactly its share. There is no over-command to make at zero. +_hostile = [] +_h, _c = fresh() +_hostile.append(("no record at all", _c)) +_h, _c = fresh() +_c.alloc_tick = 1 +_c.allocation = plan(1500.0, {A_UNIT: UnitBounds()}, scope_errors=("mis-routed",), + tick=1) +_hostile.append(("the allocator REFUSED this tick", _c)) +_h, _c = fresh() +_c.allocation = arm(_c, 1500.0) +_c.alloc_tick += 7 # ...and then seven ticks went by +_hostile.append(("only a stale record", _c)) +for _why, _c in _hostile: + check(f"0 W is authorised with {_why}", _c.write_refusal(0.0) is None) + check(f"...and a NON-zero write is still refused with {_why}, so the zero " + f"exit is a floor and not an amnesty", + _c.write_refusal(1500.0) is not None) + + +async def _failsafe_wire(): + """The refusal path END TO END, through the real cycle(). + + ⚠️ THE ROSTER HERE IS A HYPOTHESIS AND NOTHING IS MEASURED FROM IT. + INVERTER B DOES NOT EXIST. It is named only so `live_units()` can return + nothing and drive the allocator into REFUSE_EMPTY, which is the state + main.py's first refusal branch is written for and the one this check needs: + the allocator itself has failed, and layer 2 must still be able to command + zero. + """ + hass = _Hass(0.0) + + class _Blind(Controller): + def live_units(self): + return () + + c = _Blind({"setpoint_entity": "number.sp", "meter_source": "off", + "max_w": LIVE_MAX_W, "soc_entity": "sensor.soc", + "batt_entity": "sensor.batt", "meter_entity": "sensor.grid"}, + hass, _Store(), _Pub(), stop=asyncio.Event()) + # ⚠️ DEV-02: THE ROSTER IS GROWN BY ADDING AN `Inverter`, NOT BY ASSIGNING + # `units`. `Controller.units` is now DERIVED from `Controller.inverters` and + # has no setter, because a settable roster is a site that claims two + # actuators while holding one writer - a half-driven site with nothing to + # say so. The object carries the writer, which is what makes this fixture a + # roster of two rather than a tuple of two names. + c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W # while the roster is one + _b = Inverter(B_HYPO, setpoint_entity="number.sp_b", + soc_entity="sensor.soc", batt_entity="sensor.batt", + max_w=LIVE_MAX_W, store=c.store) + _b.dev_min, _b.dev_max = -LIVE_MAX_W, LIVE_MAX_W + _b.soc, _b.batt = 50.0, 0.0 + c.inverters.append(_b) + c.auto = True + for _ in range(3): + await c.read_inputs() + await c.cycle() + return hass, c + + +hass_fs, ctl_fs = asyncio.run(_failsafe_wire()) +print(f" allocator refused every tick " + f"({ctl_fs.allocation.refused!r}); the device saw {hass_fs.writes}") +check("the allocator having failed, the 0 W failsafe still REACHES THE WIRE - " + "this is the deadlock the gate must not have, driven through the real " + "cycle() and its real refusal branch rather than asserted", + not ctl_fs.allocation.ok and hass_fs.writes == [0.0, 0.0] + and ctl_fs.target == 0.0) +check("...and it was the ALLOCATOR's refusal that produced it, not the gate's " + "- if the gate were the thing zeroing this cycle the action log would " + "say WRITE REFUSED and the distinction between the two failures would " + "be lost to whoever has to debug it", + any("ARBITER: REFUSED" in e for e in ctl_fs.events) + and not any("WRITE REFUSED" in e for e in ctl_fs.events)) + +# --------------------------------------------------------------------------- # +# 7d. `unmet_w` IS NOT `ok`, AND THE GATE MUST NOT FUSE THEM +# --------------------------------------------------------------------------- # +# allocator.py says it plainly: an allocation that could not place every watt is +# still SAFE TO WRITE and strictly better than commanding zero. Fusing the two +# would let ONE SATURATED INVERTER PARK THE WHOLE SITE - the site target goes +# unmet by 3800 W, every share is legal, and a gate reading `unmet_w` would +# answer that by commanding 0 W and meeting even less of it. +_tight = plan(4000.0, {A_UNIT: UnitBounds(lo=-100.0, hi=100.0), + B_HYPO: UnitBounds(lo=-100.0, hi=100.0)}, tick=1) +_h, _c = fresh() +_c.alloc_tick, _c.allocation = 1, _tight +print(f" saturated site: {_tight.explain()}") +check("PRECONDITION: this record is ok AND has watts it could not place", + _tight.ok and _tight.unmet_w == 3800 and _tight.shares[A_UNIT] == 100) +check("the gate authorises this unit's share anyway. `unmet_w` is not read - " + "it is the site target that went unmet, not a broken allocation", + _c.write_refusal(100.0) is None) +check("...and it still refuses a value that is not the share, so 'do not read " + "unmet_w' has not been implemented as 'read nothing'", + _c.write_refusal(4000.0) is not None) + +# --------------------------------------------------------------------------- # +# 7e. A RECORD THAT CONTRADICTS ITSELF AUTHORISES NOTHING +# --------------------------------------------------------------------------- # +# `UnitPlan.explain()` renders a share outside its own interval as +# "ENVELOPE VIOLATED, DO NOT WRITE". This is the DO NOT WRITE. The shape is the +# half-open clamp defect ALLOC-01 fixed: a plan stating the floor and a share +# below it in the same breath, and agreeing with itself. +# +# ⚠️ UNREACHABLE FROM THE ALLOCATOR AS IT STANDS, AND GRADED ANYWAY. At the +# shipped roster of one the pass-through's interval is (-inf, +inf) and every +# number is inside it. PR #40's reviewer proved what an unreachable ungraded +# branch is worth by replacing one with `if False:` and watching the whole suite +# stay green, so this one is exercised on a constructed record rather than +# trusted. +_liar = Allocation(target_w=500.0, sum_w=500, shares={A_UNIT: 500}, + plans=(UnitPlan(A_UNIT, 500, 2000.0, math.inf, 1.0, + "weighted share"),), tick=1) +_h, _c = fresh() +_c.alloc_tick, _c.allocation = 1, _liar +print(f" {_liar.plans[0].explain()}") +check("PRECONDITION: the record is `ok`, and its share IS what the gate would " + "otherwise match on - so nothing but the envelope question stops it", + _liar.ok and _liar.shares[A_UNIT] == 500) +check("a share outside its own plan's interval is refused, and the refusal " + "quotes the plan's own DO NOT WRITE line", + "DO NOT WRITE" in (_c.write_refusal(500.0) or "")) + +# --------------------------------------------------------------------------- # +# 7f. AC 5 - AT N=1 NOTHING CHANGES +# --------------------------------------------------------------------------- # +# The strongest form of this is not a new assertion, it is the 17 other files in +# this suite passing unedited on real cycles. What is added here is the +# statement that the live path is not being quietly zeroed: over part 3's +# sixty-cycle replay - the 10.3 runaway, an inverter that never follows - the +# gate refused nothing at all. +check("AC 5: across part 3's 60 real cycles the gate refused ZERO writes, so " + "the shipped one-inverter path reaches the wire exactly as it did " + "before. A gate that silently substituted 0 W here would leave every " + "clamp check above green and the install dead", + not [e for e in ctl_r.events if "WRITE REFUSED" in e]) +check("...and the numbers those cycles put on the wire are the arbiter's own " + "target, unrounded and unaltered by the record it now has to appear in", + hass_r.writes and all(abs(w) <= LIVE_MAX_W for w in hass_r.writes)) + + print() if fails: print(f"{len(fails)} FAILED: {', '.join(fails)}") diff --git a/goodwe_controller/test_inverter.py b/goodwe_controller/test_inverter.py index 41e5493..474026b 100644 --- a/goodwe_controller/test_inverter.py +++ b/goodwe_controller/test_inverter.py @@ -66,6 +66,7 @@ from app.store import ( ) from app.validate import FIRMWARE_INGEST_VETO_S from app.freeze import rail_w +from app.arbiter import P_LOOP, P_SAFETY, Claim, resolve fails = [] HERE = Path(__file__).resolve().parent @@ -213,6 +214,36 @@ def inverter_b(store=None, max_w=B_RATING_HYPOTHESIS, dev=None, return inv +def arm(c, shares): + """Give `c` a real allocation record authorising exactly `shares`. + + ⚠️ SAFETY-28 MADE THIS NECESSARY AND THE FACT THAT IT IS NECESSARY IS THAT + TICKET. `write_setpoint()` refuses any number the single allocator did not + place on this tick, and the probes below call it directly - the way the + heartbeat and the operator's buttons do - so they have to say where their + number came from. Before that gate they did not, and that was the hole. + + ⚠️ AND IT IS BUILT THROUGH THE SHIPPED `resolve()`, NOT BY HAND, for the + reason test_clamp.py's twin says: a helper that assembled an `Allocation` + itself would be a second allocator living in the test file. The per-unit + numbers are forced the way the add-on forces them - each unit files its own + `supervised` envelope and the site target is their sum, so the water-fill + has exactly one answer - and the assert is what says this fixture got it. + """ + c.alloc_tick += 1 + claims = [Claim.set("loop", P_LOOP, float(sum(shares.values())), + "test stimulus")] + for unit, watts in shares.items(): + claims.append(Claim.limit("supervised", P_SAFETY, -abs(float(watts)), + abs(float(watts)), "max_w", actuator=unit)) + res = resolve(claims, units=c.units, live=c.units, tick=c.alloc_tick) + c.allocation = res.allocation + assert res.allocation.shares == {u: float(w) for u, w in shares.items()}, ( + f"arm() asked for {shares} and the shipped allocator placed " + f"{res.allocation.shares} - fix the fixture, not the assert") + return res.allocation + + def run(coro): return asyncio.run(coro) @@ -320,6 +351,15 @@ check("...and the views are PROPERTIES, not attributes, so `c.soc = 4` writes " print() print("2. AC 2: the store migration, and it is GRADED rather than performed") +async def _one_cycle_early(c): + """One real cycle. `_one_cycle` below is the same three lines; this one is + needed before it, and duplicating three lines beats moving a definition + somebody is reading in place.""" + await c.read_inputs() + await c.cycle() + return c + + # ⚠️ THE RISKY HALF, IN THE CARD'S OWN WORDS. Namespacing `sign_tripped` is # right - flat, a trip on B latches A - but a latch that silently fails to move # is a disarmed safety guard on an install whose state file still says @@ -360,24 +400,109 @@ check("...and the keys that moved are exactly the ones store.py names as " and "phase" not in LEGACY_UNIT_KEYS) # IDEMPOTENCE, AND IT IS THE ONE THAT PROTECTS THE OPERATOR'S CLEAR. -_pre.set(unit_key(UNIT_A, "sign_tripped"), False) -_pre.set(unit_key(UNIT_A, "sign_reason"), "") +# +# ⚠️ THE CLEAR IS PRESSED, NOT SIMULATED, AND THAT CHANGE IS THE TICKET. This +# check used to clear the NAMESPACED key by hand and leave the flat one saying +# `true`, then assert that a restart did not resurrect the latch. It graded a +# state the shipped clear path does not produce: `handle_action("auto_toggle")` +# clears BOTH spellings, precisely so that `flat True + namespaced False` can +# only ever mean a trip taken while the install was rolled back (see below). +# Driving the button is also the only way to find that out - a fixture that +# hand-writes the store is a check on the fixture. +run(_c2.handle_action("auto_toggle")) # the operator presses START _writes_before = _pre.writes _c2b, _ = built(store=_pre) check("⚠️ RUNNING AGAIN AFTER AN OPERATOR CLEARED THE LATCH DOES NOT RESURRECT " - "IT. The flat key still says `true` - it is a fossil, left in the file on " - "purpose - and a migration that re-read it would re-arm a failsafe the " - "operator has already dealt with, once per restart, forever", + "IT. A guard that went on comparing against a flat key nothing cleared " + "would re-arm a failsafe the operator has already dealt with, once per " + "restart, forever - which is what the `rep.ran` gate used to buy, and " + "what clearing both spellings buys instead", not _c2b.inverters[0].sign_wd.tripped and _pre.data[unit_key(UNIT_A, "sign_tripped")] is False) check("...and it wrote nothing at all the second time, which is what " "`schema_version` buys", _pre.writes == _writes_before) -check("...and the flat keys are still there, unread. Deleting them costs a " - "ROLLBACK: an operator who downgrades the add-on after a migration would " - "find no latch at all. Left in place the staleness fails toward a " - "spurious 0 W hold, which is the safe direction", - _pre.data["sign_tripped"] is True) +check("⚠️ AND THE OPERATOR'S PRESS CLEARED THE PRE-DEV-02 FLAT KEYS TOO. That " + "is what makes the two spellings comparable at all: cleared together, " + "the only disagreement they can reach is `flat says tripped, nothing " + "namespaced does` - which is a latch written by a version that had no " + "namespaces, i.e. a trip taken while this install was ROLLED BACK", + _pre.data["sign_tripped"] is False and _pre.data["sign_reason"] == "") + +# ⚠️ THE ROLLBACK ROUND TRIP, RUN END TO END. THIS IS THE STATE THE `rep.ran` +# GATE SWALLOWED, and it is not a thought experiment - a firmware version change +# with a rollback path was performed on this site on 2026-09-02. store.py's +# rule 2 keeps the flat keys "so a rollback still finds its latch"; the operator +# path that rule contemplates is a ROUND TRIP, and the guard used to return +# early on every start that did not migrate, which is every start after the +# first. +# +# 1. install migrates -> schema_version=1, unit/inverter-a/... = False +# 2. operator DOWNGRADES, and SAFETY-02 trips inside the downgrade window. +# The old add-on knows only the flat key, so that is all it writes. +# 3. operator upgrades again. `rep.ran` is False - the store is already at +# schema 1 - so the migration and the guard were both skipped and +# `Inverter.__init__` read the namespaced key, which is still False. +# +# Observed before this fix: the controller came up with `sign_wd.tripped = +# False` while /data/state.json said `sign_tripped: true`. That is store.py's +# own header sentence - the file still says tripped, the banner is green, and +# the controller resumes driving the wrong way - rebuilt by the version marker. +_trip = MemStore() +_c2r0, _ = built(store=_trip) # 1. install, migrate +check("PRECONDITION 1: the install migrated and came up armed and untripped", + _trip.data[SCHEMA_KEY] == SCHEMA_VERSION + and _trip.data[unit_key(UNIT_A, "sign_tripped")] is False + and not _c2r0.inverters[0].sign_wd.tripped) +# 2. THE DOWNGRADE WINDOW. The old add-on is the one that shipped before this +# card: it writes the flat pair and knows nothing about `unit/...`. Written the +# way that version wrote them - two `Store.set()` calls - rather than by +# reaching into `data`, so this is the file an old add-on would leave behind. +_trip.set("sign_reason", "SIGN_DISAGREEMENT: commanded -1800 W against a " + "meter reading +2100 W for 181 s") +_trip.set("sign_tripped", True) +check("PRECONDITION 2: the rolled-back add-on latched under the flat key ONLY, " + "and left the namespaced one exactly as the migration wrote it. This is " + "the state that made `rep.ran` a trap - the store is already at schema " + "1, so nothing on the next start has any reason to run", + _trip.data["sign_tripped"] is True + and _trip.data[unit_key(UNIT_A, "sign_tripped")] is False + and _trip.data[SCHEMA_KEY] == SCHEMA_VERSION) +_c2r, _ = built(store=_trip) # 3. upgrade again +print(f" the rollback round trip: {_c2r.store_migration.explain()}; " + f"inverter-a came up tripped={_c2r.inverters[0].sign_wd.tripped}") +check("PRECONDITION 3: this start did NOT migrate anything, which is exactly " + "why the guard used to be skipped on it", + not _c2r.store_migration.ran) +check("⚠️ AND THE LATCH IS CAUGHT ANYWAY. The guard runs on EVERY start now, " + "so a trip taken while the install was rolled back is re-armed instead " + "of being silently discarded by the version marker. A latch that a " + "version change disarms is worse than one that was never persisted, " + "because the state file is evidence saying it is fine", + _c2r.inverters[0].sign_wd.tripped + and "SIGN_DISAGREEMENT" in _c2r.inverters[0].sign_wd.reason + and "-1800 W" in _c2r.inverters[0].sign_wd.reason) +check("...and it is persisted under the NAMESPACED key as it re-arms, so the " + "next start reads it from the live spelling rather than re-deriving it", + _trip.data[unit_key(UNIT_A, "sign_tripped")] is True) +check("...and the operator is told in the action log, because a latch re-armed " + "on their behalf that only appears in a banner is a 0 W hold with no " + "explanation", + any("re-armed" in e for e in _c2r.events)) +run(_one_cycle_early(_c2r)) +check("...and it commands 0 W on its first cycle, which is what makes the " + "re-arm a latch rather than a log line", + _c2r.target == 0.0 and "failsafe" in _c2r.reason) +run(_c2r.handle_action("auto_toggle")) +_c2r2, _ = built(store=_trip) +check("⚠️ AND THE OPERATOR CAN STILL GET OUT OF IT - ONCE. The press clears " + "both spellings, so the next start does NOT re-arm. This is the " + "'re-arm forever' objection the `rep.ran` gate existed to answer, " + "answered where it belongs: by clearing the fossil rather than by " + "refusing to look at it", + not _c2r2.inverters[0].sign_wd.tripped + and _trip.data["sign_tripped"] is False + and _trip.data[unit_key(UNIT_A, "sign_tripped")] is False) # A FRESH INSTALL MUST NOT INVENT A LATCH. _fresh = MemStore() @@ -518,6 +643,15 @@ check("⚠️ AND THE LATCHES ARE UNDER SEPARATE KEYS, which is the half that " "it, and every Controller built afterwards read it as A's", _s3.data.get(unit_key("inverter-b", "sign_tripped")) is True and _s3.data.get(unit_key(UNIT_A, "sign_tripped")) is False) +check("⚠️ ...AND THE TRIP IS MIRRORED ONTO THE PRE-DEV-02 FLAT PAIR, WHICH IS " + "THE ROLLBACK VIEW. store.py's rule 2 keeps the flat keys 'so a rollback " + "still finds its latch', and that only ever covered the latch present AT " + "MIGRATION TIME: nothing flat was written for a trip taken afterwards, so " + "an operator who downgraded found `sign_tripped: false` and came up " + "DRIVING, into the fault. The site-level spelling is right for it - the " + "version being rolled back TO drives one actuator", + _s3.data.get("sign_tripped") is True + and "SIGN_DISAGREEMENT" in str(_s3.data.get("sign_reason", ""))) # THE RESTART, which is the whole reason the latch is persisted at all. _h3b = Hass({"sensor.grid": 2000.0, "sensor.soc_a": 50.0, @@ -563,6 +697,19 @@ check("pressing start clears EVERY latched inverter, under its own key. One " not _c3r.inverters[1].sign_wd.tripped and _s3.data[unit_key("inverter-b", "sign_tripped")] is False and _s3.data[unit_key("inverter-b", "sign_reason")] == "") +check("...and the same press cleared the flat rollback view, so a downgrade " + "after a clear does not find a trip the operator has already dealt with " + "- and neither does the next start, which now reads that key on EVERY " + "boot rather than only on the one that migrates", + _s3.data["sign_tripped"] is False and _s3.data["sign_reason"] == "") +_c3r2, _ = built({"sign_disagree_w": 300, "sign_disagree_s": 1}, store=_s3, + hass=_h3b) +check("⚠️ AND THE RESTART AFTER THAT CLEAR COMES UP UNLATCHED. This is the " + "'re-arm forever' hazard the every-start guard would have if the clear " + "did not reach both spellings, driven through the real button and a real " + "reconstruction rather than argued", + not _c3r2.inverters[0].sign_wd.tripped + and not any("re-armed" in e for e in _c3r2.events)) # ============================================================================== @@ -612,6 +759,10 @@ check("...and a bound that could NOT be read is skipped rather than read as " # THE CLAMP. Same requested value, two inverters, two answers. async def _clamp_both(): c, hass = built(store=MemStore(), second=inverter_b()) + # SAFETY-28's gate is upstream of the clamp, so the out-of-range request + # has to be one the allocator actually placed or it is substituted with + # 0 W and this part grades the gate instead of the clamp. + arm(c, {"inverter-a": 9999.0, "inverter-b": 9999.0}) await c.write_setpoint(9999.0, force=True, unit=c.inverters[0]) await c.write_setpoint(9999.0, force=True, unit=c.inverters[1]) return c, hass @@ -811,6 +962,7 @@ async def _hung_heartbeat(): hass = Hass({}, {}, hang={"number.sp_a"}, hang_s=30.0) c, _ = built(store=store, hass=hass, second=inverter_b(store)) c.inverters[0].target, c.inverters[1].target = 500.0, 700.0 + arm(c, {"inverter-a": 500.0, "inverter-b": 700.0}) t0 = time.monotonic() await c.send_heartbeats(1000.0, 10.0) return c, hass, time.monotonic() - t0, t0 @@ -851,6 +1003,7 @@ async def _serialised(): hass = Hass({}, {}, hang={"number.sp_a"}, hang_s=30.0) c, _ = built(store=store, hass=hass, second=inverter_b(store)) c.inverters[0].target, c.inverters[1].target = 500.0, 700.0 + arm(c, {"inverter-a": 500.0, "inverter-b": 700.0}) t0 = time.monotonic() b_at = None for inv in c.inverters: # <- the shape this card replaces @@ -880,6 +1033,7 @@ async def _split_deadlines(): hass = Hass() c, _ = built(store=store, hass=hass, second=inverter_b(store)) c.inverters[0].target, c.inverters[1].target = 500.0, 700.0 + arm(c, {"inverter-a": 500.0, "inverter-b": 700.0}) # A was refreshed a moment ago; B has not been refreshed for a long time. c.inverters[0].last_beat, c.inverters[1].last_beat = 1000.0, 0.0 await c.send_heartbeats(1005.0, 10.0) @@ -1152,6 +1306,26 @@ MUTATIONS = [ "test_inverter.py part 2: 'the migration REPORTS that it failed'"), ("`_grade_store_migration()` reduced to a log line", "test_inverter.py part 2: 'the controller REFUSES to come up disarmed'"), + # ⚠️ THE THREE ROWS BELOW ARE ONE DEFECT SEEN FROM THREE SIDES, and it was + # found by review rather than by a check: the latch was silently DISARMED + # by the rollback ROUND TRIP store.py's rule 2 exists for. The guard used to + # return early on `not rep.ran`, which is every start after the first, so a + # trip taken inside a downgrade window was discarded by the version marker. + ("`_grade_store_migration()` gated on `rep.ran` again", + "test_inverter.py part 2, the ROLLBACK ROUND TRIP: 'AND THE LATCH IS " + "CAUGHT ANYWAY'. Every other check in that part passes with the gate " + "restored, because every other one runs on the start that migrates"), + ("the operator's clear leaving the pre-DEV-02 flat keys set", + "test_inverter.py part 2: 'THE OPERATOR CAN STILL GET OUT OF IT - ONCE' " + "and part 3's 'the restart after that clear comes up UNLATCHED' - i.e. " + "the re-arm-forever hazard, which is why the `rep.ran` gate existed"), + ("a trip writing only the namespaced keys, not the flat rollback view", + "test_inverter.py part 3: 'THE TRIP IS MIRRORED ONTO THE PRE-DEV-02 FLAT " + "PAIR'. A downgrade after a post-migration trip found no latch at all"), + ("the guard asking only `self.inverters[0]` whether anything is latched " + "instead of asking the STORE - so B's own mirrored trip re-arms A", + "test_inverter.py part 3: 'a brand new Controller comes back with B " + "latched and A NOT' - the flat-key defect returning through its own guard"), ("one shared SignWatchdog: every unit graded on inverter A's command", "test_inverter.py part 3: 'inverter A did NOT trip'"), ("`sign_tripped` written flat again instead of namespaced", @@ -1180,6 +1354,20 @@ MUTATIONS = [ "test_inverter.py part 4: 'the pack floor is the EMPTIEST pack'"), ("`device`/`supervised` filed without `inv` - B's rating on A", "test_peakguard.py 10g: the static routing pin, at the line"), + # ⚠️ THE ROW ABOVE IS THE *ACTUATOR* VARIANT AND IT WAS THE ONLY ONE HERE + # FOR A WHOLE REVIEW ROUND. The BOUND variant below routes every claim + # correctly and builds the NUMBERS from the wrong unit - a different defect + # with a different fix, invisible to the routing pin. It survived all 19 + # files, because test_peakguard.py's AC 4 check compared B's bound against + # B's `max_w` while the fixture defaulted B's `max_w` to A's. Both halves + # are fixed: B now has a different rating and the bound is asserted against + # literal numbers. ANY CHECK ASSERTING "X DIFFERS FROM Y" MUST BE RUN ONCE + # WITH X AND Y ACTUALLY MADE DIFFERENT. + ("the per-unit `device`/`supervised` BOUNDS read off `self.inverters[0]` " + "while the `actuator=` stays `inv` - right envelope, wrong numbers", + "test_peakguard.py 10g: 'AND EACH UNIT'S supervised BOUND IS ITS OWN " + "max_w' and the two lines under it, which read literal 5000/3000 rather " + "than an expression that can compare a number against itself"), ("the stop button zeroing `self.target` but not each `inv.target`", "test_inverter.py part 6: 'the next heartbeat does NOT put the old shares " "back' - a hazard THIS CARD created, because the heartbeat re-emits a " @@ -1198,7 +1386,7 @@ for what, caught in MUTATIONS: check(f"all {len(MUTATIONS)} defeats of this card are caught by a named check " f"above. A guard whose checks survive its own defeat is not a guard, and " f"this repository has three separate records of exactly that", - len(MUTATIONS) == 19) + len(MUTATIONS) == 24) print() diff --git a/goodwe_controller/test_peakguard.py b/goodwe_controller/test_peakguard.py index dcc8809..6c1e8c0 100644 --- a/goodwe_controller/test_peakguard.py +++ b/goodwe_controller/test_peakguard.py @@ -2574,18 +2574,32 @@ check("...and the arbiter agrees the routing is legal, on the list main.py " # setpoint entity, its own rating and its own device range, so the claims below # are filed from ITS numbers rather than from a copy of A's. # -# ⚠️ INVERTER B DOES NOT EXIST YET. Its rating here is A's, copied, and that is -# a PLACEHOLDER: nothing in this file is a measured parameter for a second -# device (SPIKE-04 measures one). What is graded is the ARCHITECTURE - which -# actuator each claim lands on - and that is independent of the number. +# ⚠️ INVERTER B DOES NOT EXIST YET. Its rating here is a HYPOTHESIS: nothing in +# this file is a measured parameter for a second device (SPIKE-04 measures one). +# What is graded is the ARCHITECTURE - which actuator each claim lands on, and +# whose number each per-unit bound is built from. +# +# ⚠️ AND IT IS 3000 W RATHER THAN A's 5000 W, WHICH IS NOT COSMETIC. THE RULE, +# STATED AT THE LINE THAT NEEDED IT: ANY CHECK ASSERTING "X DIFFERS FROM Y" MUST +# BE RUN ONCE WITH X AND Y ACTUALLY MADE DIFFERENT. This helper used to default +# B's rating and device range to A's `LIVE_MAX_W`, and the AC 4 check below +# compared inverter B's `supervised` bound against `_b2.max_w` - which was +# LITERALLY THE SAME NUMBER as A's. It read as evidence and observed nothing: +# filing both per-unit claims from `self.inverters[0]`'s numbers instead of +# `inv`'s left all 19 files green. That is the third check this session whose +# DATA could not reach the failure it named (ARB-03's call-site scan saw 3 of 8 +# sites; ALLOC-01's replay generator saw 0 escapes where a widened one saw +# 39,991), so the number is different HERE, once, for every fixture in the file. +B_RATING_HYPOTHESIS = LIVE_MAX_W - 2000.0 + + def second_inverter(ctl, *, name="inverter-b", setpoint="number.sp_b", max_w=None, dev=None): """Append a real second `Inverter` to `ctl`, and return a restore().""" + rating = B_RATING_HYPOTHESIS if max_w is None else max_w inv = Inverter(name, setpoint_entity=setpoint, soc_entity="sensor.soc_b", - batt_entity="sensor.batt_b", - max_w=LIVE_MAX_W if max_w is None else max_w, - store=ctl.store) - inv.dev_min, inv.dev_max = dev or (-LIVE_MAX_W, LIVE_MAX_W) + batt_entity="sensor.batt_b", max_w=rating, store=ctl.store) + inv.dev_min, inv.dev_max = dev or (-rating, rating) # The readings a real cycle needs from it. `inputs_ok` requires EVERY unit # to have answered, which is the point - a pack nothing can grade stops the # site rather than being quietly dropped from the split. @@ -2778,12 +2792,56 @@ check("...and naming those three is not an accident of this fixture but the " {c.source for c in _cl2} == CLAIM_SOURCES - {"failsafe", "safety", "maintenance", "peak", "arbitrage"}) -check("...and each unit's `supervised` bound is ITS OWN `max_w`, not a copy of " - "the first unit's. That is the sentence AC 4 is: give inverter B a " - "different rating and the claim filed on inverter B moves with it", - [(c.lo, c.hi) for c in _cl2 - if c.source == "supervised" and c.actuator == "inverter-b"] - == [(-_b2.max_w, _b2.max_w)]) +# ⚠️ AND NOW THE HALF THAT WAS EVIDENCE-SHAPED AND OBSERVED NOTHING. The three +# checks above grade WHICH ACTUATOR each claim lands on. They say nothing about +# WHOSE NUMBERS the bound is built from - and those are two different defects +# with two different fixes. Filing the per-unit `device` and `supervised` claims +# from `self.inverters[0]`'s numbers while leaving `actuator=self.actuator_of( +# ..., inv)` alone routes every claim correctly and bounds inverter B by +# inverter A's rating: at A=5000/B=3000 the planned envelope for inverter-b +# becomes (-5000, 5000) instead of (-3000, 3000), which is a 2000 W +# over-command the arbiter believes is supervised. +# +# ⚠️ THE RULE THIS LINE EXISTS TO OBEY: ANY CHECK ASSERTING "X DIFFERS FROM Y" +# MUST BE RUN ONCE WITH X AND Y ACTUALLY MADE DIFFERENT. The version this +# replaces compared inverter B's bound against `_b2.max_w` while +# `second_inverter()` defaulted `max_w` to A's `LIVE_MAX_W` - so it compared +# 5000 against 5000, and the mutation above left all 19 files GREEN. The +# PRECONDITION below is what stops that happening again: it fails if the two +# ratings are ever made equal, so the check cannot go quietly vacuous. +_sup2 = {c.actuator: (c.lo, c.hi) for c in _cl2 if c.source == "supervised"} +_dev2 = {c.actuator: (c.lo, c.hi) for c in _cl2 if c.source == "device"} +print(f" supervised bounds by actuator: {_sup2}; device bounds: {_dev2}") +check("PRECONDITION, AND IT IS THE WHOLE POINT: A AND B HAVE DIFFERENT " + "RATINGS. A check that 'B's bound is B's own number' cannot observe " + "anything while B's number IS A's number, and this file shipped that " + "check for a whole review round", + _b2.max_w != c_src.inverters[0].max_w + and (_b2.dev_min, _b2.dev_max) + != (c_src.inverters[0].dev_min, c_src.inverters[0].dev_max)) +check("⚠️ AND EACH UNIT'S `supervised` BOUND IS ITS OWN `max_w`, NOT A COPY OF " + "THE FIRST UNIT'S. That is the sentence AC 4 is, and it is now stated " + "against LITERAL, DIFFERENT numbers rather than against an expression " + "that could be reading the same object twice: B is bounded at " + f"+/-{B_RATING_HYPOTHESIS:.0f} W and A at +/-{LIVE_MAX_W:.0f} W", + _sup2 == {"inverter-a": (-LIVE_MAX_W, LIVE_MAX_W), + "inverter-b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS)}) +check("...and so is each unit's `device` bound, which is the other number the " + "same loop files and the one that was NEVER READ before this card - one " + "`dev_min`/`dev_max` pair came off one `setpoint_entity`, so a second " + "inverter's reported range bound nothing at all", + _dev2 == {"inverter-a": (-LIVE_MAX_W, LIVE_MAX_W), + "inverter-b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS)}) +check("...and the bounds the ARBITER RESOLVED for inverter B - not just the " + "claims filed at it - are B's own, because a claim filed correctly and " + "then resolved against the wrong envelope would be the same defect one " + "layer down", + _n2_alloc is not None + and next((p for p in _n2_alloc.plans if p.unit == "inverter-b"), None) + is not None + and (lambda p: p.lo == -B_RATING_HYPOTHESIS + and p.hi == B_RATING_HYPOTHESIS)( + next(p for p in _n2_alloc.plans if p.unit == "inverter-b"))) # --- 10h. ALLOC-01: WHAT MAIN.PY DOES WITH AN ALLOCATION IT CANNOT DELIVER --- diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index f354704..0391d2b 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -19,8 +19,8 @@ import sys from app.maintenance import MAINT_OPTION_DEFAULTS, MaintConfig from app.validate import ( - ACTUATOR_WRITE_KEYS, FIRMWARE_INGEST_VETO_S, ConfigRefused, refuse, - validate_startup_config, + ACTUATOR_WRITE_KEYS, FIRMWARE_INGEST_VETO_S, ConfigRefused, + actuator_writers, refuse, validate_startup_config, ) fails = [] @@ -135,9 +135,95 @@ for blank in ("", " ", None): check("...and the message says why it is dangerous rather than untidy", why(validate_startup_config(base(setpoint_entity=""), None, None), "heartbeat")) -# specs §1.5: two writers on one actuator is not expressible in this schema. -check("there is exactly one way to reach the actuator", - ACTUATOR_WRITE_KEYS == ("setpoint_entity",)) +# --------------------------------------------------------------------------- # +# specs §1.5: TWO WRITERS ON ONE ACTUATOR, AND WHAT ACTUALLY GRADES IT +# --------------------------------------------------------------------------- # +# ⚠️ WHAT USED TO BE HERE, AND WHY IT WAS NOT ENOUGH. This was one line: +# +# check("there is exactly one way to reach the actuator", +# ACTUATOR_WRITE_KEYS == ("setpoint_entity",)) +# +# and validate.py's comment said the §1.5 non-goal was ENFORCED by construction - +# exactly one such key, so a second writer is not expressible in the options +# schema at all. Both were true. Both graded THE SHAPE OF THE OPTIONS SCHEMA +# rather than the writes, and the difference is not academic: the config below +# has TWO WRITERS ON ONE ACTUATOR and leaves that tuple's length at 1. The check +# stays green while the property it protects is gone, which is this repository's +# named failure mode - a check that observes something other than what it claims +# to observe. +# +# So the first thing this section does is DEMONSTRATE the old check passing on +# the hazardous config, rather than assert in prose that it would have. +_PER_INVERTER_HAZARD = base(inverters=[ + {"id": "inverter-a", "setpoint_entity": "number.goodwe_setpoint"}, + # ⚠️ INVERTER B DOES NOT EXIST. Nothing is measured from it here; it is a + # config fixture, and the only thing that matters about it is that its + # `setpoint_entity` NAMES THE SAME ENTITY as the scalar key above - which is + # the shape of the mistake, and the shape CFG-04 makes reachable. + {"id": "inverter-b", "setpoint_entity": "number.goodwe_setpoint"}]) + +check("⚠️ THE DEFECT, DEMONSTRATED: on a per-inverter config with two writers " + "aimed at ONE entity, the old proxy is still green - it counts option " + "KEYS, and there is still exactly one key called setpoint_entity. This " + "is the check that could not see the thing it was named after", + len(ACTUATOR_WRITE_KEYS) == 1) + +_census = actuator_writers(_PER_INVERTER_HAZARD) +print(f" the same config, censused by writer: " + f"{ {k: len(v) for k, v in _census.items()} }") +check("...and the replacement SEES it, because it counts writers per ACTUATOR " + "rather than keys: three declarations, one entity", + list(_census) == ["number.goodwe_setpoint"] + and len(_census["number.goodwe_setpoint"]) == 3) +_hazard_problems = validate_startup_config(_PER_INVERTER_HAZARD, -5000, 5000) +check("AC 3: and the config is REFUSED, where today it starts", + why(_hazard_problems, "one actuator")) +check("...and the refusal names the actuator and every writer aimed at it, so " + "an operator can see WHICH declarations to remove rather than being told " + "only that something is wrong", + why(_hazard_problems, "number.goodwe_setpoint") + and why(_hazard_problems, "inverters[0].setpoint_entity") + and why(_hazard_problems, "inverters[1].setpoint_entity")) + +# ⚠️ AND THE GATE MUST NOT REFUSE THE CONFIG CFG-04 IS ACTUALLY FOR. A validator +# that fires on the legitimate case gets switched off by the first person it +# inconveniences, and then none of this is enforced. Two inverters with two +# entities are two writers on TWO actuators, which is not the non-goal - it is +# the whole point of the per-inverter roster. +_PER_INVERTER_OK = {k: v for k, v in base().items() if k != "setpoint_entity"} +_PER_INVERTER_OK["inverters"] = [ + {"id": "inverter-a", "setpoint_entity": "number.goodwe_setpoint"}, + {"id": "inverter-b", "setpoint_entity": "number.goodwe_setpoint_b"}] +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) == []) +check("...and today's single-inverter config is untouched: one writer, one " + "actuator, and the census finds it wherever it is declared", + validate_startup_config(base(), -5000, 5000) == [] + and actuator_writers(base()) == + {"number.goodwe_setpoint": ["setpoint_entity"]}) + +# The census is found STRUCTURALLY, not by looking for a container called +# `inverters` - CFG-04 has not chosen that name yet, and a census that guessed +# wrong would go green while two writers existed, which is the same defect one +# step along. +check("a writer nested under a container this file has never heard of is " + "still counted, because the walk is over the options TREE and not over a " + "list of blessed key names", + len(actuator_writers({"setpoint_entity": "number.x", + "whatever_cfg04_calls_it": { + "unit_b": {"setpoint_entity": "NUMBER.X"}}}) + ["number.x"]) == 2) +check("...and entity ids are folded to lower case, so a second writer cannot " + "be smuggled past the census on Number.X versus number.x", + why(validate_startup_config(base(inverters=[{"setpoint_entity": + "NUMBER.GOODWE_SETPOINT"}]), + -5000, 5000), "one actuator")) +check("a BLANK per-inverter entity is not a writer - the same rule the scalar " + "key gets, since write_setpoint() returns immediately on a blank entity " + "and blank is how an operator leaves a slot unconfigured", + validate_startup_config(base(inverters=[{"setpoint_entity": " "}]), + -5000, 5000) == []) # --------------------------------------------------------------------------- # print("3. a meter that can never read") diff --git a/goodwe_controller/test_watchdog.py b/goodwe_controller/test_watchdog.py index f3fcfa1..2b76ec4 100644 --- a/goodwe_controller/test_watchdog.py +++ b/goodwe_controller/test_watchdog.py @@ -446,13 +446,20 @@ check("the trip reached the store, UNDER THE TRIPPED INVERTER'S OWN KEY", store.data[TRIPPED_KEY] is True) check("...with the reason beside it, so the operator is told why", "SIGN_DISAGREEMENT" in store.data[REASON_KEY]) -check("⚠️ ...AND THE FLAT PRE-DEV-02 KEY IS NOT THE ONE THAT MOVED. It was " - "written by the migration at construction, with the value the store then " - "held (untripped), and nothing has written it since. A controller that " - "went on writing the flat key would latch every OTHER inverter on this " - "site as well - which is the defect, and it is only visible as the " - "ABSENCE of a second write here", - store.data.get("sign_tripped") is False) +check("⚠️ ...AND THE FLAT PRE-DEV-02 KEY IS MIRRORED, AS THE ROLLBACK VIEW. " + "This assertion used to read the other way - that the flat key was NOT " + "written - on the argument that writing it would latch every other " + "inverter on the site. That argument was right about the READ and wrong " + "about the WRITE: nothing at runtime reads the flat key, and the one " + "thing that does (`_grade_store_migration()`, on every start) ignores it " + "whenever ANY namespaced latch is set, which a sibling's trip always " + "leaves. What the absence actually cost was the ROLLBACK: store.py's " + "rule 2 keeps this key so a downgrade finds the latch, and a trip taken " + "after the migration wrote nothing here - so an operator who downgraded " + "came up DRIVING. `test_inverter.py` part 3 is where 'B's trip does not " + "latch A across a restart' is graded, on a real roster of two", + store.data.get("sign_tripped") is True + and "SIGN_DISAGREEMENT" in str(store.data.get("sign_reason", ""))) check("the new Controller comes back TRIPPED", post.sign_wd.tripped) check("...and says why, restored rather than re-detected", "SIGN_DISAGREEMENT" in post.sign_wd.reason) @@ -469,9 +476,12 @@ asyncio.run(post.handle_action("auto_toggle")) # -> stopped check("pressing stop does not clear the failsafe", post.sign_wd.tripped and store.data[TRIPPED_KEY] is True) asyncio.run(post.handle_action("auto_toggle")) # -> started -check("pressing start DOES clear it, and clears the store with it", +check("pressing start DOES clear it, and clears the store with it - BOTH " + "spellings, which is what stops the every-start latch guard re-arming a " + "failsafe the operator has already dealt with", not post.sign_wd.tripped and store.data[TRIPPED_KEY] is False - and store.data[REASON_KEY] == "") + and store.data[REASON_KEY] == "" + and store.data.get("sign_tripped") is False) # One more restart from the cleared store, to prove the clear persisted too. fresh = Controller({"setpoint_entity": "number.sp", "meter_source": "off"}, _Hass(), store, _P(), stop=asyncio.Event())