glenn schrooyenandClaude Opus 5
753f700539
checks / unit (pull_request) Successful in 4m15s
PR #43 was rejected on two findings and had gone unmergeable behind SAFETY-28 and the ALLOC-01 fix. This is the merge commit; the conflict resolution and both fixes are below, because a merge that also changes behaviour has to say so. THE MERGE, AND WHAT IT COST =========================== Two conflicts in main.py, both where SAFETY-28 and this card touched the same method: * `write_setpoint()` split into a site entry point plus a per-unit `_write_unit()` here, while SAFETY-28 put its record gate in the old single-unit body. `write_refusal()` now takes the INVERTER it is about. Asking it with `self.unit` - as release/1.0 does - would authorise A's share on B's wire, and would also trip test_inverter.py part 1's AST scan, which goes red on any read of a Controller N=1 view inside Controller. * the SAFETY-28 gate sits above the clamp and below the non-finite coercion, exactly where release/1.0 put it, and is now asked about `inv`. Fallout the merge produced in the checks, re-aimed rather than relaxed: * every fixture that calls `write_setpoint()` DIRECTLY - the clamp probe, the hung-heartbeat experiment and its serialised counterfactual, the split deadline probe - now arms a real allocation record first. test_inverter.py grows an `arm()` built through the shipped `resolve()`, the same shape test_clamp.py's is: the per-unit envelopes are filed as real `supervised` claims and the site target is their sum, so the water-fill has exactly one answer and the fixture cannot quietly become its own allocator. * test_clamp.py's two-unit failsafe fixture assigned `c.units`, which DEV-02 made a derived property with no setter. It appends a real `Inverter`. BLOCKER 1 - AC 4's ARBITER-BOUND EVIDENCE WAS VACUOUS ===================================================== test_peakguard.py claimed "each unit's `supervised` bound is ITS OWN `max_w` - give inverter B a different rating and the claim filed on inverter B moves with it", and compared B's bound against `_b2.max_w`. `second_inverter()` DEFAULTED B's rating to `LIVE_MAX_W`, which is A's. IT COMPARED (-5000, 5000) AGAINST (-5000, 5000): a number against itself, unable to observe the failure it named. Reproduced: filing the per-unit `device`/`supervised` claims from `self.inverters[0]`'s numbers while leaving `actuator=self.actuator_of(..., inv)` alone left ALL 19 FILES GREEN. At a roster of two with A=5000/B=3000 that makes inverter B's planned envelope (-5000, 5000) instead of (-3000, 3000) - a 2000 W over-command the arbiter believes is supervised. It is a DIFFERENT defect from the actuator variant the static routing pin catches, and only the actuator variant was in the mutation table. * `second_inverter()` gives B 3000 W - a hypothesis, marked as one - for every fixture in the file, so no check there can compare B against A by accident. * the claim is asserted against LITERAL numbers rather than against an expression that can read the same object twice, for `supervised` AND for `device` (whose `dev` had the same defect), plus the envelope the ARBITER RESOLVED for B - a claim filed right and resolved wrong is the same defect one layer down. * a PRECONDITION check fails if the two ratings are ever made equal again. * the BOUND mutation is in the table, beside the actuator one, and was observed RED (3 checks in test_peakguard.py) via tools/mutation_run.sh. ⚠️ THIRD INSTANCE THIS SESSION of a check whose DATA cannot reach the failure it names - ARB-03's call-site scan saw 3 of 8 appends, ALLOC-01's replay generator saw 0 escapes where a widened one saw 39,991. MUTATION.md gains a section for it beside the stale-bytecode and masking ones, and the rule is written at the line: ANY CHECK ASSERTING "X DIFFERS FROM Y" MUST BE RUN ONCE WITH X AND Y ACTUALLY MADE DIFFERENT. BLOCKER 2 - THE LATCH WAS SILENTLY DISARMED BY THE ROLLBACK ROUND TRIP ====================================================================== store.py's rule 2 keeps the flat keys "so a rollback still finds its latch". The operator path that rule contemplates is a ROUND TRIP, and this card lost the latch on it: 1. install migrates -> schema_version=1, unit/inverter-a/... = False 2. operator DOWNGRADES; SAFETY-02 trips inside the downgrade window and the old add-on writes only the flat `sign_tripped: true` 3. operator upgrades again -> `rep.ran` is False, so `_grade_store_migration()` returned early, and `Inverter.__init__` read the namespaced key: False Observed: the controller came up with `sign_wd.tripped = False` while /data/state.json said `sign_tripped: true`. That is store.py's own header sentence - "the file still says sign_tripped: true, the banner is green, and the controller resumes driving the wrong way" - rebuilt by the version marker. A firmware version change with a rollback path was performed on this site on 2026-09-02, so this is not hypothetical. Three changes, each graded by a mutation observed RED: * THE OPERATOR'S CLEAR CLEARS BOTH SPELLINGS. That is what removes the "re-arm forever" objection the `rep.ran` gate existed to answer: with the fossil cleared, `flat True + namespaced False` is unambiguously a downgrade-window latch. * A TRIP MIRRORS THE FLAT PAIR. Rule 2 only ever covered the latch present AT MIGRATION TIME - nothing flat was written for a trip taken afterwards, so an operator who downgraded found `sign_tripped: false` and came up DRIVING. store.py claimed the staleness could only fail toward a SPURIOUS latch. It could not; that sentence is corrected rather than left standing. * THE GUARD RUNS ON EVERY START, and asks the STORE - not the roster - whether anything namespaced is latched. A mirrored trip on B must not re-arm a healthy A, and the store is the right place to ask because it holds every `unit/*/sign_tripped` this install has written, including one belonging to a unit that is not in today's roster. Graded by the three-step scenario itself, run end to end in test_inverter.py part 2, with all three preconditions asserted - including that the catching start did NOT migrate anything, which is what used to skip the guard. Part 2's idempotence check now PRESSES the button instead of hand-writing the store, because the state it used to construct is not one the shipped clear path can produce. Part 3 grades the mirror and the clear on a real roster of two. test_watchdog.py's "the flat key is NOT written" assertion is re-aimed rather than deleted: its argument was right about the READ and wrong about the WRITE. Nothing at runtime reads that key, and the one thing that does ignores it whenever any namespaced latch is set - which a sibling's trip always leaves. What its absence actually cost was the rollback. HANDOVER.md and DOCS.md said the flat key "is never read again", which was the instruction an operator clearing a trip by hand would follow. Both corrected: it is a maintained rollback view, it IS read on every start, and clearing a trip by hand means clearing both spellings. MUTATIONS THIS ROUND, each applied on a purged tree via tools/mutation_run.sh and observed RED: * per-unit `device`/`supervised` BOUNDS from `self.inverters[0]`, actuator left correct -> test_peakguard.py 10g * `_grade_store_migration()` gated on `rep.ran` -> test_inverter.py part 2 * the operator's clear leaving the flat keys set -> test_inverter.py part 2/3 * a trip not mirroring the flat pair -> test_inverter.py part 3 * the guard asking only `self.inverters[0]` -> test_inverter.py part 3 19/19 files green on the merged tree. No hardware was touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
201 lines
10 KiB
Markdown
201 lines
10 KiB
Markdown
# Handover — 2026-08-23
|
||
|
||
Start here. `README.md` is what the thing is; this is where it stands, what is
|
||
known to be broken, and what to do next.
|
||
|
||
---
|
||
|
||
## 1. State of the world
|
||
|
||
The add-on **is controlling a real battery right now** — the author's own house,
|
||
a GoodWe GW5000S-BP.
|
||
|
||
| | |
|
||
|---|---|
|
||
| running | `local_goodwe_controller` **0.4.0**, `boot: auto`, holding grid at zero |
|
||
| installed as | a **Local** add-on: `/addons/goodwe_controller` on HA OS at 192.168.2.158 |
|
||
| repo | Gitea `admin/goodwe-addon` (ssh://git@192.168.2.147:2222) — **private** |
|
||
| tuning | gain 0.6, `max_w` 5000, slew 1000, deadband 15, step 10, saturation 500/3, heartbeat 10 s |
|
||
| maintenance | **disabled**, and with **no completion history** — first cycle is "due" the moment it is enabled |
|
||
| e-stop | fitted at this site, Pi tap armed with `--panic` |
|
||
|
||
The thing it replaced — a working Home Assistant YAML implementation — is parked
|
||
at `/config/packages_disabled_addon_migration/` on that machine. **Rollback** is:
|
||
stop the add-on, move those four files back into `/config/packages/`, reload
|
||
automations and helpers, switch `input_boolean.goodwe_auto` on.
|
||
|
||
⚠️ **Never run both.** They write the same setpoint. Two controllers on one
|
||
actuator is the failure the whole design exists to prevent.
|
||
|
||
The research record — protocol, register map, why every tuning value is what it
|
||
is, and a long "ruled out, do not re-investigate" list — lives in the *other*
|
||
repo, `Documents/goodwe`, in `NOTES.md`. Read it before changing a constant.
|
||
|
||
---
|
||
|
||
## 2. Deploying a change
|
||
|
||
```bash
|
||
# 1. edit under goodwe_controller/
|
||
# 2. BUMP version: in goodwe_controller/config.yaml <-- not optional
|
||
scp -r goodwe_controller root@192.168.2.158:/addons/
|
||
ssh root@192.168.2.158 "ha store reload && ha addons update local_goodwe_controller"
|
||
ssh root@192.168.2.158 "ha addons logs local_goodwe_controller | tail -20"
|
||
```
|
||
|
||
⚠️ **Supervisor keys the built image by `version:`.** Without a bump it silently
|
||
reuses the old image and your change appears not to work. This cost two rounds of
|
||
"the fix doesn't work" in one evening. It applies to client updates too — a
|
||
client only gets a change when the version increments.
|
||
|
||
Run the checks before deploying. No framework needed:
|
||
|
||
```bash
|
||
cd goodwe_controller
|
||
python3 test_control.py && python3 test_maintenance.py && python3 test_arbiter.py \
|
||
&& python3 test_validate.py && python3 test_p1.py && python3 test_watchdog.py \
|
||
&& python3 test_loop.py
|
||
```
|
||
|
||
⚠️ **Mutating a module to prove a check bites? Read `goodwe_controller/
|
||
MUTATION.md` first, and run `goodwe_controller/tools/mutation_run.sh`.** A
|
||
same-size edit reverted inside one second changes neither the file size nor the
|
||
whole-second mtime, so CPython reuses the bytecode it already compiled and the
|
||
suite grades code that is not there - once already, reported as a survivor and
|
||
disproved by disassembly (TEST-15). `python3 -B` does **not** save you: it stops
|
||
bytecode being written, not a stale `.pyc` being read. The purge is the fix.
|
||
|
||
`test_watchdog.py` covers SAFETY-02's logic, its arming gate (derived from the
|
||
real arbiter, not passed in), its trip path, and the fact that the latch survives
|
||
an add-on restart. Its headline validation is not here at all: `sim/replay_sign.py`
|
||
in the infra repo drives the same module over the two captured RS485 days and must
|
||
fire on all four of the 2026-08-20 episodes and stay silent on 2026-08-23. That
|
||
needs both checkouts:
|
||
|
||
```bash
|
||
GOODWE_ADDON_REPO=/path/to/goodwe-addon python3 sim/replay_sign.py
|
||
GOODWE_ADDON_REPO=/path/to/goodwe-addon python3 sim/replay_sign.py --stub
|
||
```
|
||
|
||
The second run checks the grader still *discriminates* rather than merely still
|
||
passing — a watchdog that never trips and one that always trips must both be
|
||
rejected.
|
||
|
||
⚠️ **If you are testing the failsafe by hand, note that stopping and starting the
|
||
add-on will not clear a trip.** That is the point of it; use the *Start / stop
|
||
control* button twice, or delete the trip from `/data/state.json`.
|
||
|
||
⚠️ **DEV-02 renamed that key. There is one `SignWatchdog` per inverter, so the
|
||
LIVE latch lives under `unit/<inverter>/sign_tripped`** —
|
||
`unit/inverter-a/sign_tripped` on a single-inverter site. **That is the one to
|
||
delete if you are clearing a trip by hand**; deleting the flat one alone clears
|
||
nothing.
|
||
|
||
⚠️ **The flat `sign_tripped` is still maintained, though — it is the ROLLBACK
|
||
VIEW, not a fossil.** It is written alongside the namespaced key on a trip and
|
||
cleared alongside it when you press *Start / stop control*, so an add-on you
|
||
**downgrade** past this version still finds the latch it should. It is also read
|
||
on **every** start, and the one disagreement it can produce is acted on: *the
|
||
flat key says tripped and no inverter came up latched* means the latch was
|
||
written by an older add-on while this install was rolled back — or the migration
|
||
lost it — and the controller **re-arms the latch and shouts in the log** rather
|
||
than coming up disarmed with a green banner. So if you edit `/data/state.json`
|
||
by hand, clear **both** spellings or the next start will put the trip back.
|
||
|
||
⚠️ `test_loop.py` takes about **95 seconds** (measured 93 s) and that is not a bug. It
|
||
drives the real control loop, which sleeps 1 s a tick, eight times over, to prove a
|
||
wall-clock property about it (SAFETY-12); mocking the clock there would leave the
|
||
assertion testing the mock. The other six suites together are ~440 checks and finish in
|
||
seconds — `test_watchdog.py` spends ~3 s of that driving a real `Controller` through a
|
||
trip. If you shorten `test_loop.py`'s windows, re-measure and correct this figure —
|
||
an understated runtime is what makes a slow test look like a hung one, and a slow test
|
||
that looks hung gets deleted.
|
||
|
||
---
|
||
|
||
## 3. Known broken / unfinished
|
||
|
||
1. **MQTT status entities are not adopted by Home Assistant.** The add-on
|
||
publishes correct discovery and live state — verified on the broker itself
|
||
(`goodwe_ctl/setpoint => 940.0`). After the entities were deleted from the
|
||
registry during development, HA would not re-adopt them, even after clearing
|
||
the retained topics and reconnecting. Affects **observability only**; control
|
||
is untouched. Try again after an HA restart, which is the cheapest next test.
|
||
2. **The repository is private**, so it cannot be used as an add-on repository
|
||
URL — HA clones anonymously. Either make it public, or keep installing
|
||
locally as at this site. Decide before the first client.
|
||
3. **No real maintenance cycle has ever run.** The mechanism is proven
|
||
end-to-end in simulated time and partially on hardware, but a genuine
|
||
low→full→hold cycle takes hours and has not happened. Do not decommission a
|
||
client's vendor controller before one has.
|
||
4. **The HA-side offline alarm is unproven.** The Pi's 30 s panic write is the
|
||
real cover; treat the HA alert as "within a few minutes".
|
||
5. **Only platform 105 is supported** — see §5.
|
||
|
||
---
|
||
|
||
## 4. What to do next, in order
|
||
|
||
1. **Peak shaving as a real strategy.** This is why the arbiter exists, and it is
|
||
now purely additive: a claim producer emitting `Claim.set("peak", P_PEAK, …)`
|
||
from the quarter-hour forecast, plus a priority. Nothing existing needs to
|
||
change — the maintenance charge-only limit already cannot bind it. Highest
|
||
value per unit of work in a capacity-tariff market.
|
||
2. **Run a full maintenance cycle** at the reference site and record what it
|
||
actually costs in energy and time.
|
||
3. **Fleet health before more features.** This system's signature failure is
|
||
*silent*: an entity-id prefix killed two safety alarms for a day, a shebang
|
||
made every call 401 while looking healthy. One site with the author watching
|
||
found those. Twenty sites will not.
|
||
4. **Decide the repository question** (§3.2).
|
||
|
||
---
|
||
|
||
## 5. The commercial question nobody has answered
|
||
|
||
**A GoodWe "-20" / G2 inverter cannot be driven by this.**
|
||
|
||
Platform comes from characters 6–8 of the serial number. Only
|
||
`ESU EMU ESA BPS BPU EMJ IJL` — platform 105, the AA55-era meter bus — are
|
||
supported. SBP G2 (`SPB`/`SPN`) and ES G2 (`ESN`/`ESC`) are platform 745, a
|
||
different protocol. The reference unit is `BPS`.
|
||
|
||
That matters more than it first sounds: **the SBP G2 is the model GoodWe
|
||
currently sells to replace a GW5000S-BP.** So today's product serves the
|
||
installed base, not the base being installed. Everything above the firmware —
|
||
control law, arbiter, maintenance, add-on, field guide — is protocol-agnostic
|
||
and would carry over; only the register map is platform-specific.
|
||
|
||
Answering it is an afternoon: put the Pi tap passively on a G2 unit's meter bus
|
||
with its vendor controller running, and capture. That either hands over the new
|
||
register map or proves the approach does not transfer. **Do this before building
|
||
more features on top of a shrinking base.**
|
||
|
||
---
|
||
|
||
## 6. Things that will bite you
|
||
|
||
Each of these was found the expensive way. They are in the code comments too.
|
||
|
||
- **`run.sh` must start `#!/usr/bin/with-contenv sh`.** s6-overlay sanitises the
|
||
environment, so a plain shebang means no `SUPERVISOR_TOKEN` and every HA call
|
||
returns 401 — while `homeassistant_api: true` makes permissions look granted.
|
||
- **A wrong entity id is not an error anywhere in HA.** It just never produces a
|
||
value, and the automation's own state still reads `on`.
|
||
- **HA composes entity ids from the device area and device name.** The same
|
||
firmware yields `sensor.goodwe_master_…` at one site and
|
||
`sensor.cellar_goodwe_master_…` at another.
|
||
- **`input_datetime` states parse timezone-naive**; subtracting `now()` raises
|
||
and kills the whole automation before it records itself as triggered. Use the
|
||
`timestamp` attribute.
|
||
- **Alpine ships paho-mqtt 1.x**, which has no `CallbackAPIVersion`, and musl has
|
||
no aiohttp wheel — install from apk, not pip.
|
||
- **A mutation run can grade code that is not there.** CPython validates a
|
||
`.pyc` on (source size, source mtime in whole seconds), so a same-size edit
|
||
reverted within one second is invisible to it and the old bytecode runs. It
|
||
manufactures *survivors*, never reds. Purge `__pycache__` -
|
||
`tools/mutation_run.sh` does - and see `goodwe_controller/MUTATION.md`.
|
||
- **The inverter holds its last command forever.** It has no meter-timeout. Every
|
||
failsafe here exists because of that one fact, and anything in doubt commands
|
||
**0 W** rather than holding.
|