test(sungrow): confirm the battery before commanding it - #711
Conversation
TestSungrowZeroBatteryCommandForcesIdle sent a battery setpoint to a freshly loaded driver with no poll in between. The mock answers 0 everywhere, so register 4999 gives no device type, detection settles on "unknown", and device-drivers #40 refuses the write. The refusal is right. Sungrow ships two families behind one driver and an SG string inverter implements none of 13049-13051; writing them anyway is the SG12RT bug, and the read-back that would catch it fails on that device too, so the driver reports success. Nothing is lost by confirming first. The test's own assertions show it was never testing a release: it expects EMS mode 2 -- forced -- with command 0xCC and setpoint 0. That is a dispatch to a battery pinned at zero, not a device handed back. The release is driver_default_mode, which FTW reaches through SendDefault on shutdown, lease expiry and the watchdog, and which no guard touches. So the mock now answers 4999 as an SH8.0RT and the driver polls once before the command. What the test checks is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
![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: 8bfd525d9e
ℹ️ 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 _, err := d.Poll(context.Background()); err != nil { | ||
| t.Fatalf("poll: %v", err) | ||
| } |
There was a problem hiding this comment.
Remove this poll or add a separate test for the original scenario: in the reviewed drivers/sungrow.lua, driver_command deliberately exempts power_w == 0 from battery confirmation because that command can arrive before the first poll. By identifying the inverter and polling first, this test would remain green if that exemption were accidentally removed, leaving fresh or not-yet-polled drivers unable to receive the zero-power stop; the original pre-poll test passes against this tree.
Useful? React with 👍 / 👎.
| "ftw": patch | ||
| --- | ||
|
|
||
| The Sungrow zero-power test confirms the battery before commanding, matching the driver's refusal to write EMS registers a string inverter does not implement. |
There was a problem hiding this comment.
Drop this release entry or land the corresponding driver behavior in the same change. This commit modifies only the test, while drivers/sungrow.lua still gates confirmation with power_w ~= 0, so an unconfirmed zero-power command continues writing registers 13049–13051. Consuming this changeset would bump the package and publish a changelog claim that is not true of the released artifact.
Useful? React with 👍 / 👎.
1 participant
TestSungrowZeroBatteryCommandForcesIdlesent a battery setpoint to a freshly loaded driver with no poll in between. The mock answers 0 everywhere, so register 4999 gives no device type, detection settles onunknown, and the driver refuses.The refusal is right. Sungrow ships two families behind one driver and an SG string inverter implements none of 13049–13051. Writing them anyway is the SG12RT bug, and the read-back that would catch it fails on that device too, so the driver reports success on a battery that is not there. See srcfl/device-drivers#40 and srcfl/device-drivers#45.
Nothing is lost by confirming first. The test's own assertions show it was never testing a release — it expects EMS mode 2, forced, with command
0xCCand setpoint 0. That is a dispatch to a battery pinned at zero, not a device handed back. The release isdriver_default_mode, which this repository reaches throughSendDefaulton shutdown, lease expiry and the watchdog, and which no guard touches.So the mock now answers 4999 as an SH8.0RT and the driver polls once before the command. What the test checks is unchanged.
Worth recording: this repository briefly won the argument. srcfl/device-drivers#42 removed the refusal to make this test pass, on the reading that a zero command hands the inverter back. It does not —
set_battery_idlewrites mode 2 — and srcfl/device-drivers#45 restores the refusal. The test was encoding a misreading of the driver it tests.go test ./...passes against both the current pinned driver and the restored one.🤖 Generated with Claude Code