glenn schrooyenandClaude Opus 5
cc4e88490b
BLOCKER. `auto` was persisted and `sign_wd.tripped` was not, so reconstructing the Controller against the same store - which is what a container restart does - brought it back ARMED, into the same fault, unattended: tripped=False, banner back to NOT READY, and it resumed commanding -1000 then -2000 W before re-tripping 180 s later. "Restart the add-on" is the first thing an operator looking at a FAILSAFE banner and a 0 W inverter will try, and under a supervisor that restarts a crashed add-on it is a loop of three-minute wrong-direction bursts. DOCS.md's "it does not clear itself" was false across a restart. `sign_tripped` and `sign_reason` are Store keys now, beside the maintenance state and for the reason store.py's own docstring gives. Written the moment the trip happens - reason first, flag second, so a crash between the two can never persist a latch with no explanation - restored in Controller.__init__, cleared only by the operator's start. The regression is covered: reverting the restore fails seven checks in test_watchdog.py. Review follow-ups, no behaviour change: - The arming gate is tested against the REAL arbiter. It is the decision the ticket turns on and it was asserted in three comment blocks and verified by nothing - every case passed `following` in as a literal. Twelve claim configurations now derive it from resolve(), including the redirect case `winner == "loop"` alone would have got wrong and the `lo` limit that raises magnitude without flipping sign. - The MQTT `status: "failsafe"` value is tested. The check that claimed to cover it restated the line above it and never touched the payload. - The worked example described a config that does not ship: at the defaults maintenance_charge_w 2500 against max_w 2000 is an arbiter contradiction and 0 W, not a -2500 W charge. Rewritten at 1500 W. The contradiction is a real pre-existing bug - a default cheap-window maintenance charge silently never happens - filed separately and deliberately not fixed here. - Forward hazard recorded at the gate: `winner == "loop"` disarms the watchdog under any higher-priority SET, and a peak-shaving strategy at P_PEAK IS grid-following in sign, so it would blind this for every peak event. - disagrees() is public so the replay harness stops carrying its own copy of the predicate it grades. - held_s takes the clock instead of reading one. - The two limits are now each stated in both places rather than one each. - The no-false-positive argument is tied to control.py's freeze permitting the command to cross zero; under the shrink-only encoding control.py records as rejected, this would false-trip on a load step. - Measured figures corrected: 93.4 % / 4212 of 17280, per-cluster 0.3/0.9/10.5/0.0, and the near-circularity of those figures is now stated rather than leaned on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa
171 lines
8.2 KiB
Markdown
171 lines
8.2 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
|
||
```
|
||
|
||
`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 `sign_tripped` from `/data/state.json`.
|
||
|
||
⚠️ `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.
|
||
- **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.
|