|
| 1 | +"""Response-length validation on the shared connection hub (Issue #367). |
| 2 | +
|
| 3 | +The defect this covers produced the worst class of bug in this integration: not a |
| 4 | +crash, not a missing sensor, but a *plausible-looking wrong number* written into Home |
| 5 | +Assistant's long-term statistics. |
| 6 | +
|
| 7 | +Registers are written into the cache positionally — `regs[0]` is assumed to be the |
| 8 | +block's start address, across 11 call sites. If a response is short, or is a stale |
| 9 | +frame from a different request, the words still get written sequentially from |
| 10 | +`start`, landing on addresses they never belonged to. The reporter decoded their own |
| 11 | +corrupt values and found 0x33325354 = "32ST" — four characters of the inverter's |
| 12 | +serial number — published as 85,893,614.8 W of AC power. |
| 13 | +
|
| 14 | +The non-shared path has checked response length since v1.3.5. The shared hub did not, |
| 15 | +and because a hub is created for *every* TCP entry (not only genuinely shared ones), |
| 16 | +the guard in practice only ever covered serial/RTU users. |
| 17 | +
|
| 18 | +Length is compared with != rather than <: a response longer than requested is an |
| 19 | +equally strong sign of a misaligned frame. |
| 20 | +""" |
| 21 | +from __future__ import annotations |
| 22 | + |
| 23 | +import importlib |
| 24 | + |
| 25 | +import pytest |
| 26 | + |
| 27 | +_gm = importlib.import_module("growatt_under_test.growatt_modbus") |
| 28 | +SharedModbusConnection = _gm.SharedModbusConnection |
| 29 | + |
| 30 | + |
| 31 | +class _Response: |
| 32 | + def __init__(self, registers=None, error=False): |
| 33 | + self.registers = [] if registers is None else registers |
| 34 | + self._error = error |
| 35 | + |
| 36 | + def isError(self): # noqa: N802 - pymodbus spelling |
| 37 | + return self._error |
| 38 | + |
| 39 | + |
| 40 | +class _FakeClient: |
| 41 | + def __init__(self, response): |
| 42 | + self.response = response |
| 43 | + |
| 44 | + def close(self): |
| 45 | + pass |
| 46 | + |
| 47 | + def connect(self): |
| 48 | + return True |
| 49 | + |
| 50 | + def is_socket_open(self): |
| 51 | + return True |
| 52 | + |
| 53 | + def read_input_registers(self, *args, **kwargs): |
| 54 | + return self.response |
| 55 | + |
| 56 | + def read_holding_registers(self, *args, **kwargs): |
| 57 | + return self.response |
| 58 | + |
| 59 | + |
| 60 | +def _hub(response) -> SharedModbusConnection: |
| 61 | + hub = SharedModbusConnection(host="10.0.0.1", port=502) |
| 62 | + hub._client = _FakeClient(response) |
| 63 | + hub.begin_poll() |
| 64 | + return hub |
| 65 | + |
| 66 | + |
| 67 | +def _count_flushes(hub, monkeypatch) -> list: |
| 68 | + calls = [] |
| 69 | + monkeypatch.setattr(hub, "_flush_receive_buffer", lambda: calls.append(1)) |
| 70 | + return calls |
| 71 | + |
| 72 | + |
| 73 | +# -------------------------------------------------------------------------- |
| 74 | +# The exact-length case must still work |
| 75 | +# -------------------------------------------------------------------------- |
| 76 | + |
| 77 | +@pytest.mark.parametrize("reader", ["read_input_registers", "read_holding_registers"]) |
| 78 | +def test_exact_length_response_is_returned(reader): |
| 79 | + hub = _hub(_Response([10, 20, 30, 40])) |
| 80 | + assert getattr(hub, reader)(100, 4, 1) == [10, 20, 30, 40] |
| 81 | + |
| 82 | + |
| 83 | +# -------------------------------------------------------------------------- |
| 84 | +# Short reads — the truncation case |
| 85 | +# -------------------------------------------------------------------------- |
| 86 | + |
| 87 | +@pytest.mark.parametrize("reader", ["read_input_registers", "read_holding_registers"]) |
| 88 | +def test_short_response_is_rejected(reader): |
| 89 | + """Two of four registers arrived. Salvaging them would map regs[0..1] onto the |
| 90 | + right addresses but leave the rest stale — and the caller cannot tell.""" |
| 91 | + hub = _hub(_Response([10, 20])) |
| 92 | + assert getattr(hub, reader)(100, 4, 1) is None |
| 93 | + |
| 94 | + |
| 95 | +@pytest.mark.parametrize("reader", ["read_input_registers", "read_holding_registers"]) |
| 96 | +def test_empty_response_is_rejected(reader): |
| 97 | + hub = _hub(_Response([])) |
| 98 | + assert getattr(hub, reader)(100, 4, 1) is None |
| 99 | + |
| 100 | + |
| 101 | +# -------------------------------------------------------------------------- |
| 102 | +# Long reads — the stale/misaligned frame case |
| 103 | +# -------------------------------------------------------------------------- |
| 104 | + |
| 105 | +@pytest.mark.parametrize("reader", ["read_input_registers", "read_holding_registers"]) |
| 106 | +def test_overlong_response_is_rejected(reader): |
| 107 | + """A response longer than requested cannot be a valid answer to this request. |
| 108 | +
|
| 109 | + This is the case `< count` would have let through, and it is exactly the shape a |
| 110 | + stale frame from a *different* (larger) request takes. |
| 111 | + """ |
| 112 | + hub = _hub(_Response([10, 20, 30, 40, 50, 60])) |
| 113 | + assert getattr(hub, reader)(100, 4, 1) is None |
| 114 | + |
| 115 | + |
| 116 | +# -------------------------------------------------------------------------- |
| 117 | +# A misaligned stream must be drained, not inherited |
| 118 | +# -------------------------------------------------------------------------- |
| 119 | + |
| 120 | +@pytest.mark.parametrize("reader", ["read_input_registers", "read_holding_registers"]) |
| 121 | +def test_length_mismatch_flushes_the_buffer(reader, monkeypatch): |
| 122 | + """Why the corrupt values repeated byte-for-byte rather than varying: the same |
| 123 | + stale bytes sat at the same offset on every poll.""" |
| 124 | + hub = _hub(_Response([10, 20])) |
| 125 | + flushes = _count_flushes(hub, monkeypatch) |
| 126 | + |
| 127 | + getattr(hub, reader)(100, 4, 1) |
| 128 | + |
| 129 | + assert len(flushes) == 1 |
| 130 | + |
| 131 | + |
| 132 | +@pytest.mark.parametrize("reader", ["read_input_registers", "read_holding_registers"]) |
| 133 | +def test_good_read_does_not_flush(reader, monkeypatch): |
| 134 | + """The flush is a recovery action, not a per-read tax.""" |
| 135 | + hub = _hub(_Response([10, 20, 30, 40])) |
| 136 | + flushes = _count_flushes(hub, monkeypatch) |
| 137 | + |
| 138 | + getattr(hub, reader)(100, 4, 1) |
| 139 | + |
| 140 | + assert flushes == [] |
| 141 | + |
| 142 | + |
| 143 | +# -------------------------------------------------------------------------- |
| 144 | +# Protocol refusals keep their existing behaviour (#360, #361) |
| 145 | +# -------------------------------------------------------------------------- |
| 146 | + |
| 147 | +@pytest.mark.parametrize("reader", ["read_input_registers", "read_holding_registers"]) |
| 148 | +def test_error_response_still_returns_none_without_flushing(reader, monkeypatch): |
| 149 | + """An Illegal Address reply means the device answered and declined. Several |
| 150 | + profiles probe ranges their hardware rejects on every poll, so this path must |
| 151 | + stay cheap — no flush, no reset.""" |
| 152 | + hub = _hub(_Response([], error=True)) |
| 153 | + flushes = _count_flushes(hub, monkeypatch) |
| 154 | + |
| 155 | + assert getattr(hub, reader)(100, 4, 1) is None |
| 156 | + assert flushes == [] |
| 157 | + |
| 158 | + |
| 159 | +# -------------------------------------------------------------------------- |
| 160 | +# The regression, stated in the reporter's own terms |
| 161 | +# -------------------------------------------------------------------------- |
| 162 | + |
| 163 | +def test_serial_number_frame_cannot_reach_the_register_cache(): |
| 164 | + """0x33325354 = "32ST" — characters 9-12 of the reporter's serial number, which |
| 165 | + were published as 85,893,614.8 W of AC power. |
| 166 | +
|
| 167 | + A stale frame carrying string-register content is rejected on length before any |
| 168 | + positional write can occur, so those words never reach the addresses that decode |
| 169 | + as power. |
| 170 | + """ |
| 171 | + hub = _hub(_Response([0x3332, 0x5354])) # "32" "ST" |
| 172 | + |
| 173 | + # A four-register power block was requested; two words came back. |
| 174 | + assert hub.read_input_registers(3004, 4, 1) is None |
0 commit comments