diff --git a/goodwe_controller/CHANGELOG.md b/goodwe_controller/CHANGELOG.md index afc7f31..c24a8ac 100644 --- a/goodwe_controller/CHANGELOG.md +++ b/goodwe_controller/CHANGELOG.md @@ -2,6 +2,122 @@ ## 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. +* **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. +* **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` + 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 + `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..546af20 --- /dev/null +++ b/goodwe_controller/DECISIONS/MAINT-05-per-pack-maintenance.md @@ -0,0 +1,388 @@ +# 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. + +**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`. + +--- + +## 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. + +### 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. 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 +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 +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. + +## 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. + +### 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 13dad66..a4bd351 100644 --- a/goodwe_controller/MUTATION.md +++ b/goodwe_controller/MUTATION.md @@ -135,11 +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. 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 the sixth 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 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 +"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 @@ -167,6 +175,44 @@ 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. + + ⚠️ **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 @@ -241,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 @@ -297,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/main.py b/goodwe_controller/app/main.py index 1e18ad9..f102b5d 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,66 @@ 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, 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, [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 + # 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 +514,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 +731,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 +1454,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 +1985,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 +3483,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 +3521,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 +3640,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..324f270 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,16 +211,35 @@ 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: - 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: @@ -192,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 @@ -243,7 +301,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 +333,63 @@ 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, 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 + 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. + + ⚠️ 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. + + ⚠️ 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 + 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, "<>") if got == value: @@ -274,14 +401,142 @@ def migrate_unit_keys(store, unit: str) -> MigrationReport: 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. + # + # ⚠️ `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: + 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, 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 + `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. + + ⚠️ `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 - 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) + 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", {}) + names = [str(n) for n in roster] + rep.from_version = int(data.get(MAINT_SCHEMA_KEY, 0) or 0) + 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") + 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, 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(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()) + # ⚠️ 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/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..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) @@ -1629,11 +1681,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", @@ -2132,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 @@ -2150,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 3df01c3..8f84aea 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,14 @@ 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.inverter import UNIT_A +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 +238,1253 @@ 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)") + +# ⚠️ 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): + 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, 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 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(ROSTER[0], "last_completed")) + == "2026-08-07T12:00:00+00:00" + 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) + 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(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(ROSTER[0], "last_completed"): + "2026-09-01T09:00:00+00:00"}) +_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(ROSTER[0], "last_completed")) + == "2026-09-01T09:00:00+00:00" + 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, 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(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, 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", + 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, 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 " + "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}), + 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) + + +# --------------------------------------------------------------------------- # +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() +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}), 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 +# 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: 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": _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 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 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: 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 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) + + + +# --------------------------------------------------------------------------- # +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("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: 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()