fix(sungrow): a zero-power battery command is never refused by frahlg · Pull Request #42 · srcfl/device-drivers

frahlg · 2026-07-29T05:35:39Z

Settles the disagreement holding srcfl/ftw#704. Decision: zero always goes through.

The rule

1.5.3 required the model to have named itself a hybrid, or a battery register to have answered. That is right for a dispatch and wrong for zero.

Zero is not a dispatch. It is the host handing the device back to itself — forced mode off, setpoint nought — and it comes from the lifecycle rather than the planner, so it lands before the first poll, exactly when nothing has been confirmed.

Refusing to write stop is a different risk from refusing to write charge: a device left in a forced state stays there. On a genuine string inverter the write fails at the Modbus layer and costs nothing. The outage it prevents is a battery with no way to be told to stop.

Non-zero setpoints still need evidence, so the hole #40 closed stays shut.

Both control paths move together, as #34 taught: sungrow 1.5.3 → 1.5.4, FTW v2 target 1.3.3 → 1.3.4.

Tests

Five cases pin it, across both files: a named string inverter, an unidentified one, a hybrid, and the ordering FTW's suite exercises — load, then command, no poll.

They assert the command is not refused by the guard, not that it succeeds. On a device that does not implement 13049 the read-back fails and the driver says so, which is the honest outcome. What must never happen is the command being turned away before it is tried.

The mock was lying, and that is why this was hard to see

host_mock.lua answered a failing modbus_read with error(). FTW's binding pushes nil plus a message and returns — it never raises.

Every driver wraps the call in pcall and works either way, which is why it went unnoticed. Code written correctly for the real host did not: the v2 target's v2_read_register checks the returned error, and under the mock it threw instead, taking the whole command down rather than reporting a structured failure. That is what made this look like a driver bug when it was a fixture bug.

The mock now returns what the host returns. Nothing broke — 3109 tests pass — which is the point. It had been lying for free, and docs/WRITING-A-DRIVER.md already warns that every time this mock drifted from the host it hid a real bug.

A fourth stale copy

test_pilot_safety_metadata_is_explicit pinned the sungrow package version. That test is about what the package promises — read-only, no commands, no control permissions — and a version is not one of those. The pin made an unrelated safety test the thing that failed on every legitimate bump. Removed; the version is already held to the driver's own by the manifest checks and the package build.

Verified

FTW's entire Go suite passes with this driver, including TestSungrowZeroBatteryCommandForcesIdle — the last thing blocking srcfl/ftw#704.

make check green: 3109 passed.

🤖 Generated with Claude Code

1.5.3 required the model to have named itself a hybrid or a battery register to have answered. That is right for a dispatch and wrong for zero. Zero is not a dispatch. It is the host handing the device back to itself -- forced mode off, setpoint nought -- and it arrives from the lifecycle rather than the planner, so it lands before the first poll, which is exactly when nothing has been confirmed. Refusing to write "stop" is a different risk from refusing to write "charge": a device left in a forced state stays there. On a genuine string inverter the write fails at the Modbus layer and costs nothing; the outage it prevents is a battery with no way to be told to stop. Non-zero setpoints still need evidence, so #40's hole stays shut. Both control paths move together, as #34 taught. Five new pytest cases pin the rule on a named string inverter, an unidentified one, a hybrid, and -- the case FTW's suite exercises -- before any poll at all. Also fixes the mock that made this hard to see. host_mock.lua answered a failing modbus_read with error(), but FTW's binding pushes nil plus a message and returns; it never raises. Every driver wraps the call in pcall and works either way, which is why it went unnoticed for so long. Code written correctly for the real host did not: the v2 target checks the returned error, and under the mock it threw instead, taking the whole command down rather than reporting a structured failure. The mock now returns what the host returns, and nothing broke -- which is the point. It had been lying for free. test_pilot_safety_metadata_is_explicit pinned the sungrow package version. That test is about what the package promises -- read-only, no commands, no control permissions -- and a version is not one of those. The pin made an unrelated safety test the thing that failed on every legitimate bump. Fourth hand-maintained copy of a contract to drift here. Verified against FTW: the whole Go suite passes with this driver, including TestSungrowZeroBatteryCommandForcesIdle, which was the last thing blocking the pin move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3a30543d5

ℹ️ 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".

chatgpt-codex-connector · 2026-07-29T05:40:29Z

  "schema_version": "sourceful.driver-package-source/v1",
  "package_id": "com.sourceful.driver.sungrow",
-  "version": "1.3.2",
+  "version": "1.3.3",


Align the packaged Lua version with the recipe

The recipe still packages targets/ftw-observe.lua, whose DRIVER.version remains 1.3.2, so building this 1.3.3 package produces package metadata that disagrees with its runtime metadata. tools/driver_package.py only checks that a Lua version exists, and the removed test assertion now also masks this mismatch; bump the packaged Lua metadata and related pilot documentation or leave the recipe at 1.3.2.

AGENTS.md reference: AGENTS.md:L60-L62

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-07-29T05:40:29Z

    name = "Sungrow hybrid and string inverter",
    manufacturer = "Sungrow",
-    version = "1.3.3",
+    version = "1.3.4",


Update the FTW harness version assertion

After this version bump, test_sungrow_ftw_v2 in drivers/tests/lua_harness/test_all_drivers.lua:659 still requires DRIVER.version == "1.3.3". Running that comprehensive Lua harness therefore reports Sungrow FTW v2 metadata is wrong for the changed target; update the assertion to 1.3.4 so this adapter remains testable.

Useful? React with 👍 / 👎.

… 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>

… limits (#44) 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. It now skips them. The first draft of this change also gated set_self_consumption (driver_init's reset, the watchdog, driver_cleanup, deinit). That is unsafe as of #42: since 1.5.4 a zero-watt battery command is accepted on every family and writes EMS mode 2, so a device merely classified "string" is not proof there is nothing to release. Gating the release path would strand such a device forced, with no path left to write mode 0 back. The gate now covers configure_power_limits only, and test_the_release_is_still_reachable_on_a_string_inverter pins the boundary. sungrow 1.5.4 -> 1.5.5. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@frahlg

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>

@frahlg

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>

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>

sungrow 1.5.7 fixed a driver that reissued a refused write on every watchdog tick, forever, after a customer's SG12RT logged "self-consumption reset write failed: modbus exception function=0x06" once per tick indefinitely. This measures whether that shape is one driver or many. It is eleven. driver_default_mode runs on a timer -- lease expiry, the telemetry watchdog, the stale-meter standdown, shutdown -- and nothing reaching it can say no, so a driver that writes whatever the device answers keeps writing for the life of the session: atmoce deye ferroamp_modbus huawei pixii sigenergy solaredge solaredge_legacy solaredge_pv solinteg solis All eleven write 1 or 2 registers per call and never settle. sungrow is the only one that stops, and only because the field report forced it. refused_write_probe.lua refuses every write, calls driver_default_mode eight times and reports the per-call counts. test_refused_write_settles.py ratchets against refused-write-baseline.json the way #36 did for reads: a count that rises fails, a driver not listed must be clean, and a count that falls fails until the file is updated. Verified by putting the sungrow flap back -- it fails. This is the write-side twin of test_absent_register_settles.py, and the two failures are not equally severe. A failed read fails the whole poll and takes the site offline; a refused write costs bus traffic and a log line per tick. So this measures the flap rather than treating every occurrence as an outage -- but that log line is the one an operator reads to find the real fault, and on the SG12RT it drowned it. Also fixes host_mock.lua, which had no host.sleep. It is in spec/host-api.md and the real host provides it; solis and deye call it to space out writes. Lua turns a missing field into "attempt to call a nil value", so any test reaching their write-retry path died there rather than measuring it, and both looked like they crashed inside driver_default_mode when they do nothing of the kind. The mock now advances its clock instead of sleeping. Second time the mock has been caught lying, after #42. No driver changes. Paying the debt down is follow-up work, in batches, the way #36 did it. make check -- 3195 passed, 1361 skipped. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

frahlg merged commit edb32ff into main Jul 29, 2026
5 checks passed

frahlg deleted the sungrow-zero-always-lands branch July 29, 2026 05:36

frahlg mentioned this pull request Jul 29, 2026

fix(sungrow): a zero-power battery command needs evidence after all #43

Closed

chatgpt-codex-connector Bot reviewed Jul 29, 2026

View reviewed changes

frahlg mentioned this pull request Jul 29, 2026

fix(sungrow): stop startup asking a string inverter about its battery limits #44

Merged

frahlg mentioned this pull request Jul 29, 2026

fix(sungrow): restore the zero-power refusal that 1.5.4 removed #45

Merged

frahlg mentioned this pull request Jul 29, 2026

test(sungrow): confirm the battery before commanding it srcfl/ftw#711

Merged

This was referenced Jul 29, 2026

fix(sungrow): stop writing an EMS block the inverter has already refused #49

Merged

test: measure the refused-write flap across the catalog #50

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(sungrow): a zero-power battery command is never refused - #42

fix(sungrow): a zero-power battery command is never refused#42
frahlg merged 1 commit into
mainfrom
sungrow-zero-always-lands

frahlg commented Jul 29, 2026

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

frahlg commented Jul 29, 2026

The rule

Tests

The mock was lying, and that is why this was hard to see

A fourth stale copy

Verified

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant