fix(sungrow): stop reading a battery that is not there#28
Conversation
This is the SG12RT outage, fixed in the driver that actually caused it. Sungrow ships two families behind one driver. SH hybrids answer the 13xxx block: battery, running state, energy counters. SG string inverters have no battery and answer none of it. This driver read that block regardless, on eight separate reads. Because the host fails a whole poll when any single read fails, a string inverter did not report less telemetry -- it reported none. A customer's SG12RT lost everything, with 12 of 19 reads failing on every poll. The driver already knew how to tell the families apart: driver_fingerprint classifies the device type code for the host's auto-detection, and driver_init read the same register and threw the answer away after logging it. That classification is now shared -- one implementation, not two that drift -- and the answer is kept. Measured on the harness: an SG12RT costs zero failed reads from the first poll, where it cost eight. A healthy SH10RT is unchanged, battery and all. Three things found alongside it, each its own way of reporting something that was never read: Bounded retries. Detection and the serial number both give up after three tries. A register retried on every poll forever costs a failed read on every poll forever, which is the same outage arriving more slowly. Where detection never succeeds the driver probes once and settles. No fabricated battery. The stream was emitted unconditionally with zeros when its registers had not answered -- a false reading on a hybrid, and an entirely invented battery on a string inverter. Now emitted only when the defining read answered, and each energy counter only when that counter answered, because a lifetime counter reporting zero looks like a meter that was reset. The fault channel cleared only on a non-zero running state, but 0x0000 and 0x0040 are both documented as Running, so an inverter that recovered into 0x0000 stayed reported as faulted. It now clears on any state actually read, and a string inverter neither raises nor clears since it never reads it. Editing this driver means it is no longer byte-identical to its FTW baseline, so this repository's checks apply to it again -- which is the mechanism working as designed. Its PV stream emits mppt1_w, mppt2_w and pv_source, real fields from a production-verified driver that the catalog's allowlist had never heard of. Added rather than removed. drivers/tests/test_sungrow_model_family.py pins all of it, starting with the incident: an SG string inverter must cost zero failed reads. 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: 8c2d91bdf0
ℹ️ 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 (miss_counts[address] or 0) >= MISSES_BEFORE_SKIP then | ||
| return nil |
There was a problem hiding this comment.
When a known hybrid has three consecutive timeouts—for example, during a 15-second network or inverter restart—this branch permanently suppresses that register for the remainder of the driver session. Unlike an actually absent block on an unidentified model, these registers are expected on a detected hybrid, so battery telemetry, fault state, and energy counters never recover even after communication returns; limit permanent skipping to model probing or reset the misses after connectivity recovers.
Useful? React with 👍 / 👎.
| if ok and registers ~= nil then | ||
| miss_counts[address] = nil | ||
| return registers |
There was a problem hiding this comment.
On hybrids, address 13049 is read both as a 30-register input block and as a 3-register holding block, but both operations share this address-only counter. If the input read succeeds while the holding read is unsupported, the input success clears the holding miss on every poll, so the holding read fails forever and continues taking every poll offline; track misses by at least (kind, address) so the holding probe can reach the skip threshold independently.
Useful? React with 👍 / 👎.
| if hybrid_block_worth_reading() then | ||
| local pvgen_regs = optional_read(13002, 2, "input") |
There was a problem hiding this comment.
For every detected SG string inverter, this condition skips register 13002 but leaves pv_gen_wh at zero, which is then emitted as lifetime_wh. The resulting fresh zero falsely looks like a reset lifetime counter—the same fabricated-counter behavior this change avoids for battery and meter energy—so the field should remain absent unless this read succeeds.
Useful? React with 👍 / 👎.
…hem (#29) The driver release failed on both #27 and #28 with error: deye: changed artifact needs a higher version than 1.2.2 and it was right to. The channel refuses to publish changed bytes under a version it has already published; that immutability is what makes rollback mean anything. Nothing shipped, which is the correct outcome for a release that would have replaced 19 drivers' contents behind their own version numbers. So the 17 remaining replaced drivers move to a major version (pixii and sungrow already had). Major rather than patch because these are different implementations, not fixes, and an operator pinning a version needs to know that. bump_driver no longer rewrites the DRIVER table of a driver that is byte-identical to its FTW baseline. Doing so broke the byte-identity of 17 drivers, which cost them their test exemption and failed 13 checks -- and it spent the provenance guarantee on a field the tooling already accepts as differing. The manifest counts this repository's releases; the DRIVER table counts FTW's. Verified by building the signed publication locally against the currently published manifest: 80 drivers, 18 new, 19 bumped, zero version collisions. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
1 participant
The SG12RT outage, fixed in the driver that actually caused it.
What happened
Sungrow ships two families behind one driver. SH hybrids answer the 13xxx block — battery, running state, energy counters. SG string inverters have no battery and answer none of it.
This driver read that block regardless, on eight separate reads. Because the host fails a whole poll when any single read fails, a string inverter did not report less telemetry — it reported none. Calle's SG12RT lost everything, with 12 of 19 reads failing on every poll.
Worth noting: the earlier fix for this landed on the catalog's sungrow, which was never the driver running at the customer's site. That one has since been replaced by this one, so the fix belongs here.
The fix
The driver already knew how to tell the families apart.
driver_fingerprintclassifies the device type code for the host's auto-detection, anddriver_initread the same register and threw the answer away after logging it. That classification is now shared — one implementation, not two that drift — and the answer is kept.Measured on the harness:
0x2434), string0x0E0E), hybridThree things found alongside it
Each was a different way of reporting something that was never read.
Bounded retries. Detection and the serial number now give up after three tries. A register retried on every poll forever costs a failed read on every poll forever — the same outage arriving more slowly. Where detection never succeeds the driver probes once and settles rather than guessing.
No fabricated battery. The stream was emitted unconditionally with zeros when its registers had not answered — a false reading on a hybrid, and an entirely invented battery on a string inverter. Now emitted only when the defining read answered, and each energy counter only when that counter answered: a lifetime counter reporting zero looks like a meter that was reset.
The fault channel could not clear. It cleared only on a non-zero running state, but
0x0000and0x0040are both documented as Running. An inverter that recovered into0x0000stayed reported as faulted. It now clears on any state actually read, and a string inverter neither raises nor clears since it never reads it.optional_readalso checks every register it asked for, not just the first. A short Modbus reply left the rest nil, and arithmetic on nil ended the whole poll.The mechanism working as designed
Editing this driver means it is no longer byte-identical to its FTW baseline, so this repository's checks apply to it again — exactly as intended.
That immediately surfaced something: its PV stream emits
mppt1_w,mppt2_wandpv_source, real fields from a production-verified driver that the catalog's allowlist had never heard of. Added, not removed.Tests
drivers/tests/test_sungrow_model_family.pypins all of it, starting with the incident itself: an SG string inverter must cost zero failed reads. Also covered — that a healthy hybrid loses nothing, that an unreadable device type settles, that the fault channel goes0,1,0,1,0across running and faulted states, and that a string inverter never claims anything about faults.make check: 1981 passed, 80 drivers, sandbox and host-API clean.🤖 Generated with Claude Code