fix(sungrow): restore the zero-power refusal that 1.5.4 removed - #45
Conversation
driver_command_v2's zero-power exemption came out in the same commit that removed it from the catalog driver, but the target's own version field stayed at 1.3.4. That is the exact drift this repo has been bitten by before: a package target's version and behavior can move apart with nothing to notice. Raise it to 1.3.5 so the bytes match the number. Signed-off-by: Claude <noreply@anthropic.com>
frahlg
left a comment
frahlg
left a comment
There was a problem hiding this comment.
Reviewed against AGENTS.md/CONTRIBUTING.md. The reasoning is sound and matches the driver source: set_battery_idle does write EMS mode 2 (forced), and driver_default_mode/SendDefault is the only unguarded release path, so refusing zero before evidence is correct. configure_power_limits gating is narrowly scoped and doesn't touch the release write. Tests are thorough (29 cases across both driver copies) and I confirmed make check passes clean: 3110 passed, 1361 skipped.
One gap: packages/v1/sungrow/targets/ftw.lua's driver_command_v2 had the matching zero-exemption removed, but its own version field was left at 1.3.4. Per CONTRIBUTING.md ("Raise the version whenever the Lua source changes") and the drift this repo has been bitten by twice already (Pixii #40288), I've pushed a fix bumping it to 1.3.5 with a CHANGELOG line, and re-ran make check clean afterward. Merging.
Generated by Claude Code
![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: 1c4426267b
ℹ️ 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".
| -- outage. On a string inverter the write fails at the Modbus layer | ||
| -- and costs nothing. | ||
| if inputs.power_w ~= 0 and model_family ~= "hybrid" and not battery_confirmed then | ||
| if model_family ~= "hybrid" and not battery_confirmed then |
There was a problem hiding this comment.
This changes the FTW v2 target's zero-command behavior but leaves its DRIVER.version at 1.3.4, even though the changelog associates 1.3.4 with the opposite behavior. Any future build, cache, or provenance record keyed by the target's version would therefore treat two behaviorally different sources as the same release; bump the target's own patch version and document it alongside catalog 1.5.6.
AGENTS.md reference: AGENTS.md:L27-L30
Useful? React with 👍 / 👎.
| firmware_versions: "" | ||
| notes: "Three-phase string inverter, PV + meter only (no battery). Needs 1.2.1, which stops polling the SH battery registers this family does not answer." | ||
| min_driver_version: "1.2.1" | ||
| notes: "Three-phase string inverter, PV + meter only (no battery). Needs 1.5.3: 1.2.1 stopped polling the SH battery registers this family does not answer, and 1.5.3 refuses a battery dispatch instead of writing them. A zero-watt command is still written on every family from 1.5.4 -- it is the release, and on a model without the registers it fails harmlessly." |
There was a problem hiding this comment.
For the newly referenced 1.5.6 driver, this note states that zero is still written on every family and is the release, but the changed guard at drivers/lua/sungrow.lua:750 now refuses zero whenever the battery is unconfirmed, and the release is driver_default_mode. The same stale sentence is duplicated for SG-CX and in generated devices.yaml, leaving compatibility guidance that directly contradicts this fix.
Useful? React with 👍 / 👎.
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>
522f860 to
c46a62e
Compare
An SG12RT in the field logged "self-consumption reset write failed: modbus exception function=0x06" once per watchdog tick, indefinitely. driver_init's startup reset, the watchdog, driver_cleanup and the deinit action all write 13049-13051, and nothing counted how often the device said no. The EMS block now follows the rule the register reads have followed since #36: absence has to be proved, three refusals prove it, and the paths that write unprompted stop. Measured on the reported device: writes per watchdog tick 3,3,3,3,3,3,3,3 -> 3,3,0,0,0,0,0,0 warn lines over 8 ticks 9 -> 3 Deliberately not gated on model_family, which is the obvious fix, the one the field thread proposed, and the wrong one. The startup reset exists because the inverter can be holding a forced state this driver did not set: a container that died mid-force, an older driver version, iSolarCloud or another EMS on the same bus. None of those care what classify_device_type decided, and a hybrid shipped under a device-type code the driver has not been taught is classified "string". Skipping the release on the label leaves exactly that inverter forced, with nothing left that writes mode 0 back. What separates the cases is not the label but whether the device took the write. One that refuses it cannot be holding a forced state. One that accepts it keeps its release. It also stops the question being re-argued. Whether a zero-watt command reaches the EMS block has been settled in both directions inside one day -- #42 opened it, #45 closed it again. This rule does not depend on that answer, and did not change when it flipped. A command always attempts, however many refusals came before, and its outcome still feeds the count, so one write that lands puts the release back at once. The count lives in the process, so a restart always attempts the startup reset -- the case that reset exists for -- exactly as a restart re-probes a read. While the driver is still attempting, a refused write is reported as the failure it is. Reporting success over a write that did not land is the silent-failure shape #164 is about. What stops is the failure repeating forever, so the watchdog settles rather than escalating against an inverter that was never under control. The reported device ships as a fixture -- device type 9268, no meter wired, no hybrid block, every EMS write refused. The tests separate the three candidate designs: with no bound three fail, with a model_family gate five fail including the stranded-inverter case, and only the refusal count passes all of them. The other half of that field report needs no driver change. The same device shows "driver_poll: 2 of 9 modbus reads failed" on every poll and never comes online, which is what puts it offline. #36 already bounded those reads: against 1.4.0 they fail 2/9 on every poll forever, against main they settle to 0/7 by the fourth. The site runs 1.4.0 and its update panel reads "up to date", so what it needs is FTW's pin moved. make check -- 3115 passed, 1361 skipped. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Reverts the behaviour change in #42 and takes #43's position instead, with #44's startup gate. Supersedes both.
I was wrong, and here is the evidence
#42 accepted a zero-watt battery command on any family. The premise was that zero hands the device back to itself, so refusing it could strand an inverter in a forced state. Reading the driver, neither half holds.
set_battery_idlewrites:Mode 2 is forced. Zero pins the battery at zero under the host's control; it does not release anything. The release is
driver_default_mode→set_self_consumption, which FTW reaches throughRegistry.SendDefaultwithdriverCmd{kind: "default"}on shutdown, lease expiry and the watchdog — never throughdriver_command, and no guard touches it.So refusing withholds nothing that is not reachable by a route which cannot refuse. Accepting put mode 2 and a setpoint back on an SG inverter that implements neither and reported success, because the read-back that would catch it fails on that device too. That is the SG12RT bug through the one action nobody guarded.
Also in here: startup stops asking a string inverter about its battery
configure_power_limitsread 33046, 33047 and 13057 on every restart whatever the family. An SG12RT paid three failed reads to learn nothing.Gated on a positively named string inverter, which is safe here precisely because each write is already conditional on its read having answered, and none of them clears a forced state.
set_self_consumptionis deliberately not gated. A hybrid shipped under a device-type code the classifier has not been taught lands in that same branch, and the release is the only write that can clear mode 2. An earlier draft gated it and stranded exactly that device.Recorded, not fixed
The two sungrow copies disagree about what zero writes — catalog forces idle at mode 2, the v2 target hands the inverter back at mode 0 and clears the lease — and they use different register maps throughout. That belongs to the register-map review
packages/v1/sungrow/PILOT.mdalready requires before the v2 target ships. Itsverification_statusisexperimentaland the signed package builds fromtargets/ftw-observe.lua, so nothing there reaches hardware today. Written down because untested drift between these two files is how Pixii's 40288 came back.Verified
test_sungrow_model_family.py, both copiesmake check: 3110 passedFTW's
TestSungrowZeroBatteryCommandForcesIdleencoded the same misreading and is corrected in srcfl/ftw#711 — it confirms the battery before commanding. Its assertions were always about mode 2, so what it checks is unchanged.Closes #43, closes #44.
🤖 Generated with Claude Code