diff --git a/CHANGELOG.md b/CHANGELOG.md index ab4d404..92d3d27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ Driver versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html ## [Unreleased] +### Fixed +- **sungrow** 1.5.5 — **Startup asked a family with no battery about its battery 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. It now skips them. 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 +- **The wider version of this change was wrong, and the test that says so ships with it.** 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. But since 1.5.4 a zero-watt 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: mode 2 survived the watchdog and cleanup both. `test_the_release_is_still_reachable_on_a_string_inverter` fails if the gate is put back +- The two failed reads left at startup are 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 +- 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 again, 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 deliberate exception 1.5.4 made for zero + ### Fixed - **sungrow** 1.5.4 and its **FTW v2 target** 1.3.4 — **a zero-power battery command is never refused.** 1.5.3 required the model to have named itself a hybrid or a battery register to have answered, which was 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 and setpoint nought. It arrives 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*, because 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 - **The Lua mock raised where the host returns.** `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 nobody noticed. 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. The mock now returns what the host returns. Nothing broke — 3108 tests pass — which is the point: it had been lying for free diff --git a/SUPPORT_STATUS.md b/SUPPORT_STATUS.md index 9c3b626..f24c0a7 100644 --- a/SUPPORT_STATUS.md +++ b/SUPPORT_STATUS.md @@ -144,8 +144,8 @@ Catalog source is not proof that a target can install or run a driver. | solis_string | 1.1.1 | blixt-l1 | not_assessed | — | — | not_recorded | — | not_assessed | no | | sonnen | 2.0.1 | ftw-core | not_assessed | — | — | not_recorded | — | not_assessed | no | | sonnen | 2.0.1 | blixt-l1 | not_assessed | — | — | not_recorded | — | not_assessed | no | -| sungrow | 1.5.4 | ftw-core | observe_only_profile_blocked | 1.3.3 | 1.3.0 | exact_model_firmware_required | — | not_assessed | no | -| sungrow | 1.5.4 | blixt-l1 | not_assessed | — | — | not_recorded | — | not_assessed | no | +| sungrow | 1.5.5 | ftw-core | observe_only_profile_blocked | 1.3.3 | 1.3.0 | exact_model_firmware_required | — | not_assessed | no | +| sungrow | 1.5.5 | blixt-l1 | not_assessed | — | — | not_recorded | — | not_assessed | no | | tesla_vehicle | 0.2.0 | ftw-core | not_assessed | — | — | not_recorded | — | not_assessed | no | | tesla_vehicle | 0.2.0 | blixt-l1 | not_assessed | — | — | not_recorded | — | not_assessed | no | | tibber | 1.1.0 | ftw-core | not_assessed | — | — | not_recorded | — | not_assessed | no | diff --git a/devices.yaml b/devices.yaml index 0c7b8ba..b13e7ce 100644 --- a/devices.yaml +++ b/devices.yaml @@ -1722,29 +1722,29 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.4" + version: "1.5.5" ders: [battery, meter, pv] control: true firmware_versions: "" - notes: "Commercial/industrial three-phase string inverter. Needs 1.2.1, which stops polling the SH battery registers this family does not answer." + notes: "Commercial/industrial three-phase string inverter. 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." - name: "SG-RT (Three-Phase String)" variants: [SG10RT, SG12RT, SG15RT, SG17RT, SG20RT, SG25RT, SG5.0RT, SG6.0RT, SG7.0RT, SG8.0RT] regions: [EU] protocols: - protocol: modbus driver: "sungrow" - version: "1.5.4" + version: "1.5.5" ders: [battery, meter, pv] control: true 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." + 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." - name: "SH-RS (Single-Phase Hybrid)" variants: [SH3.0RS, SH3.6RS, SH4.0RS, SH5.0RS, SH6.0RS] regions: [EU] protocols: - protocol: modbus driver: "sungrow" - version: "1.5.4" + version: "1.5.5" ders: [battery, meter, pv] control: true firmware_versions: "" @@ -1755,7 +1755,7 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.4" + version: "1.5.5" ders: [battery, meter, pv] control: true firmware_versions: "" @@ -1766,7 +1766,7 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.4" + version: "1.5.5" ders: [battery, meter, pv] control: true firmware_versions: "" diff --git a/drivers/lua/sungrow.lua b/drivers/lua/sungrow.lua index c6a33ac..c33036d 100644 --- a/drivers/lua/sungrow.lua +++ b/drivers/lua/sungrow.lua @@ -9,7 +9,7 @@ DRIVER = { id = "sungrow-shx", name = "Sungrow SH Hybrid Inverter", manufacturer = "Sungrow", - version = "1.5.4", + version = "1.5.5", protocols = { "modbus" }, capabilities = { "meter", "pv", "battery", "pv-curtail" }, description = "Sungrow SH-series hybrid inverters with LFP battery, via Modbus TCP.", @@ -136,6 +136,20 @@ local function hybrid_block_worth_reading() return model_family == "hybrid" or model_family == "unknown" end +-- Whether the device has named itself a model with no battery registers. +-- +-- Read this narrowly. It gates startup *reads* of the battery limit block and +-- nothing else. It must never gate a write that clears a forced state: since +-- 1.5.4 a zero-watt command is accepted on any family, so a device this +-- function calls "string" can still be holding EMS mode 2, and +-- set_self_consumption is the only thing that writes mode 0 back. A device +-- classified here is not proof there is nothing to release -- Sungrow shipping +-- a hybrid under a device-type family classify_device_type has not been taught +-- lands in exactly this branch. +local function known_to_have_no_battery() + return model_family == "string" +end + -- A register that answers on one model and not another. Absence has to be -- proved: a single timeout or a busy bus must not silence a register for the -- rest of the session, so only a run of failures counts. @@ -279,6 +293,17 @@ end -- Ensure power limits allow full charge/discharge (5kW each) -- Some Sungrow units ship with discharge capped at 100W function configure_power_limits() + -- All three registers below are battery limits: charge power, discharge + -- power, and the SoC ceiling and floor. A model that named itself a string + -- inverter answers none of them, so asking costs three failed reads at + -- startup and learns nothing. Each write here is already conditional on + -- its read having answered, so skipping the reads skips the writes with + -- it -- and none of them clears a forced state, which is why this is the + -- one startup path that is safe to gate on the family alone. + if known_to_have_no_battery() then + return + end + -- Max charge power: register 33046, scale 0.01 kW local ok_chg, chg = pcall(host.modbus_read, 33046, 1, "holding") if ok_chg and chg then diff --git a/drivers/tests/test_sungrow_model_family.py b/drivers/tests/test_sungrow_model_family.py index ec7d69b..221af8f 100644 --- a/drivers/tests/test_sungrow_model_family.py +++ b/drivers/tests/test_sungrow_model_family.py @@ -525,3 +525,147 @@ def test_zero_lands_before_the_first_poll(driver: Path) -> None: f"{driver.name} refused a zero-power command issued before the first " f"poll. Detection has had no chance to run at that point, and a " f"safety idle cannot wait for it.") + + +# -------------------------------------------------------------------------- +# Startup traffic, and the line it must not cross +# +# configure_power_limits reads three battery registers at startup -- 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 they are not the SG12RT outage. +# 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 the last test here holds it. An +# earlier draft of this change gated set_self_consumption on the same family +# test. Since 1.5.4 a zero-watt command is accepted on every family, so a +# device classified "string" -- a Sungrow hybrid shipped under a device-type +# code classify_device_type has not been taught lands there -- can be holding +# EMS mode 2, and set_self_consumption is the only thing that writes mode 0 +# back. Gating it left that device forced with nothing able to release it. +# -------------------------------------------------------------------------- + +BATTERY_LIMIT_BLOCK = [13057, 33046, 33047] + + +def fail_battery_limit_block() -> str: + addresses = ", ".join(str(a) for a in BATTERY_LIMIT_BLOCK) + return f''' +for _, addr in ipairs({{{addresses}}}) do + host._modbus_read_fail_addresses[addr] = "Illegal Data Address" +end +''' + + +# The SG12RT as it really answers at startup: no hybrid block, and no battery +# limit registers either. +SG_AT_STARTUP = STRING_INVERTER + fail_battery_limit_block() + + +def startup_reads(device: str) -> str: + return device + f''' +dofile("{DRIVER}") +driver_init({{}}) +local failed, limit_reads, off_ems = 0, 0, 0 +for _, call in ipairs(host._calls) do + if call.func == "modbus_read" then + local addr = call.args[1] + if host._modbus_read_fail_addresses[addr] then + failed = failed + 1 + if addr ~= 13049 then off_ems = off_ems + 1 end + end + for _, limit in ipairs({{13057, 33046, 33047}}) do + if addr == limit then limit_reads = limit_reads + 1 end + end + end +end +print("FAILED_READS " .. tostring(failed)) +print("FAILED_OFF_EMS " .. tostring(off_ems)) +print("LIMIT_READS " .. tostring(limit_reads)) +print("WRITES " .. tostring(host._modbus_write_attempts)) +''' + + +def test_string_inverter_startup_reads_no_battery_limit_it_cannot_have() -> None: + out = run_lua(startup_reads(SG_AT_STARTUP)) + + assert out["LIMIT_READS"] == "0", ( + f"startup read the battery limit block {out['LIMIT_READS']} times on " + f"a model that named itself a string inverter. 33046, 33047 and 13057 " + f"are battery registers; this family has no battery.") + assert out["FAILED_OFF_EMS"] == "0", ( + f"startup failed {out['FAILED_OFF_EMS']} reads outside the EMS " + f"registers. Those are the ones nothing needs.") + + # Two failed reads remain, both at 13049, and both are meant to. One is + # set_self_consumption's readback, which must run on every family since + # 1.5.4 -- see test_the_release_is_still_reachable_on_a_string_inverter. + # The other is the startup EMS-state log, which is exactly where a device + # misclassified as "string" while holding mode 2 would show up. Gating + # either to make this number nicer would cost more than it saves. + assert out["FAILED_READS"] == "2", ( + f"startup cost {out['FAILED_READS']} failed reads, not the 2 expected " + f"at 13049. Read the comment above before changing this number.") + + +def test_hybrid_still_gets_its_power_limits_raised() -> None: + """The regression risk of gating: some units ship capped at 100 W.""" + body = HEALTHY_HYBRID + ''' +host._modbus_registers.holding[33046] = {10} +host._modbus_registers.holding[33047] = {10} +''' + startup_reads("") + out = run_lua(body) + + assert int(out["LIMIT_READS"]) >= 3, ( + "a hybrid must still be asked about its charge and discharge caps") + raised = int(out["WRITES"]) + assert raised >= 2, ( + f"the hybrid shipped capped at 0.1 kW and startup made {raised} " + f"writes. Some Sungrow units ship with discharge capped at 100 W, and " + f"raising it is what this function is for.") + + +def test_the_release_is_still_reachable_on_a_string_inverter() -> None: + """The line the startup gate must not cross. + + Since 1.5.4 a zero-watt 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 an + unrecognised device-type code lands in exactly that branch, takes the + forced write, and then needs mode 0 written back. + + Gate set_self_consumption on the family and this fails -- the device stays + at mode 2 through the watchdog and through cleanup, with nothing left that + can clear it. + """ + body = ''' +-- Classified "string" by device type, but its EMS registers accept writes. +host._modbus_registers.input[4999] = {0x2434} +host._modbus_registers.input[5000] = {120} +host._modbus_registers.input[5016] = {4000, 0} +''' + f''' +dofile("{DRIVER}") +driver_init({{}}) +for poll = 1, 3 do pcall(driver_poll) end +driver_command("battery", 0, {{}}) +print("FORCED " .. tostring(host._modbus_registers.holding[13049])) +driver_default_mode() +print("AFTER_WATCHDOG " .. tostring(host._modbus_registers.holding[13049])) +driver_cleanup() +print("AFTER_CLEANUP " .. tostring(host._modbus_registers.holding[13049])) +''' + out = run_lua(body) + + assert out["FORCED"] == "2", ( + "a zero-watt command is meant to reach every family since 1.5.4") + assert out["AFTER_WATCHDOG"] == "0", ( + f"the watchdog left EMS mode at {out['AFTER_WATCHDOG']}, not 0. The " + f"device is holding a forced state and the release is the only write " + f"that clears it -- gating it on the family strands the device.") + assert out["AFTER_CLEANUP"] == "0", ( + f"cleanup left EMS mode at {out['AFTER_CLEANUP']}, not 0") diff --git a/index.yaml b/index.yaml index afea978..0712026 100644 --- a/index.yaml +++ b/index.yaml @@ -561,13 +561,13 @@ drivers: size_bytes: 6158 sha256: "92163e4356b895e16b0205e849aa42a2dcea0d799f8cc6bcbce60b5e2cd92285" - name: "sungrow" - version: "1.5.4" + version: "1.5.5" tier: core protocol: modbus ders: [battery, meter, pv] control: true - size_bytes: 44353 - sha256: "94f6e4fd483e4dff54fafd79179c4700637662ca8b30bb600127189848cf503f" + size_bytes: 45670 + sha256: "9667377a29f26d4325e3df450b93d9a16b802f778b54bb15a894eab26b8be306" - name: "tesla_vehicle" version: "0.2.0" tier: core diff --git a/manifests/sungrow.yaml b/manifests/sungrow.yaml index 3618ea0..db7e29e 100644 --- a/manifests/sungrow.yaml +++ b/manifests/sungrow.yaml @@ -1,5 +1,5 @@ name: "sungrow" -version: "1.5.4" +version: "1.5.5" tier: core author: "Sourceful Labs AB" protocol: modbus @@ -32,19 +32,19 @@ tested_devices: variants: [SG5.0RT, SG6.0RT, SG7.0RT, SG8.0RT, SG10RT, SG12RT, SG15RT, SG17RT, SG20RT, SG25RT] regions: [EU] 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." + min_driver_version: "1.5.3" - manufacturer: "Sungrow" model_family: "SG-CX (Three-Phase Commercial)" variants: [SG25CX, SG30CX, SG33CX, SG36CX, SG40CX, SG50CX] regions: [EU, INT] firmware_versions: "" - notes: "Commercial/industrial three-phase string inverter. Needs 1.2.1, which stops polling the SH battery registers this family does not answer." - min_driver_version: "1.2.1" + notes: "Commercial/industrial three-phase string inverter. 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." + min_driver_version: "1.5.3" min_host_version: "2.0.0" -size_bytes: 44353 +size_bytes: 45670 dkb_id: "sungrow_sh_rt" -sha256: "94f6e4fd483e4dff54fafd79179c4700637662ca8b30bb600127189848cf503f" +sha256: "9667377a29f26d4325e3df450b93d9a16b802f778b54bb15a894eab26b8be306" signature: "" bytecode_sha256: "" diff --git a/support-status.json b/support-status.json index ef46195..44a16b6 100644 --- a/support-status.json +++ b/support-status.json @@ -1962,7 +1962,7 @@ }, { "catalog_source": true, - "catalog_version": "1.5.4", + "catalog_version": "1.5.5", "driver_id": "sungrow", "package_id": "com.sourceful.driver.sungrow", "targets": {