From fa8d4b8cf350e6a449abbe1fdfa89f4559613df4 Mon Sep 17 00:00:00 2001 From: sphings79 <43515272+sphings79@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:12:27 +0200 Subject: [PATCH] fix(marstek): let a Venus D read outlast the battery's five-minute stall A Venus D stops answering for about four seconds on a five-minute clock: 138 stalls over eleven hours, mean 4.05 s, longest 4.46 s, measured at the Modbus proxy between Home Assistant and the battery, so it is the device and not the link. The per-attempt timeout was three seconds, chosen so a pymodbus retry - which re-sends the same transaction id - would land inside such a stall and consume the late reply. Against a four second stall it does not: the attempt expires every single time, the retry duplicates the request, and the battery answers both. One of the two replies then arrives with nothing waiting for it and is discarded, either as "received pdu without a corresponding request" or as a transaction id mismatch against the request that followed. Twelve times an hour. Six seconds covers the stall with room over the longest one seen. The cost is that a battery that is genuinely gone gets longer before the attempt is abandoned - the outer safety net grows from 11 s to 20 s. No value was being lost either way, the retry saw to that (one failed key in eleven hours, unrelated). What goes is the wasted round trip and a log that reports a fault twelve times an hour for a battery that is merely four seconds late. v3 and vA keep their three seconds: same family, but the measurement is from a Venus D on EMS v150 and nowhere else. --- CHANGELOG.md | 2 ++ .../omnibattery/const/registers_common.py | 25 ++++++++++++-- tests/test_marstek_driver.py | 34 +++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1b25292..9d554864 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ### Fixed +- **A Venus D no longer loses a reply every five minutes**: the battery stops answering for about four seconds on a five-minute clock — 138 times over eleven hours, longest 4.46 s, measured at the Modbus proxy, so it is the device and not the network. The per-attempt read timeout was three seconds, so it expired inside every one of those stalls; the retry then put a duplicate request on the wire and the battery answered both, leaving one reply nobody was waiting for. That is the `received pdu without a corresponding request` and `transaction_id mismatch` pair in the log, twelve times an hour. The Venus D now waits six seconds per attempt, which covers the stall with room to spare. No values were being lost either way — this removes the wasted round trip and the noise. Unchanged for v3 and vA, where the stall has not been measured. + - **Predictive charging now covers the night through to sunrise** (#410, #344): every layer that decided to buy grid energy stopped at midnight, so the hours between midnight and sunrise belonged to no plan and the battery reached the morning peak empty. If you raised the solar safety margin or the grid charge margin to cover the night by hand, lower them again. The "Guaranteed Minimum SOC" help text now describes what that setting actually does — a floor on the current SOC, not a morning target. - **The discharge reserve now holds energy for pre-dawn price peaks** (#410): it stopped reserving at midnight, on the grounds that tomorrow's sun would refill the battery anyway. There is no sun before dawn, so a peak at 06:00 was left to the grid. The reserve now reaches the next sunrise, and still never holds energy for tomorrow's evening peak. Only affects the opt-in "Discharge reserve" (#400). - **Dynamic Pricing re-plans when tomorrow's prices are published**: the 00:05 plan can only see today, so energy for the small hours was booked into today's slots even when the next day's were cheaper. Once the provider publishes (~13:00 CET), the remaining day is re-planned. Once a day, and never while a charge slot is running. diff --git a/custom_components/omnibattery/const/registers_common.py b/custom_components/omnibattery/const/registers_common.py index fb32a84b..054732f2 100644 --- a/custom_components/omnibattery/const/registers_common.py +++ b/custom_components/omnibattery/const/registers_common.py @@ -88,13 +88,32 @@ # occasionally stalls for several seconds and then flushes queued replies in a # burst. Each pymodbus-internal retry re-sends the SAME transaction_id (>=3.8), # so a reply arriving after a per-attempt timeout still matches the retry and -# is consumed (issue #361 history in infra/modbus_client.py). Keep v3 attempts -# short so the retries land inside the stall window. +# is consumed (issue #361 history in infra/modbus_client.py). +# +# That is why these were kept short: let the retry land inside the stall. On a +# Venus D it does not work out that way. The stall is a regular one - every +# five minutes the battery stops answering for about four seconds, 138 times +# over eleven hours, mean 4.05 s, longest 4.46 s, measured at the Modbus proxy +# so it is the device and not the client. A three second attempt expires inside +# every one of them, the retry puts a duplicate request on the wire, and the +# battery answers both: the first reply arrives with nothing waiting for it and +# is discarded ("received pdu without a corresponding request"), or lands while +# the next request is outstanding and is discarded as a transaction id mismatch. +# Twelve times an hour, for a stall the battery always comes back from. +# +# Six seconds covers it with room to spare and the noise stops. The cost is +# that a battery that is genuinely gone is given longer before the attempt is +# abandoned - the outer safety net in infra/modbus_client.py grows from 11 s to +# 20 s - which is the right trade for a device whose only failure mode here is +# being four seconds late. +# +# v3 and vA are left as they are: the same family, but this was measured on a +# Venus D (EMS v150) and nowhere else. READ_TIMEOUT_S = { "v2": 10, "v3": 3, "vA": 3, - "vD": 3, + "vD": 6, } # Standalone bit-description maps — used by both sensor definitions and the diff --git a/tests/test_marstek_driver.py b/tests/test_marstek_driver.py index d6ad4b6f..667ca19f 100644 --- a/tests/test_marstek_driver.py +++ b/tests/test_marstek_driver.py @@ -22,6 +22,7 @@ from custom_components.omnibattery.const import ( MESSAGE_WAIT_MS, MESSAGE_WAIT_MS_RS485_GATEWAY, + READ_TIMEOUT_S, REGISTER_MAP, max_power_for_battery_version, ) @@ -1145,3 +1146,36 @@ def _fake_client_factory(*args, **kwargs): # The gateway replaces the TCP server for every firmware version, v2 too. MarstekModbusDriver("1.2.3.4", 502, "v2", rs485_gateway=True) assert captured["message_wait_ms"] == MESSAGE_WAIT_MS_RS485_GATEWAY + + +# The stall a Venus D takes every five minutes, measured at the Modbus proxy +# over eleven hours: 138 of them, mean 4.05 s, longest 4.46 s. +VENUS_D_STALL_S = 4.46 + + +def test_venus_d_attempt_outlasts_the_five_minute_stall(): + """A per-attempt timeout shorter than the stall expires in every one of them. + + The retry then duplicates the request, the battery answers both, and one of + the two replies is discarded as unmatched - twelve times an hour on a + battery that always came back. + """ + assert READ_TIMEOUT_S["vD"] > VENUS_D_STALL_S + 1 + + +def test_per_attempt_timeout_reaches_the_client(monkeypatch): + """Whatever the map says for a version is what the client is built with.""" + captured = {} + + def _fake_client_factory(*args, **kwargs): + captured.update(kwargs) + return _fake_client() + + monkeypatch.setattr( + "custom_components.omnibattery.drivers.marstek.MarstekModbusClient", + _fake_client_factory, + ) + + for version in ("v2", "v3", "vA", "vD"): + MarstekModbusDriver("1.2.3.4", 502, version) + assert captured["timeout"] == READ_TIMEOUT_S[version], version