diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 464a770..3508c42 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,70 @@ ## Unreleased +**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/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 4b7ffd4..92a3665 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -299,6 +299,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. @@ -406,6 +425,67 @@ class Controller: elif self.inputs_bad_since is None: self.inputs_bad_since = datetime.now(timezone.utc) + def write_refusal(self, value: float) -> str | None: + """Why `value` may NOT be written 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. + """ + if value == 0.0: + return None + rec = self.allocation + if rec is None: + return ("no allocation record exists yet - nothing has authorised " + f"{value:+.0f} W") + return rec.authorises(self.unit, value, self.alloc_tick) + async def write_setpoint(self, value: float, *, force: bool = False) -> None: entity = self.o.get("setpoint_entity", "") if not entity: @@ -430,6 +510,39 @@ 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. + refusal = self.write_refusal(value) + if refusal is not None: + self.log_event(f"WRITE REFUSED {value:+.0f} W - commanding 0 W") + _LOG.error("SAFETY-28 REFUSED a write of %+.0f W: %s. Commanding " + "0 W instead. Every setpoint must be one the allocator " + "placed on THIS tick - see Controller.write_refusal().", + value, refusal) + value = 0.0 + result = clamp_to_rating(value, float(self.o.get("max_w", 2000)), self.dev_min, self.dev_max) self.clamp_last = result @@ -881,7 +994,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 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 914cd2e..e306684 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,7 +81,8 @@ 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.main import Controller @@ -533,16 +541,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 @@ -799,6 +839,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) @@ -844,6 +889,310 @@ 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()) + c.units = (A_UNIT, B_HYPO) + c.auto = True + c.dev_min, c.dev_max = -LIVE_MAX_W, LIVE_MAX_W + 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] + 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_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")