fix(sungrow): stop startup asking a string inverter about its battery limits - #44
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d10fb93349
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if known_to_have_no_battery() then | ||
| return true |
There was a problem hiding this comment.
When the FTW control-v2 target is used with an SG string inverter, this catalog-only guard leaves packages/v1/sungrow/targets/ftw.lua inconsistent: its curtail path still writes 13050/13049 and driver_default_mode_v2 still writes and reads 13049, even though this change establishes that the family has no EMS block. Consequently that target will fail curtail/default operations or continue sending absent-register traffic. Update the target in the same change—by aligning its curtail implementation with the PV registers or rejecting the unsupported operation—rather than leaving the separate copy behind.
AGENTS.md reference: AGENTS.md:L27-L30
Useful? React with 👍 / 👎.
|
Scheduled review pass: this PR's lifecycle-guard logic looks sound and doesn't collide with anything else already on
To land this, it needs: a rebase (or merge) onto current Separately, worth flagging per I haven't rebased/pushed a fix myself since I'm restricted to my own working branch this session — flagging for a maintainer (or a follow-up session) to rebase and re-version. Generated by Claude Code |
… limits configure_power_limits reads 33046, 33047 and 13057 -- charge power, discharge power, and the SoC ceiling and floor -- and raises the first two when the unit shipped capped. A model that named itself a string inverter answers none of them, so an SG12RT paid three failed reads on every restart to learn nothing. These run once rather than on every poll, so this is not the SG12RT outage returning. The reason to gate them is that they are free to gate: each write is already conditional on its read having answered, and none of them clears a forced state. That last clause is the whole boundary, and getting it wrong is most of what this commit learned. The first draft gated set_self_consumption on the same family test, covering driver_init's startup reset, the watchdog, driver_cleanup and deinit -- three writes at startup and three on every watchdog tick, aimed at a block the family has no use for. That is wrong as of #42. Since 1.5.4 a zero-watt battery command is accepted whatever the family, and it writes EMS mode 2. So "the device named itself a string inverter" is not proof there is nothing to release: Sungrow shipping a hybrid under a device-type code classify_device_type has not been taught lands in exactly that branch, takes the forced write, and is then left with no path that writes mode 0 back. Measured against the combination: mode 2 survived the watchdog and survived cleanup. The device would have been stranded forced, which is the outage #42 exists to prevent, arriving through a different door. So the gate covers configure_power_limits and nothing else, and test_the_release_is_still_reachable_on_a_string_inverter fails if it is put back on the release. The comment on known_to_have_no_battery says why it must stay narrow. Two failed reads remain at startup, both at 13049 and both deliberate: set_self_consumption's readback, which must run on every family, and the EMS-state log, which is the one place a device misclassified as "string" while holding mode 2 would show itself. The test pins the number and points at the reason rather than letting someone tidy it to zero. Neither package target changes. packages/v1/sungrow/targets/ftw.lua maps curtail onto 13050 and 13049 rather than the Active Power Limitation pair, so its driver_default_mode_v2 is the release for a curtail it accepts on a string inverter -- the same reasoning, and that register map belongs to the review packages/v1/sungrow/PILOT.md already requires. packages/v1/sungrow/targets/ftw-observe.lua is read-only by construction. The SG-RT and SG-CX manifest notes named 1.2.1, which stopped the reads, and said nothing about the writes. They now name 1.5.3, which refuses a battery dispatch on those families, and the exception 1.5.4 made for zero. min_driver_version moves to 1.5.3. make check -- 3112 passed, 1361 skipped. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d10fb93 to
e21eb38
Compare
frahlg
changed the title
|
The rewrite ( Verified locally: Merging as Generated by Claude Code |
1.5.4 exempted zero from the guard #40 added, on three grounds. None of them holds. "Zero hands the device back to itself: forced mode off." It does not. set_battery_idle writes 13049 = 2 -- forced mode on -- and pins the battery at 0 W under the host's control. Its own comment two lines above says so: hold at 0 W *without* handing control back. The write that returns an inverter to self-consumption is mode 0, in driver_default_mode. FTW's two tests distinguish them: default mode asserts 13049 == 0, the zero command asserts 13049 == 2. #44 states the same fact from the other side, in the reason it gives for not gating set_self_consumption. "It arrives from the lifecycle rather than the planner." Not on FTW. The only producer of {"action":"battery","power_w":0} is the dispatch loop in go/cmd/ftw/main.go -- planner targets, or the self-tune override. Every path that has to release a device without waiting for a poll goes through Registry.SendDefault to driver_default_mode: shutdown, lease expiry, the telemetry watchdog, the stale-site-meter standdown. That entrypoint is not gated. driver_init writes self-consumption as well, before the run loop exists to accept a command. "On a string inverter the write fails at the Modbus layer and costs nothing." The incident says otherwise. first_write_error returns false if any of the three writes errors, so had the SG12RT rejected them the driver would have stopped there and reported failure. It went on to the read-back and reported success instead -- which is only reachable if the writes were taken. Some firmware accepts a write to a register it does not implement. So refusing withholds nothing that is not reachable by a route which cannot refuse, and the exemption put mode 2 and a setpoint back on an SG inverter that implements neither -- with the read-back that would catch it failing on that same device. Under 1.5.4 a zero command could also put an SG inverter *into* forced mode with no battery command able to take it out again, since non-zero stays refused. Both control paths move together: sungrow 1.5.5 -> 1.5.6, FTW v2 target 1.3.4 -> 1.3.5. Four pytest cases carry the reasoning. Two are load-bearing and both are proven to fail when the behaviour is put back: the release is never gated, on either copy, and driver_init reaches self-consumption on its own. Those are what make refusing safe; gate either and the argument collapses. #44 keeps its conclusion and loses its premise. Not gating set_self_consumption is still right -- an inverter can arrive already holding mode 2, from a container that died mid-force or an installer app, and a hybrid shipped under an unrecognised device-type code lands in the "string" branch too. Its test now seeds that state instead of creating it with a zero command, and covers driver_init as well. Keeps 1.5.4's host_mock fix, which was right and independent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1.5.4 exempted zero from the guard #40 added, on three grounds. None of them holds. "Zero hands the device back to itself: forced mode off." It does not. set_battery_idle writes 13049 = 2 -- forced mode on -- and pins the battery at 0 W under the host's control. Its own comment two lines above says so: hold at 0 W *without* handing control back. The write that returns an inverter to self-consumption is mode 0, in driver_default_mode. FTW's two tests distinguish them: default mode asserts 13049 == 0, the zero command asserts 13049 == 2. #44 states the same fact from the other side, in the reason it gives for not gating set_self_consumption. "It arrives from the lifecycle rather than the planner." Not on FTW. The only producer of {"action":"battery","power_w":0} is the dispatch loop in go/cmd/ftw/main.go -- planner targets, or the self-tune override. Every path that has to release a device without waiting for a poll goes through Registry.SendDefault to driver_default_mode: shutdown, lease expiry, the telemetry watchdog, the stale-site-meter standdown. That entrypoint is not gated. driver_init writes self-consumption as well, before the run loop exists to accept a command. "On a string inverter the write fails at the Modbus layer and costs nothing." The incident says otherwise. first_write_error returns false if any of the three writes errors, so had the SG12RT rejected them the driver would have stopped there and reported failure. It went on to the read-back and reported success instead -- which is only reachable if the writes were taken. Some firmware accepts a write to a register it does not implement. So refusing withholds nothing that is not reachable by a route which cannot refuse, and the exemption put mode 2 and a setpoint back on an SG inverter that implements neither -- with the read-back that would catch it failing on that same device. Under 1.5.4 a zero command could also put an SG inverter *into* forced mode with no battery command able to take it out again, since non-zero stays refused. Both control paths move together: sungrow 1.5.5 -> 1.5.6, FTW v2 target 1.3.4 -> 1.3.5. Four pytest cases carry the reasoning. Two are load-bearing and both are proven to fail when the behaviour is put back: the release is never gated, on either copy, and driver_init reaches self-consumption on its own. Those are what make refusing safe; gate either and the argument collapses. #44 keeps its conclusion and loses its premise. Not gating set_self_consumption is still right -- an inverter can arrive already holding mode 2, from a container that died mid-force or an installer app, and a hybrid shipped under an unrecognised device-type code lands in the "string" branch too. Its test now seeds that state instead of creating it with a zero command, and covers driver_init as well. Keeps 1.5.4's host_mock fix, which was right and independent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Reverts the behaviour change in #42. set_battery_idle writes EMS mode 2 (forced), not a release: it pins the battery at zero under the host's control rather than releasing it. The release is driver_default_mode, which FTW reaches through SendDefault on shutdown, lease expiry and the watchdog, never through driver_command, and which no guard touches. Refusing zero before either the family or the battery is confirmed withholds nothing that is not reachable by a route that cannot refuse -- while accepting put mode 2 and a setpoint back on an SG inverter that implements neither, reporting success, because the read-back that would catch it fails on that device too. Rebases the original PR (#45 by @frahlg) onto main so it lands on top of #44's already-merged startup gate instead of reimplementing it, and bumps the FTW v2 target's own version field (1.3.4 -> 1.3.5), which #45 left unbumped despite changing that file's driver_command_v2 logic too. sungrow 1.5.5 -> 1.5.6, FTW v2 target 1.3.4 -> 1.3.5. make check: 3110 passed, 1361 skipped. Co-authored-by: frahlg <6883838+frahlg@users.noreply.github.com> Signed-off-by: Claude <noreply@anthropic.com>
Reverts the behaviour change in #42. set_battery_idle writes EMS mode 2 (forced), not a release: it pins the battery at zero under the host's control rather than releasing it. The release is driver_default_mode, which FTW reaches through SendDefault on shutdown, lease expiry and the watchdog, never through driver_command, and which no guard touches. Refusing zero before either the family or the battery is confirmed withholds nothing that is not reachable by a route that cannot refuse -- while accepting put mode 2 and a setpoint back on an SG inverter that implements neither, reporting success, because the read-back that would catch it fails on that device too. Rebases the original PR (#45 by @frahlg) onto main so it lands on top of #44's already-merged startup gate instead of reimplementing it, and bumps the FTW v2 target's own version field (1.3.4 -> 1.3.5), which #45 left unbumped despite changing that file's driver_command_v2 logic too. sungrow 1.5.5 -> 1.5.6, FTW v2 target 1.3.4 -> 1.3.5. make check: 3110 passed, 1361 skipped. Signed-off-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: frahlg <6883838+frahlg@users.noreply.github.com>
1 participant
Rewritten after #42 landed. The original version of this PR gated the release path, and #42 makes that unsafe — the story is below, because the reasoning is the useful part.
What lands
configure_power_limitsreads 33046, 33047 and 13057 — charge power, discharge power, and the SoC ceiling and floor — and raises the first two when the unit shipped capped. A model that named itself a string inverter answers none of them, so an SG12RT paid three failed reads on every restart to learn nothing. It now skips them.These run once rather than on every poll, so this is not the SG12RT outage returning. The reason to gate them is that they are free to gate: each write is already conditional on its read having answered, and none of them clears a forced state.
The version that was wrong
The first draft gated
set_self_consumptionon the same family test, coveringdriver_init's startup reset, the watchdog,driver_cleanupanddeinit— three writes at startup and three on every watchdog tick.#42 makes that unsafe. Since 1.5.4 a zero-watt battery command is accepted whatever the family, and it writes EMS mode 2. So "the device named itself a string inverter" is not proof there is nothing to release: Sungrow shipping a hybrid under a device-type code
classify_device_typehas not been taught lands in exactly that branch, takes the forced write, and is then left with no path that writes mode 0 back.Measured against the combination:
driver_command("battery", 0)The device would have been stranded forced — the outage #42 exists to prevent, arriving through a different door.
So the gate covers
configure_power_limitsand nothing else.test_the_release_is_still_reachable_on_a_string_inverterfails if it is put back on the release, and the comment onknown_to_have_no_batterysays why it must stay narrow.The two failed reads that remain
Both at 13049, both deliberate:
set_self_consumption's readback, which must run on every family, and the startup EMS-state log — the one place a device misclassified asstringwhile holding mode 2 would show itself. The test pins the number and points at the reason, rather than leaving it looking like something to tidy to zero.Tests
Three in
drivers/tests/test_sungrow_model_family.py. Both directions verified by breaking the driver on purpose:set_self_consumption→ the boundary test failsPackage targets
Neither changes.
packages/v1/sungrow/targets/ftw.luamapscurtailonto 13050 and 13049 rather than the Active Power Limitation pair, so itsdriver_default_mode_v2is the release for a curtail it accepts on a string inverter — the same reasoning, and that register map belongs to the reviewpackages/v1/sungrow/PILOT.mdalready requires.packages/v1/sungrow/targets/ftw-observe.luais read-only by construction.Manifest
The SG-RT and SG-CX notes named 1.2.1, which stopped the reads, and said nothing about the writes. They now name 1.5.3, which refuses a battery dispatch on those families, and the exception 1.5.4 made for zero.
min_driver_versionmoves to 1.5.3.Checks
make check— 3112 passed, 1361 skipped.🤖 Generated with Claude Code