From 1aef19e995af9c5675a5ba43a909fc65fa58dd50 Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Fri, 4 Sep 2026 13:38:29 +0200 Subject: [PATCH 1/3] MAINT-05: one recalibration per pack, and only one pack moves power at a time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit §12's monthly cycle drains a pack to empty and refills it to true its coulomb counter. There was exactly one `Maintenance` and one un-namespaced history, so at two packs only the first was ever recalibrated - and nothing prevented the cycle draining pack A while the loop charged pack B, which is two power-moving phases on one grid connection pulling against each other. * ONE `Maintenance` PER ROSTER ENTRY, each with its own schedule, phase and history under `unit//...`. `store.migrate_maint_keys()` moves an install's flat history into the first pack's namespace under `migrate_unit_keys()`'s three rules - shared in code via `_move_flat_keys()` and `_verify_moves()`, not restated - on its own marker, because unlike DEV-02's it runs only where the roster has more than one pack. The flat keys are kept LIVE as the rollback view. * ONE MAINTENANCE SLOT PER SITE (`maintenance.arbitrate()`). At most one pack in a power-moving phase - drain, charge AND hold. The holder is DERIVED from the packs' own stored phases, never persisted, so a restart mid-drain re-derives it. Several packs due in the same hour: the slot goes to the one that has gone longest without a completed cycle; roster order breaks a numeric tie and nothing else. Nothing preempts a cycle in flight. * THE DEFERRED PACK IS NOT STARVED AND IS NOT A FAULT. A deferral does not stamp `last_start_attempt` and does not move the phase, so neither the 12 h drain nor the 36 h charge timeout is reachable by waiting. * The claim is filed on the pack whose state machine produced it, and stays SCOPE_UNIT - ARCH-01's rule. * `validate.py` rule 6 grades `maintenance_charge_w` against the WEAKEST cap on the roster now, as its own comment said it must once `maint_unit` stopped being `inverters[0]`. * At a roster of one nothing changes, and it is asserted: no new store key, the migration does not run, the four cycle events are the shipped strings, the banner is the bare phase. * RECORDED NOT FIXED: a drain pins its pack while the loop's target is a site claim, so below the pin the allocator refuses the split and the site parks at 0 W. Pre-existing at 2d025ee. Pinned as a running check. Verified: 19/19 suites green in the CI image; 17 mutations run through tools/mutation_run.sh (bytecode purged), every one observed red. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 54 ++ .../MAINT-05-per-pack-maintenance.md | 182 ++++ goodwe_controller/app/main.py | 411 ++++++--- goodwe_controller/app/maintenance.py | 325 ++++++- goodwe_controller/app/store.py | 177 +++- goodwe_controller/app/validate.py | 55 +- goodwe_controller/test_inverter.py | 27 +- goodwe_controller/test_maintenance.py | 860 +++++++++++++++++- goodwe_controller/test_validate.py | 58 ++ 9 files changed, 1980 insertions(+), 169 deletions(-) create mode 100644 goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index afc7f31..d122b6e 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,60 @@ ## Unreleased +**Every pack is recalibrated on its own schedule, and only one moves power at a +time (MAINT-05).** §12's monthly cycle drains a pack to empty and refills it to +true its coulomb counter. There was exactly one `Maintenance` state machine and +one un-namespaced history, so at two packs **only the first was ever +recalibrated** and the second's counter drifted forever — and nothing prevented +the cycle draining pack A while the loop or arbitrage charged pack B, which is +two power-moving phases on one grid connection pulling against each other. + +* **ONE `Maintenance` PER ROSTER ENTRY**, each with its own schedule, phase and + history under `unit//...`. `store.migrate_maint_keys()` moves an + existing install's flat history into the first pack's namespace under + `migrate_unit_keys()`'s three rules — shared in code, not restated — with the + same read-back postcondition, and the flat keys are kept **live** as the + rollback view so a downgrade finds a current history rather than one dated + migration day. +* **ONE MAINTENANCE SLOT PER SITE.** At most one pack may be in a power-moving + phase (`drain`, `charge` **and** `hold` — `charge` is in the set because a + pack pulled full from the grid beside a pack exporting to empty is 4 kW + circulating with the meter at zero). The holder is **derived** from the packs' + own stored phases, not persisted, so a restart mid-drain re-derives it. When + several packs are due in the same hour the slot goes to the one that has gone + **longest without a completed cycle**; roster order breaks a numeric tie and + nothing else. Nothing preempts a cycle in flight. +* **THE DEFERRED PACK IS NOT STARVED AND IS NOT A FAULT.** A deferral does not + stamp `last_start_attempt` (that is the 90 min post-abort cooldown, inside a + 60 min start hour), and does not move the phase, so neither the 12 h `drain` + nor the 36 h `charge` timeout — *"the battery is not following commands"* — + can be reached by waiting. It says WAITING, names the pack it waits for, and + recalibrates at the next start hour; a test drives the winner's whole cycle + and observes it start. +* **THE SCHEDULE IS NOT STAGGERED BY CONSTRUCTION.** The interlock defers and + the stagger is what comes out of it: one contested month separates two packs + by one cycle length and they stay separated. `DECISIONS/MAINT-05-per-pack-maintenance.md` + §3 carries the argument. +* **`validate.py` rule 6 now grades `maintenance_charge_w` against the WEAKEST + cap on the roster**, like rule 5. Its own comment said it would have to move + the day `maint_unit` stopped being `inverters[0]`; that day is this card. It + refuses configurations that used to start — a 3000 W charge beside a 2000 W + pack that is now recalibrated too — which is the `lo > hi` contradiction rule + 6 exists to refuse, caught when the config is written. Identical at N=1. +* **AT A ROSTER OF ONE, NOTHING CHANGES**, and it is asserted rather than + assumed: the state file keeps the flat keys and gains none, the migration does + not run, the four cycle events are the shipped strings character for + character, and the status banner is the bare phase with no pack name. +* ⚠️ **FOUND, RECORDED, NOT FIXED.** A maintenance drain **pins** its pack while + the loop's target is a **site** claim, so when the site target is below the + pin the only split asks the sibling to charge against the drain and + `allocator.plan()` refuses it — the site parks at 0 W and the pack's 12 h + timeout would eventually report *"drain stuck"* about a healthy battery. + Pre-existing on `release/1.0` at `2d025ee` with the unit names swapped; fixing + it means changing what the loop's target means while a unit is pinned, which + is loop and allocator territory. Pinned as a running check + (`test_maintenance.py` part 12) so it cannot be forgotten. + **At two inverters the sign watchdog stops comparing one unit against the whole site meter, and becomes two checks instead of one (SAFETY-30).** `disagrees(commanded_w, grid_w, ...)` compared the command **this controller put diff --git a/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md b/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md new file mode 100644 index 0000000..a2cae17 --- /dev/null +++ b/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md @@ -0,0 +1,182 @@ +# MAINT-05 — one recalibration per pack, one at a time, and what the card left open + +**Status: DECIDED 2026-09-04.** There is one `Maintenance` per roster entry, +each with its own schedule, phase and history. The site holds exactly **one +maintenance slot**: at most one pack may be in a power-moving phase, the slot is +derived from the packs' own stored phases rather than persisted separately, and +when several packs are due in the same hour it goes to the one that has gone +**longest without a completed cycle**. The schedule is **not** staggered by +construction — the interlock defers, and the stagger emerges from it. + +Filed with MAINT-05. Card `6a9a9e8503872905f298d80a`. + +--- + +## 0. What the card asked to be decided, and the answers + +| the card's open question | the decision | +|---|---| +| §2 the arbitration | **one pack at a time; the other stays under the loop.** Nothing preempts a cycle in flight. | +| §2 who wins when several are due | **the longest-waiting pack**, `last_completed` oldest first, `inf` for never-recalibrated. Roster order breaks a numeric tie and nothing else. | +| §4 staggering | **not by construction.** The interlock defers the second pack; the stagger is what comes out of it. §3 below. | +| AC 7 rule 6's `maint_unit` comment | **no, the first entry's cap is no longer right.** It grades against the weakest cap on the roster now, like rule 5. §4 below. | +| the store layout at N=1 | **flat, unchanged.** The namespace appears the day the roster grows. §2 below. | + +## 1. Why the slot is derived and not persisted + +The holder is *"whichever pack's stored phase is not `idle`"* — a fact already on +disk, written by the packs themselves in the per-pack history this card +introduces. A `maint_slot_holder` key would be a **second opinion about the same +thing**, and the two would disagree the first time the add-on was killed between +writing the phase and writing the holder. Derived, a controller that comes back +up mid-drain reads the drain and re-derives the holder from it; there is no +recovery path to get wrong because there is no second fact. + +`CHARGE` is in `POWER_MOVING` even though the control **loop** owns the setpoint +in that phase, and that is the decision most likely to be "simplified" later. +Pack A charging from the grid at 2000 W while pack B drains 2000 W to export is +4 kW going round in a circle with the meter reading zero and both packs +believing they are behaving — the circulating power [[SAFETY-31]] exists to +detect, arriving as something this module can simply not do. `HOLD` is in the +tuple for the other half of the sentence: it moves nothing, and it **owns the +actuator**, so a second cycle starting under it would still be two +recalibrations at once. + +## 2. Why the store layout at a roster of one does not change + +The maintenance keys are flat — `phase`, `phase_started`, `last_completed`, +`last_start_attempt` — on every install since the add-on shipped. At one pack +there is nothing for a namespace to distinguish that pack **from**: §6.2's +identity, the same one `arbiter.actuator_for()` leans on when it routes every +claim to the site actuator at N=1. So `Maintenance` is constructed with +`unit=None` there, reads and writes the flat keys directly, and +`migrate_maint_keys()` **returns without writing anything**. A one-inverter state +file comes out of this card byte-identical, which is AC 6 and which +`test_maintenance.py` part 10 asserts by driving a whole cycle and comparing the +key set. + +### The migration has its own marker, and that is not tidiness + +`SCHEMA_VERSION` / `schema_version` is DEV-02's, and it is **monotone**: every +install runs that migration exactly once. This one runs **only where the roster +has more than one pack**, so a single counter could not express *"migrated on +this site, deliberately not migrated on that one"* without lying to the next +version that reads it — a one-inverter store would have to be pinned at the old +number forever, and a genuine future v2 migration could then never be told apart +from a site that simply never grew. `maint_schema_version` answers one question +and answers it honestly. + +The **three rules are shared in code**, not restated: `_move_flat_keys()` is +rule 1 (never overwrite an existing namespaced value) and rule 2 (the flat keys +stay in the file), `_verify_moves()` is the read-back postcondition, and the +marker is written last. A second transcription of a migration is a second +opinion about what "it landed" means. + +### And the flat keys are kept LIVE, which is the half DEV-02 had to add later + +Rule 2 originally said the flat keys are left in place and go stale. DEV-02 +amended it, for the sign latch, to *"left in place is not enough"* — and the +same sentence holds for history. A fossil dated the day the second inverter was +declared would hand a rolled-back single-FSM add-on a `last_completed` months +old (it recalibrates a pack that does not need it) or a stale `phase` with a +`phase_started` old enough to abort on the 12 h timeout the moment it comes up. +So `UnitHistory` **mirrors the first pack's writes back onto the flat keys** for +as long as the roster has more than one entry. Only the first pack: the flat +spelling means `inverters[0]` on every install that has one, and a mirror from +pack B would overwrite pack A's rollback view with pack B's phase. + +## 3. Staggering: the interlock defers, and the stagger is the residue + +Two packs on one monthly schedule are due in the same hour every month. The +options were a stagger **by construction** — offset each pack's first cycle by +`interval_days / N` — or **deferral**. Deferral, for three reasons: + +1. **A constructed stagger cannot remove the need for the interlock.** A forced + start, a commissioning cycle, an abort-and-retry, or a pack added mid-month + desynchronises any static offset immediately, so the interlock has to be + there anyway — and then the stagger is a second mechanism that must also be + correct, for a case the first one already handles. +2. **A deferral is observable and a stagger is not.** The deferred pack says, in + the action log and on the status page, that it is waiting, which pack it is + waiting for, and that it will run at the next start hour. A silent offset + moves an operator's configured start day and explains nothing. +3. **The stagger comes out of the deferral for free and then persists.** Pack A + completes on day 1 and is next due on day 29; pack B, deferred, completes on + day 2 and is next due on day 30. One contested month separates them by one + cycle length and they stay separated. + +**Starvation was the risk and it is closed at two points.** A deferral does +**not** stamp `last_start_attempt` — that key is the post-abort cooldown, and at +90 minutes inside a 60-minute start hour, stamping it would lock the deferred +pack out of exactly the hour it is waiting for. And the winner is the +longest-waiting pack rather than the first in the roster, so a pack that keeps +aborting cannot monopolise the slot month after month while its sibling drifts — +which is this card's own defect wearing a different costume. + +**The deferral must not read as a fault.** §12's 12 h and 36 h phase timeouts +mean *"the battery is not following commands"*. A deferred pack stays `IDLE` +with `phase_started` untouched, so no timeout is reachable by waiting, and the +line it logs says WAITING, names the pack it waits for and says it will run +next — none of which any failure path in `maintenance.py` says. + +## 4. Rule 6 (the card calls it rule 8): the first entry's cap is no longer right + +`validate.py`'s rule 6 comment said, in as many words, that +`maintenance_charge_w` was graded against `inverters[0]`'s cap because there was +exactly one `Maintenance` and it could only ever recalibrate the first pack — +and that taking `min()` would be an over-refusal, because it would refuse *"a +5000 W pack being recalibrated beside a 2000 W sibling the cycle never +touches"*. It closed: **"when `maint_unit` stops being `inverters[0]`, this line +moves with it."** + +It has stopped. Every pack is recalibrated on its own schedule, so **the sibling +the cycle never touches does not exist** and the over-refusal argument has no +subject. The shaping claim is filed on whichever pack is charging, so the cap +that can contradict it is any cap on the roster and the one that contradicts it +first is the smallest. + +**This refuses configurations that used to start**, and that is the point rather +than a side effect: `max_w` 5000 on pack A beside 2000 on pack B with +`maintenance_charge_w` 3000 was correct while only pack A was ever recalibrated, +and is now a cheap-window charge asking the arbiter for `hi = -3000` against a +supervised `lo = -2000` — `lo > hi`, arbiter clause 3, 0 W, on the pack nobody +was watching. Refused when it is **written**, which is rule 6's whole job. At a +roster of one `min()` over one cap is that cap: same comparison, same message, +same boundary. + +## 5. The claim stays per-unit, and nothing here files a site claim + +`arbiter.CLAIM_SCOPE` has classified `maintenance` as `SCOPE_UNIT` since it was +written. Two FSMs mean two claims on two actuators, which the allocator consumes +as per-unit intervals. [[ARCH-01]] is explicit that this must never become a +site claim, because per-inverter constraints filed site-wide are the +cross-inverter `lo > hi` contradiction at `arbiter.py:114`. The only line this +card changed there is **which** roster entry the claim is filed on: the one whose +state machine produced the result, rather than `inverters[0]`. + +## 6. FOUND AND NOT FIXED: the site loop still contradicts a drain at N>=2 + +Measured while wiring this card, and **pre-existing on `release/1.0` at +`2d025ee`** — the same drain filed on `inverters[0]` produces the identical +refusal with the two unit names swapped. + +The maintenance SET **pins** its pack. The loop's target is a **site** claim. +When the site target is *below* the pin — a 2000 W drain while the house imports +less than that, which is most of a night — the only split that sums to the site +target asks the sibling to **charge** while the maintained pack **discharges**, +and `allocator.plan()` refuses it by name as circulating power. The refusal is +correct and fails safe: the whole site goes to 0 W with an `ARBITER: REFUSED` +line in the action log. + +**The consequence an operator would see is a misdiagnosis.** The pack is held at +0 W while its state machine believes it is draining, so after 12 h the DRAIN +timeout fires and reports *"drain stuck"* — §12's words for a battery that is not +following commands. The battery is fine; the site refused to allocate. + +**Why it is not fixed here.** The fix is to change what the site loop's target +*means* while a unit is pinned — regulate the meter using only the unpinned +packs — which is loop and allocator territory and changes the integrator's +subject. MAINT-05 moves which pack is pinned and nothing else about it. It is +pinned as a running check (`test_maintenance.py` part 12) rather than left as a +comment, so it cannot be forgotten or quietly tuned away, and it wants a card of +its own. diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 1e18ad9..0b3942d 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -54,14 +54,16 @@ from .hass import HomeAssistant # window an operator would look up here is the one the roster actually uses. from .inverter import (LINK_STALE_S, UNIT_A, build_roster, roster_entries) -from .maintenance import IDLE, MaintConfig, Maintenance +from .maintenance import ( + IDLE, POWER_MOVING, MaintConfig, Maintenance, arbitrate as maint_arbitrate, +) from .mqtt import MqttPublisher from . import peakguard from .p1 import P1Ingest, build_source, is_enabled from .price import Uncertainty, price_option from .static_tariff import resolve_tariff, static_status from .tariff import PeakStore, PeakTracker -from .store import migrate_unit_keys +from .store import migrate_maint_keys, migrate_unit_keys from .validate import (ConfigRefused, refuse, validate_startup_config, validate_unit_ranges) from .watchdog import ( @@ -265,7 +267,53 @@ class Controller: # maintenance.MAINT_OPTION_DEFAULTS before adding a ninth key: it goes # in config.yaml's `options:` and `schema:` blocks and in that dict, and # nowhere else. - self.maint = Maintenance(MaintConfig.from_options(opts), store) + # --- MAINT-05: ONE `Maintenance` PER ROSTER ENTRY -------------------- + # ⚠️ AND THE MIGRATION RUNS FIRST, FOR THE REASON THE SAFETY-02 ONE + # DOES. These objects READ their history in their constructors' wake - + # `phase`, `last_completed` - so a history still sitting under the flat + # spelling when they are built is a pack that comes up believing it has + # never been recalibrated, on a site whose state file says otherwise. + # It targets `self.inverters[0].name` and NOT `UNIT_A`: the flat keys + # belonged to whichever pack the one FSM was driving, which is the first + # roster entry, and a declared `inverters:` roster names its own entries + # (`inverter.build_roster()` says so). Writing `UNIT_A` here would + # deposit an install's whole maintenance history under a name that is + # not on the roster at all. + self.maint_migration = migrate_maint_keys( + store, self.inverters[0].name, len(self.inverters)) + _LOG.info("maintenance history: %s", self.maint_migration.explain()) + if not self.maint_migration.ok: + # Not a disarmed guard - history, not a latch - so it does not + # re-arm anything. It is still the difference between a pack that is + # recalibrated on schedule and one that silently is not, which is + # §12's opening sentence, so it is said where an operator looks. + self.log_event("maintenance history did NOT migrate cleanly: " + + self.maint_migration.explain()) + # ⚠️ ONE CONFIG, N MACHINES. The `maintenance_*` options describe the + # PROCEDURE - how hard to drain, how full to charge, how long to hold - + # and the procedure is the same on every pack in the roster; what is per + # pack is the SCHEDULE and the HISTORY, which live in the store. A + # per-entry config block would be a second shape for CFG-04's roster to + # carry and a fifth place for a `maintenance_charge_w` to disagree with + # config.yaml. If a site ever needs two different procedures, that is a + # roster-schema card and it starts here. + _maint_cfg = MaintConfig.from_options(opts) + _one_pack = len(self.inverters) <= 1 + self.maints = [ + Maintenance( + _maint_cfg, store, + # ⚠️ `None` AT A ROSTER OF ONE IS THE FLAT LAYOUT, NOT A MISSING + # NAME. §6.2's identity: one pack, one history, and the keys it + # has been written under since the add-on shipped. See + # `maintenance.UnitHistory` and `store.migrate_maint_keys()` - + # this is the line that keeps a one-inverter state file + # byte-identical. + unit=None if _one_pack else inv.name, + name=inv.name, + # The rollback view, and only ever the first pack's: the flat + # keys mean `inverters[0]` on every install that has them. + mirror_flat=(not _one_pack and i == 0)) + for i, inv in enumerate(self.inverters)] # SAFETY-02. ONE INSTANCE PER ACTUATOR - which is what watchdog.py's own # docstring has always said and what this Controller did not do. Each @@ -453,6 +501,9 @@ class Controller: self._peak_saturated = False self._peak_floor_limited = False self._maint_shape_suspended = False + # [(pack name, why)] for packs that are due and waiting their turn. + # Empty at a roster of one, where nothing can defer anything. + self._maint_deferred = [] # Set by amain() once the transport is built, so the status page can show # what only the transport knows (homewizard_local's unchanged_s). Stays # None when P1 is off, or under a transport that has no such counter. @@ -667,18 +718,79 @@ class Controller: return tuple(inv.name for inv in self.inverters) @property - def maint_unit(self): - """The pack the ONE maintenance state machine is recalibrating. + def maint_index(self) -> int: + """Which roster slot holds the site's ONE maintenance slot right now. - ⚠️ NAMED RATHER THAN ASSUMED. This add-on holds exactly one - `Maintenance`, whose history lives under the store's un-namespaced - `phase` / `last_completed` keys, so the pack it drives is the first in - the roster. Two packs recalibrating on two schedules needs a second FSM - and a namespaced history of its own; what THIS card fixes is only that - the claim is filed on ONE actuator instead of on whichever one happened - to be asked. Read store.LEGACY_UNIT_KEYS on why those keys did not move. + The pack that is mid-cycle if there is one, otherwise the first - which + at a roster of one is always 0, always was, and is the whole of this + property's N=1 behaviour. """ - return self.inverters[0] + for i, m in enumerate(self.maints): + if m.phase in POWER_MOVING: + return i + return 0 + + @property + def maint(self): + """The `Maintenance` the site's one maintenance slot belongs to. + + ⚠️ MAINT-05 TURNED THIS FROM AN ATTRIBUTE INTO A VIEW, AND THE THING IT + IS A VIEW OF CHANGED WITH IT. There are now `self.maints` - one FSM per + roster entry, each with its own schedule, phase and history - and every + SITE-level surface that used to read `self.maint` (the `phase` MQTT + value, the status banner, the operator's start/abort buttons) wants THE + ONE THAT IS RECALIBRATING, not "the first one". `maintenance.arbitrate()` + guarantees at most one pack is ever out of IDLE, so "the one that is + recalibrating" is well defined; when none is, this is `maints[0]` and + the surfaces read exactly what they read before. + + ⚠️ AT A ROSTER OF ONE IT IS `maints[0]` ON EVERY TICK, in both branches, + which is the object that used to be assigned here. Nothing downstream + can tell the difference - that is AC 6, and test_maintenance.py part 9 + asserts it rather than trusting this paragraph. + """ + return self.maints[self.maint_index] + + @property + def maint_unit(self): + """The pack the site's one maintenance slot is currently on. + + ⚠️ THIS USED TO BE `self.inverters[0]`, FLATLY, AND THE COMMENT THAT + STOOD HERE EXPLAINED WHY: there was exactly one `Maintenance`, its + history lived under the store's un-namespaced `phase` / `last_completed` + keys, so the pack it drove could only be the first in the roster. That + sentence is what MAINT-05 removed. There is now one FSM and one + namespaced history PER ROSTER ENTRY, so the pack being recalibrated is + whichever one holds the site's maintenance slot, and `maint_index` - + not the roster's order - is what answers it. + + ⚠️ AND `validate.py` RULE 6 MOVED WITH IT, exactly as its own comment + said it must ("when `maint_unit` stops being `inverters[0]`, this line + moves with it"). It graded `maintenance_charge_w` against the FIRST + entry's cap on the argument that the cycle never touches the others; + that argument is now false, so it grades against the weakest cap on the + roster, like rule 5. See the ⚠️ block there. + """ + return self.inverters[self.maint_index] + + def _maint_button(self, now): + """The pack an operator's one maintenance button acts on. + + The pack holding the site's maintenance slot when one holds it, so + "abort" stops the cycle that is running and "start" restarts it - + exactly what both buttons meant while there was one pack, and the reason + this is not simply "the longest-waiting one". When every pack is idle it + IS the longest-waiting one, which is `arbitrate()`'s own clause 2, so a + forced start and a scheduled start choose the same pack rather than two + different ones. + + ⚠️ AT A ROSTER OF ONE BOTH BRANCHES ARE `maints[0]`. There is no third + case: `max()` over a one-element list is that element. + """ + if self.maints[self.maint_index].phase in POWER_MOVING: + return self.maints[self.maint_index] + return max(enumerate(self.maints), + key=lambda p: (p[1].waited_days(now), -p[0]))[1] def inverter(self, name: str): """The roster entry called `name`, or None.""" @@ -1329,14 +1441,36 @@ class Controller: """ now = datetime.now(timezone.utc) - # ⚠️ THE PACK IT IS MAINTAINING, NOT "the SoC". §12's recalibration - # drains and refills ONE pack and grades ONE coulomb counter, so it is - # graded on that pack's own reading - see `maint_unit`. A minimum across - # packs would abort the cycle on a sibling's state, and an average would - # declare a pack full that is not. - result = self.maint.tick(now, self.maint_unit.soc) - for msg in result.events: - self.log_event(msg) + # ⚠️ EACH PACK ON ITS OWN READING, NOT "the SoC". §12's recalibration + # drains and refills ONE pack and grades ONE coulomb counter, so each + # state machine is ticked with the SoC of the inverter it belongs to. A + # minimum across packs would abort a cycle on a sibling's state, and an + # average would declare a pack full that is not. The pairing is + # positional and both lists are built from the roster in `__init__`, so + # a pack cannot be graded on its neighbour's meter. + # + # ⚠️ AND `arbitrate()` RUNS FIRST, ONCE, OVER ALL OF THEM. It is the + # site's one maintenance slot - see maintenance.py - and a pack that + # does not hold it is told so by name rather than left to discover it. + # Calling `tick()` without the answer this produced is the + # two-packs-draining-at-once defect with the interlock present but not + # wired, which is why the plan is built here and not inside the loop. + maint_plan = maint_arbitrate(self.maints, now) + maint_results = [] + for inv, machine, blocked in zip(self.inverters, self.maints, maint_plan): + res = machine.tick(now, inv.soc, blocked_by=blocked) + for msg in res.events: + self.log_event(msg) + maint_results.append(res) + # ⚠️ CARRIED TO THE STATUS PAGE, NOT LEFT IN THE 40-LINE ACTION LOG. The + # deferral is edge-logged (it lasts as long as the other pack's cycle), + # so by the time an operator asks "why has pack B not recalibrated" the + # line that said so has scrolled off. §12's whole failure mode is a + # schedule that silently stops firing; a pack that is due, idle and + # WAITING has to be able to say so at the moment it is looked at. + self._maint_deferred = [(m.name, r.deferred) + for m, r in zip(self.maints, maint_results) + if r.deferred] # --- SAFETY-02, and it runs BEFORE the early return below ------------- # ⚠️ ORDER MATTERS HERE. If this were evaluated after the "inputs missing @@ -1838,88 +1972,114 @@ class Controller: self.log_event(self.arb.explain()) # --- maintenance ----------------------------------------------------- - if result.owns_setpoint: - # PER-INVERTER. §12's monthly recalibration is a PACK procedure - - # drain THIS pack to its floor, charge THIS pack full, recalibrate - # THIS pack's coulomb counter. Two packs recalibrate on two - # schedules. See arbiter.CLAIM_SCOPE. - # ⚠️ FILED ON THE UNIT BEING MAINTAINED, WHICH IS THE FIRST ONE. - # §12's monthly recalibration is a PACK procedure and this add-on - # holds exactly ONE `Maintenance` state machine, so the pack it is - # maintaining is `self.maint_unit`. Two packs recalibrating on two - # schedules is a second FSM with its own namespaced history, which - # is a card of its own - what this card must not do is file one - # pack's procedure on every actuator. - claims.append(Claim.set("maintenance", P_MAINTENANCE, - float(result.setpoint_w or 0.0), result.phase, - actuator=self.actuator_of("maintenance", - self.maint_unit))) - self.sat_count = 0 - elif result.charge_only: - # Money outranks the maintenance schedule: while the quarter-hour - # projection is over the cap, the charge-only shaping is simply not - # claimed, so the loop can discharge and shave the peak. When a - # dedicated peak-shaving strategy arrives it will claim SET at - # P_PEAK and outrank this limit without any code here changing. - # ⚠️ AND SAFETY-07 IS THE OTHER HALF OF THIS CONDITION, NOT A - # SECOND MECHANISM. The shaping is `hi <= 0`; the peak guard's claim - # is `lo >= 0`. Both are limits, arbiter.py applies EVERY limit at or - # above the winner's priority, and most restrictive wins - so a peak - # event during a maintenance charge window files `lo > hi`, which is - # a CONTRADICTION and resolves to 0 W. The battery would park in the - # exact quarter it should be shaving. Priority does not save it: - # clause 2 has no notion of a higher-priority limit overriding a - # lower one, and it should not - "most restrictive" is what makes the - # safety bounds unconditional. - # - # So the higher-priority claimant SUPPRESSES the lower one rather - # than out-voting it, which is what this branch already did for - # `peak_forecast_entity`. That entity is optional and empty on a - # default install, so until now the suspension could not fire on the - # site that needed it. `self.peak_guard` is measured from the meter - # and is always there. - # - # ⚠️ THE CONJUNCTION LIVES IN peakguard.py, NOT HERE, and that is not - # tidiness. It was inline, and a reviewer mutated it back to its - # historical broken form - re-gated on `peak_forecast_entity` alone - - # and every suite in this repository stayed green while a default - # install parked at 0 W in the quarter that sets the month's bill. - # Read shaping_suspended() before touching this: both terms are - # there, and why neither is sufficient alone. - if peakguard.shaping_suspended( - self.peak_guard, self.peak_fc, - float(self.o.get("peak_cap_w", 3500))): - # Edge-logged: level-logging here would write one line per cycle - # for as long as the event lasts and flush the 40-line action log - # of the very context that explains it. - if not self._maint_shape_suspended: - self._maint_shape_suspended = True - self.log_event("peak at risk - maintenance charge shaping " - "suspended so the peak claim is not " - "contradicted") - else: - self._maint_shape_suspended = False - hi = 0.0 - reason = "charge-only" - if self.cheap: - # ⚠️ CFG-03. `self.maint.cfg.charge_w`, NOT A SECOND READ - # OF `self.o`. This line used to re-open the options dict - # with its own literal fallback, so the rate the shaping - # asked for and the rate the state machine drained at could - # be different numbers from the same install - and, with the - # key absent, they were. The Controller already parsed this - # once in __init__; asking the object that owns it cannot - # drift from it. - floor = maintenance_charge_floor( - self.maint.cfg.charge_w, - self.peak_fc, float(self.o.get("peak_cap_w", 3500))) - if floor > 0: - hi, reason = -floor, "cheap-window charge" - # PER-INVERTER, like the SET above and for the same reason - - # the shaping is about the pack being charged. - claims.append(Claim.limit("maintenance", P_MAINT_SHAPE, hi=hi, reason=reason, - actuator=self.actuator_of("maintenance", - self.maint_unit))) + # ⚠️ ONE PASS PER PACK, AND `arbitrate()` IS WHY THE LIST STAYS SHORT. + # At most one machine is out of IDLE on any tick, so at most one of + # these branches fires and the claim list is the shape it has always + # been. The loop is not a widening of what maintenance may ask for; it + # is what makes the ONE claim it does file land on the pack that is + # actually being recalibrated, whichever roster slot that is. + for inv, result in zip(self.inverters, maint_results): + if result.owns_setpoint: + # PER-INVERTER. §12's monthly recalibration is a PACK procedure - + # drain THIS pack to its floor, charge THIS pack full, + # recalibrate THIS pack's coulomb counter. Two packs recalibrate + # on two schedules. See arbiter.CLAIM_SCOPE. + # + # ⚠️ FILED ON THE PACK WHOSE STATE MACHINE PRODUCED IT - `inv`, + # the roster entry this `result` was ticked from - and that is + # the whole of MAINT-05 at this line. It used to be + # `self.maint_unit`, which was `inverters[0]` flatly, because + # there was one FSM and its history had nowhere else to live. + # Pack B's drain filed on pack A is one pack being commanded for + # another's procedure while the pack that is actually draining + # stays under the loop: the site would move the right total and + # recalibrate nothing. + # + # ⚠️ AND IT STAYS `SCOPE_UNIT`. Two FSMs mean two claims on two + # actuators, which the allocator consumes as per-unit intervals; + # ARCH-01 is explicit that this must never become a site claim, + # because per-inverter constraints filed site-wide are the + # cross-inverter `lo > hi` contradiction at arbiter.py:114. + claims.append(Claim.set("maintenance", P_MAINTENANCE, + float(result.setpoint_w or 0.0), result.phase, + actuator=self.actuator_of("maintenance", inv))) + self.sat_count = 0 + elif result.charge_only: + # Money outranks the maintenance schedule: while the quarter-hour + # projection is over the cap, the charge-only shaping is simply not + # claimed, so the loop can discharge and shave the peak. When a + # dedicated peak-shaving strategy arrives it will claim SET at + # P_PEAK and outrank this limit without any code here changing. + # ⚠️ AND SAFETY-07 IS THE OTHER HALF OF THIS CONDITION, NOT A + # SECOND MECHANISM. The shaping is `hi <= 0`; the peak guard's + # claim is `lo >= 0`. Both are limits, arbiter.py applies EVERY + # limit at or above the winner's priority, and most restrictive + # wins - so a peak event during a maintenance charge window files + # `lo > hi`, which is a CONTRADICTION and resolves to 0 W. The + # battery would park in the exact quarter it should be shaving. + # Priority does not save it: clause 2 has no notion of a + # higher-priority limit overriding a lower one, and it should not - + # "most restrictive" is what makes the safety bounds unconditional. + # + # So the higher-priority claimant SUPPRESSES the lower one rather + # than out-voting it, which is what this branch already did for + # `peak_forecast_entity`. That entity is optional and empty on a + # default install, so until now the suspension could not fire on the + # site that needed it. `self.peak_guard` is measured from the meter + # and is always there. + # + # ⚠️ THE CONJUNCTION LIVES IN peakguard.py, NOT HERE, and that is + # not tidiness. It was inline, and a reviewer mutated it back to its + # historical broken form - re-gated on `peak_forecast_entity` alone + # - and every suite in this repository stayed green while a default + # install parked at 0 W in the quarter that sets the month's bill. + # Read shaping_suspended() before touching this: both terms are + # there, and why neither is sufficient alone. + # + # ⚠️ ONE SUSPENSION FLAG FOR THE SITE, NOT ONE PER PACK, AND THE + # INTERLOCK IS WHAT MAKES THAT TRUE. The quantity it edge-logs is + # a peak event at the grid connection, and `arbitrate()` allows + # at most one pack to be in CHARGE at a time, so this branch can + # run at most once per tick. Were two packs ever allowed to + # charge together, this flag would have to become per-pack - and + # so would the reasoning above it. + if peakguard.shaping_suspended( + self.peak_guard, self.peak_fc, + float(self.o.get("peak_cap_w", 3500))): + # Edge-logged: level-logging here would write one line per + # cycle for as long as the event lasts and flush the 40-line + # action log of the very context that explains it. + if not self._maint_shape_suspended: + self._maint_shape_suspended = True + self.log_event("peak at risk - maintenance charge shaping " + "suspended so the peak claim is not " + "contradicted") + else: + self._maint_shape_suspended = False + hi = 0.0 + reason = "charge-only" + if self.cheap: + # ⚠️ CFG-03. `self.maint.cfg.charge_w`, NOT A SECOND READ + # OF `self.o`. This line used to re-open the options dict + # with its own literal fallback, so the rate the shaping + # asked for and the rate the state machine drained at + # could be different numbers from the same install - and, + # with the key absent, they were. The Controller already + # parsed this once in __init__; asking the object that + # owns it cannot drift from it. Every pack's machine + # holds the same `MaintConfig` instance, so this is the + # rate the pack in front of us is charging at. + floor = maintenance_charge_floor( + self.maint.cfg.charge_w, + self.peak_fc, float(self.o.get("peak_cap_w", 3500))) + if floor > 0: + hi, reason = -floor, "cheap-window charge" + # PER-INVERTER, like the SET above and for the same reason - + # the shaping is about the pack being charged, which is `inv`. + claims.append(Claim.limit("maintenance", P_MAINT_SHAPE, hi=hi, + reason=reason, + actuator=self.actuator_of("maintenance", + inv))) # --- the grid-following loop ----------------------------------------- if self.auto and self.inputs_ok: @@ -3310,14 +3470,29 @@ class Controller: elif not self.auto: level, banner = "warn", "Stopped — inverter commanded to 0 W" elif self.maint.phase != IDLE: + # ⚠️ THE PACK IS NAMED ONLY WHEN THERE IS MORE THAN ONE. At a roster + # of one this is the string it has always been, character for + # character; at two, "Maintenance: drain" without a name is a + # banner an operator cannot act on, because the remedy is at one + # device and the site has two. level, banner = "warn", f"Maintenance: {self.maint.phase}" + if len(self.inverters) > 1: + banner += f" ({self.maint_unit.name})" else: level, banner = "ok", "Running — holding grid at zero" def w(v): return "—" if v is None else f"{v:,.0f} W".replace(",", " ") - next_due = self.maint.next_due(datetime.now(timezone.utc)) + # ⚠️ THE EARLIEST ACROSS THE PACKS, NOT THE FIRST PACK'S. One date on a + # site with two schedules has to be the one that comes first, or the row + # says "2026-10-01" while a pack recalibrates tomorrow. `None` means + # "never run, due now" and has to win over any date, so it is tested for + # rather than sorted with. At a roster of one this is that pack's own + # date, unchanged. + _now = datetime.now(timezone.utc) + _dues = [m.next_due(_now) for m in self.maints] + next_due = None if any(d is None for d in _dues) else min(_dues) rows = [ ("Grid", w(self.grid)), ("Battery", w(self.site_batt_w)), @@ -3333,7 +3508,14 @@ class Controller: else f"{self.pack_floor_soc:g} %"), ("Commanded", w(self.target)), ("Why", self.reason), - ("Maintenance phase", self.maint.phase), + ("Maintenance phase", self.maint.phase if len(self.inverters) <= 1 + else f"{self.maint.phase} ({self.maint_unit.name})"), + # ⚠️ ONLY WHEN A PACK IS ACTUALLY WAITING, so a one-inverter status + # page - where `_maint_deferred` is empty on every tick - carries + # exactly the rows it carried before. + *((("Maintenance waiting", "; ".join(f"{n}: {why}" + for n, why in self._maint_deferred)),) + if self._maint_deferred else ()), ("Maintenance due", "now" if next_due is None else next_due.strftime("%Y-%m-%d")), ("Cheap window", "yes" if self.cheap else "no"), ("Tariff", self.tariff_words), @@ -3445,11 +3627,26 @@ class Controller: await self.command_site_zero() return {"auto": self.auto} if what == "maint_start": - for msg in self.maint.force_start(now): + # ⚠️ WHICH PACK, BECAUSE THERE ARE NOW SEVERAL AND THE BUTTON HAS + # ONE LABEL. `_maint_button()` answers it: the pack that holds the + # site's maintenance slot if one does - which is what "start" + # already meant at a roster of one, where pressing it mid-drain + # re-entered DRAIN - and otherwise the pack that has gone longest + # without a cycle, which is the same pack `arbitrate()` would have + # picked on its own. At N=1 both branches are `maints[0]` and this + # is the shipped call, byte for byte. + # + # A PER-PACK BUTTON IS A UI CARD, NOT THIS ONE. `web.py` posts a + # bare `maint_start` with no unit, and inventing a unit parameter + # here would ship half a surface: the page would still have one + # button and the API would have two meanings. + for msg in self._maint_button(now).force_start(now): self.log_event(msg) return {"phase": self.maint.phase} if what == "maint_abort": - for msg in self.maint.abort(now, "operator"): + # Aborts the pack that is actually cycling - `_maint_button()` again + # - and then zeroes the SITE, which is what it always did. + for msg in self._maint_button(now).abort(now, "operator"): self.log_event(msg) await self.command_site_zero() return {"phase": self.maint.phase} diff --git a/goodwe_controller/app/maintenance.py b/goodwe_controller/app/maintenance.py index 6656b8c..44848e2 100644 --- a/goodwe_controller/app/maintenance.py +++ b/goodwe_controller/app/maintenance.py @@ -18,16 +18,47 @@ OWNERSHIP, not priority. Exactly one thing writes the setpoint at any moment: The loop is never "disabled" - it yields. The heartbeat to the inverter keeps running in every phase, so the hardware watchdog stays fed and a crash here still ends with the inverter at 0 W rather than latched. + +ONE PACK AT A TIME (MAINT-05). There is now one `Maintenance` per roster entry, +each with its own schedule, its own phase and its own history under its own +Store keys - and therefore, for the first time, two of them able to move power +on ONE grid connection at once. Read `arbitrate()` before touching any of it: +the site holds exactly ONE maintenance slot, a pack that is not IDLE holds it, +and every other pack is held at IDLE under the loop until it is released. That +is not a scheduling nicety. A drain of pack A against a charge of pack B is two +power-moving phases pulling opposite ways across the meter - the circulating +power SAFETY-31 exists to DETECT, arriving here as something this module can +simply not do. """ import logging +import math from dataclasses import dataclass, field from datetime import datetime, timedelta, timezone +from .store import MAINT_UNIT_KEYS, unit_key + _LOG = logging.getLogger("goodwe.maint") IDLE, DRAIN, CHARGE, HOLD = "idle", "drain", "charge", "hold" +# ⚠️ THE PHASES THAT MOVE POWER, WHICH IS NOT THE SAME LIST AS "owns the +# setpoint". The table above is about WHO WRITES the number; this is about +# whether the pack is being pushed. DRAIN and HOLD own the setpoint. CHARGE does +# NOT - the loop owns it - and CHARGE is in here anyway, because the charge-only +# clamp plus the cheap-window floor is what pulls a second pack full while the +# first is being emptied. Leaving CHARGE out is precisely the interlock hole +# this card is about: pack A charging from the grid at 2000 W while pack B +# drains 2000 W to export is 4 kW going round in a circle with the meter at +# zero, and both packs believing they are behaving. +# +# HOLD commands 0 W and moves nothing, and it is in here for the other half of +# the sentence: it OWNS the actuator - the pack is parked at the top for the BMS +# and is deliberately not available to the loop - so a second cycle starting +# under it would still be two recalibrations at once, which is the thing whose +# value is a clean uninterrupted coulomb count. +POWER_MOVING = (DRAIN, CHARGE, HOLD) + # A drain of a 13.8 kWh usable pack (300 Ah x 51.2 V, DoD 90) at the shipped # 2000 W is ~6.9 h even with no house load helping - and less in practice, since # house load drains alongside the export and soc_floor stops short of empty. @@ -153,17 +184,102 @@ class MaintResult: setpoint_w: float | None = None charge_only: bool = False events: list = field(default_factory=list) + # ⚠️ WHY A DEFERRAL IS A FIELD AND NOT JUST A LOG LINE. A pack that is due + # and is not starting looks, from every surface an operator has, exactly + # like a pack whose schedule has quietly stopped firing - §12's opening + # sentence, the failure this whole module exists because of. It is carried + # so the status page can SAY "waiting for the other pack" rather than + # showing an idle machine and a due date in the past. Empty on every path + # that is not deferring, so a roster of one never sees it. + deferred: str = "" + + +class UnitHistory: + """One pack's maintenance history in the Store, under one spelling. + + ⚠️ THE NAMESPACE IS A CONSTRUCTOR ARGUMENT AND `None` IS A REAL VALUE, NOT + AN OMISSION. `unit=None` means the FLAT keys - `phase`, `phase_started`, + `last_completed`, `last_start_attempt` - which is what every install has + written since the add-on shipped and what a one-inverter site keeps writing + after MAINT-05. §6.2's identity: with one pack there is nothing for a + namespace to distinguish it FROM, and adding one would change the layout of + a live state file to say something it already said. `arbiter.actuator_for()` + makes the same move for the same reason and its docstring argues it. + + ⚠️ `mirror_flat` IS THE ROLLBACK VIEW AND IT IS ONLY EVER THE FIRST PACK'S. + At N>=2 pack A's history has been migrated into its namespace and is written + there; the flat keys are ALSO kept in step, so an operator who rolls the + add-on back to a single-FSM version finds a CURRENT history rather than one + dated the day the second inverter was declared. store.migrate_maint_keys() + carries the argument, and it is DEV-02's rule 2 applied to history instead + of to a latch: "left in the file" is not the same promise as "still true". + Pack B has no flat spelling to mirror to - the flat keys mean pack A on + every install that has them - so it is constructed with mirroring off, and + a mirror from B would OVERWRITE A's rollback view with B's phase. + """ + + def __init__(self, store, unit: str | None = None, mirror_flat: bool = False): + self.store = store + self.unit = unit + self.mirror_flat = bool(mirror_flat) + + def key(self, name: str) -> str: + """The Store key this pack's `name` lives under.""" + return name if self.unit is None else unit_key(self.unit, name) + + def get(self, name: str, default=None): + return getattr(self.store, "data", {}).get(self.key(name), default) + + def set(self, name: str, value) -> None: + self.store.set(self.key(name), value) + if self.mirror_flat: + self.store.set(name, value) + + def get_time(self, name: str): + return self.store.get_time(self.key(name)) + + def set_time(self, name: str, when) -> None: + self.store.set_time(self.key(name), when) + if self.mirror_flat: + self.store.set_time(name, when) class Maintenance: - def __init__(self, cfg: MaintConfig, store): + """ONE pack's recalibration cycle: its schedule, its phase, its history. + + ⚠️ ONE PER ROSTER ENTRY SINCE MAINT-05, AND THE SLOT IS NOT ITS OWN. This + object decides WHEN ITS PACK would like to start; `arbitrate()` decides + which of them may, because the constraint is a property of the grid + connection and no single FSM can see it. Never call `tick()` in a loop + without passing the `blocked_by` that `arbitrate()` returned for it: that is + the two-packs-at-once bug with the interlock present but not wired. + """ + + def __init__(self, cfg: MaintConfig, store, unit: str | None = None, + name: str = "", mirror_flat: bool = False): self.cfg = cfg - self.store = store + # ⚠️ `self.store` IS THE VIEW, NOT THE Store. Everything in this class + # names an UNQUALIFIED key - "phase", "last_completed" - and the view is + # the one place that turns it into the key this pack owns. A raw + # `store.set("phase", ...)` anywhere below would write pack A's history + # from pack B's state machine, which is the shared-state bug this card + # is about, and there would be nothing to see at a roster of one. + self.store = UnitHistory(store, unit, mirror_flat) + self.unit = unit + # What an event or a refusal calls this pack. At a roster of one it is + # never rendered - a single machine cannot be deferred - so the shipped + # one-inverter reason strings are untouched. + self.name = str(name or unit or "the pack") + # Edge-memo for the deferral note. The action log holds 40 lines and a + # deferral lasts as long as the other pack's cycle - hours - so a line + # per cycle would flush the very context that explains it. Same pattern + # as `Controller._arb_action` and `_maint_shape_suspended`. + self._deferred = "" # -- state --------------------------------------------------------------- @property def phase(self) -> str: - return self.store.data.get("phase", IDLE) + return self.store.get("phase", IDLE) def _enter(self, phase: str, now: datetime, events: list, msg: str) -> None: self.store.set("phase", phase) @@ -185,14 +301,60 @@ class Maintenance: last = self.store.get_time("last_completed") return None if last is None else last + timedelta(days=self.cfg.interval_days) + def waited_days(self, now: datetime) -> float: + """Days since this pack last COMPLETED a cycle; `inf` if it never has. + + ⚠️ THE ARBITRATION KEY, AND IT IS DELIBERATELY NOT `due()`. `due()` is a + yes/no and two packs that are both due are indistinguishable by it - + which is how a roster-order tie-break gets written and how the first + entry then wins every contested month forever while the second drifts. + This is an ORDERING: the pack whose coulomb counter has gone longest + without a recalibration goes first. `inf` for never-recalibrated is the + same statement `due()` makes about an absent `last_completed`, kept as + one so a fresh pack cannot lose to one that ran last month. + """ + last = self.store.get_time("last_completed") + return math.inf if last is None else (now - last).total_seconds() / 86400.0 + + def _cooled(self, now: datetime) -> bool: + """Is the post-abort cooldown over?""" + last_try = self.store.get_time("last_start_attempt") + return (last_try is None + or (now - last_try) >= timedelta(minutes=RESTART_COOLDOWN_MIN)) + + def wants_start(self, now: datetime) -> bool: + """Would this pack begin a cycle on this tick if the site let it? + + ⚠️ THE SAME PREDICATE `tick()` USES, NOT A COPY OF IT. `arbitrate()` has + to know who is asking before anybody moves, and a second transcription + of "enabled, in the start hour, due and cooled down" is a second opinion + about who was asking - so the IDLE branch of `tick()` calls this one. + """ + return (self.phase == IDLE and self.cfg.enabled + and now.hour == self.cfg.start_hour and self.due(now) + and self._cooled(now)) + # -- operations ---------------------------------------------------------- - def force_start(self, now: datetime) -> list: + def force_start(self, now: datetime, blocked_by: str = "") -> list: """Start a cycle regardless of the calendar. Commissioning uses this: a cycle must be proven end to end before a site is signed off, and nobody waits a month to find out the schedule never fires. + + ⚠️ REGARDLESS OF THE CALENDAR, NOT REGARDLESS OF THE INTERLOCK. The + calendar is a preference and an operator may overrule it; "one pack + moves power at a time" is a property of the grid connection and an + operator pressing a button does not add a second one. A forced start + against a busy site is REFUSED, by name, and the phase machine does not + move - the refusal is returned as an event so the operator reads it in + the action log rather than watching nothing happen. """ + if blocked_by: + msg = (f"maintenance: forced start REFUSED on {self.name} - " + f"{blocked_by}") + _LOG.warning("%s", msg) + return [msg] events = [] self.store.set_time("last_start_attempt", now) self._enter(DRAIN, now, events, "maintenance: forced start -> drain") @@ -203,23 +365,80 @@ class Maintenance: self._enter(IDLE, now, events, f"maintenance: ABORT ({why})") return events + def _defer_note(self, why: str, events: list) -> list: + """Say once, not every second, that this pack is waiting its turn. + + ⚠️ THE WORDING IS DELIBERATELY NOT AN ABORT AND NOT A TIMEOUT. §12's + notes are explicit that a 12 h phase timeout means "the battery is not + following commands - a FAULT, not a slow day", and the one thing that + must not happen here is an operator reading a deferral as that. This + line says the pack is WAITING, names the pack it is waiting for, and + says it will run next - none of which any failure path in this module + says. + """ + if self._deferred != why: + self._deferred = why + msg = (f"maintenance: {self.name} is due and is WAITING its turn - " + f"{why}. It is idle under the control loop and will " + f"recalibrate at the next start hour once the slot is free; " + f"this is not a stalled phase.") + _LOG.info("%s", msg) + events.append(msg) + return events + # -- the machine --------------------------------------------------------- - def tick(self, now: datetime, soc: float | None) -> MaintResult: + def tick(self, now: datetime, soc: float | None, + blocked_by: str = "") -> MaintResult: + """One cycle of this pack's state machine. + + `blocked_by` is `arbitrate()`'s answer for THIS pack: empty when the + site's one maintenance slot is available to it, otherwise the reason it + is not. It gates two things and nothing else - a start, and a phase that + is already moving power on a pack that does not hold the slot. + """ events: list = [] phase = self.phase + # ⚠️ THE SLOT IS CHECKED BEFORE THE PHASE, AND THIS BRANCH IS A RECOVERY + # RATHER THAN AN EXPECTED PATH. `arbitrate()` grants the slot to the + # pack that is already mid-cycle, so a pack that is BOTH moving power + # and blocked means two histories on disk say "drain" - a hand-edited + # state file, or a store carried across a version that had no interlock. + # The fail-safe direction is unambiguous: two packs pulling against each + # other on one connection is the fault SAFETY-31 latches the whole site + # for. Stand this one down to 0 W and say why; the holder keeps its + # cycle, which is the outcome that loses no coulomb count at all. + if blocked_by and phase in POWER_MOVING: + events += self.abort(now, f"interlock - {blocked_by}") + return MaintResult(IDLE, True, 0.0, False, events) + if phase == IDLE: - if self.cfg.enabled and now.hour == self.cfg.start_hour and self.due(now): - last_try = self.store.get_time("last_start_attempt") - cooled = ( - last_try is None - or (now - last_try) >= timedelta(minutes=RESTART_COOLDOWN_MIN) - ) - if cooled: - self.store.set_time("last_start_attempt", now) - self._enter(DRAIN, now, events, - "maintenance: due -> drain (exports the surplus)") - return MaintResult(DRAIN, True, self.cfg.discharge_w, False, events) + if self.wants_start(now): + if blocked_by: + # ⚠️ `last_start_attempt` IS NOT STAMPED HERE, AND THAT IS + # THE STARVATION FIX RATHER THAN AN OMISSION. That key is + # the post-ABORT cooldown: it exists so a cycle that failed + # does not re-trigger inside the same start hour. A deferral + # is not an attempt - nothing was tried and nothing failed - + # and stamping it would put this pack under a 90 minute + # cooldown for a start hour that is 60 minutes long, so the + # pack that lost the arbitration would still be locked out + # at the moment the winner released the slot. + # + # ⚠️ AND THE PHASE DOES NOT MOVE, WHICH IS THE OTHER HALF. + # `phase_started` is untouched, so the 12 h DRAIN and 36 h + # CHARGE timeouts - which mean "the battery is not following + # commands, a fault" - cannot be reached by waiting. A + # deferred pack is IDLE under the loop, not a stuck one. + return MaintResult(IDLE, False, None, False, + self._defer_note(blocked_by, events), + blocked_by) + self._deferred = "" + self.store.set_time("last_start_attempt", now) + self._enter(DRAIN, now, events, + "maintenance: due -> drain (exports the surplus)") + return MaintResult(DRAIN, True, self.cfg.discharge_w, False, events) + self._deferred = "" return MaintResult(IDLE, False, None, False, events) # ⚠️ Fail toward inaction. Without a state-of-charge reading none of the @@ -273,3 +492,77 @@ class Maintenance: _LOG.error("maintenance: unknown phase %r, forcing idle", phase) events += self.abort(now, f"unknown phase {phase!r}") return MaintResult(IDLE, True, 0.0, False, events) + + +def _days(n: float) -> str: + return "never recalibrated" if n == math.inf else f"{n:.0f} d since its last cycle" + + +def arbitrate(machines, now: datetime) -> list: + """Grant the site's ONE maintenance slot. Returns a list parallel to + `machines`: "" for the pack that may move power, otherwise the reason the + pack may not. + + ⚠️ THE SLOT IS A PROPERTY OF THE GRID CONNECTION, WHICH IS WHY IT IS NOT ON + `Maintenance`. Each FSM can see its own pack's schedule and nothing else; the + quantity being rationed - "how many phases are moving power across one + meter" - is a sum over the roster, exactly like `sum(|B_i|) - |sum(B_i)|` in + SAFETY-31 and for the same reason. A per-pack answer to a site-level + question is the shape of defect this repository keeps finding. + + ⚠️ IT IS DERIVED, NOT PERSISTED, AND THAT IS DELIBERATE. The holder is + "whichever pack's stored phase is not IDLE" - a fact already on disk, in the + per-pack history the packs themselves write. A separate `maint_slot_holder` + key would be a second opinion about the same thing, and the two would + disagree the first time the add-on was killed between writing the phase and + writing the holder. Restart-safe by construction: a controller that comes + back up mid-drain reads the drain and re-derives the holder from it. + + TWO CLAUSES, IN ORDER: + + 1. A PACK ALREADY MOVING POWER KEEPS THE SLOT. Nothing preempts a cycle in + flight, because a recalibration interrupted halfway is worth less than + nothing - the coulomb counter is graded on ONE uninterrupted drain and + refill, so a cycle stopped at 40 % has spent the energy and bought no + calibration. If more than one pack somehow comes up moving power, the + FIRST in roster order holds and `tick()` stands the others down; see the + recovery branch there. + + 2. OTHERWISE THE LONGEST-WAITING CANDIDATE STARTS. `wants_start()` is asked + of every pack, and among those that say yes the slot goes to the one with + the largest `waited_days()`. NOT roster order: two packs on one monthly + schedule are due in the same hour every month, and a roster-order tie + would hand the slot to `inverters[0]` every single time while the second + pack's coulomb counter drifted forever - which is, in a different + costume, the exact defect this card was opened for. Roster order breaks a + genuine numeric tie and nothing else. + + ⚠️ AT A ROSTER OF ONE THIS IS THE IDENTITY FUNCTION. One machine is never + another machine's blocker: clause 1 finds at most itself and clause 2 has at + most one candidate, so it returns `[""]` on every tick and no reason string + it can build is ever rendered. That is what makes the one-inverter behaviour + byte-identical rather than merely similar. + """ + blocks = [""] * len(machines) + busy = [(i, m) for i, m in enumerate(machines) if m.phase in POWER_MOVING] + if busy: + held, holder = busy[0] + for i, m in enumerate(machines): + if i != held: + blocks[i] = (f"{holder.name} is mid-cycle ({holder.phase}) and " + f"holds the site's one maintenance slot") + return blocks + + wants = [(i, m) for i, m in enumerate(machines) if m.wants_start(now)] + if len(wants) <= 1: + return blocks + # Largest waited_days wins; `-index` makes a numeric tie fall to roster + # order rather than to whatever `max()` happens to see first. + won, winner = max(wants, key=lambda p: (p[1].waited_days(now), -p[0])) + for i, m in wants: + if i != won: + blocks[i] = (f"{winner.name} has waited longer " + f"({_days(winner.waited_days(now))} against " + f"{_days(m.waited_days(now))}) and takes the site's " + f"one maintenance slot first") + return blocks diff --git a/goodwe_controller/app/store.py b/goodwe_controller/app/store.py index 282b938..273f546 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -126,16 +126,53 @@ SCHEMA_VERSION = 1 SCHEMA_KEY = "schema_version" # Which flat keys belong to ONE inverter, and therefore move into its namespace. -# ⚠️ THE MAINTENANCE KEYS ARE NOT IN HERE AND THAT IS DELIBERATE. `phase`, -# `phase_started`, `last_completed` and `last_start_attempt` belong to the -# maintenance state machine, of which this add-on still holds exactly ONE - a -# monthly recalibration is a pack procedure and per-pack SCHEDULING is a card of -# its own, not something to infer from a key rename. Moving them here would -# quietly re-home an install's whole maintenance history under a unit name; they -# stay where they are, untouched, which is the other half of "migrates without -# losing its latch OR ITS HISTORY". +# ⚠️ THE MAINTENANCE KEYS ARE STILL NOT IN HERE, AND SINCE MAINT-05 THAT IS A +# DIFFERENT DECISION RATHER THAN THE SAME ONE. They DO move now - one +# `Maintenance` per roster entry needs one history per roster entry - but they +# move on their OWN marker, `MAINT_SCHEMA_KEY`, and only on a site that actually +# has more than one pack. Two conditions the SAFETY-02 keys do not share, so +# folding them into this tuple would either drag them across on a +# single-inverter install (which is the byte-identical store layout MAINT-05 +# must not change) or hold the sign latch hostage to a roster count it has +# nothing to do with. `MAINT_UNIT_KEYS` below carries them. LEGACY_UNIT_KEYS = ("sign_tripped", "sign_reason") +# --------------------------------------------------------------------------- # +# MAINT-05: THE SECOND MIGRATION, AND WHY IT IS A SECOND ONE +# +# 0 = the flat, one-history layout every install has had since the add-on +# shipped: `phase` / `phase_started` / `last_completed` / +# `last_start_attempt` describe THE pack, because there was one FSM. +# 1 = one history per roster entry, under `unit//...`. +# +# ⚠️ ITS OWN MARKER, NOT A BUMP OF `SCHEMA_VERSION`, AND THE REASON IS THAT THE +# TWO MIGRATIONS DO NOT HAVE THE SAME PRECONDITION. DEV-02's runs on every +# install exactly once and is monotone in the version number. This one runs ONLY +# WHERE THE ROSTER HAS MORE THAN ONE PACK, because at N=1 §6.2's identity holds +# and the flat keys ARE that pack's namespace - moving them would add four keys +# and a version marker to a store MAINT-05's own acceptance criterion requires +# to be byte-identical. A single monotone counter cannot express "migrated on +# this site, deliberately not migrated on that one" without lying to the next +# version that reads it: a one-inverter store would have to be left at the old +# number forever, and then a genuine v2 migration could never be told apart from +# a site that simply never grew. +# +# ⚠️ AND IT IS TWO MARKERS, NOT TWO COPIES OF THE THREE RULES. +# `_move_flat_keys()` and `_verify_moves()` below are shared verbatim by both +# migrations, so rules 1 and 3 and the READ-BACK POSTCONDITION are one +# implementation. A second transcription of a migration is a second opinion +# about what "it landed" means. +MAINT_SCHEMA_VERSION = 1 +MAINT_SCHEMA_KEY = "maint_schema_version" + +# The flat maintenance-history keys, in the order they are moved. +# ⚠️ THE VALUES ARE THE FLAT SPELLINGS AND THE MODULE THAT READS THEM INDEXES +# THIS SAME TUPLE (`maintenance.UnitHistory`), so a key renamed here cannot +# silently stop being migrated - the same property `UNIT_STORE_KEYS` gives the +# SAFETY-02 pair. +MAINT_UNIT_KEYS = ("phase", "phase_started", "last_completed", + "last_start_attempt") + def unit_key(unit: str, key: str) -> str: """The namespaced Store key for `key` on inverter `unit`. @@ -174,6 +211,8 @@ class MigrationReport: # to shout: an alarm that fires on the harmless case is an alarm an # operator learns to dismiss before the real one arrives. self.lost = [] + # Why nothing ran, when nothing ran. Empty means "already at schema". + self.skipped = "" @property def ok(self) -> bool: @@ -182,8 +221,15 @@ class MigrationReport: def explain(self) -> str: if not self.ran: - return (f"store already at schema {self.from_version}; no " - f"per-inverter migration needed") + # ⚠️ `skipped` RATHER THAN A SECOND SENTENCE ABOUT VERSIONS. Since + # MAINT-05 there are two reasons a migration does not run - the + # store is already at this schema, or the roster has one pack and + # the flat keys ARE its namespace - and an operator reading the + # startup log needs to be told which, because only one of them + # changes when a second inverter is declared. + return self.skipped or (f"store already at schema " + f"{self.from_version}; no " + f"per-inverter migration needed") out = (f"migrated {len(self.moved)} flat key(s) to inverter " f"namespaces: " + ", ".join(sorted(self.moved))) if self.kept: @@ -243,7 +289,30 @@ def migrate_unit_keys(store, unit: str) -> MigrationReport: return rep rep.ran = True - for legacy in LEGACY_UNIT_KEYS: + _move_flat_keys(store, unit, LEGACY_UNIT_KEYS, rep) + _verify_moves(store, rep) + + if rep.ok: + store.set(SCHEMA_KEY, SCHEMA_VERSION) + else: + # ⚠️ NOT MARKED DONE. A migration that lost state must run again on the + # next start rather than be recorded as finished - the one thing worse + # than a failed migration is a failed migration a later version skips. + _LOG.error("per-inverter store migration LOST STATE and has not been " + "marked done: %s", rep.explain()) + return rep + + +def _move_flat_keys(store, unit: str, keys, rep: MigrationReport) -> None: + """Rule 1, once, for both migrations. + + Copies each flat `key` to `unit//` UNLESS the namespaced spelling + is already present - a store that has both has been written by a version + that knew about the namespace, and that value is the live one. Rule 2 is the + absence of a delete: the flat keys are left exactly where they were. + """ + data = getattr(store, "data", {}) + for legacy in keys: target = unit_key(unit, legacy) if target in data: rep.kept.append(target) @@ -252,17 +321,25 @@ def migrate_unit_keys(store, unit: str) -> MigrationReport: # tripped has no `sign_tripped` in its file at all, and the value it # then lands on has to be the one `Store.load()` would have supplied - # otherwise "absent" and "never tripped" stop being the same statement, - # which is the distinction store.py's own header is about. + # which is the distinction store.py's own header is about. The same + # sentence holds for `last_completed`: absent means "never recalibrated" + # and a literal here would be a second opinion about that. value = data.get(legacy, DEFAULTS.get(legacy)) store.set(target, value) rep.moved[target] = value - # THE POSTCONDITION, AND IT IS A READ-BACK RATHER THAN AN ASSUMPTION. This - # is the difference between this function and a two-line copy: the caller - # can ask "is the latch that was in the file before now under the key the - # watchdog reads", through the same `unit_key()` the watchdog's constructor - # reads through, and a `False` there is a guard that is off on an install - # whose state file says it is on. + +def _verify_moves(store, rep: MigrationReport) -> None: + """THE POSTCONDITION, AND IT IS A READ-BACK RATHER THAN AN ASSUMPTION. + + This is the difference between these functions and a two-line copy: the + caller can ask "is the latch that was in the file before now under the key + the watchdog reads" - and, since MAINT-05, "is the history that was in the + file before now under the key THIS PACK's state machine reads" - through the + same `unit_key()` those readers read through. A `False` here is a guard that + is off, or a month of coulomb-count history gone, on an install whose state + file says otherwise. + """ for target, value in rep.moved.items(): got = getattr(store, "data", {}).get(target, "<>") if got == value: @@ -274,14 +351,66 @@ def migrate_unit_keys(store, unit: str) -> MigrationReport: if value != DEFAULTS.get(legacy): rep.lost.append(note) + +def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport: + """Move the flat maintenance history into the FIRST pack's namespace. + + MAINT-05. Until this card there was one `Maintenance` and one history, and + `store.py`'s own header argued the keys should stay flat because per-pack + scheduling was a card of its own. This is that card: there is now one FSM + per roster entry, so there is one history per roster entry, and the flat one + belongs to the pack that was being recalibrated - `inverters[0]`, which is + what `Controller.maint_unit` named and what `validate.py` rule 6 graded + against. + + ⚠️ `n_units <= 1` IS A RETURN, NOT A SPECIAL CASE. At one inverter the flat + keys ARE that pack's namespace (§6.2's identity, the same one + `arbiter.actuator_for()` leans on), `Maintenance` is constructed to read + them directly, and a migration would add four keys plus a marker to a store + that must come out byte-identical. The migration happens on the day the + roster grows, which is the day the flat spelling stops being able to answer + "which pack". + + THE THREE RULES ARE `migrate_unit_keys()`'s, SHARED IN CODE rather than + restated: `_move_flat_keys()` is rule 1 (never overwrite a namespaced value) + and rule 2 (the flat keys are left in the file), `_verify_moves()` is the + read-back postcondition, and the marker is written LAST so a crash before it + leaves the migration to run again - which rule 1 makes a no-op. + + ⚠️ AND RULE 2 IS KEPT LIVE HERE TOO, WHICH IS THE HALF DEV-02 HAD TO ADD + LATER. "Left in place" alone makes the flat history a fossil dated the day + the second inverter was declared: an operator who rolls back to a + single-FSM add-on would find a `last_completed` months old and recalibrate a + pack that does not need it, or - worse - a stale `phase` and a + `phase_started` old enough to abort on the 12 h timeout the moment it comes + up. So `maintenance.UnitHistory` MIRRORS the first pack's writes back onto + the flat keys for as long as the roster has more than one entry. The flat + pair is the ROLLBACK VIEW of the history, maintained, exactly as + `sign_tripped` is the rollback view of the latch. + """ + rep = MigrationReport() + data = getattr(store, "data", {}) + rep.from_version = int(data.get(MAINT_SCHEMA_KEY, 0) or 0) + if n_units <= 1: + rep.skipped = ("one inverter: the flat maintenance keys ARE that pack's " + "history and are read in place, so there is nothing to " + "move and the store layout does not change") + return rep + if rep.from_version >= MAINT_SCHEMA_VERSION: + rep.skipped = (f"maintenance history already at schema " + f"{rep.from_version}; written by a multi-inverter " + f"version and not touched") + return rep + + rep.ran = True + _move_flat_keys(store, unit, MAINT_UNIT_KEYS, rep) + _verify_moves(store, rep) + if rep.ok: - store.set(SCHEMA_KEY, SCHEMA_VERSION) + store.set(MAINT_SCHEMA_KEY, MAINT_SCHEMA_VERSION) else: - # ⚠️ NOT MARKED DONE. A migration that lost state must run again on the - # next start rather than be recorded as finished - the one thing worse - # than a failed migration is a failed migration a later version skips. - _LOG.error("per-inverter store migration LOST STATE and has not been " - "marked done: %s", rep.explain()) + _LOG.error("per-pack maintenance-history migration LOST STATE and has " + "not been marked done: %s", rep.explain()) return rep diff --git a/goodwe_controller/app/validate.py b/goodwe_controller/app/validate.py index 0cb22b2..6504c9e 100644 --- a/goodwe_controller/app/validate.py +++ b/goodwe_controller/app/validate.py @@ -802,24 +802,47 @@ def validate_startup_config(opts: dict, dev_min=None, dev_max=None) -> list[str] # rule 8 below already makes with `PEAK_OPTION_DEFAULTS` - and # test_maintenance.py pins it by reading the shipped file. # - # ⚠️ AND IT IS GRADED AGAINST THE FIRST UNIT'S CAP, NOT THE SMALLEST AND NOT - # THE FLAT ONE. Rule 5 takes the weakest unit because there is a watchdog on - # every one of them; there is exactly ONE `Maintenance` in this add-on and - # `Controller.maint_unit` names the pack it recalibrates - the first in the - # roster, because the cycle's history lives under un-namespaced Store keys. - # The shaping claim is filed on THAT actuator and on no other, so the cap - # that can contradict it is THAT unit's. Taking `min()` here would refuse a - # perfectly workable site - a 5000 W pack being recalibrated beside a 2000 W - # sibling the cycle never touches - which is the over-refusal this file's - # docstring says gets a validator switched off. When `maint_unit` stops - # being `inverters[0]`, this line moves with it. + # ⚠️ MAINT-05: IT IS GRADED AGAINST THE WEAKEST UNIT'S CAP NOW, AND THE + # COMMENT THIS REPLACES SAID SO WOULD HAPPEN. It read: "there is exactly ONE + # `Maintenance` in this add-on and `Controller.maint_unit` names the pack it + # recalibrates - the first in the roster, because the cycle's history lives + # under un-namespaced Store keys. The shaping claim is filed on THAT + # actuator and on no other, so the cap that can contradict it is THAT + # unit's. Taking `min()` here would refuse a perfectly workable site - a + # 5000 W pack being recalibrated beside a 2000 W sibling the cycle never + # touches ... When `maint_unit` stops being `inverters[0]`, this line moves + # with it." + # + # `maint_unit` has stopped being `inverters[0]`. There is one `Maintenance` + # per roster entry, each with its own namespaced history, and EVERY pack is + # recalibrated on its own schedule - so the sibling "the cycle never + # touches" does not exist any more and the over-refusal argument has no + # subject. The shaping claim is filed on whichever pack is charging, so the + # cap that can contradict it is any cap on the roster, and the one that + # contradicts it FIRST is the smallest. This is rule 5's conjunction for + # rule 5's reason: a threshold that is fine against the biggest unit says + # nothing about the smallest, and the smallest is the one that goes to 0 W. + # + # ⚠️ THIS REFUSES CONFIGS THAT USED TO START, AND THAT IS THE POINT RATHER + # THAN A SIDE EFFECT. `max_w` 5000 on pack A beside 2000 on pack B with + # `maintenance_charge_w` 3000 was accepted before this card and was correct + # while only pack A was ever recalibrated. Pack B now recalibrates too, and + # its cheap-window charge asks the arbiter for `hi = -3000` against a + # supervised `lo = -2000`: `lo > hi`, arbiter clause 3, 0 W, contradiction - + # a maintenance charge that silently never happens on that pack, which is + # exactly what rule 6 exists to refuse at the moment it is WRITTEN. At a + # roster of one `min()` over one cap is that cap and the comparison, the + # message and the boundary are identical. charge_w = _number(opts, "maintenance_charge_w", float(MAINT_OPTION_DEFAULTS["maintenance_charge_w"])) - # ⚠️ THE SAME PAIRING AS RULE 5, FOR THE SAME REASON. `caps[0]` is the first - # unit WITH A USABLE CAP, which is `inverters[0]` only while every earlier - # entry has one; `_unit_label(opts, 0)` was hard-wired to `inverters[0]` - # regardless. Both come off the one pair now. - maint_index, maint_cap = caps_ix[0] if caps_ix else (0, None) + # ⚠️ THE SAME PAIRING AS RULE 5, FOR THE SAME REASON: `unit_caps_indexed()` + # SKIPS entries with no usable `max_w`, so a caps-list position is not a + # roster position and `_unit_label()` indexes the roster. The index travels + # with the number, so the unit the message names is the unit the number came + # off - which on a roster whose first entry has a junk `max_w` is not the + # same thing at all. + maint_index, maint_cap = (min(caps_ix, key=lambda p: p[1]) if caps_ix + else (0, None)) maint_where = _unit_label(opts, maint_index) if charge_w is None: problems.append( diff --git a/goodwe_controller/test_inverter.py b/goodwe_controller/test_inverter.py index db2f548..0ee1c63 100644 --- a/goodwe_controller/test_inverter.py +++ b/goodwe_controller/test_inverter.py @@ -1629,11 +1629,28 @@ check("⚠️ RULE 5 NAMES THE UNIT THE NUMBER CAME OFF. The weakest cap is " "is inverter-B. The refusal says inverter-c", _why(_p_skew, "max_w (inverter-c) is 1000 W") and not _why(_p_skew, "max_w (inverter-b) is 1000 W")) -check("⚠️ RULE 6 TOO. `caps[0]` is the first entry WITH a usable cap - " - "inverter-b - while `_unit_label(opts, 0)` was hard-wired to inverter-a, " - "so the maintenance refusal quoted B's rating under A's name", - _why(_p_skew, "max_w (inverter-b) is 2000 W") - and not _why(_p_skew, "max_w (inverter-a) is 2000 W")) +# ⚠️ MAINT-05 MOVED RULE 6's SUBJECT, AND THIS CHECK MOVED WITH IT. It used to +# assert the refusal named inverter-b, because rule 6 graded the FIRST entry +# with a usable cap - `Controller.maint_unit` was `inverters[0]` and the shaping +# claim could be filed nowhere else. There is now one `Maintenance` per roster +# entry, so the shaping claim can be filed on ANY pack and the cap that +# contradicts it first is the SMALLEST - inverter-c's 1000 W. The property this +# check exists for is unchanged and is the reason it is still here: the index +# has to travel with the cap, or the message quotes one unit's rating under +# another unit's name. +check("⚠️ RULE 6 TOO, AND SINCE MAINT-05 IT IS THE WEAKEST PACK IT NAMES. " + "inverter-c's 1000 W sits at caps index 1, and `_unit_label(opts, 1)` is " + "inverter-B - so a caps index leaking into a roster index would say " + "inverter-b here. The refusal says inverter-c, with inverter-c's number", + _why(_p_skew, "maintenance_charge_w is 3000 W but max_w (inverter-c) is " + "1000 W") + and not _why(_p_skew, "maintenance_charge_w is 3000 W but max_w " + "(inverter-b)")) +check("...and it is no longer graded against the FIRST usable cap at all, which " + "is the behaviour change MAINT-05 makes here: inverter-b's 2000 W was the " + "pre-card answer and appears nowhere in the maintenance refusal now", + not _why(_p_skew, "maintenance_charge_w is 3000 W but max_w " + "(inverter-b) is 2000 W")) check("...and the counterfactual, so this is not a check that would pass on a " "validator that named nothing: the OLD expressions still evaluate, on " "this same config, to the wrong names", diff --git a/goodwe_controller/test_maintenance.py b/goodwe_controller/test_maintenance.py index 3df01c3..a5fa4fc 100644 --- a/goodwe_controller/test_maintenance.py +++ b/goodwe_controller/test_maintenance.py @@ -1,5 +1,12 @@ """Runnable check for the maintenance state machine. `python3 test_maintenance.py` +⚠️ SINCE MAINT-05 THIS FILE IMPORTS `app.main`, so it needs the add-on image's +`aiohttp` like test_inverter.py and test_clamp.py do. Parts 8-10 grade behaviour +that only exists once a Controller has built a roster - which pack a claim is +filed on, which pack's SoC a machine is ticked with, what a one-inverter store +looks like afterwards - and none of that is observable from the state machine +alone. + Walks a whole cycle in simulated time, which is the test the YAML version never had - and it is exactly the phase transitions that a real cycle takes ten hours to exercise once a month. @@ -12,6 +19,7 @@ is TEST-15, and it has already turned one live mutation into a false survivor. Read MUTATION.md before you report a survivor. """ +import asyncio import dataclasses import re import sys @@ -19,7 +27,13 @@ from datetime import datetime, timedelta, timezone from pathlib import Path from app.maintenance import ( - CHARGE, DRAIN, HOLD, IDLE, MAINT_OPTION_DEFAULTS, MaintConfig, Maintenance, + CHARGE, DRAIN, HOLD, IDLE, MAINT_OPTION_DEFAULTS, POWER_MOVING, MaintConfig, + Maintenance, UnitHistory, arbitrate, +) +from app.main import Controller +from app.store import ( + DEFAULTS, MAINT_SCHEMA_KEY, MAINT_SCHEMA_VERSION, MAINT_UNIT_KEYS, + migrate_maint_keys, unit_key, ) fails = [] @@ -223,6 +237,850 @@ check("...and an explicit falsey value is honoured rather than defaulted away", MaintConfig.from_options({"maintenance_start_hour": 0}).start_hour == 0 and MaintConfig.from_options({"maintenance_enabled": False}).enabled is False) + + +# =========================================================================== # +# MAINT-05. TWO PACKS, ONE GRID CONNECTION. +# +# ⚠️ THE FIXTURE RULE THIS REPOSITORY KEEPS PAYING FOR (MUTATION.md habit 4). +# Every two-pack fixture below gives the packs DIFFERENT phases and DIFFERENT +# `last_completed` times, because the bug being hunted is SHARED STATE and two +# equal values cannot show it. `phase("drain") == phase("drain")` is true whether +# the two machines have one history between them or two, and a fixture where +# both packs last completed on the same day cannot tell "the longest-waiting +# pack goes first" from "the first roster entry goes first". SAFETY-30 and +# CFG-04 were each amended for exactly this. +# =========================================================================== # + + +class MemStore: + """`app.store.Store`'s surface, no disk, and it keeps keys VERBATIM. + + ⚠️ VERBATIM IS THE WHOLE POINT. A namespaced write has to land under a + different key from a flat one, and a fake that normalised or ignored the + prefix would make every check in parts 6-10 pass on a version that has no + namespacing at all. Times go in and come out the way the real Store stores + them - ISO-8601 with an offset - so the migration below moves the same kind + of value a real state file holds. + """ + + def __init__(self, data=None): + self.data = dict(DEFAULTS, **(data or {})) + self.writes = [] + + def set(self, k, v): + self.data[k] = v + self.writes.append(k) + + def get_time(self, k): + raw = self.data.get(k) + if not raw: + return None + if isinstance(raw, datetime): + return raw + parsed = datetime.fromisoformat(raw) + return parsed if parsed.tzinfo else parsed.replace(tzinfo=timezone.utc) + + def set_time(self, k, when): + self.data[k] = when.astimezone(timezone.utc).isoformat() if when else None + self.writes.append(k) + + def save(self): + pass + + +def two_packs(store=None, mirror=True): + """Pack A and pack B on ONE store, each under its own namespace.""" + store = store if store is not None else MemStore() + a = Maintenance(cfg, store, unit="inverter-a", name="inverter-a", + mirror_flat=mirror) + b = Maintenance(cfg, store, unit="inverter-b", name="inverter-b", + mirror_flat=False) + return store, a, b + + +def drive(machines, now, socs): + """One site tick: arbitrate once, then tick every machine with ITS pack's + SoC and the answer arbitration gave it. This is `Controller.cycle()`'s + three lines, and calling `tick()` without the plan is the defect.""" + plan = arbitrate(machines, now) + return [m.tick(now, soc, blocked_by=b) + for m, soc, b in zip(machines, socs, plan)] + + +# --------------------------------------------------------------------------- # +print() +print("6. per-pack schedule, phase and history (AC1, AC2)") + +_t = datetime(2026, 9, 4, 10, 0, tzinfo=timezone.utc) +_store, _a, _b = two_packs() +# ⚠️ DIFFERENT PHASES, DIFFERENT DATES. A is mid-charge and last ran 40 days +# ago; B is idle and last ran 33 days ago. Shared state shows up as either of +# those pairs collapsing into one value. +_a.store.set("phase", CHARGE) +_a.store.set_time("phase_started", _t - timedelta(hours=4)) +_a.store.set_time("last_completed", _t - timedelta(days=40)) +_b.store.set("phase", IDLE) +_b.store.set_time("last_completed", _t - timedelta(days=33)) + +print(f" A: phase={_a.phase} last={_a.store.get_time('last_completed'):%Y-%m-%d}" + f" B: phase={_b.phase} last={_b.store.get_time('last_completed'):%Y-%m-%d}") +check("the two packs hold DIFFERENT phases at the same moment, which one FSM " + "with one history cannot do - and which a fixture with two equal phases " + "could not have shown", + _a.phase == CHARGE and _b.phase == IDLE and _a.phase != _b.phase) +check("...and DIFFERENT last-completed dates, so 'A completed' and 'B " + "completed' are distinguishable statements rather than the same one", + _a.store.get_time("last_completed") != _b.store.get_time("last_completed") + and _a.waited_days(_t) > _b.waited_days(_t)) +check("...and they live under different Store keys, spelt by the same " + "`unit_key()` the migration and the watchdog use", + _store.data.get(unit_key("inverter-a", "phase")) == CHARGE + and _store.data.get(unit_key("inverter-b", "phase")) == IDLE) + +# AC: completing A's cycle must not mark B complete. +_b_before = _b.store.get_time("last_completed") +_a.store.set("phase", HOLD) +_a.store.set_time("phase_started", _t - timedelta(minutes=cfg.hold_min + 1)) +_r = drive([_a, _b], _t, [100.0, 50.0]) +print(f" after A's hold expires: A={_r[0].phase} completed " + f"{_a.store.get_time('last_completed'):%Y-%m-%d %H:%M}, B={_r[1].phase} " + f"completed {_b.store.get_time('last_completed'):%Y-%m-%d %H:%M}") +check("A's cycle COMPLETES and stamps A's own history", + _r[0].phase == IDLE + and _a.store.get_time("last_completed") == _t) +check("⚠️ ...and B's history is untouched by it. This is the card's first " + "acceptance criterion and the reason the two dates were seeded " + "differently: with one shared `last_completed`, B would have just been " + "marked recalibrated without a single watt moving through it", + _b.store.get_time("last_completed") == _b_before + and _b.store.get_time("last_completed") != _a.store.get_time("last_completed")) +check("...and B is still DUE, which is the consequence an operator would " + "actually feel: a pack marked complete by its sibling stops being " + "scheduled", + _b.due(_t)) + + +# --------------------------------------------------------------------------- # +print() +print("7. the interlock: one pack moves power at a time (AC3)") + +# Both packs due in the same start hour - the case the card says two packs on +# one monthly schedule drift into. ⚠️ B IS THE MORE OVERDUE ONE, deliberately: +# roster order would pick A, so a check that passed with B waiting longer AND A +# winning would be a check that cannot see the arbitration at all. +_store2, _a2, _b2 = two_packs() +_a2.store.set_time("last_completed", _t - timedelta(days=30)) +_b2.store.set_time("last_completed", _t - timedelta(days=45)) +check("the fixture is discriminating: both packs want to start on this tick, " + "and the one that has waited LONGER is the one roster order would NOT " + "pick", + _a2.wants_start(_t) and _b2.wants_start(_t) + and _b2.waited_days(_t) > _a2.waited_days(_t)) + +_plan = arbitrate([_a2, _b2], _t) +_r2 = drive([_a2, _b2], _t, [80.0, 80.0]) +print(f" plan -> A: {_plan[0] or 'GO'} | B: {_plan[1] or 'GO'}") +print(f" result -> A: {_r2[0].phase} owns={_r2[0].owns_setpoint} | " + f"B: {_r2[1].phase} owns={_r2[1].owns_setpoint}") +check("⚠️ EXACTLY ONE PACK PROCEEDS. Two DRAINs at once is two power-moving " + "phases on one grid connection - the circulating power SAFETY-31 exists " + "to detect - and this is the line that makes it unreachable", + sum(1 for r in _r2 if r.phase in POWER_MOVING) == 1 + and sum(1 for r in _r2 if r.owns_setpoint) == 1) +check("...and it is the LONGEST-WAITING pack, not the first in the roster. " + "Roster order would recalibrate inverter-a every month forever and let " + "inverter-b's coulomb counter drift - this card's own defect in a " + "different costume", + _r2[1].phase == DRAIN and _r2[0].phase == IDLE) +check("...and the deferred pack states a reason that NAMES the pack it is " + "waiting for and the two waits it was compared on, so the arbitration is " + "auditable rather than a silent skip", + "inverter-b" in _r2[0].deferred and "45 d" in _r2[0].deferred + and "30 d" in _r2[0].deferred) + +# The mirror image, so the ordering is a comparison and not a constant. +_store3, _a3, _b3 = two_packs() +_a3.store.set_time("last_completed", _t - timedelta(days=45)) +_b3.store.set_time("last_completed", _t - timedelta(days=30)) +_r3 = drive([_a3, _b3], _t, [80.0, 80.0]) +check("...and with the waits SWAPPED the winner swaps too, which is what " + "distinguishes an arbitration from a hard-wired index", + _r3[0].phase == DRAIN and _r3[1].phase == IDLE) + +# A tie falls to roster order - stated, checked, and the only thing roster +# order is allowed to decide. +_store4, _a4, _b4 = two_packs() +_a4.store.set_time("last_completed", _t - timedelta(days=40)) +_b4.store.set_time("last_completed", _t - timedelta(days=40)) +_r4 = drive([_a4, _b4], _t, [80.0, 80.0]) +check("a genuine numeric tie breaks on roster order, and only a tie does", + _r4[0].phase == DRAIN and _r4[1].phase == IDLE) + +# ⚠️ NEVER RECALIBRATED OUTRANKS ANY FINITE WAIT, and it is checked because +# `waited_days()` returns `inf` there rather than 0 - which is the same +# statement `due()` makes about an absent `last_completed`. A 0 would make a +# brand-new pack the LAST to be recalibrated on every contested month, which is +# the one pack whose coulomb counter has never been trued at all. +_store_new, _a_new, _b_new = two_packs() +_a_new.store.set_time("last_completed", _t - timedelta(days=100)) +_r_new = drive([_a_new, _b_new], _t, [80.0, 80.0]) +print(f" A last ran 100 d ago, B has NEVER run -> A: {_r_new[0].phase}, " + f"B: {_r_new[1].phase}") +check("a pack that has never been recalibrated outranks one that completed 100 " + "days ago, because 'never' is not a small number - it is the absence of a " + "coulomb count", + _b_new.waited_days(_t) > _a_new.waited_days(_t) + and _r_new[1].phase == DRAIN and _r_new[0].phase == IDLE) + +# ⚠️ CHARGE HOLDS THE SLOT TOO. This is the check that bites if CHARGE is +# dropped from POWER_MOVING - the phase where the LOOP owns the setpoint and +# the pack is nonetheless being pulled full from the grid. +_store5, _a5, _b5 = two_packs() +_a5.store.set("phase", CHARGE) +_a5.store.set_time("phase_started", _t - timedelta(hours=2)) +_a5.store.set_time("last_completed", _t - timedelta(days=40)) +_b5.store.set_time("last_completed", _t - timedelta(days=60)) +_r5 = drive([_a5, _b5], _t, [40.0, 80.0]) +print(f" A charging, B due and MORE overdue -> A: {_r5[0].phase}, " + f"B: {_r5[1].phase} ({_r5[1].deferred[:60]}...)") +check("⚠️ A PACK IN **CHARGE** HOLDS THE SITE'S MAINTENANCE SLOT, even though " + "the control loop owns the setpoint in that phase. A charging at 2000 W " + "from the grid while B drains 2000 W to export is 4 kW going round in a " + "circle with the meter reading zero and both packs believing they are " + "behaving - which is why CHARGE is in POWER_MOVING", + _r5[0].phase == CHARGE and _r5[1].phase == IDLE and _r5[1].deferred) +check("...and B is deferred even though it is the MORE overdue pack, because " + "nothing preempts a cycle in flight: a recalibration stopped half way has " + "spent the energy and bought no coulomb count", + _b5.waited_days(_t) > _a5.waited_days(_t) and _r5[1].phase == IDLE) + +# HOLD, for the same reason stated differently: it commands 0 W and moves +# nothing, but it owns the actuator and the pack is parked for the BMS. +_store6, _a6, _b6 = two_packs() +_a6.store.set("phase", HOLD) +_a6.store.set_time("phase_started", _t - timedelta(minutes=5)) +_a6.store.set_time("last_completed", _t - timedelta(days=40)) +_b6.store.set_time("last_completed", _t - timedelta(days=60)) +_r6 = drive([_a6, _b6], _t, [100.0, 80.0]) +check("a pack in HOLD holds the slot too - it commands 0 W and moves nothing, " + "and it owns the actuator while the BMS balances", + _r6[0].phase == HOLD and _r6[1].phase == IDLE and _r6[1].deferred) + +# The recovery branch: a store that says BOTH packs are mid-cycle. Only +# reachable from a hand-edited state file or a version with no interlock, and +# the fail-safe direction is unambiguous. +_store7, _a7, _b7 = two_packs() +for _m, _ph in ((_a7, DRAIN), (_b7, DRAIN)): + _m.store.set("phase", _ph) + _m.store.set_time("phase_started", _t - timedelta(hours=1)) +_r7 = drive([_a7, _b7], _t, [80.0, 80.0]) +print(f" a store that says BOTH are draining -> A: {_r7[0].phase}, " + f"B: {_r7[1].phase} @ {_r7[1].setpoint_w} W") +check("a store carrying TWO packs mid-cycle is recovered from rather than " + "obeyed: the first keeps its cycle, the second is stood down to 0 W and " + "says why", + _r7[0].phase == DRAIN and _r7[1].phase == IDLE + and _r7[1].setpoint_w == 0.0 + and any("interlock" in e for e in _r7[1].events)) + +# The operator's button, and the API a future price-gated start goes through. +_store8, _a8, _b8 = two_packs() +_a8.store.set("phase", DRAIN) +_a8.store.set_time("phase_started", _t) +_ev = _b8.force_start(_t, blocked_by=arbitrate([_a8, _b8], _t)[1]) +print(f" forced start on B while A drains -> {_ev}") +check("a FORCED start is refused while another pack holds the slot, and the " + "phase machine does not move. The calendar is a preference an operator " + "may overrule; one pack at a time is a property of the grid connection " + "and a button does not add a second one", + _b8.phase == IDLE and len(_ev) == 1 and "REFUSED" in _ev[0] + and "inverter-a" in _ev[0]) +check("...and an unblocked forced start still works, so the refusal is a " + "condition and not a disabled feature", + Maintenance(cfg, MemStore(), unit="inverter-b", name="inverter-b") + .force_start(_t) and True) + + +# --------------------------------------------------------------------------- # +print() +print("8. the deferred pack is not starved, and is not mistaken for a fault (AC4)") + +_store9, _a9, _b9 = two_packs() +_a9.store.set_time("last_completed", _t - timedelta(days=45)) +_b9.store.set_time("last_completed", _t - timedelta(days=30)) +_r9 = drive([_a9, _b9], _t, [80.0, 80.0]) +check("A takes the slot and B is deferred, which is the starting state for " + "everything below", + _r9[0].phase == DRAIN and _r9[1].phase == IDLE and _r9[1].deferred) + +# ⚠️ THE STARVATION MECHANISM, ASSERTED DIRECTLY. `last_start_attempt` is the +# post-ABORT cooldown (90 min) and the start hour is 60 minutes long, so a +# deferral that stamped it would lock the deferred pack out of the very hour it +# is waiting for - and out of the moment the winner releases the slot. +check("⚠️ A DEFERRAL DOES NOT STAMP `last_start_attempt`. Nothing was attempted " + "and nothing failed; stamping it would put the deferred pack under a " + "90 min cooldown inside a 60 min start hour, which is starvation " + "implemented by accident", + _b9.store.get_time("last_start_attempt") is None) +check("...and the deferred pack's PHASE does not move either, so " + "`phase_started` stays empty and neither the 12 h DRAIN nor the 36 h " + "CHARGE timeout can be reached by waiting. §12's note is explicit that a " + "phase timeout means 'the battery is not following commands - a FAULT'; " + "a deferred pack must never raise it", + _b9.phase == IDLE and _b9.store.get_time("phase_started") is None) +check("...and the words say WAITING, name the pack it waits for, and say it " + "will run next - none of which any failure path in this module says", + "WAITING" in _r9[1].events[0] + and "inverter-a" in _r9[1].events[0] + and "not a stalled phase" in _r9[1].events[0] + and "ABORT" not in _r9[1].events[0] and "stuck" not in _r9[1].events[0]) +# ⚠️ THE SECOND TICK SAYS SOMETHING NEW AND THE THIRD SAYS NOTHING. On the +# first tick both packs wanted to start and the reason was the comparison that +# settled it; on the second, A is mid-cycle and the reason is that instead - a +# genuinely different sentence, so it is logged. From there it is constant and +# the log falls silent, which is what edge-logging means. +_r9b = drive([_a9, _b9], _t + timedelta(minutes=1), [80.0, 80.0]) +_r9c = drive([_a9, _b9], _t + timedelta(minutes=2), [80.0, 80.0]) +check("...and it is said once per REASON, not once a second: a wait that lasts " + "a whole cycle would otherwise write one line per second and flush the " + "40-line action log of the very context that explains it", + _r9b[1].deferred and _r9b[1].events and len(_r9b[1].events) == 1 + and _r9c[1].deferred == _r9b[1].deferred and _r9c[1].events == []) +check("...and the reason it changed to is the one that is true now - A is " + "mid-cycle - rather than the comparison that was true when the slot was " + "still open", + "mid-cycle" in _r9c[1].deferred + and "has waited longer" in _r9[1].deferred) + +# Now let A finish, and prove B recalibrates on the NEXT opportunity. This is +# the acceptance criterion that asks for a test rather than a comment. +_when = _t +for _step, _soc_a in ((timedelta(hours=3), 10.0), # A drains to its floor + (timedelta(hours=20), 99.0), # A charges to target + (timedelta(hours=23), 100.0)): # A's hold expires + _when = _t + _step + drive([_a9, _b9], _when, [_soc_a, 80.0]) +check("A's cycle runs to completion while B waits", + _a9.phase == IDLE and _a9.store.get_time("last_completed") is not None) +_next = (_t + timedelta(days=1)).replace(hour=cfg.start_hour) +_r9n = drive([_a9, _b9], _next, [80.0, 80.0]) +print(f" next start hour ({_next:%Y-%m-%d %H:%M}) -> A: {_r9n[0].phase}, " + f"B: {_r9n[1].phase}") +check("⚠️ AND B RECALIBRATES AT THE NEXT START HOUR. Not a comment claiming it " + "is not starved - the deferred pack driven through the winner's whole " + "cycle and observed starting", + _r9n[1].phase == DRAIN and _r9n[1].owns_setpoint) +check("...and A, which has just completed, does not contend for the slot " + "again - so the two schedules come out of one contested month STAGGERED, " + "which is the property the card asks the PR to state", + _r9n[0].phase == IDLE and not _a9.due(_next)) +check("...and B's deferral produced no claim on the tick it was deferred: a " + "deferred pack owns no setpoint and shapes nothing, so it is simply " + "under the loop like any idle pack", + not _r9[1].owns_setpoint and not _r9[1].charge_only) + + + +# --------------------------------------------------------------------------- # +print() +print("9. the store migration: no lost history, and the three rules hold (AC5)") + +# A real pre-MAINT-05 single-pack store: a month of history under the flat keys, +# with SAFETY-02's own migration already done (schema_version 1). +def flat_store(**over): + return MemStore(dict({ + "schema_version": 1, + "phase": CHARGE, + "phase_started": "2026-09-03T22:00:00+00:00", + "last_completed": "2026-08-07T12:00:00+00:00", + "last_start_attempt": "2026-09-03T10:00:00+00:00", + }, **over)) + + +_m1 = flat_store() +_rep1 = migrate_maint_keys(_m1, "inverter-a", 2) +print(f" a single-pack store growing to two: {_rep1.explain()}") +check("every flat maintenance key lands in the FIRST pack's namespace, because " + "the flat keys can only ever have belonged to the pack the one FSM was " + "driving", + _rep1.ran and _rep1.ok and _rep1.verified + and all(_m1.data.get(unit_key("inverter-a", k)) == _m1.data[k] + for k in MAINT_UNIT_KEYS)) +check("...and the VALUES are the history, not defaults wearing its name - a " + "migration that lands `last_completed: None` has lost the month it was " + "moving and would recalibrate a pack that does not need it", + _m1.data.get(unit_key("inverter-a", "last_completed")) + == "2026-08-07T12:00:00+00:00" + and _m1.data.get(unit_key("inverter-a", "phase")) == CHARGE) +check("RULE 2: the flat keys are LEFT IN THE FILE, so an operator who rolls the " + "add-on back to a single-FSM version finds the history it had", + all(k in _m1.data for k in MAINT_UNIT_KEYS) + and _m1.data["last_completed"] == "2026-08-07T12:00:00+00:00") +check("RULE 3: the marker is written and the store is now at the per-pack " + "schema", + _m1.data[MAINT_SCHEMA_KEY] == MAINT_SCHEMA_VERSION) +check("...and the pack that has just arrived gets NOTHING, which is the right " + "answer rather than an omission: absent means never recalibrated, means " + "due, and a brand-new pack should be", + not any(k.startswith(unit_key("inverter-b", "")) for k in _m1.data) + and Maintenance(cfg, _m1, unit="inverter-b").due(_t)) + +# Rule 1, and it is the rule that makes re-running safe. +_m2 = flat_store(**{unit_key("inverter-a", "last_completed"): + "2026-09-01T09:00:00+00:00"}) +_rep2 = migrate_maint_keys(_m2, "inverter-a", 2) +check("RULE 1: an EXISTING namespaced value is never overwritten by the flat " + "one. A store with both spellings has been written by a version that knew " + "about the namespace, and preferring the flat one would roll a pack's " + "history back to whatever it was on migration day", + _m2.data.get(unit_key("inverter-a", "last_completed")) + == "2026-09-01T09:00:00+00:00" + and unit_key("inverter-a", "last_completed") in _rep2.kept) +check("...so running the migration TWICE is a no-op, which is what makes a " + "crash between the copies and the marker safe", + migrate_maint_keys(_m1, "inverter-a", 2).ran is False) + +# The store a multi-inverter version wrote. AC: not touched. +_m3 = MemStore({MAINT_SCHEMA_KEY: MAINT_SCHEMA_VERSION, + unit_key("inverter-a", "last_completed"): "2026-09-01T09:00:00+00:00", + unit_key("inverter-b", "last_completed"): "2026-08-20T09:00:00+00:00", + "last_completed": "2026-09-01T09:00:00+00:00"}) +_before3 = dict(_m3.data) +_rep3 = migrate_maint_keys(_m3, "inverter-a", 2) +print(f" a store already written by a multi-inverter version: {_rep3.explain()}") +check("a store already at the per-pack schema is NOT TOUCHED - not one key " + "written - and says so", + not _rep3.ran and _m3.data == _before3 and _m3.writes == [] + and "not touched" in _rep3.explain()) + +# ⚠️ AND BREAK IT. The read-back postcondition is the whole difference between +# this and a two-line copy, and it is inherited from `migrate_unit_keys()` +# rather than re-implemented - so it has to be shown working HERE. +class LosesTheHistory(MemStore): + """A store that accepts the namespaced write and drops it on the floor. + + What a partial write, a full disk and a silently-swallowed OSError all look + like from here: `Store.save()` catches OSError and carries on by design. + """ + + def set(self, k, v): + if k.startswith("unit/"): + return + super().set(k, v) + + +_m4 = LosesTheHistory(dict(flat_store().data)) +_rep4 = migrate_maint_keys(_m4, "inverter-a", 2) +print(f" a store that silently drops the migrated keys: {_rep4.explain()}") +check("⚠️ THE MIGRATION REPORTS THAT IT FAILED, because it READS BACK what it " + "wrote. A month of coulomb-count history under a key nothing reads is a " + "pack that has quietly stopped being maintained on an install whose state " + "file says it has not", + _rep4.ran and not _rep4.verified and not _rep4.ok and _rep4.lost) +check("...and it does NOT mark the store migrated, so the next start runs it " + "again rather than skipping a migration that never happened", + MAINT_SCHEMA_KEY not in _m4.data) +check("...while a store whose only failed write was a DEFAULT is not reported " + "as lost state: a fresh install has no `last_completed` at all, and " + "'absent' and 'never recalibrated' are the same statement - an alarm that " + "fires on the harmless case is one an operator learns to dismiss", + migrate_maint_keys(LosesTheHistory({"schema_version": 1}), + "inverter-a", 2).ok is True) + + +# --------------------------------------------------------------------------- # +print() +print("10. a roster of ONE is byte-identical (AC6)") + +# ⚠️ THE STATE FILE IS THE ASSERTION, NOT A CLAIM ABOUT IT. A one-inverter +# install must come out of this card with the same keys it went in with: no +# namespace, no marker, no fifth key. `unit=None` is what makes that true and +# this is the check that bites if it stops being passed. +_solo_store = MemStore() +_solo = Maintenance(cfg, _solo_store) +_solo_events = [] +_solo_events += _solo.force_start(t0) +for _dt, _soc in ((timedelta(0), 80.0), (timedelta(hours=3), 10.5), + (timedelta(hours=20), 99.0), (timedelta(hours=21), 100.0), + (timedelta(hours=22, minutes=5), 100.0)): + _solo_events += _solo.tick(t0 + _dt, _soc).events +print(f" one pack, one whole cycle -> store keys " + f"{sorted(k for k in _solo_store.data if k not in DEFAULTS)}") +check("⚠️ A WHOLE CYCLE ON A ROSTER OF ONE WRITES NO KEY THAT DID NOT EXIST " + "BEFORE THIS CARD. No `unit/...` namespace and no `maint_schema_version` " + "- the state file is the one an install already has", + not any(k.startswith("unit/") for k in _solo_store.data) + and MAINT_SCHEMA_KEY not in _solo_store.data + and set(_solo_store.data) == set(DEFAULTS)) +check("...and the flat keys are the ones that carried the cycle, so 'no new " + "keys' is not 'no history'", + _solo_store.data["phase"] == IDLE + and _solo_store.get_time("last_completed") + == t0 + timedelta(hours=22, minutes=5)) +check("...and `migrate_maint_keys()` refuses to run at all on a one-pack " + "roster, which is why the layout above stays put", + migrate_maint_keys(MemStore({"phase": DRAIN}), "inverter-a", 1).ran is False) + +# ⚠️ THE REASON STRINGS, PINNED AS LITERALS. The card asks for byte-identical +# behaviour AND reason strings at N=1, and the only way a check can see a +# reworded event is to hold the words. Every one of these is the string the +# shipped module produced before this card; the interlock added no clause to any +# of them, because a single machine is never another machine's blocker. +check(f"the events of a whole one-pack cycle are the shipped strings, " + f"unchanged {_solo_events}", + _solo_events == [ + "maintenance: forced start -> drain", + "maintenance: drain complete at 10 % after 180 min -> charge", + "maintenance: charge complete at 99 % after 1020 min -> holding " + "120 min to balance", + "maintenance: cycle COMPLETE (125 min hold) - handing back to the loop", + ]) +check("...and nothing about a deferral, a slot or another pack appears in any " + "of them - `arbitrate()` over one machine is the identity function and " + "no reason string it can build is reachable", + not any("slot" in e or "WAITING" in e or "REFUSED" in e + for e in _solo_events) + and arbitrate([_solo], t0) == [""]) + +# The abort path's words too, since it is the one an operator reads under +# pressure. +_solo2 = Maintenance(cfg, MemStore()) +_solo2.force_start(t0) +check("...and so is the abort, which is the string an operator reads under " + "pressure", + _solo2.tick(t0 + timedelta(hours=13), 60.0).events + == ["maintenance: ABORT (drain stuck at 60 % after 780 min)"]) + + + +# --------------------------------------------------------------------------- # +print() +print("11. the wiring: a real Controller, a real cycle, a real claim (AC3, " + "point 3 of the card)") + + +class Hass: + """Answers PER ENTITY, which is the property every check below needs. + + A stub that returned one number whatever it was asked cannot tell "pack B's + machine was ticked with pack B's SoC" from "it was handed pack A's", and + that is one of the two defects this part exists to catch. + """ + + def __init__(self, numbers, limits): + self.numbers = dict(numbers) + self.limits_by = dict(limits) + self.writes = [] + + async def number(self, entity, invert=False): + v = self.numbers.get(entity) + return None if v is None else (-v if invert else v) + + async def limits(self, entity): + return self.limits_by.get(entity, (None, None, None)) + + async def set_number(self, entity, value): + self.writes.append((entity, value)) + return True + + async def call(self, domain, service, data): + return True + + +class Pub: + enabled = False + + def publish(self, *a, **k): + pass + + def close(self): + pass + + +# ⚠️ 3000 W FOR PACK B IS A HYPOTHESIS AND IS MARKED AS ONE EVERYWHERE IT +# APPEARS. Inverter B does not physically exist; the number is here only because +# it DIFFERS from pack A's 5000 W, which is the only property these checks use. +B_RATING_HYPOTHESIS = 3000.0 +A_RATING = 5000.0 +FLAT_OPTS = dict(setpoint_entity="number.sp_a", soc_entity="sensor.soc_a", + batt_entity="sensor.batt_a", meter_source="off", + meter_entity="sensor.grid", max_w=A_RATING, gain=0.6, + # ⚠️ `slew_w` IS 6000 AND THE METER READS +5000 W FOR ONE + # REASON, STATED HERE RATHER THAN TUNED UNTIL GREEN: the + # allocator REFUSES a split whose shares have opposite signs, + # so a drain pinned at +2000 W beside a site target below + # +2000 W would park the whole site at 0 W and these checks + # would grade a failsafe instead of a claim. A house importing + # 5000 W and a slew that lets one cycle reach it puts the site + # target ABOVE the pin, which is the state a real site is in + # for most of a drain. The opposite state is a real defect and + # it is recorded below rather than tuned away. + slew_w=6000, deadband_w=15, target_grid_w=0, step_w=10, + heartbeat_s=10, stale_input_s=15, + maintenance_discharge_w=2000, maintenance_charge_w=2000) +TWO_OPTS = {k: v for k, v in FLAT_OPTS.items() + if k not in ("setpoint_entity", "soc_entity", "batt_entity")} +TWO_OPTS["inverters"] = [ + dict(name="inverter-a", setpoint_entity="number.sp_a", + soc_entity="sensor.soc_a", batt_entity="sensor.batt_a", + max_w=A_RATING), + dict(name="inverter-b", setpoint_entity="number.sp_b", + soc_entity="sensor.soc_b", batt_entity="sensor.batt_b", + max_w=B_RATING_HYPOTHESIS), +] + + +def controller(opts, store=None, soc_a=80.0, soc_b=80.0, grid=5000.0): + """A real Controller against a stub HA. ⚠️ SIMULATED - no rig, no device.""" + hass = Hass({"sensor.grid": grid, "sensor.soc_a": soc_a, "sensor.batt_a": 0.0, + "sensor.soc_b": soc_b, "sensor.batt_b": 0.0}, + {"number.sp_a": (-A_RATING, A_RATING, 1.0), + "number.sp_b": (-B_RATING_HYPOTHESIS, B_RATING_HYPOTHESIS, 1.0)}) + c = Controller(dict(opts), hass, store if store is not None else MemStore(), + Pub(), stop=asyncio.Event()) + for inv in c.inverters: + inv.dev_min, inv.dev_max = -inv.max_w, inv.max_w + c.auto = True + return c, hass + + +def one_cycle(c): + async def go(): + await c.read_inputs() + await c.cycle() + asyncio.run(go()) + return c + + +_now = datetime.now(timezone.utc) + +# --- the claim is filed on the pack whose machine produced it --------------- +# ⚠️ PACK **B** IS THE ONE RECALIBRATING, and that is the whole fixture. With +# the claim filed on `maint_unit` - `inverters[0]`, which is what shipped - +# pack A would be pinned at the drain rate while pack B, the pack actually being +# recalibrated, stayed under the loop: the site would move the right total and +# recalibrate nothing. +_cB, _hB = controller(TWO_OPTS) +_cB.maints[1].force_start(_now) +one_cycle(_cB) +print(f" B recalibrating -> targets A={_cB.inverters[0].target:.0f} W, " + f"B={_cB.inverters[1].target:.0f} W; wire saw {_hB.writes}") +check("⚠️ THE DRAIN LANDS ON PACK B, THE PACK WHOSE STATE MACHINE ASKED FOR IT", + _cB.maints[1].phase == DRAIN + and _cB.inverters[1].target == _cB.maint.cfg.discharge_w + and ("number.sp_b", _cB.maint.cfg.discharge_w) in _hB.writes) +check("...and pack A is NOT commanded the drain rate - it carries the REST of " + "the site target, as any unpinned unit does. This is the half that makes " + "the check discriminating: filing B's procedure on A would move the same " + "site total and recalibrate the wrong pack, and the site sum would look " + "identical", + _cB.inverters[0].target != _cB.maint.cfg.discharge_w + and _cB.inverters[0].target + _cB.inverters[1].target == _cB.target) +check("...and the claim is a UNIT claim on B's own actuator, never a site one - " + "ARCH-01's rule, and what structurally prevents the cross-inverter " + "`lo > hi` contradiction", + _cB.actuator_of("maintenance", _cB.inverters[1]) == "inverter-b" + and _cB.actuator_of("maintenance", _cB.inverters[0]) == "inverter-a") + +# The mirror image: pack A recalibrating must land on pack A. +_cA, _hA = controller(TWO_OPTS) +_cA.maints[0].force_start(_now) +one_cycle(_cA) +check("...and with pack A recalibrating instead, the drain lands on A and not " + "on B - so the routing follows the machine rather than a constant", + _cA.inverters[0].target == _cA.maint.cfg.discharge_w + and _cA.inverters[1].target != _cA.maint.cfg.discharge_w) + +# --- each machine is ticked with ITS OWN pack's SoC ------------------------- +# ⚠️ THE TWO SoCs ARE DELIBERATELY ON OPPOSITE SIDES OF THE FLOOR. B is at +# 10.5 % (below `soc_floor` 11) and A is at 90 %, so "B's drain ends" and "A's +# drain continues" are different answers to the same tick - which a fixture with +# two equal SoCs could not have produced. +_cS, _ = controller(TWO_OPTS, soc_a=90.0, soc_b=10.5) +_cS.maints[1].force_start(_now) +one_cycle(_cS) +print(f" A at 90 %, B at 10.5 %, floor {cfg.soc_floor} % -> B's phase " + f"{_cS.maints[1].phase}") +check("⚠️ PACK B'S MACHINE IS GRADED ON PACK B'S OWN SoC: at 10.5 % against an " + "11 % floor its drain is complete and it moves to CHARGE. Handed pack A's " + "90 % it would still be draining a pack that is already empty", + _cS.maints[1].phase == CHARGE) +_cS2, _ = controller(TWO_OPTS, soc_a=90.0, soc_b=10.5) +_cS2.maints[0].force_start(_now) +one_cycle(_cS2) +check("...and pack A's machine, on the same site with the same two readings, " + "stays in DRAIN at 90 % - the same tick, the opposite answer, because the " + "readings are not interchangeable", + _cS2.maints[0].phase == DRAIN) + +# --- the operator-facing surfaces ------------------------------------------ +# ⚠️ TWO DIFFERENT DUE DATES, BECAUSE ONE ROW HAS TO CARRY BOTH SCHEDULES. A +# fixture with two equal dates cannot tell "the earliest across the packs" from +# "the first pack's", which is the whole content of the row. +_cD, _ = controller(TWO_OPTS) +_cD.maints[0].store.set_time("last_completed", _now - timedelta(days=2)) +_cD.maints[1].store.set_time("last_completed", _now - timedelta(days=20)) +_rows = dict(_cD.status_payload()["rows"]) +print(f" A last ran 2 d ago, B 20 d ago -> due row {_rows['Maintenance due']!r}") +check("the 'Maintenance due' row is the EARLIEST date across the packs, not the " + "first pack's - one row on a site with two schedules has to be the one " + "that comes first, or it says October while a pack recalibrates tomorrow", + _rows["Maintenance due"] + == (_now + timedelta(days=cfg.interval_days - 20)).strftime("%Y-%m-%d")) +_cD.maints[1].force_start(_now) +one_cycle(_cD) +_rows2 = dict(_cD.status_payload()["rows"]) +print(f" B recalibrating -> banner {_cD.status_payload()['banner']!r}, " + f"phase row {_rows2['Maintenance phase']!r}") +check("...and at two packs the banner and the phase row NAME the pack, because " + "the remedy is at one device and the site has two", + "inverter-b" in _cD.status_payload()["banner"] + and "inverter-b" in _rows2["Maintenance phase"]) +_cD1, _ = controller(FLAT_OPTS) +_cD1.maint.force_start(_now) +one_cycle(_cD1) +_cD1rows = dict(_cD1.status_payload()["rows"]) +print(f" the same state at ONE pack -> banner " + f"{_cD1.status_payload()['banner']!r}, phase row " + f"{_cD1rows['Maintenance phase']!r}") +check("...while at a roster of one the banner and the phase row are the bare " + "phase they have always been, with no pack name appended - which is what " + "'byte-identical reason strings' means on the surfaces an operator reads", + _cD1rows["Maintenance phase"] == _cD1.maint.phase + and "(" not in _cD1rows["Maintenance phase"] + and _cD1.status_payload()["banner"] == f"Maintenance: {_cD1.maint.phase}") + +# --- and the Controller actually WIRES the interlock ------------------------ +# ⚠️ THE INTERLOCK CAN BE PRESENT AND NOT CONNECTED, which is the failure this +# check exists for: `arbitrate()` computed and its answer never handed to +# `tick()` is two packs draining with a correct, unread plan sitting beside +# them. A store that comes up with BOTH packs mid-drain is the state that +# exercises it through a REAL cycle rather than through this file's own helper. +_cI, _ = controller(TWO_OPTS) +for _m in _cI.maints: + _m.store.set("phase", DRAIN) + _m.store.set_time("phase_started", _now - timedelta(hours=1)) +one_cycle(_cI) +print(f" both packs came up draining -> {[m.phase for m in _cI.maints]}") +check("⚠️ ONE REAL CYCLE LEAVES EXACTLY ONE PACK DRAINING. This is the check " + "that bites if `Controller.cycle()` computes the plan and does not pass " + "it to `tick()` - an interlock that is present, correct and not connected", + [m.phase for m in _cI.maints] == [DRAIN, IDLE]) +check("...and the pack that was stood down was commanded 0 W on the wire, not " + "merely marked idle", + _cI.inverters[1].target == 0.0) + +# --- and at a roster of one, everything above is the shipped object --------- +_c1store = MemStore() +_c1, _ = controller(FLAT_OPTS, store=_c1store) +print(f" one inverter -> {len(_c1.maints)} machine(s), namespace " + f"{_c1.maints[0].unit!r}, store keys " + f"{sorted(k for k in _c1store.data if k not in DEFAULTS)}") +check("a one-inverter Controller builds exactly ONE machine, on the FLAT keys, " + "with no mirror to make - which is what keeps the state file the one the " + "install already has", + len(_c1.maints) == 1 and _c1.maints[0].unit is None + and _c1.maints[0].store.mirror_flat is False + and MAINT_SCHEMA_KEY not in _c1store.data) +check("...and `self.maint` is that machine on every tick, so `publish()`, the " + "status banner and both operator buttons read exactly what they read " + "before this card", + _c1.maint is _c1.maints[0] + and _c1.maint_unit is _c1.inverters[0] + and _c1._maint_button(_now) is _c1.maints[0]) +one_cycle(_c1) +print(f" after a real one-inverter cycle: " + f"{sorted(k for k in _c1store.data if k not in DEFAULTS)}") +# ⚠️ THE NAMESPACED KEYS THAT *ARE* THERE ARE DEV-02's SIGN LATCH AND ITS +# MARKER, WRITTEN BY THE MIGRATION THAT SHIPPED A CARD AGO. This card must add +# none of its own, which is the assertion: no maintenance key is namespaced and +# there is no `maint_schema_version` at all. +check("...and a real cycle on it writes no MAINTENANCE key that did not exist " + "before this card: every one of the four is flat, none is namespaced, and " + "the per-pack marker is absent", + not any(k.endswith(tuple("/" + n for n in MAINT_UNIT_KEYS)) + for k in _c1store.data) + and all(k in _c1store.data for k in MAINT_UNIT_KEYS) + and MAINT_SCHEMA_KEY not in _c1store.data) + +# --- two inverters: A's history is namespaced AND mirrored flat ------------- +_c2store = MemStore({"schema_version": 1, + "last_completed": "2026-08-07T12:00:00+00:00"}) +_c2, _ = controller(TWO_OPTS, store=_c2store) +print(f" two inverters -> namespaces " + f"{[m.unit for m in _c2.maints]}, mirrors " + f"{[m.store.mirror_flat for m in _c2.maints]}") +check("a two-inverter Controller builds one machine per roster entry, each in " + "its own namespace, and migrates the flat history into the FIRST pack's", + [m.unit for m in _c2.maints] == ["inverter-a", "inverter-b"] + and _c2.maints[0].store.get_time("last_completed") + == datetime(2026, 8, 7, 12, 0, tzinfo=timezone.utc) + and _c2.maints[1].store.get_time("last_completed") is None) +check("...and ONLY the first pack mirrors back to the flat keys, because the " + "flat spelling means pack A on every install that has one - a mirror from " + "B would overwrite A's rollback view with B's phase", + [m.store.mirror_flat for m in _c2.maints] == [True, False]) +_c2.maints[0].force_start(_now) +_c2.maints[1].store.set("phase", HOLD) +check("...and the mirror is LIVE: pack A's new phase reaches the flat key, so a " + "rollback finds a current history rather than one dated migration day", + _c2store.data.get(unit_key("inverter-a", "phase")) == DRAIN + and _c2store.data["phase"] == DRAIN) +check("...while pack B's write reaches only pack B's namespace", + _c2store.data.get(unit_key("inverter-b", "phase")) == HOLD + and _c2store.data["phase"] == DRAIN) + + + +# --------------------------------------------------------------------------- # +print() +print("12. RECORDED, NOT FIXED: the site loop still contradicts a drain at N>=2") + +# ⚠️ THIS IS A DEFECT THIS CARD FOUND AND DELIBERATELY DID NOT FIX, AND IT IS +# WRITTEN DOWN AS A RUNNING CHECK RATHER THAN AS A COMMENT SO IT CANNOT BE +# FORGOTTEN OR QUIETLY "FIXED" BY A TUNING CHANGE. +# +# WHAT IT IS. The maintenance SET is a per-unit claim and PINS its pack. The +# loop's target is a SITE claim. When the site target is BELOW the pin - a +# drain of 2000 W while the house is importing less than that, which is most of +# a night - the only split that sums to the site target asks the sibling to +# CHARGE while the maintained pack DISCHARGES, and `allocator.plan()` refuses it +# by name as circulating power. The refusal is correct and fails safe: the whole +# site goes to 0 W with an ARBITER line in the action log. +# +# WHY IT IS NOT THIS CARD'S. It is PRE-EXISTING - measured on release/1.0 at +# 2d025ee, where the same drain filed on `inverters[0]` produces the identical +# refusal with the two unit names swapped - and fixing it means changing what +# the site loop's target MEANS while a unit is pinned (regulate the meter using +# only the unpinned packs), which is loop and allocator territory, not +# maintenance's. MAINT-05 moves which pack is pinned and nothing else about it. +# +# ⚠️ AND THE CONSEQUENCE AN OPERATOR WOULD SEE IS A MISDIAGNOSIS, WHICH IS WHY +# IT MATTERS ENOUGH TO PIN. The pack is held at 0 W while its FSM believes it is +# draining, so after 12 h the DRAIN timeout fires and reports "drain stuck" - +# §12's words for "the battery is not following commands, a fault". The battery +# is fine; the site refused to allocate. That is the same class of wrong story +# this card's own notes warn about for the deferral. +_cX, _ = controller(TWO_OPTS, grid=0.0) +_cX.maints[1].force_start(_now) +one_cycle(_cX) +print(f" house at 0 W, B draining 2000 W -> reason {_cX.reason!r}; " + f"{_cX.allocation.explain()}") +check("a maintenance drain BELOW the site target is refused by the allocator " + "and the site parks at 0 W - recorded here so the next card can find it, " + "not asserted as desirable", + _cX.target == 0.0 and _cX.allocation.refused + and "circulating power" in _cX.allocation.explain()) +check("...and the pack that is supposedly draining is commanded 0 W, so its " + "12 h DRAIN timeout would eventually report 'drain stuck' - a fault " + "message about a healthy battery", + _cX.maints[1].phase == DRAIN and _cX.inverters[1].target == 0.0) +check("...while the SAME site with the house importing more than the drain " + "allocates cleanly, which is what makes this a CONDITION rather than " + "'maintenance does not work at two inverters'", + _cB.allocation.refused is None and _cB.inverters[1].target == 2000.0) + print() print() if fails: diff --git a/goodwe_controller/test_validate.py b/goodwe_controller/test_validate.py index ef0d60f..d3d74e1 100644 --- a/goodwe_controller/test_validate.py +++ b/goodwe_controller/test_validate.py @@ -353,6 +353,64 @@ check("...and it is refused with maintenance_enabled off, as shipped", base(max_w=2000, maintenance_charge_w=2500, maintenance_enabled=False), -5000, 5000), "THE CEILING WINS")) +# ⚠️ MAINT-05: AND AT TWO PACKS IT IS THE WEAKEST CAP, NOT THE FIRST ENTRY'S. +# The comment this replaces in validate.py said, in as many words, that it took +# the FIRST unit's cap because there was exactly one `Maintenance` and it could +# only ever recalibrate `inverters[0]` - and that "when `maint_unit` stops being +# `inverters[0]`, this line moves with it". It has. Every pack is now +# recalibrated on its own schedule, so the shaping claim can be filed on any of +# them and the cap that contradicts it first is the smallest. +# +# ⚠️ THE FIXTURE'S TWO CAPS ARE DELIBERATELY DIFFERENT AND THE RATE SITS +# BETWEEN THEM. Two equal caps could not tell "grades the first" from "grades +# the smallest", and a rate above both would be refused by either rule - so +# 3000 W against caps of 5000 W and 2000 W is the one arrangement where the old +# answer and the new one disagree. ⚠️ 2000 W for the second pack is a +# HYPOTHESIS: inverter B does not physically exist and nothing here is measured +# from one. +def _roster(a_max, b_max, **over): + o = base(**over) + for k in ("setpoint_entity", "soc_entity", "batt_entity", "max_w"): + o.pop(k, None) + o["inverters"] = [ + dict(name="inverter-a", setpoint_entity="number.sp_a", + soc_entity="sensor.soc_a", batt_entity="sensor.batt_a", + max_w=a_max), + dict(name="inverter-b", setpoint_entity="number.sp_b", + soc_entity="sensor.soc_b", batt_entity="sensor.batt_b", + max_w=b_max)] + return o + + +_two = _roster(5000, 2000, maintenance_charge_w=3000, maintenance_discharge_w=1000, + sign_disagree_w=300, unit_sign_disagree_w=300) +_p_two = [p for p in validate_startup_config(_two, -5000, 5000) + if "maintenance_charge_w" in p] +print(f" 5000 W and 2000 W packs, charge 3000 W -> {_p_two}") +check("⚠️ A CHARGE RATE THE SMALLEST PACK CANNOT TAKE IS REFUSED, even though " + "the FIRST pack could. Before MAINT-05 the second pack was never " + "recalibrated, so its cap could not contradict anything; it is now, and " + "its cheap-window charge would ask for hi=-3000 against lo=-2000 - the " + "`lo > hi` contradiction, 0 W, on the pack nobody was watching", + len(_p_two) == 1 and why(_p_two, "max_w (inverter-b) is 2000 W")) +check("...and the message names the pack the number came off, not the first " + "entry - an operator told to lower a number on the wrong device is being " + "sent to the wrong device by the message that exists to stop that", + not why(_p_two, "max_w (inverter-a)")) +check("...while the same rate against two packs that can both take it starts, " + "so this is a comparison and not a blanket refusal of rosters", + [p for p in validate_startup_config( + _roster(5000, 4000, maintenance_charge_w=3000, + maintenance_discharge_w=1000, sign_disagree_w=300, + unit_sign_disagree_w=300), -5000, 5000) + if "maintenance_charge_w" in p] == []) +check("...and at a ROSTER OF ONE `min()` over one cap is that cap: the same " + "message, the same boundary, the same refusal the flat config always got", + why(validate_startup_config(base(max_w=2000, maintenance_charge_w=2500), + -5000, 5000), "THE CEILING WINS") + and validate_startup_config(base(max_w=2000, maintenance_charge_w=2000), + -5000, 5000) == []) + # ⚠️ THE SIBLING, CHECKED RATHER THAN ASSUMED. The shaping is NOT symmetric: # drain files Claim.set at P_MAINTENANCE and owns the actuator, so the ceiling # CLAMPS it (+2000 W) instead of contradicting it, and the arbiter's explain() -- 2.54.0 From 65cddf35f12b824e62fef2b68ef00e28f5fdf4c3 Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Fri, 4 Sep 2026 16:18:34 +0200 Subject: [PATCH 2/3] MAINT-05 fix round: grade the migration against the ROSTER, not against the name it was handed REJECTED because a survivor undoes AC 5, and it survived for the recurring reason: two fixture values that must be distinguishable were EQUAL. THE SURVIVOR. app/main.py:282 mutated from `self.inverters[0].name` to `UNIT_A` left the full 19-file suite green. `inverter.UNIT_A == "inverter-a"`, and every two-pack fixture in test_maintenance.py named its first roster entry "inverter-a" - so the two expressions were the same string in every check that existed. main.py's own comment made the distinction load-bearing and nothing in the repository could see whether that comment was true. THE FAILURE IS REAL. `build_roster()` names a declared roster's entries from the config and numbers a nameless one POSITIONALLY (`inverter-1`), so "inverter-a" is not even the fallback. On ["garage","shed"] with a flat last_completed of 2026-08-07 the mutant migrates into unit/inverter-a/, pack 0 reads back None, and both packs come up believing they have never been recalibrated. AND IT FAILED SILENTLY, which was the worse half: `_verify_moves()` read back the keys it had just WRITTEN, under the namespace it had just been HANDED, was satisfied, and MAINT_SCHEMA_KEY was stamped - so the next start skipped the migration too. A postcondition that reads back the namespace it was handed cannot detect being handed the wrong one. WHAT CHANGED 1. THE POSTCONDITION IS GRADED AGAINST THE ROSTER. `_verify_moves()` takes the names the MACHINES read and reports a namespace that is not among them as `misdirected` - a new, differently-named failure from `lost`: * `lost` is "the store took the write and dropped it" - a storage fault, with a harmless-default case (a dropped `last_completed: None` loses nothing, because absent and never-recalibrated are the same statement); * `misdirected` is "the store did exactly as it was told, by a caller that was wrong". There is no harmless-default version of it - a write nobody reads is not information whatever its value - so it fails `ok` alone. `ok` gates the marker, so a misdirected migration is NOT stamped and the next start re-runs it. Asymmetric on purpose: a re-run is recoverable (rule 1 makes it a no-op, rule 2 left the flat keys to recover from), a silent skip is not. Part 9b MEASURES the recovery rather than asserting it. 2. `migrate_maint_keys()` TAKES THE ROSTER IN PLACE OF AN `n_units` COUNT. The count was `len(self.inverters)` at the only call site, so the roster carries it, and a count beside the names is a second opinion about how many packs there are. 3. THE FIXTURES CAN NOW REACH THE FAILURE THEY NAME. Parts 9 and 11 run on ["garage","shed"], each behind a PRECONDITION check that the first name is not UNIT_A - so an edit that makes them equal again goes red instead of making the claims vacuous. Part 11's fixture date is relative to `now`, not a literal, so "history preserved" and "history lost" cannot converge as the file ages past `interval_days`. MUTATION LEDGER - all red, purged tree, none at +0 B, full detail on the PR: M1 main.py inverters[0].name -> UNIT_A -16 B 4 checks red M2 store.py drop `roster=names` from _verify_moves -14 B 5 checks red M3 store.py drop the misdirected.append -37 B 5 checks red M4 store.py ok: drop `and not self.misdirected` -25 B 3 checks red M5 fixture garage -> inverter-a (the trap itself) +4 B 5 checks red M6 store.py parts[1] -> parts[-1] +1 B 1 check red FOUND AND NOT FIXED - `migrate_unit_keys()` HAS THE SAME HOLE AND IT IS A LATCH. It runs BEFORE `build_roster()` (each Inverter restores its latch in its constructor), so it is handed the constant UNIT_A with no roster to grade against. On a declared roster a pre-DEV-02 `sign_tripped: true` lands under unit/inverter-a/sign_tripped, which no unit reads - and `_grade_store_migration()`, whose whole job is to re-arm exactly this, is DEFEATED BY THE KEY THE DEFECT WROTE: its "is anything namespaced latched" test is satisfied by the misdirected value itself. Measured, with the flat-config counterfactual, and pinned as a running check (test_maintenance.py part 13) whose assertions state the WRONG behaviour on purpose. It wants its own card: the remedy is in inverter.py and SAFETY-02's semantics, and a silent expansion into a safety latch is how a rejected PR becomes a broken one. Suite: 19/19 PASS, purged tree, bind-mounted :ro in goodwe-addon-ci. No hardware was touched; every fixture here is SIMULATED and pack B's 3000 W remains a marked hypothesis. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 29 ++ .../MAINT-05-per-pack-maintenance.md | 117 +++++++ goodwe_controller/MUTATION.md | 33 +- goodwe_controller/app/main.py | 21 +- goodwe_controller/app/store.py | 101 +++++- goodwe_controller/test_maintenance.py | 328 ++++++++++++++++-- 6 files changed, 580 insertions(+), 49 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index d122b6e..1dde8f0 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -46,6 +46,35 @@ two power-moving phases on one grid connection pulling against each other. assumed: the state file keeps the flat keys and gains none, the migration does not run, the four cycle events are the shipped strings character for character, and the status banner is the bare phase with no pack name. +* **THE MIGRATION IS GRADED AGAINST THE ROSTER, NOT AGAINST THE NAME IT WAS + HANDED** (fix round). `migrate_maint_keys()` takes the roster rather than a + pack count and `_verify_moves()` checks the namespace it wrote against the + names the machines actually read. A postcondition that reads back the + namespace it was given cannot detect being given the wrong one: handed + `inverter.UNIT_A` on a roster of `["garage", "shed"]`, the first version of + this card wrote all four keys under `unit/inverter-a/...`, read all four back + equal, reported **verified**, and stamped `maint_schema_version` — so both + packs came up believing they had never been recalibrated and the next start + skipped the migration too. That is now `misdirected`: not `ok`, the marker is + **not** written, and the next start re-runs and recovers (rule 1 makes the + re-run a no-op). `build_roster()` numbers a nameless roster entry + `inverter-1`, so `inverter-a` is not even the fallback for a declared roster. + Every two-pack fixture that could see this now names its first pack + `garage`; with them all called `inverter-a`, `self.inverters[0].name` and + `UNIT_A` were the same string and the swap left all 19 suites green. +* ⚠️ **FOUND, RECORDED, NOT FIXED (SECOND ONE).** `migrate_unit_keys()` — + SAFETY-02's latch migration — has the identical hole and it costs a **guard** + rather than a history. It runs before `build_roster()` (each `Inverter` + restores its latch in its constructor), so it is handed the constant `UNIT_A` + and there is no roster to grade against. On a declared roster a pre-DEV-02 + `sign_tripped: true` lands under `unit/inverter-a/sign_tripped`, which no unit + reads — and `_grade_store_migration()`, whose job is to re-arm exactly this, + is **defeated by the key the defect wrote**: its "is anything namespaced + latched" test is satisfied by the misdirected value itself. Fixing it means + naming the migration's target from the config before any `Inverter` exists, + which is `inverter.py` and SAFETY-02's semantics, with its own battery. + Measured and pinned as a running check (`test_maintenance.py` part 13), whose + assertions state the WRONG behaviour on purpose. * ⚠️ **FOUND, RECORDED, NOT FIXED.** A maintenance drain **pins** its pack while the loop's target is a **site** claim, so when the site target is below the pin the only split asks the sibling to charge against the drain and diff --git a/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md b/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md index a2cae17..ab18205 100644 --- a/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md +++ b/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md @@ -8,6 +8,11 @@ when several packs are due in the same hour it goes to the one that has gone **longest without a completed cycle**. The schedule is **not** staggered by construction — the interlock defers, and the stagger emerges from it. +**Fix round 2026-09-04.** The store migration is now graded against the +**roster** rather than against the unit name it was handed — see §2's +*"The read-back postcondition is graded against the ROSTER"* — and the same +hole in SAFETY-02's latch migration is recorded in §7. + Filed with MAINT-05. Card `6a9a9e8503872905f298d80a`. --- @@ -72,6 +77,69 @@ stay in the file), `_verify_moves()` is the read-back postcondition, and the marker is written last. A second transcription of a migration is a second opinion about what "it landed" means. +### The read-back postcondition is graded against the ROSTER (fix round) + +**A postcondition that reads back the namespace it was handed cannot detect +being handed the wrong one.** That sentence is the whole finding, and this card +shipped it broken. + +`_verify_moves()` iterated `rep.moved` — the keys it had just written — and +asked whether each read back equal. On a store where the write lands, it always +does. So a migration handed a `unit` that is on no roster wrote four keys, read +four keys back, set `verified = True`, and the caller stamped +`maint_schema_version`. Every symptom of a clean migration, and an install's +whole maintenance history parked under a name nothing will ever look up. And +because the marker was stamped, **the next start skipped the migration too** — +which is precisely the state `store.py`'s header says must never happen: a +failed migration a later version treats as done. + +Reproduced on the reviewer's roster, a house with a garage pack and a shed pack, +carrying a flat `last_completed` of 2026-08-07: + +| | `unit` the migration was handed | key written | pack 0 reads back | due now? | +|---|---|---|---|---| +| correct | `self.inverters[0].name` = `garage` | `unit/garage/last_completed` | `2026-08-07` | **False** | +| wrong | `inverter.UNIT_A` = `inverter-a` | `unit/inverter-a/last_completed` | `None` | **True** | + +`build_roster()` names a declared roster's entries **from the config**, and its +fallback for a nameless entry is a **positional** `inverter-1` — so `inverter-a` +is not the name of anything on a declared roster, not even by default. + +**The remedy, and why this one.** `migrate_maint_keys()` now takes the **roster** +in place of an `n_units` count (the count was `len(self.inverters)` at the only +call site, so the roster carries it, and a count passed beside the names is a +second opinion about how many packs there are). `_verify_moves()` grades the +namespace it wrote against those names, and a unit that is not among them is +reported as `misdirected` — a **different** failure from `lost`: + +* `lost` is *the store took the write and dropped it* — a storage fault, and it + has a harmless-default case (a write of `last_completed: None` loses nothing, + because "absent" and "never recalibrated" are the same statement). +* `misdirected` is *the store did exactly as it was told, by a caller that was + wrong*. There is no harmless-default version of it: a write nobody reads is + not information, whatever its value. So anything in it fails `ok` on its own. + +And **`ok` gates the marker**, so a misdirected migration is not stamped and the +next start runs it again. That is the decision the fix round was asked to +record, and the argument for it is asymmetric: a re-run is recoverable — rule 1 +makes it a no-op where keys did land, and rule 2 left the flat keys in the file +to recover *from* — while a silent skip is not recoverable at all, because the +store now claims the migration happened. `test_maintenance.py` part 9b measures +the recovery rather than asserting it: the same store, re-run with the roster's +own first name, comes back with its month of history. + +**Why the fixtures had to change, and this is the part that got the card +rejected.** Nothing in the repository could see whether any of the above was +true, because `inverter.UNIT_A` **is** the string `"inverter-a"` and every +two-pack fixture in `test_maintenance.py` named its first roster entry +`inverter-a`. `self.inverters[0].name` and `UNIT_A` were therefore the same +value in every check that existed, and swapping one for the other at the call +site left **all 19 suites green**. MUTATION.md's third defect exactly — a check +whose *data* cannot reach the failure it names — and the fifth consecutive card +to carry it. Parts 9 and 11 now run on `["garage", "shed"]`, each behind a +**precondition check** asserting that the first name is not `UNIT_A`, so an edit +that makes them equal again goes red instead of making the claims vacuous. + ### And the flat keys are kept LIVE, which is the half DEV-02 had to add later Rule 2 originally said the flat keys are left in place and go stale. DEV-02 @@ -180,3 +248,52 @@ subject. MAINT-05 moves which pack is pinned and nothing else about it. It is pinned as a running check (`test_maintenance.py` part 12) rather than left as a comment, so it cannot be forgotten or quietly tuned away, and it wants a card of its own. + +## 7. FOUND AND NOT FIXED: `migrate_unit_keys()` has the same hole, and it is a LATCH + +Found while fixing §2's instance. It is the same defect one module over and it +costs a **disarmed safety guard** rather than a lost history. + +`Controller.__init__` calls `migrate_unit_keys(store, UNIT_A)` — a hardcoded +constant where the maintenance migration now passes the roster's own first name. +It **has** to run before `build_roster()`, because each `Inverter` restores its +sign latch in its constructor, so there is no roster to name yet and +`_verify_moves()` is passed `roster=None`. On a **declared** roster a pre-DEV-02 +`sign_tripped: true` therefore lands under `unit/inverter-a/sign_tripped`, which +no unit on that site reads. + +**And the guard that exists for exactly this is defeated by the key the defect +wrote.** `_grade_store_migration()` re-arms the latch when the flat key says +tripped and *nothing namespaced is latched*. The misdirected write **is** +namespaced and **is** latched, so `namespaced_latched` is `True`, the guard +returns, and no inverter on the roster comes up tripped — on an install whose +`/data/state.json` still reads `sign_tripped: true`. That is DEV-02's own +sentence for the worst case: the state file says protected, the banner is green, +and the controller resumes commanding into the fault that tripped it. + +Measured, on the same `["garage", "shed"]` roster: + +``` +pre-DEV-02 flat latch + declared roster ['garage', 'shed'] -> + migrated 2 flat key(s): unit/inverter-a/sign_reason, unit/inverter-a/sign_tripped +latch keys in the store: ['sign_tripped', 'site_sign_tripped', 'unit/inverter-a/sign_tripped'] +roster came up tripped: [('garage', False), ('shed', False)] +the same store on the FLAT config -> [('inverter-a', True)] +``` + +The last line is the counterfactual: on the flat shape, where `UNIT_A` really is +the roster's only name, the same store migrates and comes up latched. So this is +a defect of the **declared roster**, not of the migration as such. + +**Why it is not fixed here.** The remedy is to name the migration's target from +the config before any `Inverter` exists, which means factoring `build_roster()`'s +naming rule out of `build_roster()` so there is still only **one** spelling of +it — `inverter.py` and SAFETY-02's latch semantics, with its own mutation battery +over `test_inverter.py` and `test_watchdog.py`. MAINT-05 moves the maintenance +history and nothing about the sign latch, and a silent expansion into a safety +latch is how a rejected PR becomes a broken one. + +It is pinned as a running check (`test_maintenance.py` part 13) rather than left +as a comment. ⚠️ **Its assertions state the WRONG behaviour on purpose** — read +them as the specification of a defect; the card that fixes it should expect them +to go red and should replace them. diff --git a/goodwe_controller/MUTATION.md b/goodwe_controller/MUTATION.md index 13dad66..843b8f8 100644 --- a/goodwe_controller/MUTATION.md +++ b/goodwe_controller/MUTATION.md @@ -135,12 +135,19 @@ 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. Six instances in two weeks; three +It is not hypothetical and it is not rare. Seven instances in two weeks; four were found by review, two - CFG-04's fixture pair - by an author's own mutation -battery, which is the cheaper place, and the sixth by review again, in the one +battery, which is the cheaper place, and one by review again, in the one place nobody had thought to point the rule at: not a check in the suite, but a COMMITTED EVIDENCE ARTIFACT four documents cite. +⚠️ **AND IT HAS NOW APPEARED IN FIVE CONSECUTIVE CARDS.** The seventh instance, +MAINT-05's, is the one that says why: the defect is not carelessness about +fixtures, it is that **a check and the thing it checks were built from the same +constant**. Ask of every difference-claim not "is this assertion true" but +"where did each side of this comparison come from" - and if the answer is the +same place twice, the check is decoration. + * **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 @@ -167,6 +174,28 @@ COMMITTED EVIDENCE ARTIFACT four documents cite. than against its behaviour. It now drives the shipped `run_control()` on a two-inverter config, **under a deadline**, because the mutation's real symptom is that the controller enters its loop and never returns. +* **MAINT-05** shipped `Controller.__init__` calling + `migrate_maint_keys(store, self.inverters[0].name, ...)` under a comment + saying *"it targets `self.inverters[0].name` and NOT `UNIT_A` ... writing + `UNIT_A` here would deposit an install's whole maintenance history under a + name that is not on the roster at all"*. `inverter.UNIT_A` **is** the string + `"inverter-a"`, and every two-pack fixture in `test_maintenance.py` named its + first roster entry `inverter-a` — so the two expressions were the same value + in every check that existed, and swapping them left **all 19 files green** + while losing a real install's whole maintenance history. Two-pack fixtures are + now `["garage", "shed"]`, each behind a precondition check (habit 1) that the + first name is not `UNIT_A`. + + ⚠️ **AND THE POSTCONDITION HAD THE SAME SHAPE AS THE FIXTURE.** + `store._verify_moves()` read back the keys it had just written, under the + namespace it had just been handed — so it verified clean, and the caller + stamped the schema marker, on a migration nothing would ever read. *A check + that reads back the namespace it was handed cannot detect being handed the + wrong one*: it compared a thing against itself, exactly as DEV-02's fixture + compared `(-5000, 5000)` against `(-5000, 5000)`. The habit-2 fix is the same + one — grade against something the thing under test did not supply, here the + ROSTER the machines read. + * **CFG-04's `tools/supervisor_shape_probe.py` §B** — and this one is the rule reaching outside the suite. The probe is the committed evidence that `config.yaml`'s `inverters: []` default is load-bearing, and four documents diff --git a/goodwe_controller/app/main.py b/goodwe_controller/app/main.py index 0b3942d..f102b5d 100644 --- a/goodwe_controller/app/main.py +++ b/goodwe_controller/app/main.py @@ -276,11 +276,24 @@ class Controller: # It targets `self.inverters[0].name` and NOT `UNIT_A`: the flat keys # belonged to whichever pack the one FSM was driving, which is the first # roster entry, and a declared `inverters:` roster names its own entries - # (`inverter.build_roster()` says so). Writing `UNIT_A` here would - # deposit an install's whole maintenance history under a name that is - # not on the roster at all. + # (`inverter.build_roster()` says so, and its fallback for a nameless + # entry is a POSITIONAL `inverter-1`, so `inverter-a` is not even the + # default there). Writing `UNIT_A` here would deposit an install's whole + # maintenance history under a name that is not on the roster at all. + # + # ⚠️ AND THAT SENTENCE USED TO BE A COMMENT NOTHING COULD GRADE, WHICH + # IS WHY THE ROSTER IS PASSED RATHER THAN A COUNT. Every two-pack + # fixture in the suite named its first entry `inverter-a`, so + # `self.inverters[0].name` and `UNIT_A` were the same string in every + # check that existed and swapping them left all 19 files green - while + # on a roster of `["garage", "shed"]` it silently loses the history and + # stamps the marker, so the next start skips the migration too. + # `migrate_maint_keys()` now grades the namespace it wrote against the + # names the machines read; test_maintenance.py sections 9 and 11 drive + # a `["garage", "shed"]` roster, which is what makes the mutation red + # instead of invisible. self.maint_migration = migrate_maint_keys( - store, self.inverters[0].name, len(self.inverters)) + store, self.inverters[0].name, [inv.name for inv in self.inverters]) _LOG.info("maintenance history: %s", self.maint_migration.explain()) if not self.maint_migration.ok: # Not a disarmed guard - history, not a latch - so it does not diff --git a/goodwe_controller/app/store.py b/goodwe_controller/app/store.py index 273f546..d1a4d24 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -211,13 +211,23 @@ class MigrationReport: # to shout: an alarm that fires on the harmless case is an alarm an # operator learns to dismiss before the real one arrives. self.lost = [] + # ⚠️ A DIFFERENT FAILURE FROM `lost`, AND IT HAS TO BE, BECAUSE THE + # READ-BACK CANNOT SEE IT. `lost` is "the store took the write and + # dropped it". This is "the store took the write, kept it perfectly, and + # NOTHING WILL EVER READ IT" - the migration was handed a unit name that + # is not on the roster, so every machine looks under a different key and + # finds nothing. There is no harmless-default version of this the way + # there is for `lost`: a write nobody reads is not information, whatever + # its value, so anything in here fails `ok` on its own. + self.misdirected = [] # Why nothing ran, when nothing ran. Empty means "already at schema". self.skipped = "" @property def ok(self) -> bool: - """Nothing that CARRIED STATE failed to land.""" - return not self.lost + """Nothing that CARRIED STATE failed to land, and it landed WHERE THE + MACHINES LOOK.""" + return not self.lost and not self.misdirected def explain(self) -> str: if not self.ran: @@ -238,6 +248,8 @@ class MigrationReport: out += " - PROBLEMS: " + "; ".join(self.problems) if self.lost: out += " - STATE LOST: " + "; ".join(self.lost) + if self.misdirected: + out += " - WRONG NAMESPACE: " + "; ".join(self.misdirected) return out @@ -329,7 +341,7 @@ def _move_flat_keys(store, unit: str, keys, rep: MigrationReport) -> None: rep.moved[target] = value -def _verify_moves(store, rep: MigrationReport) -> None: +def _verify_moves(store, rep: MigrationReport, roster=None) -> None: """THE POSTCONDITION, AND IT IS A READ-BACK RATHER THAN AN ASSUMPTION. This is the difference between these functions and a two-line copy: the @@ -339,6 +351,34 @@ def _verify_moves(store, rep: MigrationReport) -> None: same `unit_key()` those readers read through. A `False` here is a guard that is off, or a month of coulomb-count history gone, on an install whose state file says otherwise. + + ⚠️ AND `roster` IS THE OTHER HALF, WHICH THE READ-BACK ON ITS OWN CANNOT BE. + A POSTCONDITION THAT READS BACK THE NAMESPACE IT WAS HANDED CANNOT DETECT + BEING HANDED THE WRONG ONE. That is not a hypothetical: MAINT-05 shipped + with `migrate_maint_keys()` taking a bare `unit` string, and a call site that + passed `inverter.UNIT_A` instead of the first roster entry's name migrated an + install's whole maintenance history into `unit/inverter-a/...` on a roster of + `["garage", "shed"]`, read all four keys back under that same name, was + SATISFIED, and stamped the schema marker - so the next start skipped the + migration too and both packs came up believing they had never been + recalibrated. Every symptom of a clean migration, and the history gone. + `build_roster()` names a declared roster's entries from the config and falls + back to a POSITIONAL `inverter-N`, so `inverter-a` is not even the default + there; there is no roster on which that write could have been read. + + So the namespace is graded against the one the MACHINES read - the roster - + rather than against the argument this function was handed, which is the + thing under test. A unit that is not on the roster is `misdirected`, which + fails `ok`, which means the caller does NOT stamp the version marker and the + next start runs the migration again. A re-run is recoverable, by rule 1; a + silent skip is the state store.py's header says must never happen. + + `roster=None` means "not graded", and the one caller that passes it is + `migrate_unit_keys()`, which runs BEFORE `build_roster()` for the reason its + own docstring gives - the `Inverter` objects restore their latch in their + constructors, so the keys have to be in place before there is a roster to + check against. That gap is real and is recorded as a running check rather + than left to a comment: see test_maintenance.py section 13. """ for target, value in rep.moved.items(): got = getattr(store, "data", {}).get(target, "<>") @@ -351,8 +391,24 @@ def _verify_moves(store, rep: MigrationReport) -> None: if value != DEFAULTS.get(legacy): rep.lost.append(note) + if roster is None: + return + names = [str(n) for n in roster] + # `unit_key()` builds `unit//`, so the unit is field 1. Split + # through the same spelling the writer used rather than re-deriving it. + for target in sorted(rep.moved): + parts = target.split("/") + unit = parts[1] if len(parts) > 2 else "" + if unit in names: + continue + rep.verified = False + note = (f"{target} was migrated into namespace {unit!r}, which is NOT " + f"on the roster {names!r} - nothing reads that key") + rep.problems.append(note) + rep.misdirected.append(note) -def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport: + +def migrate_maint_keys(store, unit: str, roster) -> MigrationReport: """Move the flat maintenance history into the FIRST pack's namespace. MAINT-05. Until this card there was one `Maintenance` and one history, and @@ -363,14 +419,32 @@ def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport: what `Controller.maint_unit` named and what `validate.py` rule 6 graded against. - ⚠️ `n_units <= 1` IS A RETURN, NOT A SPECIAL CASE. At one inverter the flat - keys ARE that pack's namespace (§6.2's identity, the same one + ⚠️ `len(roster) <= 1` IS A RETURN, NOT A SPECIAL CASE. At one inverter the + flat keys ARE that pack's namespace (§6.2's identity, the same one `arbiter.actuator_for()` leans on), `Maintenance` is constructed to read them directly, and a migration would add four keys plus a marker to a store that must come out byte-identical. The migration happens on the day the roster grows, which is the day the flat spelling stops being able to answer "which pack". + ⚠️ `roster` IS REQUIRED, AND IT REPLACED AN `n_units: int`. Two reasons, and + the second is the one MAINT-05 was rejected over. + + The small one: `n_units` was `len(self.inverters)` at the only call site, so + the roster carries it - `len(roster)` - and a count passed beside the names + is a second opinion about how many packs there are. + + The load-bearing one: THE NAMES ARE THE POSTCONDITION. `unit` says which + pack the flat keys belonged to and that is genuinely a decision - the first + roster entry, because that is the pack the one FSM was driving - but a + decision the caller can get WRONG, and until this signature existed nothing + could tell. `_verify_moves()` read back the namespace it was handed, so a + migration into a name no machine reads verified clean and stamped the + marker. Handing the roster in means the read-back is graded against the + keys the MACHINES read, and a `unit` that is not among these names comes + back `misdirected`: not `ok`, marker NOT written, migration runs again next + start. See `_verify_moves()` for the install this was measured on. + THE THREE RULES ARE `migrate_unit_keys()`'s, SHARED IN CODE rather than restated: `_move_flat_keys()` is rule 1 (never overwrite a namespaced value) and rule 2 (the flat keys are left in the file), `_verify_moves()` is the @@ -390,8 +464,9 @@ def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport: """ rep = MigrationReport() data = getattr(store, "data", {}) + names = [str(n) for n in roster] rep.from_version = int(data.get(MAINT_SCHEMA_KEY, 0) or 0) - if n_units <= 1: + if len(names) <= 1: rep.skipped = ("one inverter: the flat maintenance keys ARE that pack's " "history and are read in place, so there is nothing to " "move and the store layout does not change") @@ -404,13 +479,19 @@ def migrate_maint_keys(store, unit: str, n_units: int) -> MigrationReport: rep.ran = True _move_flat_keys(store, unit, MAINT_UNIT_KEYS, rep) - _verify_moves(store, rep) + _verify_moves(store, rep, roster=names) if rep.ok: store.set(MAINT_SCHEMA_KEY, MAINT_SCHEMA_VERSION) else: - _LOG.error("per-pack maintenance-history migration LOST STATE and has " - "not been marked done: %s", rep.explain()) + # ⚠️ NOT MARKED DONE, FOR EITHER FAILURE. The same sentence + # `migrate_unit_keys()` carries: the one thing worse than a failed + # migration is a failed migration a later version skips. Rule 1 makes + # the re-run a no-op where the keys did land, so refusing the marker + # costs one repeated pass and buys the difference between a migration + # that can still be fixed and one the store says already happened. + _LOG.error("per-pack maintenance-history migration did NOT land and " + "has not been marked done: %s", rep.explain()) return rep diff --git a/goodwe_controller/test_maintenance.py b/goodwe_controller/test_maintenance.py index a5fa4fc..9ed9b47 100644 --- a/goodwe_controller/test_maintenance.py +++ b/goodwe_controller/test_maintenance.py @@ -30,6 +30,7 @@ from app.maintenance import ( CHARGE, DRAIN, HOLD, IDLE, MAINT_OPTION_DEFAULTS, POWER_MOVING, MaintConfig, Maintenance, UnitHistory, arbitrate, ) +from app.inverter import UNIT_A from app.main import Controller from app.store import ( DEFAULTS, MAINT_SCHEMA_KEY, MAINT_SCHEMA_VERSION, MAINT_UNIT_KEYS, @@ -586,6 +587,30 @@ check("...and B's deferral produced no claim on the tick it was deferred: a " print() print("9. the store migration: no lost history, and the three rules hold (AC5)") +# ⚠️ THE ROSTER IS ["garage", "shed"], AND THE FIRST NAME IS THE WHOLE POINT OF +# THIS SECTION'S FIX ROUND. Every check here used to name its packs +# "inverter-a" / "inverter-b" - and "inverter-a" is the string +# `inverter.UNIT_A` holds, so `self.inverters[0].name` and `UNIT_A` were the +# SAME VALUE in every fixture in this file. main.py's migration call site could +# be changed from the first to the second with all 19 suites still green: +# MUTATION.md's third defect exactly, a check whose DATA cannot reach the +# failure it names. +# +# A declared `inverters:` roster names its own entries (`build_roster()`), and +# the fallback for a NAMELESS entry is a POSITIONAL `inverter-1` - so +# `inverter-a` is not the default for a declared roster either. It is a name +# nothing on this site reads, which is what makes migrating into it silent. +ROSTER = ["garage", "shed"] + +check("PRECONDITION, WRITTEN AS ITS OWN CHECK. The first pack on this " + f"section's roster is NOT `UNIT_A`: {ROSTER[0]!r} != {UNIT_A!r}, and " + f"{UNIT_A!r} is not on it at all. 'the first roster entry' and " + "'inverter-a' are therefore DIFFERENT strings in every check below - and " + "if a later edit makes them equal again this goes red rather than the " + "claims going quietly vacuous", + ROSTER[0] != UNIT_A and UNIT_A not in ROSTER) + + # A real pre-MAINT-05 single-pack store: a month of history under the flat keys, # with SAFETY-02's own migration already done (schema_version 1). def flat_store(**over): @@ -599,20 +624,28 @@ def flat_store(**over): _m1 = flat_store() -_rep1 = migrate_maint_keys(_m1, "inverter-a", 2) +_rep1 = migrate_maint_keys(_m1, ROSTER[0], ROSTER) print(f" a single-pack store growing to two: {_rep1.explain()}") +print(f" keys after migration: " + f"{sorted(k for k in _m1.data if k.startswith('unit/'))}") check("every flat maintenance key lands in the FIRST pack's namespace, because " "the flat keys can only ever have belonged to the pack the one FSM was " "driving", - _rep1.ran and _rep1.ok and _rep1.verified - and all(_m1.data.get(unit_key("inverter-a", k)) == _m1.data[k] + _rep1.ran and _rep1.ok and _rep1.verified and not _rep1.misdirected + and all(_m1.data.get(unit_key(ROSTER[0], k)) == _m1.data[k] for k in MAINT_UNIT_KEYS)) +check("...and the namespace is THE ROSTER'S FIRST NAME, not `UNIT_A` - which is " + "a claim this file could not make until its packs stopped being called " + "`inverter-a`. Not one key was written under `unit/inverter-a/`, on a " + "store where that is the name a wrong call site would have used", + not any(k.startswith(unit_key(UNIT_A, "")) for k in _m1.data) + and unit_key(ROSTER[0], "last_completed") in _m1.data) check("...and the VALUES are the history, not defaults wearing its name - a " "migration that lands `last_completed: None` has lost the month it was " "moving and would recalibrate a pack that does not need it", - _m1.data.get(unit_key("inverter-a", "last_completed")) + _m1.data.get(unit_key(ROSTER[0], "last_completed")) == "2026-08-07T12:00:00+00:00" - and _m1.data.get(unit_key("inverter-a", "phase")) == CHARGE) + and _m1.data.get(unit_key(ROSTER[0], "phase")) == CHARGE) check("RULE 2: the flat keys are LEFT IN THE FILE, so an operator who rolls the " "add-on back to a single-FSM version finds the history it had", all(k in _m1.data for k in MAINT_UNIT_KEYS) @@ -623,31 +656,31 @@ check("RULE 3: the marker is written and the store is now at the per-pack " check("...and the pack that has just arrived gets NOTHING, which is the right " "answer rather than an omission: absent means never recalibrated, means " "due, and a brand-new pack should be", - not any(k.startswith(unit_key("inverter-b", "")) for k in _m1.data) - and Maintenance(cfg, _m1, unit="inverter-b").due(_t)) + not any(k.startswith(unit_key(ROSTER[1], "")) for k in _m1.data) + and Maintenance(cfg, _m1, unit=ROSTER[1]).due(_t)) # Rule 1, and it is the rule that makes re-running safe. -_m2 = flat_store(**{unit_key("inverter-a", "last_completed"): +_m2 = flat_store(**{unit_key(ROSTER[0], "last_completed"): "2026-09-01T09:00:00+00:00"}) -_rep2 = migrate_maint_keys(_m2, "inverter-a", 2) +_rep2 = migrate_maint_keys(_m2, ROSTER[0], ROSTER) check("RULE 1: an EXISTING namespaced value is never overwritten by the flat " "one. A store with both spellings has been written by a version that knew " "about the namespace, and preferring the flat one would roll a pack's " "history back to whatever it was on migration day", - _m2.data.get(unit_key("inverter-a", "last_completed")) + _m2.data.get(unit_key(ROSTER[0], "last_completed")) == "2026-09-01T09:00:00+00:00" - and unit_key("inverter-a", "last_completed") in _rep2.kept) + and unit_key(ROSTER[0], "last_completed") in _rep2.kept) check("...so running the migration TWICE is a no-op, which is what makes a " "crash between the copies and the marker safe", - migrate_maint_keys(_m1, "inverter-a", 2).ran is False) + migrate_maint_keys(_m1, ROSTER[0], ROSTER).ran is False) # The store a multi-inverter version wrote. AC: not touched. _m3 = MemStore({MAINT_SCHEMA_KEY: MAINT_SCHEMA_VERSION, - unit_key("inverter-a", "last_completed"): "2026-09-01T09:00:00+00:00", - unit_key("inverter-b", "last_completed"): "2026-08-20T09:00:00+00:00", + unit_key(ROSTER[0], "last_completed"): "2026-09-01T09:00:00+00:00", + unit_key(ROSTER[1], "last_completed"): "2026-08-20T09:00:00+00:00", "last_completed": "2026-09-01T09:00:00+00:00"}) _before3 = dict(_m3.data) -_rep3 = migrate_maint_keys(_m3, "inverter-a", 2) +_rep3 = migrate_maint_keys(_m3, ROSTER[0], ROSTER) print(f" a store already written by a multi-inverter version: {_rep3.explain()}") check("a store already at the per-pack schema is NOT TOUCHED - not one key " "written - and says so", @@ -671,7 +704,7 @@ class LosesTheHistory(MemStore): _m4 = LosesTheHistory(dict(flat_store().data)) -_rep4 = migrate_maint_keys(_m4, "inverter-a", 2) +_rep4 = migrate_maint_keys(_m4, ROSTER[0], ROSTER) print(f" a store that silently drops the migrated keys: {_rep4.explain()}") check("⚠️ THE MIGRATION REPORTS THAT IT FAILED, because it READS BACK what it " "wrote. A month of coulomb-count history under a key nothing reads is a " @@ -686,7 +719,92 @@ check("...while a store whose only failed write was a DEFAULT is not reported " "'absent' and 'never recalibrated' are the same statement - an alarm that " "fires on the harmless case is one an operator learns to dismiss", migrate_maint_keys(LosesTheHistory({"schema_version": 1}), - "inverter-a", 2).ok is True) + ROSTER[0], ROSTER).ok is True) + + +# --------------------------------------------------------------------------- # +print() +print("9b. THE OTHER HALF OF THE POSTCONDITION: a migration into a namespace " + "the machines do not read (AC5)") + +# ⚠️ WHY THIS EXISTS, IN ONE SENTENCE: A POSTCONDITION THAT READS BACK THE +# NAMESPACE IT WAS HANDED CANNOT DETECT BEING HANDED THE WRONG ONE. +# +# `_verify_moves()` shipped reading back `rep.moved` - the keys it had just +# written - so a migration handed a `unit` that is on no roster wrote four keys, +# read all four back equal, set `verified = True`, and the caller stamped +# `maint_schema_version`. Every symptom of a clean migration, and an install's +# whole maintenance history parked under a name nothing will ever look up. The +# next start then sees the marker and SKIPS the migration, which is the one +# state store.py's header says must never happen: a failed migration a later +# version treats as done. +# +# It is not a hypothetical failure and it is not a hypothetical call site. +# main.py passes `self.inverters[0].name`; `inverter.UNIT_A` is one token away, +# reads correctly, and was indistinguishable from it in every fixture in this +# file until the roster above stopped being called `inverter-a`. +_m5 = flat_store() +_rep5 = migrate_maint_keys(_m5, UNIT_A, ROSTER) +print(f" handed {UNIT_A!r} on a roster of {ROSTER}: {_rep5.explain()}") +print(f" keys written: {sorted(k for k in _m5.data if k.startswith('unit/'))}" + f"; marker {_m5.data.get(MAINT_SCHEMA_KEY, '<>')!r}") +check("⚠️ THE MIGRATION REFUSES: a namespace that is not on the roster is not a " + "namespace anything reads, so the four keys landing perfectly is not the " + "question. `ok` is False and `verified` is False even though every single " + "read-back matched, which is the difference this postcondition adds", + _rep5.ran and not _rep5.ok and not _rep5.verified and _rep5.misdirected + and all(_m5.data.get(unit_key(UNIT_A, k)) is not None + for k in MAINT_UNIT_KEYS)) +check("...and it is reported as a DIFFERENT failure from a dropped write, " + "because the remedy is different: `lost` is empty and `misdirected` is " + "not. A store that dropped the write has a storage fault; this store did " + "exactly as it was told, by a caller that was wrong", + not _rep5.lost and len(_rep5.misdirected) == len(MAINT_UNIT_KEYS)) +check("...and the refusal NAMES the namespace it used AND the roster it was " + "graded against, rather than merely saying something went wrong - the " + "operator's next question is 'which name, and which one should it have " + "been', and a bare `not ok` answers neither", + "WRONG NAMESPACE" in _rep5.explain() + and unit_key(UNIT_A, "last_completed") in _rep5.explain() + and repr(ROSTER) in _rep5.explain()) +check("⚠️ AND THE MARKER IS NOT STAMPED, WHICH IS THE WHOLE REMEDY. A stamped " + "marker makes the next start skip a migration that never reached a " + "readable key; an unstamped one costs a repeated pass that rule 1 makes a " + "no-op. Recoverable beats silent", + MAINT_SCHEMA_KEY not in _m5.data) +_rep5b = migrate_maint_keys(_m5, ROSTER[0], ROSTER) +print(f" the same store, re-run with the roster's own first name: " + f"{_rep5b.explain()}") +check("...and 'recoverable' is measured, not asserted: the SAME store re-run " + "with the roster's own first name migrates cleanly, and the pack reads " + "back the month of history it started with. Rule 2 left the flat keys in " + "the file, which is what there was to recover FROM", + _rep5b.ran and _rep5b.ok + and _m5.data.get(unit_key(ROSTER[0], "last_completed")) + == "2026-08-07T12:00:00+00:00" + and _m5.data[MAINT_SCHEMA_KEY] == MAINT_SCHEMA_VERSION) +check("...and the misdirected keys are LEFT WHERE THEY LANDED rather than " + "deleted: this function moves history, and a migration that starts " + "removing keys it did not put there is a second way to lose a month", + _m5.data.get(unit_key(UNIT_A, "last_completed")) + == "2026-08-07T12:00:00+00:00") + +# The counterfactual, so this is not a check that passes on a validator that +# refuses everything: the SAME store, the SAME call, one argument different. +_m6 = flat_store() +_rep6 = migrate_maint_keys(_m6, ROSTER[0], ROSTER) +check("...and the counterfactual: one argument different - the roster's own " + "first name instead of `UNIT_A` - and the identical call is accepted and " + "stamped. So the refusal above is about the NAME, not about this fixture " + "being refused whatever it is handed", + _rep6.ran and _rep6.ok and not _rep6.misdirected + and _m6.data[MAINT_SCHEMA_KEY] == MAINT_SCHEMA_VERSION) +check("...and a roster of ONE still returns before any of this, so the " + "namespace postcondition cannot reach a single-pack install: nothing is " + "moved, nothing is graded, nothing is stamped", + migrate_maint_keys(MemStore({"phase": DRAIN}), UNIT_A, [UNIT_A]).ran + is False) + # --------------------------------------------------------------------------- # @@ -720,7 +838,8 @@ check("...and the flat keys are the ones that carried the cycle, so 'no new " == t0 + timedelta(hours=22, minutes=5)) check("...and `migrate_maint_keys()` refuses to run at all on a one-pack " "roster, which is why the layout above stays put", - migrate_maint_keys(MemStore({"phase": DRAIN}), "inverter-a", 1).ran is False) + migrate_maint_keys(MemStore({"phase": DRAIN}), UNIT_A, [UNIT_A]).ran + is False) # ⚠️ THE REASON STRINGS, PINNED AS LITERALS. The card asks for byte-identical # behaviour AND reason strings at N=1, and the only way a check can see a @@ -1004,31 +1123,89 @@ check("...and a real cycle on it writes no MAINTENANCE key that did not exist " and all(k in _c1store.data for k in MAINT_UNIT_KEYS) and MAINT_SCHEMA_KEY not in _c1store.data) -# --- two inverters: A's history is namespaced AND mirrored flat ------------- +# --- two inverters: the first pack's history is namespaced AND mirrored flat - +# ⚠️ THE ROSTER HERE IS ["garage", "shed"] AND THAT IS THE FIX ROUND. Run on +# `TWO_OPTS`, whose entries are named `inverter-a` / `inverter-b`, this whole +# block is satisfied by a Controller that migrates into `UNIT_A` instead of into +# `self.inverters[0].name` - the two strings are equal there, so the check could +# not reach the failure it names (MUTATION.md, third defect). On an operator's +# real roster they are not equal, and `inverter-a` is not even what a nameless +# entry falls back to (`build_roster()` numbers those positionally). The names +# below are the reviewer's: a house with a garage pack and a shed pack. +NAMED_OPTS = {k: v for k, v in TWO_OPTS.items() if k != "inverters"} +NAMED_OPTS["inverters"] = [ + dict(name="garage", setpoint_entity="number.sp_a", + soc_entity="sensor.soc_a", batt_entity="sensor.batt_a", + max_w=A_RATING), + # ⚠️ 3000 W IS THE MARKED HYPOTHESIS AGAIN - inverter B does not exist. + dict(name="shed", setpoint_entity="number.sp_b", + soc_entity="sensor.soc_b", batt_entity="sensor.batt_b", + max_w=B_RATING_HYPOTHESIS), +] +_C2_NAMES = [e["name"] for e in NAMED_OPTS["inverters"]] +check("PRECONDITION for the migration checks below: this roster's first entry " + f"is {_C2_NAMES[0]!r}, which is NOT `UNIT_A` ({UNIT_A!r}), and `UNIT_A` " + "is not on the roster at all. Every claim under it about 'the first " + "roster entry's namespace' is therefore a claim a wrong constant would " + "fail", + _C2_NAMES[0] != UNIT_A and UNIT_A not in _C2_NAMES) + +# ⚠️ THE DATE IS RELATIVE TO `_now`, NOT A LITERAL. A pack's history is only +# observable through `due()` if the interval has not run out on its own: an +# absolute date drifts towards `interval_days` as this file ages and the day it +# crosses, "history preserved" and "history lost" stop being different answers - +# which is the same degenerate-fixture defect one level up. +_C2_LAST = _now - timedelta(days=3) _c2store = MemStore({"schema_version": 1, - "last_completed": "2026-08-07T12:00:00+00:00"}) -_c2, _ = controller(TWO_OPTS, store=_c2store) -print(f" two inverters -> namespaces " + "last_completed": _C2_LAST.isoformat()}) +_c2, _ = controller(NAMED_OPTS, store=_c2store) +print(f" roster {_C2_NAMES} -> namespaces " f"{[m.unit for m in _c2.maints]}, mirrors " f"{[m.store.mirror_flat for m in _c2.maints]}") +print(f" flat last_completed {_C2_LAST:%Y-%m-%d} (3 d ago, interval " + f"{cfg.interval_days} d) -> migration: {_c2.maint_migration.explain()}") +print(f" namespaced keys now: " + f"{sorted(k for k in _c2store.data if k.startswith('unit/') and not k.endswith(('sign_tripped', 'sign_reason')))}") check("a two-inverter Controller builds one machine per roster entry, each in " - "its own namespace, and migrates the flat history into the FIRST pack's", - [m.unit for m in _c2.maints] == ["inverter-a", "inverter-b"] - and _c2.maints[0].store.get_time("last_completed") - == datetime(2026, 8, 7, 12, 0, tzinfo=timezone.utc) + "its own namespace, and migrates the flat history into the FIRST ROSTER " + "ENTRY'S - `garage`, the name the config gave it, not a constant", + [m.unit for m in _c2.maints] == _C2_NAMES + and _c2.maints[0].store.get_time("last_completed") == _C2_LAST and _c2.maints[1].store.get_time("last_completed") is None) +check("⚠️ AND NOT ONE MAINTENANCE KEY IS WRITTEN UNDER `unit/inverter-a/`. That " + "is the namespace a call site handing the migration `UNIT_A` would use, " + "and on this roster nothing reads it - so a store carrying one is an " + "install whose whole maintenance history has been parked out of sight", + not any(k.startswith(unit_key(UNIT_A, "")) for k in _c2store.data) + and _c2store.data.get(unit_key(_C2_NAMES[0], "last_completed")) + == _C2_LAST.isoformat()) +check("⚠️ AND THE PACK IS NOT DUE, WHICH IS WHAT 'NO LOST HISTORY' MEANS TO AN " + "OPERATOR (AC5). It recalibrated 3 days ago and the interval is " + f"{cfg.interval_days} days. A migration into a namespace this pack does " + "not read leaves it reading NOTHING - absent means never recalibrated, " + "means due now - so this one line separates a preserved month from a lost " + "one", + _c2.maints[0].store.get_time("last_completed") is not None + and not _c2.maints[0].due(_now)) +check("...and the migration REPORTS clean and stamps the marker on this roster, " + "so 'not due' above is a migration that landed rather than one that was " + "refused", + _c2.maint_migration.ok and _c2.maint_migration.verified + and not _c2.maint_migration.misdirected + and _c2store.data.get(MAINT_SCHEMA_KEY) == MAINT_SCHEMA_VERSION) check("...and ONLY the first pack mirrors back to the flat keys, because the " - "flat spelling means pack A on every install that has one - a mirror from " - "B would overwrite A's rollback view with B's phase", + "flat spelling means the first roster entry on every install that has " + "one - a mirror from the second would overwrite its rollback view", [m.store.mirror_flat for m in _c2.maints] == [True, False]) _c2.maints[0].force_start(_now) _c2.maints[1].store.set("phase", HOLD) -check("...and the mirror is LIVE: pack A's new phase reaches the flat key, so a " - "rollback finds a current history rather than one dated migration day", - _c2store.data.get(unit_key("inverter-a", "phase")) == DRAIN +check("...and the mirror is LIVE: the first pack's new phase reaches the flat " + "key, so a rollback finds a current history rather than one dated " + "migration day", + _c2store.data.get(unit_key(_C2_NAMES[0], "phase")) == DRAIN and _c2store.data["phase"] == DRAIN) -check("...while pack B's write reaches only pack B's namespace", - _c2store.data.get(unit_key("inverter-b", "phase")) == HOLD +check("...while the second pack's write reaches only its own namespace", + _c2store.data.get(unit_key(_C2_NAMES[1], "phase")) == HOLD and _c2store.data["phase"] == DRAIN) @@ -1081,6 +1258,91 @@ check("...while the SAME site with the house importing more than the drain " "'maintenance does not work at two inverters'", _cB.allocation.refused is None and _cB.inverters[1].target == 2000.0) + + +# --------------------------------------------------------------------------- # +print() +print("13. RECORDED, NOT FIXED: SAFETY-02's migration has the same " + "wrong-namespace hole, and it is a LATCH") + +# ⚠️ FOUND WHILE FIXING THIS CARD'S OWN INSTANCE, AND WRITTEN DOWN AS A RUNNING +# CHECK RATHER THAN A COMMENT SO IT CANNOT BE FORGOTTEN OR QUIETLY "FIXED" BY AN +# UNRELATED EDIT. It is the SAME defect one module over, and it costs more. +# +# WHAT IT IS. `Controller.__init__` calls `migrate_unit_keys(store, UNIT_A)` - +# a hardcoded constant, where the maintenance migration now passes the roster's +# own first name. It has to run BEFORE `build_roster()`, because each +# `Inverter` restores its sign latch in its constructor, so there is no roster +# to name yet and `_verify_moves()` is passed `roster=None`. On a DECLARED +# roster the pre-DEV-02 flat `sign_tripped: true` therefore lands under +# `unit/inverter-a/sign_tripped`, which no unit on this site reads. +# +# WHY THE EXISTING GUARD DOES NOT CATCH IT, WHICH IS THE PART WORTH PINNING. +# `_grade_store_migration()` re-arms the latch when the flat key says tripped +# and NOTHING NAMESPACED IS LATCHED. The misdirected write IS namespaced and IS +# latched, so `namespaced_latched` is True, the guard returns, and no inverter +# on the roster comes up tripped. The guard is defeated by the very key the +# defect wrote. +# +# WHY IT IS NOT FIXED HERE. The remedy is to name the migration's target from +# the config before any `Inverter` exists, which means factoring +# `build_roster()`'s naming rule out of `build_roster()` so there is still only +# ONE spelling of it - inverter.py and SAFETY-02's latch semantics, with its own +# mutation battery over test_inverter.py and test_watchdog.py. MAINT-05 moves +# the maintenance history and nothing about the sign latch. +# +# ⚠️ AND IT IS A DISARMED SAFETY GUARD, NOT LOST HISTORY. Read the checks below +# as the SPECIFICATION OF A DEFECT: every one of them is asserting the WRONG +# behaviour, on purpose, and the card that fixes it should expect them to go +# red and should replace them. +# +# ⚠️ NO `schema_version` IN THIS STORE, AND THAT IS THE PRE-DEV-02 SHAPE +# ITSELF: an install that has never run a version with namespaced keys, which is +# what makes `migrate_unit_keys()` run at all. +_lat_store = MemStore({"sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: measured pre-DEV-02"}) +_lat, _ = controller(NAMED_OPTS, store=_lat_store) +print(f" pre-DEV-02 flat latch + declared roster {_C2_NAMES} -> " + f"{_lat.store_migration.explain()}") +print(f" latch keys in the store: " + f"{sorted(k for k in _lat_store.data if k.endswith('sign_tripped'))}") +print(f" roster came up tripped: " + f"{[(i.name, i.sign_wd.tripped) for i in _lat.inverters]}") +check("the flat pre-DEV-02 latch is migrated into `unit/inverter-a/` - a name " + "that is NOT on this roster, so no unit reads it. This is the same " + "wrong-namespace write `migrate_maint_keys()` now refuses, in the " + "migration that runs before there is a roster to grade against", + _lat_store.data.get(unit_key(UNIT_A, "sign_tripped")) is True + and unit_key(_C2_NAMES[0], "sign_tripped") not in _lat_store.data) +check("⚠️ AND NO INVERTER ON THE ROSTER COMES UP LATCHED, on a store whose file " + "still reads `sign_tripped: true`. That is DEV-02's own sentence for the " + "worst case - a disarmed safety guard on an install that believes it is " + "protected", + not any(i.sign_wd.tripped for i in _lat.inverters) + and _lat_store.data["sign_tripped"] is True) +check("...and `_grade_store_migration()` is DEFEATED BY THE KEY THE DEFECT " + "WROTE, which is why this needs a card rather than a tweak: its " + "'is anything namespaced latched' test is satisfied by the misdirected " + "value itself, so it returns instead of re-arming", + any(k.startswith("unit/") and k.endswith("/sign_tripped") and v + for k, v in _lat_store.data.items()) + and not any(i.sign_wd.tripped for i in _lat.inverters)) +# The counterfactual, and it is the accepted baseline habit 7 asks for: the +# same store, the same migration, one thing different - the roster shape. +_flat_lat_store = MemStore({"sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: measured pre-DEV-02"}) +_flat_lat, _ = controller(FLAT_OPTS, store=_flat_lat_store) +print(f" the same store on the FLAT config -> roster " + f"{[(i.name, i.sign_wd.tripped) for i in _flat_lat.inverters]}") +_flat_lat_ok = (all(i.sign_wd.tripped for i in _flat_lat.inverters) + and _flat_lat_store.data.get(unit_key(UNIT_A, "sign_tripped")) + is True) +check("...while the SAME store on the FLAT config - the shape where `UNIT_A` " + "really is the roster's only name - migrates and re-arms correctly. So " + "this is a defect of the DECLARED roster, not of the migration as such, " + "and that is the counterfactual that says which half is broken", + _flat_lat_ok) + print() print() if fails: -- 2.54.0 From 905b6fd2b3b26bbfb993634ecf7f87daced5e457 Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Fri, 4 Sep 2026 19:40:51 +0200 Subject: [PATCH 3/3] MAINT-05 fix round 2: the refusal has to survive the NEXT start, and both marker guards have to be visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REJECTED because the fix round's own load-bearing sentence was FALSE, and the whole suite was blind to that. `_verify_moves()` produced the red the reviewer reproduced - that half stood - but DECISIONS §2 claimed "`ok` gates the marker, so a misdirected migration is not stamped and the next start runs it again ... a re-run is recoverable". THE REFUSAL LASTED EXACTLY ONE START. MEASURED, Controller level, ONE persisted store, roster ["garage","shed"], flat last_completed 2026-08-07, call site still handing `inverter-a` on both starts: construction 1: ok=False misdirected=4 moved=4 kept=0 marker= construction 2: ok=True misdirected=0 moved=0 kept=4 marker=1 history parked under unit/inverter-a/...; marker stamped after ONE restart. WHY. `_verify_moves()` graded only `rep.moved`. On start 2 the same wrong call site finds the four keys already present, so rule 1 routes every one of them to `rep.kept`, `rep.moved` is EMPTY, the roster loop iterates over nothing, `misdirected` is empty and `ok` is True. Start 3 then takes the `from_version >= MAINT_SCHEMA_VERSION` early return FOREVER - including after the call site is fixed. Loud-and-recoverable became silent-and-permanent after one restart: the exact state store.py's header says must never happen, reached by the fix written to prevent it. WHY THE CHECK MISSED IT - the recurring defect, this time INSIDE the remedy. Part 9b's recovery check was `migrate_maint_keys(_m5, ROSTER[0], ROSTER)` - the CORRECTED name. It measures recovery along the one path where the operator has ALREADY fixed the bug. Nothing in the 19 files called `migrate_maint_keys()` twice with the SAME WRONG `unit`. The check was true and passed for the wrong reason. WHAT CHANGED 1. app/store.py `_verify_moves()` grades `set(rep.moved) | set(rep.kept)`. Sound by construction: every entry in both lists is a `unit_key(unit, legacy)` built from the SAME `unit` argument, so grading `kept` grades the same namespace decision `moved` does. A correct call site's `kept` entries sit under a roster name and pass; a wrong one cannot launder its namespace by having already written it. `migrate_unit_keys()` passes `roster=None` and returns before the loop, so it is untouched. 2. test_maintenance.py part 9c - THE CHECK THAT WAS MISSING. Two starts, SAME wrong unit, ONE store: the marker is still withheld on the second, the refusal names the same roster, the flat history is still there to recover from, and a third start with the CORRECTED name still gets a run (which a stamped marker would have made impossible). Its PRECONDITION asserts the SHAPE of the second report - `moved` empty, `kept` full - so it cannot quietly revert to grading 9b's path. Counterfactual beside it: the same two calls with the right name are accepted, then skipped as `already at schema`. 3. test_maintenance.py part 9d - THE OTHER GUARD ON THE SAME LINE, and a survivor nothing in the repository could see. `if rep.ok:` is `not lost and not misdirected`; every check that reached the marker did so on a store whose `kept` was empty, so if rep.ok: -> if rep.ok or rep.kept: (+12 B) stamped the marker on a migration that LOST A MONTH OF HISTORY - defeating the older, settled `lost` guard as well as the new one - and ALL 19 SUITES WERE GREEN on it (measured at 65cddf3, purged, :ro). 9d's fixture is the one shape that separates the guards: two keys already namespaced (`kept` non-empty, the mutant's `or` fires) and two whose write the store drops (`lost` non-empty), `misdirected` empty throughout. 4. FOUND HERE AND CLOSED: THE SAME MUTATION ONE FUNCTION UP. `migrate_unit_keys()` - SAFETY-02's LATCH migration - gates its own marker the same way, and every check that reached it ran on a store with an empty `kept`. `if rep.ok or rep.kept:` at store.py:307 SURVIVED ALL 19 FILES. It stamps `schema_version` on a migration that DROPPED THE LATCH: the guard is off, the file still reads `sign_tripped: true`, and the store now says the migration finished, so the next start skips the one thing that would re-arm it. test_inverter.py part 10 now drops the `sign_tripped` write BESIDE an already-namespaced `sign_reason`, with the accepted-baseline counterfactual. No source change - the shipped gate was right; nothing could see it. Mutation table 34 -> 35. 5. DECISIONS/MAINT-05 §2 CORRECTED. The paragraph that stated the false guarantee now states what was measured, why grading `moved` alone could not hold, and why the union is sound. §7 records the marker-gate hole found in `migrate_unit_keys()` and that it was closed. MUTATION.md gains habit 9 - "exercise a refusal a SECOND time with the SAME wrong input, not the corrected one" - and two survey rows. MUTATION LEDGER - 11 mutations, ALL RED on the final tree, every run on a fresh copy with the bytecode cache purged (entry count printed before and after) and the tree mounted :ro so none could be written. Two were survivors BEFORE this commit and both are recorded as such. Full transcript on the PR. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/CHANGELOG.md | 33 ++++ .../MAINT-05-per-pack-maintenance.md | 103 ++++++++++++- goodwe_controller/MUTATION.md | 39 ++++- goodwe_controller/app/store.py | 49 +++++- goodwe_controller/test_inverter.py | 64 +++++++- goodwe_controller/test_maintenance.py | 142 ++++++++++++++++++ 6 files changed, 416 insertions(+), 14 deletions(-) diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index 1dde8f0..c24a8ac 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -62,6 +62,39 @@ two power-moving phases on one grid connection pulling against each other. Every two-pack fixture that could see this now names its first pack `garage`; with them all called `inverter-a`, `self.inverters[0].name` and `UNIT_A` were the same string and the swap left all 19 suites green. +* **AND THE REFUSAL NOW SURVIVES THE NEXT START** (second fix round). The fix + above graded only the keys it had just *written*, so it refused exactly once: + measured at Controller level over one persisted store, roster + `["garage", "shed"]`, with the call site still wrong on both starts — + construction 1 `ok=False misdirected=4 moved=4 kept=0 marker=`, + construction 2 `ok=True misdirected=0 moved=0 kept=4` and the marker + **stamped**. On the second start rule 1 finds the wrong keys already there and + routes all four to `kept`, so `moved` is empty and the roster loop grades + nothing — and start 3 skips the migration forever, *including after the call + site is fixed*. Loud-and-recoverable became silent-and-permanent after one + restart. `_verify_moves()` now grades `set(moved) | set(kept)`; both lists are + built from the same `unit` argument, so grading `kept` grades the same + namespace decision. `test_maintenance.py` part **9c** drives two starts with + the same wrong unit over one store — the shape nothing in the suite had, since + 9b measured recovery along the path where the operator has *already* fixed the + call site. +* **AND THE MARKER GATE IS PINNED ON BOTH GUARDS.** `if rep.ok:` is + `not lost and not misdirected`, and the mutation `if rep.ok or rep.kept:` + stamped the marker on a migration that **lost a month of history** with all 19 + suites green. Part **9d** is the fixture that separates the two guards: two + keys already namespaced (`kept` non-empty) and two whose write the store + drops (`lost` non-empty), `misdirected` empty throughout. +* ⚠️ **AND THE SAME MUTATION ONE FUNCTION UP WAS A SURVIVOR, FOUND HERE AND + CLOSED.** `migrate_unit_keys()` — SAFETY-02's **latch** migration — gates its + own marker on `rep.ok` too, and every check that reached that gate ran on a + store where nothing was namespaced yet, so `rep.kept` was empty in all of + them. `if rep.ok or rep.kept:` there stamps `schema_version` on a migration + that **dropped the latch**: the guard is off, the file still says + `sign_tripped: true`, and the next start skips the migration that would have + re-armed it. Measured a survivor of all 19 files; now red on + `test_inverter.py` part 10, which drops the `sign_tripped` write beside an + already-namespaced `sign_reason`. No source change — the shipped gate was + right; nothing could see it. * ⚠️ **FOUND, RECORDED, NOT FIXED (SECOND ONE).** `migrate_unit_keys()` — SAFETY-02's latch migration — has the identical hole and it costs a **guard** rather than a history. It runs before `build_roster()` (each `Inverter` diff --git a/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md b/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md index ab18205..546af20 100644 --- a/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md +++ b/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md @@ -120,13 +120,76 @@ reported as `misdirected` — a **different** failure from `lost`: not information, whatever its value. So anything in it fails `ok` on its own. And **`ok` gates the marker**, so a misdirected migration is not stamped and the -next start runs it again. That is the decision the fix round was asked to -record, and the argument for it is asymmetric: a re-run is recoverable — rule 1 -makes it a no-op where keys did land, and rule 2 left the flat keys in the file -to recover *from* — while a silent skip is not recoverable at all, because the -store now claims the migration happened. `test_maintenance.py` part 9b measures -the recovery rather than asserting it: the same store, re-run with the roster's -own first name, comes back with its month of history. +next start runs it again. The argument for it is asymmetric: a re-run is +recoverable — rule 1 makes it a no-op where keys did land, and rule 2 left the +flat keys in the file to recover *from* — while a silent skip is not recoverable +at all, because the store now claims the migration happened. + +### ⚠️ THE PARAGRAPH ABOVE USED TO END HERE, AND AS IT STOOD IT WAS FALSE + +**The refusal lasted exactly one start. The second start stamped the marker.** +That is a correction to what the first fix round wrote down, not a new +qualification of it, and it is recorded here because a decision document that +states a guarantee the code does not provide is worse than one that says +nothing. + +`_verify_moves()` graded only `rep.moved`. Measured at Controller level over +**one persisted store**, roster `["garage", "shed"]`, flat `last_completed` +2026-08-07, with the call site still handing `inverter-a` on **both** starts: + +``` +construction 1: ok=False misdirected=4 moved=4 kept=0 marker= (withheld) +construction 2: ok=True misdirected=0 moved=0 kept=4 marker=1 (STAMPED) +history parked under unit/inverter-a/…; marker stamped after ONE restart. +``` + +On start 2 the same wrong call site finds the four keys it wrote already +present, so **rule 1** — never overwrite a namespaced value — routes every one +of them to `rep.kept`. `rep.moved` is empty, the roster-grading loop iterates +over nothing, `misdirected` is empty, `ok` is True, the marker is stamped, and +start 3 takes the `from_version >= MAINT_SCHEMA_VERSION` early return **forever +— including after the call site is fixed**. Loud-and-recoverable became +silent-and-permanent after one restart: exactly the state `store.py`'s header +says must never happen, reached by the fix written to prevent it. + +**The remedy is one line**, and it is sound by construction rather than by +argument: grade `set(rep.moved) | set(rep.kept)`. Every entry in *both* lists is +a `unit_key(unit, legacy)` built from the **same** `unit` argument, so grading +`kept` grades the same namespace decision `moved` does. A correct call site's +`kept` entries are under a roster name and pass; a wrong one cannot launder its +namespace by having already written it. `migrate_unit_keys()` passes +`roster=None` and returns before the loop, so it is unaffected. + +**Why the check missed it, which is the part worth keeping.** Part 9b's recovery +check was `migrate_maint_keys(_m5, ROSTER[0], ROSTER)` — the **corrected** name. +It measures recovery along the one path where the operator has *already fixed +the bug*. The real failure is a call site that is still wrong on the next start, +and nothing in the 19 files called `migrate_maint_keys()` twice with the **same +wrong** `unit`. The check was true, passed, and could not reach the failure its +sentence named — MUTATION.md's third defect, this time sitting inside the fix +written to close it. Part **9c** now drives two starts with the same wrong unit +over one store, asserts the marker is still withheld on the second, and asserts +the *shape* of the second report (`moved` empty, `kept` full) so it cannot +quietly revert to 9b's path. + +### The marker gate is `rep.ok` and nothing weaker — both guards, each pinned + +`if rep.ok:` is `not lost and not misdirected`, and until this round only the +`misdirected` half was reachable by a check that also had a non-empty `kept`. +The mutation + +``` +if rep.ok: -> if rep.ok or rep.kept: +``` + +stamps the marker on a migration that **lost a month of history** — defeating +the older, settled `lost` guard as well as the new one — and **all 19 suites +were green on it**. Part **9d** is the fixture that separates them: two keys +already namespaced (`kept` non-empty, so the mutant's `or` fires) and two keys +whose write the store drops (`lost` non-empty, so `ok` is False), with +`misdirected` empty throughout so the section cannot pass on the other guard by +accident. Measured differential on it: shipped → marker absent; mutant → marker +`1`, both reporting `ok=False lost=2 kept=2`. **Why the fixtures had to change, and this is the part that got the card rejected.** Nothing in the repository could see whether any of the above was @@ -297,3 +360,29 @@ It is pinned as a running check (`test_maintenance.py` part 13) rather than left as a comment. ⚠️ **Its assertions state the WRONG behaviour on purpose** — read them as the specification of a defect; the card that fixes it should expect them to go red and should replace them. + +### And its MARKER GATE had the round-3 hole too — found here, closed here + +Separate from the namespace hole above, and **fixed** rather than recorded, +because closing it cost one test fixture and no source change. +`migrate_unit_keys()` gates `schema_version` on `rep.ok` exactly as +`migrate_maint_keys()` gates its own marker, and every check that reached that +gate ran on a store where **nothing** was namespaced yet — so `rep.kept` was +empty in all of them and + +``` +if rep.ok: -> if rep.ok or rep.kept: +``` + +**survived all 19 files**. What it does is stamp the schema marker on a +migration that DROPPED THE LATCH: the guard is off, the state file still reads +`sign_tripped: true`, and because the store now says the migration finished, the +next start skips the one thing that would have re-armed it. DEV-02's own worst +case, filed under *done*. + +`test_inverter.py` part 10 now drops the `sign_tripped` write **beside an +already-namespaced `sign_reason`** — `lost` non-empty *and* `kept` non-empty, +which is the one shape none of the existing fixtures had — and asserts the +marker is withheld, with the accepted-baseline counterfactual beside it (habit +7). The shipped gate was correct throughout; what changed is that a check can +now see it. diff --git a/goodwe_controller/MUTATION.md b/goodwe_controller/MUTATION.md index 843b8f8..a4bd351 100644 --- a/goodwe_controller/MUTATION.md +++ b/goodwe_controller/MUTATION.md @@ -135,13 +135,14 @@ 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. Seven instances in two weeks; four +It is not hypothetical and it is not rare. EIGHT instances in two weeks; four were found by review, two - CFG-04's fixture pair - by an author's own mutation -battery, which is the cheaper place, and one by review again, in the one +battery, which is the cheaper place, one by review again, in the one place nobody had thought to point the rule at: not a check in the suite, but a -COMMITTED EVIDENCE ARTIFACT four documents cite. +COMMITTED EVIDENCE ARTIFACT four documents cite - and the eighth INSIDE THE FIX +WRITTEN TO CLOSE THE SEVENTH. -⚠️ **AND IT HAS NOW APPEARED IN FIVE CONSECUTIVE CARDS.** The seventh instance, +⚠️ **AND IT HAS NOW APPEARED IN SIX CONSECUTIVE CARDS.** The seventh instance, MAINT-05's, is the one that says why: the defect is not carelessness about fixtures, it is that **a check and the thing it checks were built from the same constant**. Ask of every difference-claim not "is this assertion true" but @@ -196,6 +197,22 @@ same place twice, the check is decoration. one — grade against something the thing under test did not supply, here the ROSTER the machines read. + ⚠️ **AND THE FIX ITSELF CARRIED THE DEFECT, WHICH IS THE SIXTH CARD IN A ROW + AND THE FIRST TIME IT HAS APPEARED *INSIDE* A REMEDY.** The fix round's + recovery check was `migrate_maint_keys(_m5, ROSTER[0], ROSTER)` — the + **corrected** name. It measures recovery along the one path where the operator + has *already fixed the bug*, and nothing in the 19 files called + `migrate_maint_keys()` twice with the **same wrong** `unit`. So the refusal + the card was rejected twice to obtain lasted **exactly one start**: measured + at Controller level over one persisted store, roster `["garage", "shed"]` — + construction 1 `ok=False misdirected=4 moved=4 kept=0 marker=`, + construction 2 `ok=True misdirected=0 moved=0 kept=4 marker=1`. On start 2 + rule 1 routes the wrongly-written keys to `rep.kept`, `rep.moved` is empty, + the roster loop iterates over nothing, and the marker is stamped on a + migration that reached no readable key — after which the early return skips it + forever, *including after the call site is fixed*. The check was true and + passed for the wrong reason. Habit **9** below is what falls out of it. + * **CFG-04's `tools/supervisor_shape_probe.py` §B** — and this one is the rule reaching outside the suite. The probe is the committed evidence that `config.yaml`'s `inverters: []` default is load-bearing, and four documents @@ -270,6 +287,18 @@ Cheap habits fall out of it, and all of them are now used in this repository: red. `inverters: []` was deleted from `config.yaml` and the probe re-run against `ghcr.io/home-assistant/amd64-hassio-supervisor:2026.08.0` for exactly that reason. +9. **Exercise a refusal a SECOND time with the SAME wrong input — not the + corrected one.** A guard that refuses once and then admits the same bad + caller is not a guard, it is a delay, and the second call is a *different + code path* whenever the first call left state behind. MAINT-05's roster + grading iterated the keys it had just **written**; on the second start the + same wrong caller found them already present, they moved to the + `already namespaced` list, and the loop graded an empty set. Re-running with + the **fixed** argument — which is what the check did — measures the operator's + recovery, never the persistence of the refusal. Ask of every refusal: *what + does this do when the caller has not been fixed and simply runs again?* And + assert the **shape** of that second report, not only its verdict, so the + check cannot silently revert to grading the first path. 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 @@ -326,6 +355,8 @@ commit and PR bodies. | **TARIFF-01** reviewer survivor: `record_block()` ignoring its `policy` argument (0dbd485) | **re-verified — still a survivor** | re-run here through `tools/mutation_run.sh` against the 15 suites that were on the record, all green with `billed_w(block, policy)` mutated to `billed_w(block, POLICY_NET)`. The recorded diagnosis holds: every block reaching the store is single-phase, where NET, SUM and MAX collapse. **Now CLOSED by TEST-13** (`test_tariff.py` section 7), which round-trips section 7's own unbalanced `(3000, 500, -200) W` block THROUGH the store under each policy - three distinct banked figures, so a hardcoded policy takes at least two of them red. Third instance of "a stimulus evaluated where the quantity it grades is degenerate", after WITNESS-02's span normalisation and TARIFF-01's own DST window. | | **SAFETY-25** 8 mutations against `test_peakguard.py` part 10, then 9 more after review, all red | **not at risk** | each run against a tree copied fresh with `__pycache__` excluded and mounted `:ro`, so no `.pyc` existed to go stale - the same structural immunity recorded for CI above. Byte deltas recorded on the PR; none is `+0 B`. All are **reds**, which is the direction this defect cannot fabricate. ⚠️ The second round exists because the first round's mutations were all aimed at DISARMING the guard, and the claim part 10 actually made was about the guard being FIXED - the reviewer built the fix and the suite stayed green. Bytecode was never the issue there; an unaimed battery was. | | **SAFETY-03** reviewer survivor: `abs()` dropped from `clamp_to_rating`'s limit (6aabb83) | **re-verified — still a survivor** | re-run the same way, all 15 green. Also as recorded: unreachable under the `int(100, 5000)` schema. | +| **MAINT-05 round 3**, `migrate_maint_keys()` marker gate `if rep.ok:` → `if rep.ok or rep.kept:` (`+12 B`) | **was a survivor — now CLOSED** | measured against head `65cddf3` under the purge, tree mounted `:ro`, `19/19 PASS`. It stamps the schema marker on a migration that LOST state, defeating the older `lost` guard as well as the new `misdirected` one, because every check that reached the marker did so on a store whose `kept` list was empty. Closed by `test_maintenance.py` **9d**; the mutation is now red on three named checks. | +| **MAINT-05 round 3**, `migrate_unit_keys()` marker gate `if rep.ok:` → `if rep.ok or rep.kept:` (`+12 B`) | **found here — SAFETY-02's half, now CLOSED** | the SAME mutation one function up, on the LATCH migration rather than the history one. Measured a survivor against this branch under the purge, `:ro`, `19/19 PASS`, then closed by `test_inverter.py` part 10's new fixture — a store with `unit/inverter-a/sign_reason` already present (so `kept` is non-empty) whose `sign_tripped: true` write is dropped. Shipped: marker withheld. Mutant: marker stamped on a **disarmed latch**, and the next start skips the migration that would have re-armed it. | | **TARIFF-01**'s other three survivors, **SAFETY-03**'s `describe()` survivor, **TARIFF-02**'s M3/M5/M7, **SAFETY-18**'s three | **still unverified** | recorded before the purge discipline, with no byte delta on the record, so a `+0 B` same-second revert cannot be excluded. All are survivors, which is the direction this defect fabricates. TARIFF-02's and SAFETY-18's were **closed by later commits whose claims are reds**, so what is unverified is the *diagnosis*, not the current state of the code. | **Nothing here says a past result is wrong.** The two survivors re-run above diff --git a/goodwe_controller/app/store.py b/goodwe_controller/app/store.py index d1a4d24..324f270 100644 --- a/goodwe_controller/app/store.py +++ b/goodwe_controller/app/store.py @@ -373,6 +373,16 @@ def _verify_moves(store, rep: MigrationReport, roster=None) -> None: next start runs the migration again. A re-run is recoverable, by rule 1; a silent skip is the state store.py's header says must never happen. + ⚠️ AND THE GRADED SET IS `moved` UNION `kept`, WHICH IS WHAT MAKES THAT + PARAGRAPH TRUE ON THE SECOND START AS WELL AS THE FIRST. Graded over `moved` + alone the refusal lasted exactly one start: the wrong call site is still + wrong next time, finds the keys it wrote already present, and rule 1 routes + every one of them to `kept` - so `moved` is empty, this loop sees nothing, + `ok` comes back True and the marker is stamped on a migration that has + reached no readable key. The measurement is in the comment on the loop + itself. `test_maintenance.py` section 9c calls this twice with the SAME + wrong `unit` over one store, which is the shape nothing in the suite had. + `roster=None` means "not graded", and the one caller that passes it is `migrate_unit_keys()`, which runs BEFORE `build_roster()` for the reason its own docstring gives - the `Inverter` objects restore their latch in their @@ -396,7 +406,32 @@ def _verify_moves(store, rep: MigrationReport, roster=None) -> None: names = [str(n) for n in roster] # `unit_key()` builds `unit//`, so the unit is field 1. Split # through the same spelling the writer used rather than re-deriving it. - for target in sorted(rep.moved): + # + # ⚠️ `moved` UNION `kept`, AND THE UNION IS THE WHOLE OF THE SECOND FIX + # ROUND. Grading only `moved` made the refusal last EXACTLY ONE START. + # Measured at Controller level over one persisted store, roster + # ["garage", "shed"], flat `last_completed` 2026-08-07, with the call site + # still handing `inverter-a` on both starts: + # + # construction 1: ok=False misdirected=4 moved=4 kept=0 marker= + # construction 2: ok=True misdirected=0 moved=0 kept=4 marker=1 + # + # On start 2 the four keys the WRONG start 1 wrote are already present, so + # rule 1 routes every one of them to `rep.kept`, `rep.moved` is EMPTY, this + # loop iterates over nothing, `misdirected` is empty and `ok` is True - so + # the caller stamps the marker and start 3 takes the `from_version >= + # SCHEMA` early return FOREVER, including after the call site is fixed. + # Loud-and-recoverable became silent-and-permanent after one restart: the + # exact state this module's header says must never happen, reached by the + # fix that was written to prevent it. + # + # The union is sound by construction rather than by hope: every entry in + # both `rep.moved` and `rep.kept` is a `unit_key(unit, legacy)` built from + # the SAME `unit` argument, so grading `kept` grades the same namespace + # decision `moved` does. A correct call site puts its `kept` entries under a + # roster name and they pass; a wrong one cannot launder its namespace by + # having already written it. + for target in sorted(set(rep.moved) | set(rep.kept)): parts = target.split("/") unit = parts[1] if len(parts) > 2 else "" if unit in names: @@ -443,7 +478,9 @@ def migrate_maint_keys(store, unit: str, roster) -> MigrationReport: marker. Handing the roster in means the read-back is graded against the keys the MACHINES read, and a `unit` that is not among these names comes back `misdirected`: not `ok`, marker NOT written, migration runs again next - start. See `_verify_moves()` for the install this was measured on. + start - and again the start after that, because `_verify_moves()` grades + `kept` as well as `moved`. See `_verify_moves()` for the install this was + measured on, and for what grading `moved` alone cost on start 2. THE THREE RULES ARE `migrate_unit_keys()`'s, SHARED IN CODE rather than restated: `_move_flat_keys()` is rule 1 (never overwrite a namespaced value) @@ -481,6 +518,14 @@ def migrate_maint_keys(store, unit: str, roster) -> MigrationReport: _move_flat_keys(store, unit, MAINT_UNIT_KEYS, rep) _verify_moves(store, rep, roster=names) + # ⚠️ `rep.ok` AND NOTHING ELSE - NOT `ok or kept`, NOT `not lost`, NOT + # `not misdirected`. BOTH guards gate this line and each one is pinned by a + # check that the other cannot satisfy: section 9c refuses a store whose + # failure is `misdirected` with `lost` empty, and section 9d refuses one + # whose failure is `lost` with `misdirected` empty AND `kept` non-empty - + # the mutation `if rep.ok or rep.kept:` stamps the marker on a migration + # that lost a month of history, and until section 9d existed all 19 suites + # were green on it. if rep.ok: store.set(MAINT_SCHEMA_KEY, MAINT_SCHEMA_VERSION) else: diff --git a/goodwe_controller/test_inverter.py b/goodwe_controller/test_inverter.py index 0ee1c63..e24dbb8 100644 --- a/goodwe_controller/test_inverter.py +++ b/goodwe_controller/test_inverter.py @@ -572,6 +572,58 @@ check("...and a store that only fails to persist a DEFAULT is not reported as " "dismiss before the real one arrives", migrate_unit_keys(LosesTheLatch(), UNIT_A).ok is True) +# ⚠️ AND THE MARKER GATE, ON A STORE WHERE SOMETHING WAS ALREADY NAMESPACED. +# FOUND BY MAINT-05's ROUND-3 BATTERY, AND IT WAS A SURVIVOR OF ALL 19 FILES. +# `migrate_unit_keys()` gates `SCHEMA_KEY` on `rep.ok`, and every check above +# reaches that gate on a store where NOTHING was already under `unit/...` - so +# `rep.kept` is empty in all of them, and the mutation +# +# if rep.ok: -> if rep.ok or rep.kept: +# +# stamped the marker on a migration that DROPPED THE LATCH, with the whole suite +# green. That is a disarmed safety guard plus a store that says the migration +# finished, so the next start skips the one thing that would have re-armed it - +# DEV-02's own worst case with the paper trail filed under "done". +# +# The fixture is the one shape that separates them: `sign_reason` ALREADY +# namespaced (so `kept` is non-empty and the mutant's `or` fires) and the +# `sign_tripped: true` write dropped on the floor (so `lost` is non-empty and +# `ok` is False). `migrate_unit_keys()` is passed no roster, so `misdirected` +# cannot be what refuses this - it is the `lost` guard or nothing. +_kept_reason = "SIGN_DISAGREEMENT: already namespaced by a later version" +_broken_kept = LosesTheLatch({ + "sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: measured", + unit_key(UNIT_A, "sign_reason"): _kept_reason}) +_rep_kept = migrate_unit_keys(_broken_kept, UNIT_A) +print(f" a dropped latch write beside an already-namespaced reason: " + f"{_rep_kept.explain()}") +print(f" lost={len(_rep_kept.lost)} kept={len(_rep_kept.kept)} " + f"ok={_rep_kept.ok}; marker " + f"{_broken_kept.data.get(SCHEMA_KEY, '<>')!r}") +check("PRECONDITION: this fixture reaches the marker gate through the `lost` " + "guard with a NON-EMPTY `kept`, which is the combination none of the " + "checks above has. One key dropped, one key left alone, `ok` False. If a " + "later edit empties `kept` here the mutation goes invisible again, so the " + "shape is asserted rather than assumed", + _rep_kept.ran and len(_rep_kept.lost) == 1 and len(_rep_kept.kept) == 1 + and not _rep_kept.ok + and _broken_kept.data[unit_key(UNIT_A, "sign_reason")] == _kept_reason) +check("⚠️ AND THE STORE IS NOT MARKED MIGRATED. The key that was ALREADY there " + "does not buy a marker for the latch that was not: stamping here would " + "tell the next start that the migration which dropped `sign_tripped` had " + "finished, so it would never run again and the guard would stay off on an " + "install whose file still reads `sign_tripped: true`", + SCHEMA_KEY not in _broken_kept.data) +check("...and the counterfactual, one property different: the SAME store shape " + "on a store that does not drop writes IS marked migrated. So the refusal " + "above is about the latch that was lost, not about `kept` being non-empty", + migrate_unit_keys( + MemStore({"sign_tripped": True, + "sign_reason": "SIGN_DISAGREEMENT: measured", + unit_key(UNIT_A, "sign_reason"): _kept_reason}), + UNIT_A).ok is True) + _broken2 = LosesTheLatch({"sign_tripped": True, "sign_reason": "SIGN_DISAGREEMENT: measured"}) _c2d, _ = built(store=_broken2) @@ -2149,6 +2201,16 @@ MUTATIONS = [ ("validate.py allowing two roster entries with the SAME name", "part 10: 'two entries with the SAME name are refused' - one Store " "namespace for two units is DEV-02's defect rebuilt in YAML"), + ("`migrate_unit_keys()`'s marker gate `if rep.ok:` -> `if rep.ok or " + "rep.kept:` - the schema marker stamped on a migration that DROPPED THE " + "LATCH, because one key happened to be namespaced already", + "part 10: 'AND THE STORE IS NOT MARKED MIGRATED'. ⚠️ THIS ONE SURVIVED " + "ALL 19 FILES and was found by MAINT-05's round-3 battery, not by this " + "card: every migration check here reached the gate on a store where " + "NOTHING was namespaced yet, so `rep.kept` was empty in all of them and " + "the `or` never fired. Stamping there disarms the latch AND tells the " + "next start the migration finished, so it never re-arms - DEV-02's own " + "worst case filed under 'done'"), ] # ⚠️ FOUR OF THESE SURVIVED THE FIRST TIME AND THE CHECKS WERE CHANGED, NOT THE # TABLE - two in DEV-02, two more in CFG-04, and the CFG-04 pair are recorded in @@ -2167,7 +2229,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) == 34) + len(MUTATIONS) == 35) print() diff --git a/goodwe_controller/test_maintenance.py b/goodwe_controller/test_maintenance.py index 9ed9b47..8f84aea 100644 --- a/goodwe_controller/test_maintenance.py +++ b/goodwe_controller/test_maintenance.py @@ -806,6 +806,148 @@ check("...and a roster of ONE still returns before any of this, so the " is False) +# --------------------------------------------------------------------------- # +print() +print("9c. THE REFUSAL HAS TO SURVIVE THE NEXT START: the same wrong call " + "site, twice, over ONE store") + +# ⚠️ WHY 9b WAS NOT ENOUGH, AND IT IS THIS FILE'S OWN RECURRING DEFECT ONE +# LEVEL UP. 9b's recovery check re-runs the migration with `ROSTER[0]` - the +# CORRECTED name - so it measures recovery along the one path where the +# operator has ALREADY fixed the call site. The real failure is a call site that +# is STILL WRONG on the next start, and until this section nothing in the 19 +# files called `migrate_maint_keys()` twice with the SAME wrong `unit`. 9b was +# true and passed for the wrong reason. +# +# WHAT THAT HID. Measured at Controller level over one persisted store, roster +# ["garage", "shed"], flat `last_completed` 2026-08-07, call site handing +# `inverter-a` on both starts: +# +# construction 1: ok=False misdirected=4 moved=4 kept=0 marker= +# construction 2: ok=True misdirected=0 moved=0 kept=4 marker=1 +# +# `_verify_moves()` graded only `rep.moved`. On start 2 the wrong call site +# finds the keys it wrote already present, rule 1 routes all four to `rep.kept`, +# `rep.moved` is EMPTY, the roster loop iterates over nothing, `misdirected` is +# empty and `ok` is True - so the marker is stamped and start 3 takes the +# `from_version >= MAINT_SCHEMA_VERSION` early return FOREVER, including after +# the call site is fixed. Loud-and-recoverable became silent-and-permanent after +# ONE restart. The fix grades `set(rep.moved) | set(rep.kept)`. +_m7 = flat_store() +_rep7a = migrate_maint_keys(_m7, UNIT_A, ROSTER) +_rep7b = migrate_maint_keys(_m7, UNIT_A, ROSTER) +print(f" start 1: ok={_rep7a.ok} misdirected={len(_rep7a.misdirected)} " + f"moved={len(_rep7a.moved)} kept={len(_rep7a.kept)} marker=" + f"{_m7.data.get(MAINT_SCHEMA_KEY, '<>')!r}") +print(f" start 2: ok={_rep7b.ok} misdirected={len(_rep7b.misdirected)} " + f"moved={len(_rep7b.moved)} kept={len(_rep7b.kept)} marker=" + f"{_m7.data.get(MAINT_SCHEMA_KEY, '<>')!r}") +check("PRECONDITION, SO THIS SECTION CANNOT PASS FOR THE WRONG REASON: start 2 " + "goes down the KEPT path, which is the path no other check in this file " + "reaches. Not one key is written a second time - all four are already " + "there from start 1, so `moved` is EMPTY and `kept` holds every one of " + "them. If a later edit makes start 2 re-WRITE them, this goes red rather " + "than the claim below quietly reverting to 9b's", + _rep7b.ran and _rep7b.moved == {} + and sorted(_rep7b.kept) == sorted(unit_key(UNIT_A, k) + for k in MAINT_UNIT_KEYS)) +check("⚠️ THE MARKER IS STILL WITHHELD ON THE SECOND START. This is the whole " + "section: a refusal that lasts exactly one start is not a refusal, it is " + "a delay - the next start stamps the marker on a migration that has " + "reached no readable key, and every start after that skips the migration " + "entirely", + MAINT_SCHEMA_KEY not in _m7.data) +check("...and it is refused for the SAME reason as the first time, graded " + "against the same roster: four keys under a namespace nothing on this " + "site reads. `ok` False, `misdirected` full, `lost` empty - the caller " + "was wrong, the store was not", + not _rep7b.ok and not _rep7b.verified and not _rep7b.lost + and len(_rep7b.misdirected) == len(MAINT_UNIT_KEYS) + and repr(ROSTER) in _rep7b.explain()) +check("...and the history is still THERE to recover: rule 2 left every flat key " + "in the file across both starts, and the roster's first pack still reads " + "nothing - which is what makes this loud rather than lost", + all(_m7.data[k] is not None for k in MAINT_UNIT_KEYS) + and _m7.data["last_completed"] == "2026-08-07T12:00:00+00:00" + and unit_key(ROSTER[0], "last_completed") not in _m7.data) +_rep7c = migrate_maint_keys(_m7, ROSTER[0], ROSTER) +print(f" start 3, call site FIXED: {_rep7c.explain()}") +check("⚠️ AND BECAUSE THE MARKER WAS NEVER STAMPED, THE FIXED CALL SITE STILL " + "GETS A RUN. A stamped marker would have made this an early return " + "forever - the operator fixes the bug and the migration never happens " + "again. Here the third start migrates the month into `garage` and stamps", + _rep7c.ran and _rep7c.ok + and _m7.data.get(unit_key(ROSTER[0], "last_completed")) + == "2026-08-07T12:00:00+00:00" + and _m7.data[MAINT_SCHEMA_KEY] == MAINT_SCHEMA_VERSION) + +# The counterfactual, so this is not a check satisfied by a validator that +# refuses every second call: the same two calls with the RIGHT name. +_m7ok = flat_store() +_rep7ok_a = migrate_maint_keys(_m7ok, ROSTER[0], ROSTER) +_rep7ok_b = migrate_maint_keys(_m7ok, ROSTER[0], ROSTER) +check("...and the counterfactual: the SAME two calls with the roster's own " + "first name are accepted - stamped on the first, and the second is the " + "`already at schema` early return, not a refusal. So 9c is about the " + "NAME surviving into the second start, not about second calls", + _rep7ok_a.ok and _m7ok.data[MAINT_SCHEMA_KEY] == MAINT_SCHEMA_VERSION + and _rep7ok_b.ran is False and "already at schema" in _rep7ok_b.explain()) + + +# --------------------------------------------------------------------------- # +print() +print("9d. THE OTHER GUARD ON THE SAME LINE: a migration that LOST STATE is " + "not stamped either, even when some keys were already in place") + +# ⚠️ WHAT THIS PINS, AND IT IS A SURVIVOR THE SECTIONS ABOVE CANNOT SEE. +# `migrate_maint_keys()` gates the marker on `rep.ok`, which is BOTH guards: +# `not lost and not misdirected`. Every check in this file that reaches the +# marker reaches it through `misdirected` (9b, 9c) or through a store where +# NOTHING was already namespaced (part 9's `LosesTheHistory`, whose `kept` is +# empty). So the mutation +# +# if rep.ok: -> if rep.ok or rep.kept: +# +# stamped the marker on a migration that lost a month of history - defeating the +# OLDER, settled `lost` guard as well as the new `misdirected` one - and all 19 +# suites stayed green on it. Measured differential on this exact fixture: +# shipped -> marker absent; mutant -> marker 1, both reporting ok=False lost=2. +# +# The fixture is the one shape that separates them: TWO keys already namespaced +# (so `kept` is non-empty and the mutant's `or` fires) and TWO keys whose write +# the store drops (so `lost` is non-empty and `ok` is False). `misdirected` is +# empty throughout - the unit handed in IS the roster's first name - so this +# section cannot pass on the `misdirected` guard by accident. +_m8 = LosesTheHistory(dict(flat_store(**{ + unit_key(ROSTER[0], "phase"): CHARGE, + unit_key(ROSTER[0], "phase_started"): "2026-09-03T22:00:00+00:00"}).data)) +_rep8 = migrate_maint_keys(_m8, ROSTER[0], ROSTER) +print(f" two keys already namespaced, two dropped on write: {_rep8.explain()}") +print(f" ok={_rep8.ok} lost={len(_rep8.lost)} kept={len(_rep8.kept)} " + f"misdirected={len(_rep8.misdirected)}; marker " + f"{_m8.data.get(MAINT_SCHEMA_KEY, '<>')!r}") +check("PRECONDITION: this fixture reaches the marker through the `lost` guard " + "and ONLY that one - two keys in `kept`, two in `lost`, `misdirected` " + "empty. If it ever stops having a non-empty `kept` the mutation below " + "goes invisible again, so the shape is asserted rather than assumed", + _rep8.ran and len(_rep8.kept) == 2 and len(_rep8.lost) == 2 + and not _rep8.misdirected and not _rep8.ok) +check("⚠️ THE MARKER IS NOT STAMPED ON A MIGRATION THAT LOST A MONTH OF " + "HISTORY, and the two keys that DID land do not buy it one. `rep.ok` is " + "the gate and nothing weaker: stamping here would tell the next start a " + "migration finished that dropped half of what it was moving", + MAINT_SCHEMA_KEY not in _m8.data) +check("...and the counterfactual, one property different: the SAME fixture on a " + "store that does not drop writes IS stamped. So the refusal above is " + "about the state that was lost, not about `kept` being non-empty", + migrate_maint_keys( + MemStore(dict(flat_store(**{ + unit_key(ROSTER[0], "phase"): CHARGE, + unit_key(ROSTER[0], "phase_started"): + "2026-09-03T22:00:00+00:00"}).data)), + ROSTER[0], ROSTER).ok is True) + + # --------------------------------------------------------------------------- # print() -- 2.54.0