Files
goodwe-addon/HANDOVER.md
glenn schrooyenandClaude Opus 5 cc4e88490b SAFETY-02 review: a latch that a restart clears is not a latch
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
2026-08-28 22:33:15 +02:00

8.2 KiB
Raw Permalink Blame History

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

# 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:

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:

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.