glenn schrooyenandClaude Opus 5
aae74d7571
Review fixes. A. test_loop.py takes 73 s, not the ~35 s claimed in three places. That number is the whole defence against a future developer deleting a slow test, so it was understated by 2x in exactly the place it matters. Corrected - and it is now ~95 s (measured 93 s), because this commit adds the scenario below rather than trimming windows to make an inconvenient number smaller. Both call sites carry an instruction to re-measure if the windows change. B. "242 checks" was test_p1's own count quoted as the total for all five suites. The real figure is 395 across the five, plus 14 here. And the finding this PR should have led with. stale_input_s and the `inputs missing -> 0 W` claim both live INSIDE cycle(), so pre-fix, on a meter whose value happened to repeat, that claim could not fire at all. Constant meter, SoC entity dying at t=4 s, one binary: pre-fix final target 220.0 W, reason 'loop -> 220 W' post-fix final target 0.0 W, reason 'safety -> 0 W' The old controller held 220 W through the whole input outage with its status line still reading loop -> 220 W, and layer 1 does not cover that on a `meter_source: off` install - no P1 age sensor is published, so the firmware has nothing to veto on, while the heartbeat keeps its watchdog fed. The pre-fix behaviour was not "holds a stale command safely"; it was a failsafe a repeating meter switched off. Asserted in test_loop.py, both halves. Three points of reasoning tightened, none of them wrong before: - the re-tune is unreachable by construction, not merely by the reset: the combined cycle rate is max(arrival rate, 1/floor), never the sum, and 1/15 s is strictly slower than the ~1/5 s arrival rate. That holds only while the floor exceeds the meter's update interval - and the schema minimum of 5 s is exactly the reference meter's interval. Flagged for validate.py, not fixed here. - the ~11 s lower bracket is not only a dropped poll: a 5.0 s poller against a 4.97 s meter beats, so about every 14 minutes the poller steps over an update entirely. Reaching 15 s takes a beat-skip AND a failed poll. - the "freshness is decided upstream" guarantee is transport-conditional. On `meter_source: off` the grid figure comes straight from hass.number() with no age gate. Running the full law is still right; the claim is narrower than it was stated. The 15 s default is not independently justified - it rests on the firmware's 30 s ingest veto and moves if that ever does. Said beside the constant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Du77usMj8XNKNFZGmUiWDa
149 lines
7.1 KiB
Markdown
149 lines
7.1 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_loop.py
|
||
```
|
||
|
||
⚠️ `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 five suites together are ~395 checks and finish in
|
||
seconds. 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.
|