fix(sungrow): stop writing an EMS block the inverter has already refused - #49
Conversation
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>
![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: ff64d0fe91
ℹ️ 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 not ems_block_worth_writing() then | ||
| return true |
There was a problem hiding this comment.
When a hybrid is already in forced mode and the bus returns errors for three EMS operations—such as failed commands or watchdog ticks—ems_write_failures reaches the limit; after communication recovers, this branch returns success without writing mode 0. The inverter can therefore remain at its previous forced charge/discharge setpoint while the host is told that default mode was restored, until a restart or another successful command resets the counter. Only unsupported-register refusals should disable these writes; generic transport failures must not permanently suppress the safe-default release.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
| name = "Sungrow SH Hybrid Inverter", | ||
| manufacturer = "Sungrow", | ||
| version = "1.5.6", | ||
| version = "1.5.7", |
There was a problem hiding this comment.
This version bump fixes only the catalog copy, while packages/v1/sungrow/targets/ftw.lua:449-456 still has driver_default_mode_v2 write register 13049 and report failure on every invocation. If that control-v2 target is used with the same SG inverter, it retains the original indefinite write/failure behavior, allowing the separately maintained target to reintroduce the bug when packaged; update the target and its versioned metadata with the catalog change.
AGENTS.md reference: AGENTS.md:L22-L30
Useful? React with 👍 / 👎.
1 participant
From a field report: an SG12RT logging
self-consumption reset write failed: modbus exception function=0x06once per watchdog tick, indefinitely.Rebased onto #45. The earlier draft of this PR argued from #42's always-accept-zero; that premise is gone and the conclusion did not move, which is most of the point.
What it does
driver_init's startup reset, the watchdog,driver_cleanupand thedeinitaction 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.3,3,3,3,3,3,3,33,3,0,0,0,0,0,0Why not
if model_family == "string"That is the obvious fix and the one the field thread proposed. It strands inverters.
The startup reset exists because the device 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_typedecided, and a hybrid shipped under a device-type code the driver has not been taught is classifiedstring. Gate on the label and that inverter stays forced, with nothing left to write mode 0 back.The label does not separate the cases. Whether the device took the write does. 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. This rule does not depend on that answer, and did not change when it flipped.
The tests separate the three designs
model_familygateThe reported device ships as a fixture: device type 9268, no meter wired, no hybrid block, every EMS write refused.
Deliberately unchanged
The other half of that report needs no driver change
The same device shows
driver_poll: 2 of 9 modbus reads failedon every poll and never comes online — that, not the writes, is what puts it offline. #36 already bounded those reads:2/9 2/9 2/9 2/9 2/9 2/92/9 2/9 2/9 0/7 0/7 0/7The site runs 1.4.0 and its update panel reads "up to date", so what it needs is FTW's pin moved, not a patch.
Checks
make check— 3115 passed, 1361 skipped.🤖 Generated with Claude Code