diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e9191e..e7863bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ Driver versions follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html ## [Unreleased] +### Fixed +- **sungrow** 1.5.7 — **The driver kept writing a register block the inverter had already refused.** 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 went `3,3,3,3,3,3,3,3` to `3,3,0,0,0,0,0,0`, and the warn lines over eight ticks from 9 to 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 to write 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 now been settled in both directions inside one day — 1.5.4 opened it, 1.5.6 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 instead of escalating against an inverter that was never under control +- `drivers/tests/test_sungrow_model_family.py` carries the reported device 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: reproduced against 1.4.0 they fail `2/9` on every poll forever, and 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, not a patch + ### Fixed - **sungrow** 1.5.6 — **the zero-power refusal is restored. 1.5.4 was wrong, on a premise that did not survive reading the driver.** The case for waving zero through was that it hands the device back to itself, so refusing it could strand an inverter in a forced state. Neither half holds. `set_battery_idle` writes EMS mode **2** — forced — with command `0xCC` and setpoint 0: 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. So refusing 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, and reported success, because the read-back that would catch it fails on that device too. That is the SG12RT bug through the one action nobody guarded - The two sungrow copies disagree about what zero writes — the catalog driver forces idle at mode 2, the FTW v2 target hands the inverter back at mode 0 and clears the lease — and they use different register maps throughout. That belongs to the register-map review `packages/v1/sungrow/PILOT.md` already requires before the v2 target ships; its `verification_status` is `experimental` and the signed package builds from `targets/ftw-observe.lua`, so nothing there reaches hardware today. Recorded because untested drift between these two files is how Pixii's 40288 came back diff --git a/SUPPORT_STATUS.md b/SUPPORT_STATUS.md index c577b4b..99ba5bc 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.6 | ftw-core | observe_only_profile_blocked | 1.3.3 | 1.3.0 | exact_model_firmware_required | — | not_assessed | no | -| sungrow | 1.5.6 | blixt-l1 | not_assessed | — | — | not_recorded | — | not_assessed | no | +| sungrow | 1.5.7 | ftw-core | observe_only_profile_blocked | 1.3.3 | 1.3.0 | exact_model_firmware_required | — | not_assessed | no | +| sungrow | 1.5.7 | 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 a2d101b..fa738a6 100644 --- a/devices.yaml +++ b/devices.yaml @@ -1722,7 +1722,7 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.6" + version: "1.5.7" ders: [battery, meter, pv] control: true firmware_versions: "" @@ -1733,7 +1733,7 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.6" + version: "1.5.7" ders: [battery, meter, pv] control: true firmware_versions: "" @@ -1744,7 +1744,7 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.6" + version: "1.5.7" ders: [battery, meter, pv] control: true firmware_versions: "" @@ -1755,7 +1755,7 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.6" + version: "1.5.7" ders: [battery, meter, pv] control: true firmware_versions: "" @@ -1766,7 +1766,7 @@ manufacturers: protocols: - protocol: modbus driver: "sungrow" - version: "1.5.6" + version: "1.5.7" ders: [battery, meter, pv] control: true firmware_versions: "" diff --git a/drivers/lua/sungrow.lua b/drivers/lua/sungrow.lua index c049054..43a9639 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.6", + version = "1.5.7", protocols = { "modbus" }, capabilities = { "meter", "pv", "battery", "pv-curtail" }, description = "Sungrow SH-series hybrid inverters with LFP battery, via Modbus TCP.", @@ -150,6 +150,65 @@ local function known_to_have_no_battery() return model_family == "string" end +---------------------------------------------------------------------------- +-- The EMS control block, 13049-13051 +-- +-- The write-side twin of the register probe below, and it needs the same +-- rule: absence has to be proved, and once proved, stop paying for it. +-- +-- An SG string inverter rejects every write here with a Modbus exception. The +-- driver retried three of them at startup and three more on every watchdog +-- tick for the life of the session -- an SG12RT logged "self-consumption reset +-- write failed" once per tick, indefinitely, while it was already offline for +-- an unrelated reason. +-- +-- Deliberately NOT gated on model_family, which is the obvious fix 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 this driver has not been taught is classified "string". +-- Skipping the release on the label would leave exactly that inverter forced, +-- with nothing left that writes mode 0 back. +-- +-- What separates the two cases is not the label but whether the device took +-- the write. One that rejects it cannot be holding a forced state, so there is +-- nothing to release. One that accepts it can, and keeps its release. +-- +-- This also stops the question being re-argued. Whether a zero-watt command +-- reaches the EMS block has now been settled in both directions inside a +-- single day -- 1.5.4 opened it, 1.5.6 closed it again. The rule here does not +-- depend on that answer. +-- +-- Three failures rather than one for the same reason the read probe uses +-- three: a busy bus is not proof. A restart clears the count, so the startup +-- reset is always attempted at least once -- which is the case it exists for. +local EMS_WRITE_ATTEMPTS = 3 +local ems_write_failures = 0 + +-- True while the EMS block is still worth writing unprompted. +local function ems_block_worth_writing() + return ems_write_failures < EMS_WRITE_ATTEMPTS +end + +-- Record what the device did with one EMS operation. Counts the operation, +-- not each of its three registers: the trio goes out together and fails +-- together, so one rejected trio is one piece of evidence. +local function note_ems_write(write_err) + if write_err == nil then + ems_write_failures = 0 + return + end + ems_write_failures = ems_write_failures + 1 + if ems_write_failures == EMS_WRITE_ATTEMPTS then + host.log("info", "Sungrow: the EMS control block rejected " + .. ems_write_failures .. " writes in a row; treating it as absent " + .. "and no longer writing it unprompted") + end +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. @@ -806,6 +865,10 @@ function set_battery_power(power_w) local cmd_err = host.modbus_write(13050, want_cmd) -- 2. charge/discharge cmd local power_err = host.modbus_write(13051, watts) -- 3. power setpoint local write_err = first_write_error(mode_err, cmd_err, power_err) + -- A command always attempts, however many times the block has refused + -- before: only the unprompted paths give up. But the outcome still counts, + -- and a success here clears the count and puts the release back. + note_ems_write(write_err) if write_err then host.log("warn", "Sungrow: force command write failed: " .. write_err) return false @@ -846,6 +909,9 @@ function set_battery_idle() local cmd_err = host.modbus_write(13050, 0xCC) -- stop forced charge/discharge local power_err = host.modbus_write(13051, 0) -- zero power setpoint local write_err = first_write_error(mode_err, cmd_err, power_err) + -- Reached by a zero-watt command, which 1.5.4 accepts on every family. + -- It still attempts; the count only decides the unprompted paths. + note_ems_write(write_err) if write_err then host.log("warn", "Sungrow: idle write failed: " .. write_err) return false @@ -868,7 +934,21 @@ function set_battery_idle() end -- Return to self-consumption mode (safe default) +-- +-- Four paths reach this with no command in sight: driver_init's startup reset, +-- the watchdog, driver_cleanup and the deinit action. The watchdog is the one +-- that runs on a timer forever, so this is where a device that refuses the +-- write has to be let go of. function set_self_consumption() + -- Nothing to hand back on a device that has refused this block three + -- times: a write it rejects cannot have latched a forced state. Report the + -- default as held rather than failed -- the watchdog reads a false as the + -- safe state being unreachable and escalates, which is the wrong answer + -- for an inverter that was never under control to begin with. + if not ems_block_worth_writing() then + return true + end + -- Stop first, clear the stale power setpoint, then hand control back to -- the inverter. This makes the post-restart state deterministic instead -- of leaving e.g. mode=0/cmd=CC/power=880W latched in diagnostics. @@ -876,6 +956,7 @@ function set_self_consumption() local power_err = host.modbus_write(13051, 0) local mode_err = host.modbus_write(13049, 0) local write_err = first_write_error(cmd_err, power_err, mode_err) + note_ems_write(write_err) if write_err then host.log("warn", "Sungrow: self-consumption reset write failed: " .. write_err) return false diff --git a/drivers/tests/test_sungrow_model_family.py b/drivers/tests/test_sungrow_model_family.py index 8ecf249..009c7d5 100644 --- a/drivers/tests/test_sungrow_model_family.py +++ b/drivers/tests/test_sungrow_model_family.py @@ -837,3 +837,184 @@ def test_the_release_is_still_reachable_on_a_string_inverter() -> None: 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") + + +# -------------------------------------------------------------------------- +# The EMS block, and letting go of a write the device refuses +# +# An SG12RT in the field logged "self-consumption reset write failed: modbus +# exception function=0x06" once per watchdog tick, forever. The startup reset, +# the watchdog, cleanup and deinit all write 13049-13051, and nothing counted +# how often the device said no. +# +# The obvious fix -- skip the writes when model_family is "string" -- is the +# wrong one, and the last test here is why. Since 1.5.4 a zero-watt command is +# accepted whatever the family and writes EMS mode 2, so a device the driver +# calls "string" can be holding a forced state: a Sungrow hybrid shipped under +# a device-type code classify_device_type has not been taught lands in exactly +# that branch. The label does not separate the two cases. Whether the device +# took the write does. +# -------------------------------------------------------------------------- + +# The SG12RT as its Diagnose panel described it: device type 9268 (0x2434), +# no Sungrow meter wired, no hybrid block, and every EMS write refused. +SG12RT_IN_THE_FIELD = ''' +host._modbus_registers.input[4999] = {9268} +host._modbus_registers.input[5000] = {120} +host._modbus_registers.input[5016] = {4000, 0} +host._modbus_registers.input[5600] = {1500, 0} +host._modbus_write_error = "modbus exception function=0x06 code=0x02" +''' + fail_hybrid_block() + fail_battery_limit_block() + + +def watchdog_ticks(device: str, ticks: int = 8) -> str: + return device + f''' +dofile("{DRIVER}") +driver_init({{}}) +local per_tick, defaulted = {{}}, {{}} +for tick = 1, {ticks} do + host._modbus_write_attempts = 0 + defaulted[#defaulted + 1] = tostring(driver_default_mode() == true) + per_tick[#per_tick + 1] = host._modbus_write_attempts +end +print("WRITES_PER_TICK " .. table.concat(per_tick, ",")) +print("DEFAULTED " .. table.concat(defaulted, ",")) +local warns = 0 +for _, line in ipairs(host._logs) do + if string.find(line, "self-consumption reset write failed", 1, true) then + warns = warns + 1 + end +end +print("WARN_LINES " .. tostring(warns)) +''' + + +def test_the_watchdog_stops_writing_a_block_the_device_refuses() -> None: + """The field report. It never stopped.""" + out = run_lua(watchdog_ticks(SG12RT_IN_THE_FIELD)) + per_tick = [int(n) for n in out["WRITES_PER_TICK"].split(",")] + + assert per_tick[-1] == 0, ( + f"the watchdog was still writing on the last tick: {per_tick}. On an " + f"inverter that answers 13049-13051 with a Modbus exception this " + f"repeats for the life of the session, once per tick.") + assert per_tick[-3:] == [0, 0, 0], f"it must stay stopped: {per_tick}" + assert int(out["WARN_LINES"]) <= 3, ( + f"{out['WARN_LINES']} warn lines for one absent register block. The " + f"log is where an operator looks for the real fault, and this drowned " + f"it at one line per tick.") + + +def test_giving_up_on_the_write_is_not_reported_as_failing_it() -> None: + """driver_default_mode is the host's evidence the device is safe. + + While the driver is still attempting, a refused write is a real failure + and says so -- reporting success over a write that did not land is the + silent-failure shape #164 is about. What must not last is the failure: + once the driver has stopped attempting, there is nothing left to fail, and + a permanent false would have the watchdog escalate forever against an + inverter that never accepted a forced state in the first place. + """ + out = run_lua(watchdog_ticks(SG12RT_IN_THE_FIELD)) + defaulted = out["DEFAULTED"].split(",") + + assert defaulted[-1] == "true", ( + f"driver_default_mode answered {out['DEFAULTED']}. It still reports " + f"failure after it stopped trying, so the watchdog never settles.") + assert defaulted[-4:] == ["true"] * 4, ( + f"it has to stay settled: {out['DEFAULTED']}") + + +def test_the_startup_reset_is_always_attempted_at_least_once() -> None: + """A restart is exactly when a stale forced state has to be cleared. + + The count lives in the process, so a fresh start always tries. Persisting + it would break the one case the startup reset exists for. + """ + body = SG12RT_IN_THE_FIELD + f''' +dofile("{DRIVER}") +driver_init({{}}) +print("INIT_WRITES " .. tostring(host._modbus_write_attempts)) +''' + out = run_lua(body) + + assert int(out["INIT_WRITES"]) >= 3, ( + f"driver_init made {out['INIT_WRITES']} writes. A container that died " + f"mid-force leaves the inverter holding the last command, and this is " + f"the write that clears it.") + + +def test_a_forced_state_this_driver_did_not_set_is_still_released() -> None: + """Why the guard counts refusals rather than reading model_family. + + 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. + + This device is classified "string": 0x2434 is not a device-type code the + driver knows. Its EMS registers work anyway, and they are latched at mode + 2. Gate the release on the family and it stays there. Count refusals and it + is cleared, because nothing here has ever refused a write. + """ + body = ''' +host._modbus_registers.input[4999] = {9268} +host._modbus_registers.input[5000] = {120} +host._modbus_registers.input[5016] = {4000, 0} +-- Latched by something that is not this driver. +host._modbus_registers.holding[13049] = 2 +host._modbus_registers.holding[13050] = 0xAA +host._modbus_registers.holding[13051] = 2000 +''' + f''' +dofile("{DRIVER}") +print("BEFORE " .. tostring(host._modbus_registers.holding[13049])) +driver_init({{}}) +print("AFTER_INIT " .. tostring(host._modbus_registers.holding[13049])) +for tick = 1, 5 do driver_default_mode() end +print("AFTER_WATCHDOG " .. tostring(host._modbus_registers.holding[13049])) +''' + out = run_lua(body) + + assert out["BEFORE"] == "2", "the fixture has to start out forced" + assert out["AFTER_INIT"] == "0", ( + f"startup left the inverter at EMS mode {out['AFTER_INIT']}. It " + f"accepts these registers and was holding a forced charge set by " + f"something else, which is the case this reset exists for -- and the " + f"case a family-based guard would skip.") + assert out["AFTER_WATCHDOG"] == "0", out + + +def test_a_success_puts_the_release_back() -> None: + """Refusals are evidence, not a verdict. + + A confirmed hybrid whose bus refuses three writes stops being written + unprompted, like any other device. One command that lands proves the block + is there and puts the release back at once, without waiting for a restart. + """ + body = HEALTHY_HYBRID + f''' +dofile("{DRIVER}") +driver_init({{}}) +for poll = 1, 3 do pcall(driver_poll) end + +host._modbus_write_error = "modbus exception function=0x06 code=0x02" +for tick = 1, 4 do driver_default_mode() end +host._modbus_write_attempts = 0 +driver_default_mode() +print("GAVE_UP " .. tostring(host._modbus_write_attempts == 0)) + +-- The bus recovers and a battery command proves it. +host._modbus_write_error = nil +print("COMMAND_OK " .. tostring(driver_command("battery", 500, {{}}) == true)) +host._modbus_write_attempts = 0 +driver_default_mode() +print("RESUMED_WRITES " .. tostring(host._modbus_write_attempts)) +''' + out = run_lua(body) + + assert out["GAVE_UP"] == "true", "it must stop while the device refuses" + assert out["COMMAND_OK"] == "true", "a command always attempts" + assert int(out["RESUMED_WRITES"]) >= 3, ( + f"the release stayed off after a write landed. One success is proof " + f"the block is there, and a driver that will not hand back a device it " + f"can hand back is the worse failure. A restart also re-probes, but " + f"waiting for one is not the answer when the evidence is already in.") diff --git a/index.yaml b/index.yaml index acaa556..5d4fcaa 100644 --- a/index.yaml +++ b/index.yaml @@ -561,13 +561,13 @@ drivers: size_bytes: 6158 sha256: "92163e4356b895e16b0205e849aa42a2dcea0d799f8cc6bcbce60b5e2cd92285" - name: "sungrow" - version: "1.5.6" + version: "1.5.7" tier: core protocol: modbus ders: [battery, meter, pv] control: true - size_bytes: 44846 - sha256: "8f350fc209f7231139c23bc336695b6e00267fb4b3618e26c754c77fd3ac7e7d" + size_bytes: 48889 + sha256: "b549ad08470d6fe1600c3dec2eff8fc972dbb87aded9f15181cfa598b4b4ac42" - name: "tesla_vehicle" version: "0.2.0" tier: core diff --git a/manifests/sungrow.yaml b/manifests/sungrow.yaml index 99da9d3..88e5577 100644 --- a/manifests/sungrow.yaml +++ b/manifests/sungrow.yaml @@ -1,5 +1,5 @@ name: "sungrow" -version: "1.5.6" +version: "1.5.7" tier: core author: "Sourceful Labs AB" protocol: modbus @@ -42,9 +42,9 @@ tested_devices: 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: 44846 +size_bytes: 48889 dkb_id: "sungrow_sh_rt" -sha256: "8f350fc209f7231139c23bc336695b6e00267fb4b3618e26c754c77fd3ac7e7d" +sha256: "b549ad08470d6fe1600c3dec2eff8fc972dbb87aded9f15181cfa598b4b4ac42" signature: "" bytecode_sha256: "" diff --git a/support-status.json b/support-status.json index 90368a2..1dc75ef 100644 --- a/support-status.json +++ b/support-status.json @@ -1962,7 +1962,7 @@ }, { "catalog_source": true, - "catalog_version": "1.5.6", + "catalog_version": "1.5.7", "driver_id": "sungrow", "package_id": "com.sourceful.driver.sungrow", "targets": {