OPT-11 P2: HA entities for battery modes #152

Merged
admin merged 3 commits from opt11-p2-ha-entities into release/1.0 2026-09-29 00:13:50 +02:00
Owner

OPT-11 phase P2 (design §4 and §6 P2). Base: release/1.0 at ed739d4. This PR does not touch the allocator, arbiter or loop.

What it does

  • Entities. Each battery gets select Mode (options "Solar first", "Leftover solar", in packmode.MODES order) and switch Use (ON/OFF). HA composes select.goodwe_inverter_a_mode, switch.goodwe_inverter_a_use and so on, as the design names them. The unique_ids are stable per battery: goodwe_ctl_unit_<slug>_mode|use. The device rule is TEL-07's: the first battery's entities go on the site device with its name in front, and later batteries' go on their own device. At N=1 they go on the site device. They are announced last, only when battery_modes_enabled is true. P3 adds a mode as one MODE_LABELS entry and one option at the end, and no id changes (test 10b simulates this).
  • Removal. The config topics announced are stored in mqtt_mode_configs. When modes are off (or a battery leaves the roster), each stored topic gets an empty retained config. Store values that are not our mode config topics are ignored.
  • Commands. The add-on subscribes to goodwe_ctl/unit/+/mode/set and .../use/set only when modes are on. paho's thread hands (topic, bytes, retained) to the controller's loop. parse_command() checks the exact topic shape, a 64-byte cap, UTF-8 and the exact payload (ON/OFF, or an offered label), then Controller.handle_action() runs the same function and checks as /action. A refusal is logged at WARNING once per (topic, reason), at most 32 times, and at DEBUG after that.
  • Retained commands are never applied. A message the broker replays on subscribe has retain set, and it is dropped and logged once. The store already holds the last choice, and a replay could undo a newer one. The discovery payload also says "retain": false.
  • State topics goodwe_ctl/unit/<slug>/mode|use are published from the store every second and at once after a change, so they are right after a restart.
  • packmode.enabled(opts) is now the one predicate for the controller and the announcement.

Tests (test_mqtt.py part 10; full suite green locally in the CI image)

  • 10a: with modes off, all 36 configurations (6 rosters × omit × include) have the same bytes and order as ed739d4's _announce(). I generated digest 501a7d93782a260b from that commit's module, and there is no subscription.
  • 10b: ordered diffs with modes on. N=1: 0 missing, 0 changed, +2 (pinned bytes). N=2: 0 missing, 0 changed, +4, and the modes-off list is an unchanged prefix.
  • 10c: the empty retained configs come before availability; roster shrink; store filtering.
  • 10d: state topics = announced topics; defaults; false and absent publish the same keys as before.
  • 10e: good commands go through handle_action() to P1's keys, the state topic updates, and the action log records the change. 21 bad inputs are refused with the store unchanged: case, whitespace, a key instead of a label, the P3 mode, a Greek-omicron lookalike, NUL, JSON, shell and SQL-style, non-UTF-8, oversize, an unknown battery, .., extra levels and None. Log-once and the cap are also tested.
  • 10f: a retained command is not applied and never reaches handle_action(). After a restart the state comes from the store.

Mutations (all red)

  • In test (app/mqtt.py, 14): entities announced with modes off; subscribed with modes off; retained applied; no size cap; UTF-8 replaced; switch case-folded; select accepts keys; select accepts a labelled mode that is not offered; topic loosened; topic matched as a prefix; store list not filtered; current configs retracted; retain: true; retractions not published.
  • By hand (app/main.py, 11, 0 survivors): parse rejection ignored; unknown slug used as a name; log-once off; no cap; states with modes off; no publish after a change; publish() omits states; modes always announced; bind removed; switch state inverted; state read from the wrong battery.
  • The 12 TEL-07 mutations still run. One anchor moved into the shared helper unit_unique_id().

Left out

  • The goodwe_<inv>_plan text sensor (§4) has no content before P3.
  • Labels are EN only (HA MQTT options are not translated).

Risks for the reviewer

  • _unit_places() now holds TEL-07's device rule for both lists. With modes off the output is byte-identical (10a), but the refactor touches that code.
  • The Mode labels live in mqtt.py. If the page's EN labels change, HA options do not follow, which keeps the options stable. Automations use the label.
  • After modes go off, the stored topics are retracted on every start (idempotent) and never cleared.
  • Nothing was run against a real broker or HA. The fake-client tests cover the wire bytes only. A TEST-23 check is still needed: entities appear, commands apply, removal works, and a retained command is ignored across a restart.

🤖 Generated with Claude Code

https://claude.ai/code/session_01UBtqPrG1mDLXzTEBnhEkkV

OPT-11 phase P2 (design §4 and §6 P2). Base: release/1.0 at ed739d4. This PR does not touch the allocator, arbiter or loop. ## What it does - **Entities.** Each battery gets `select` *Mode* (options "Solar first", "Leftover solar", in `packmode.MODES` order) and `switch` *Use* (`ON`/`OFF`). HA composes `select.goodwe_inverter_a_mode`, `switch.goodwe_inverter_a_use` and so on, as the design names them. The unique_ids are stable per battery: `goodwe_ctl_unit_<slug>_mode|use`. The device rule is TEL-07's: the first battery's entities go on the site device with its name in front, and later batteries' go on their own device. At N=1 they go on the site device. They are announced last, only when `battery_modes_enabled` is true. P3 adds a mode as one `MODE_LABELS` entry and one option at the end, and no id changes (test 10b simulates this). - **Removal.** The config topics announced are stored in `mqtt_mode_configs`. When modes are off (or a battery leaves the roster), each stored topic gets an empty retained config. Store values that are not our mode config topics are ignored. - **Commands.** The add-on subscribes to `goodwe_ctl/unit/+/mode/set` and `.../use/set` only when modes are on. paho's thread hands `(topic, bytes, retained)` to the controller's loop. `parse_command()` checks the exact topic shape, a 64-byte cap, UTF-8 and the exact payload (`ON`/`OFF`, or an offered label), then `Controller.handle_action()` runs the same function and checks as `/action`. A refusal is logged at WARNING once per (topic, reason), at most 32 times, and at DEBUG after that. - **Retained commands** are never applied. A message the broker replays on subscribe has `retain` set, and it is dropped and logged once. The store already holds the last choice, and a replay could undo a newer one. The discovery payload also says `"retain": false`. - **State topics** `goodwe_ctl/unit/<slug>/mode|use` are published from the store every second and at once after a change, so they are right after a restart. - `packmode.enabled(opts)` is now the one predicate for the controller and the announcement. ## Tests (test_mqtt.py part 10; full suite green locally in the CI image) - 10a: with modes off, all 36 configurations (6 rosters × omit × include) have the same bytes and order as **ed739d4's** `_announce()`. I generated digest `501a7d93782a260b` from that commit's module, and there is no subscription. - 10b: ordered diffs with modes on. N=1: 0 missing, 0 changed, +2 (pinned bytes). N=2: 0 missing, 0 changed, +4, and the modes-off list is an unchanged prefix. - 10c: the empty retained configs come before availability; roster shrink; store filtering. - 10d: state topics = announced topics; defaults; `false` and absent publish the same keys as before. - 10e: good commands go through `handle_action()` to P1's keys, the state topic updates, and the action log records the change. 21 bad inputs are refused with the store unchanged: case, whitespace, a key instead of a label, the P3 mode, a Greek-omicron lookalike, NUL, JSON, shell and SQL-style, non-UTF-8, oversize, an unknown battery, `..`, extra levels and None. Log-once and the cap are also tested. - 10f: a retained command is not applied and never reaches `handle_action()`. After a restart the state comes from the store. ## Mutations (all red) - **In test (app/mqtt.py, 14):** entities announced with modes off; subscribed with modes off; retained applied; no size cap; UTF-8 replaced; switch case-folded; select accepts keys; select accepts a labelled mode that is not offered; topic loosened; topic matched as a prefix; store list not filtered; current configs retracted; `retain: true`; retractions not published. - **By hand (app/main.py, 11, 0 survivors):** parse rejection ignored; unknown slug used as a name; log-once off; no cap; states with modes off; no publish after a change; publish() omits states; modes always announced; bind removed; switch state inverted; state read from the wrong battery. - The 12 TEL-07 mutations still run. One anchor moved into the shared helper `unit_unique_id()`. ## Left out - The `goodwe_<inv>_plan` text sensor (§4) has no content before P3. - Labels are EN only (HA MQTT options are not translated). ## Risks for the reviewer - `_unit_places()` now holds TEL-07's device rule for both lists. With modes off the output is byte-identical (10a), but the refactor touches that code. - The *Mode* labels live in `mqtt.py`. If the page's EN labels change, HA options do not follow, which keeps the options stable. Automations use the label. - After modes go off, the stored topics are retracted on every start (idempotent) and never cleared. - Nothing was run against a real broker or HA. The fake-client tests cover the wire bytes only. A TEST-23 check is still needed: entities appear, commands apply, removal works, and a retained command is ignored across a restart. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01UBtqPrG1mDLXzTEBnhEkkV
admin added 1 commit 2026-09-28 23:12:32 +02:00
OPT-11 P2: HA entities for battery modes
checks / unit (pull_request) Successful in 11m12s
infra-pin / fresh (pull_request) Successful in 2s
b310282729
Each battery gets an MQTT-discovered Mode select and Use switch, only while
battery_modes_enabled is true. Commands go through handle_action(), the
/action path; refusals are logged once. Retained commands are never applied.
Turning modes off removes exactly the configs announced (kept in the store).
With modes off the announcement is byte-identical to ed739d4.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
admin added 2 commits 2026-09-28 23:58:49 +02:00
- A refusal past the log-once set or the 32 cap is counted in the log at
  1, 10, 100 and so on, so a repeated attack stays visible.
- The logged topic is printable only (no forged log lines).
- mode_announcement(): the start-up store logic, now tested with a real
  Store. A removed config topic stays stored, so a start with no broker
  cannot forget it (rename case tested).
- A test ties each HA mode option to the page's English name.
- packmode.enabled() is tested for "true", 1 and "false".
- Retained-command text, DOCS and DECISIONS updated to match.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge release/1.0 (7995e4d, UX-13 P3) into opt11-p2-ha-entities
checks / unit (pull_request) Successful in 11m29s
infra-pin / fresh (pull_request) Successful in 2s
308adc9277
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Author
Owner

Review of PR #152 (OPT-11 P2): findings, and fixes pushed as c664e38 (merged with release/1.0 7995e4d as 308adc9)

release/1.0 moved to 7995e4d (#150, UX-13 P3) during the review. I merged it into the branch the normal way. There were no conflicts, and the mode names in en.json are unchanged.

1. One command path: OK. parse_command() can only produce pack_use or pack_mode (the regex group picks one of the two). mqtt_command() calls only parse_command, handle_action, mode_values and mqtt.publish. handle_action sends both actions to _pack_action(), the same function /action reaches through web.py, with the same roster, bool and MODES checks. The only side effects are store_set, log_event and _maint_stop_off (a maintenance abort, as on the page). mqtt.py imports only asyncio and re in addition. Nothing reaches the allocator, arbiter, loop or a HA service call.

2. Retained and replayed commands: OK, with one refutation.

  • paho's default is clean_session=True with QoS 0 subscriptions, so the broker queues nothing while we are offline. The only replay is the retained one, which arrives with retain=1 and is dropped in parse_command() before handle_action(). On an established subscription the broker delivers with retain=0 (MQTT 3.1.1 §3.3.1.3), so a live command still works.
  • Race: Store() is loaded at the top of amain(), before the publisher exists. A message that arrives before bind() (the retained burst right after subscribe usually lands here) is dropped at DEBUG. After bind() the handler runs on the controller's loop, as /action does.
  • Stale state (refuted as a defect): state topics are not retained. After an add-on restart HA shows its last value until the first publish(), one control cycle in (a few seconds). That value came from the same store, so they differ only if state.json was lost. I accept that window and made no change.

3. Feature-off identity: OK, regenerated, and the harness bites. I loaded git show ed739d4:goodwe_controller/app/mqtt.py as its own module and ran the same 36-configuration matrix through its _announce(). It gave 501a7d93782a260b, and the head gave the same, with no subscriptions. Five payload mutants each move the digest: a site sensor icon gives bf7693b2…, a unit sensor icon 6d60b79e…, a later unit's name prefix 355159a1…, the second unit on the site device 9641ff0f…, and mode rows announced with modes off f3a6616d….

4. Shared placement helper: OK. The digest covers all six rosters, including the colliding and non-ASCII ones, so the existing sensors are byte-identical. The unique_ids are a pure function of the names, the same on every restart, and do not change when the roster order changes if the names are distinct. Inherited limit (follow-up, not this PR): two names that make the same slug (inverter-b and Inverter B) get _2 by roster index. Swapping their order swaps which battery select.…_inverter_b_2_mode controls. For a sensor that is only confusing, but for a control it matters. I suggest a validate.py rule that refuses names whose slugs collide.

5. Removal: FIXED. It removes exactly what was announced (10c). Sending the removals again on every start is harmless: an empty retained publish to a topic with no retained message changes nothing, and HA ignores an empty config for an entity it does not know. The list does not grow: it holds at most every (battery, key) pair ever announced. Defect: with modes on, amain() overwrote the list with only the current topics before hass.mqtt_service(). So after a rename or a smaller roster, a start with no broker (or no MQTT service) lost the removal for good, and the entity was orphaned. Fix: mqtt.mode_configs() keeps pending removals in the list, and main.mode_announcement() holds the start-up logic. It is tested with a real Store (never on, on, shrink, off, and B renamed to C). An install that never turned modes on still gets no store write.

6. Inputs: FIXED (two defects). The 64-byte cap, UTF-8, the exact ON/OFF and the offered labels are all correct (21 bad inputs, plus my own case-insensitive and 640-byte mutants, all caught).

  • Log-once hid a repeated attack. After 32 (topic, reason) pairs, and for any repeat, nothing was logged above DEBUG. Now each refusal that is not logged is counted, with a WARNING at 1, 10, 100 and so on: "Home Assistant commands still ignored: 100 more so far." It is tested for the 100-topic flood and for one topic hit 100 times.
  • Log injection. The logged topic was raw. A topic that fails the regex could carry \n or ESC and forge a log line. It is now printable-only, and tested.

7. Label drift: separate sources kept (refuted), with a test tying them together. A HA select option is what automations send, so one shared source would let a copy edit on the page silently break users' automations. A new test in 10b requires each MODE_LABELS entry to equal both English names on the page (home_words.modes and home.batteries.choice.*.name). A rename then goes red, and the author must choose on purpose. DECISIONS says so.

8. Mutations. The dev's 14 in app/mqtt.py are all red (automated, run again on the final code). By hand, from the dev's main.py list: unknown slug used as a name, log-once off, state from the wrong battery, no publish after a change. All 4 are caught. My 12 were: paho passes retained=False, retained sent to handle_action, select case-insensitive, cap 32→64, count logged once only, topic not sanitised, states with modes off, removal forgotten, store written on a never-on install, store keeps only the current list, 64→640 bytes, and enabled() truthy. Three survived at first (store written on a never-on install, store keeps only the current list, enabled() truthy). Each now has a test, and all 12 are caught.

9. Records: OK after edits. DOCS now says later refusals are counted. DECISIONS records the kept removals and the label rule in 4 lines. The retained text was "A saved command is not used after a restart.", which is also logged on a broker reconnect, so it now reads "An old saved command is not used." The other new texts are short and plain, and test_messages passes. The PR's claims match what ran. It honestly states that nothing ran against a real broker. The TEST-23 check listed in the PR is still needed after the merge.

Full suite on 308adc9 in the CI image, run locally: 48/48 test files pass (test_freeze without the pinned sim checkout; CI has it). CI on 308adc9: checks / unit success, infra-pin / fresh success. Merging with head_commit_id pinned.

## Review of PR #152 (OPT-11 P2): findings, and fixes pushed as c664e38 (merged with release/1.0 7995e4d as 308adc9) release/1.0 moved to 7995e4d (#150, UX-13 P3) during the review. I merged it into the branch the normal way. There were no conflicts, and the mode names in `en.json` are unchanged. **1. One command path: OK.** `parse_command()` can only produce `pack_use` or `pack_mode` (the regex group picks one of the two). `mqtt_command()` calls only `parse_command`, `handle_action`, `mode_values` and `mqtt.publish`. `handle_action` sends both actions to `_pack_action()`, the same function `/action` reaches through `web.py`, with the same roster, bool and `MODES` checks. The only side effects are `store_set`, `log_event` and `_maint_stop_off` (a maintenance abort, as on the page). `mqtt.py` imports only `asyncio` and `re` in addition. Nothing reaches the allocator, arbiter, loop or a HA service call. **2. Retained and replayed commands: OK, with one refutation.** - paho's default is `clean_session=True` with QoS 0 subscriptions, so the broker queues nothing while we are offline. The only replay is the retained one, which arrives with `retain=1` and is dropped in `parse_command()` before `handle_action()`. On an established subscription the broker delivers with `retain=0` (MQTT 3.1.1 §3.3.1.3), so a live command still works. - Race: `Store()` is loaded at the top of `amain()`, before the publisher exists. A message that arrives before `bind()` (the retained burst right after subscribe usually lands here) is dropped at DEBUG. After `bind()` the handler runs on the controller's loop, as `/action` does. - Stale state (refuted as a defect): state topics are not retained. After an add-on restart HA shows its last value until the first `publish()`, one control cycle in (a few seconds). That value came from the same store, so they differ only if `state.json` was lost. I accept that window and made no change. **3. Feature-off identity: OK, regenerated, and the harness bites.** I loaded `git show ed739d4:goodwe_controller/app/mqtt.py` as its own module and ran the same 36-configuration matrix through its `_announce()`. It gave `501a7d93782a260b`, and the head gave the same, with no subscriptions. Five payload mutants each move the digest: a site sensor icon gives `bf7693b2…`, a unit sensor icon `6d60b79e…`, a later unit's name prefix `355159a1…`, the second unit on the site device `9641ff0f…`, and mode rows announced with modes off `f3a6616d…`. **4. Shared placement helper: OK.** The digest covers all six rosters, including the colliding and non-ASCII ones, so the existing sensors are byte-identical. The unique_ids are a pure function of the names, the same on every restart, and do not change when the roster order changes if the names are distinct. **Inherited limit (follow-up, not this PR):** two names that make the same slug (`inverter-b` and `Inverter B`) get `_2` by roster index. Swapping their order swaps which battery `select.…_inverter_b_2_mode` controls. For a sensor that is only confusing, but for a control it matters. I suggest a validate.py rule that refuses names whose slugs collide. **5. Removal: FIXED.** It removes exactly what was announced (10c). Sending the removals again on every start is harmless: an empty retained publish to a topic with no retained message changes nothing, and HA ignores an empty config for an entity it does not know. The list does not grow: it holds at most every (battery, key) pair ever announced. **Defect:** with modes on, `amain()` overwrote the list with only the current topics *before* `hass.mqtt_service()`. So after a rename or a smaller roster, a start with no broker (or no MQTT service) lost the removal for good, and the entity was orphaned. Fix: `mqtt.mode_configs()` keeps pending removals in the list, and `main.mode_announcement()` holds the start-up logic. It is tested with a real Store (never on, on, shrink, off, and B renamed to C). An install that never turned modes on still gets no store write. **6. Inputs: FIXED (two defects).** The 64-byte cap, UTF-8, the exact `ON`/`OFF` and the offered labels are all correct (21 bad inputs, plus my own case-insensitive and 640-byte mutants, all caught). - **Log-once hid a repeated attack.** After 32 (topic, reason) pairs, and for any repeat, nothing was logged above DEBUG. Now each refusal that is not logged is counted, with a WARNING at 1, 10, 100 and so on: *"Home Assistant commands still ignored: 100 more so far."* It is tested for the 100-topic flood and for one topic hit 100 times. - **Log injection.** The logged topic was raw. A topic that fails the regex could carry `\n` or ESC and forge a log line. It is now printable-only, and tested. **7. Label drift: separate sources kept (refuted), with a test tying them together.** A HA select option is what automations send, so one shared source would let a copy edit on the page silently break users' automations. A new test in 10b requires each `MODE_LABELS` entry to equal both English names on the page (`home_words.modes` and `home.batteries.choice.*.name`). A rename then goes red, and the author must choose on purpose. DECISIONS says so. **8. Mutations.** The dev's 14 in `app/mqtt.py` are all red (automated, run again on the final code). By hand, from the dev's `main.py` list: unknown slug used as a name, log-once off, state from the wrong battery, no publish after a change. All 4 are caught. My 12 were: paho passes retained=False, retained sent to handle_action, select case-insensitive, cap 32→64, count logged once only, topic not sanitised, states with modes off, removal forgotten, store written on a never-on install, store keeps only the current list, 64→640 bytes, and `enabled()` truthy. Three survived at first (store written on a never-on install, store keeps only the current list, `enabled()` truthy). Each now has a test, and all 12 are caught. **9. Records: OK after edits.** DOCS now says later refusals are counted. DECISIONS records the kept removals and the label rule in 4 lines. The retained text was "A saved command is not used after a restart.", which is also logged on a broker reconnect, so it now reads "An old saved command is not used." The other new texts are short and plain, and test_messages passes. The PR's claims match what ran. It honestly states that nothing ran against a real broker. The TEST-23 check listed in the PR is still needed after the merge. Full suite on 308adc9 in the CI image, run locally: 48/48 test files pass (test_freeze without the pinned sim checkout; CI has it). CI on 308adc9: `checks / unit` success, `infra-pin / fresh` success. Merging with `head_commit_id` pinned.
admin merged commit 7337b791e5 into release/1.0 2026-09-29 00:13:50 +02:00
Sign in to join this conversation.
No Reviewers
No labels
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: admin/goodwe-addon#152