Skip to content

Apply READ_ATTEMPTS to the identity read #687

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
heinrich321 wants to merge 2 commits into
base: main
Choose a base branch
from
+39 −3
Open

Apply READ_ATTEMPTS to the identity read #687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
457df52
Apply READ_ATTEMPTS to the identity read
heinrich321 Aug 31, 2026
e363948
Keep protocol errors in the read ExceptionGroup
heinrich321 Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/ha_addon_sunsynk_multi/a_inverter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,11 @@ async def lifecycle_attempt_recovery(self) -> None:

async def read_identity(self) -> Identity:
"""Read device type, protocol, and serial via the Identity Component."""
# HoldingUnit duck-types ModbusUnit for FC03; SolarmanUnit is the same shape.
identity = Identity(self.inv.unit) # type: ignore[arg-type]
# Read through Sunsynk, not self.inv.unit: same read_holding_registers
# signature, but with READ_ATTEMPTS retries and the serial flush.
# ponytail: only holds while Identity reads holding registers only. A field
# in another space would need the unit's read_coils/read_input_registers.
identity = Identity(self.inv) # type: ignore[arg-type]

@kellerza kellerza Aug 31, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do you get the modbus ID from? This is part of the unit?

@heinrich321 heinrich321 Aug 31, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the modbus ID is on the unit, and it stays there.

Sunsynk.read_holding_registers just calls self.unit.read_holding_registers (sunsynk.py:170), so passing self.inv instead of self.inv.unit still lands on the same unit that driver.py:82 builds with conn.for_unit(iopt.modbus_id). Same for solarman, where the id sits on SolarmanUnit(server_id=iopt.modbus_id) at driver.py:67.

The only thing added in between is the retry, the serial flush and the timeouts counter.

I checked it with a real connection and for_unit(3), recording what actually hits the wire for the old and the new call:

wire calls: [{'unit_id': 3, 'address': 0, 'count': 8},
             {'unit_id': 3, 'address': 0, 'count': 8}]
serial before: 2303218594   after: 2303218594

First one is Identity(unit), second is Identity(ss). Same unit id, same result.

Happy to add an assert on the unit id to the test if you want that locked in.

@kellerza kellerza Sep 4, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but we still pass something that is not really a HoldingUnit

Can we rather add this to sunsynk.py?

Add a tiny retrying unit facade on Sunsynk, and pass that to Identity:

@dataclass(frozen=True, slots=True)
class _RetryingUnit:
    """HoldingUnit that routes FC03 through Sunsynk retry policy."""
    ss: Sunsynk
    @property
    def connected(self) -> bool:
        return self.ss.unit.connected
    async def read_holding_registers(self, address: int, count: int) -> list[int]:
        return list(await self.ss.read_holding_registers(address, count))
    async def write_registers(self, address: int, values: list[int]) -> None:
        await self.ss.unit.write_registers(address, values)

Then in read_identity:

identity = Identity(self.inv.retrying_unit)  # property returning _RetryingUnit(self)
await identity.async_update()

That gives you:

  • Correct modbus id (still ends up on self.inv.unit)
  • Sunsynk retry/flush behavior
  • A object that actually matches HoldingUnit
  • No coupling of Identity to the whole Sunsynk API

await identity.async_update()
self.identity = identity
return identity
Expand Down
3 changes: 2 additions & 1 deletion src/sunsynk/sunsynk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ async def read_holding_registers(self, start: int, length: int) -> Sequence[int]
self.connection._params, ModbusSerialParams
):
await self._flush_modbus_connection(reason=type(err).__name__)
except ModbusProtocolError:
except ModbusProtocolError as err:
errs.append(err)
_LOG.error(
"Read register %s x%s: protocol error [attempt %s/%s]",
start,
Expand Down
20 changes: 20 additions & 0 deletions src/tests/ha_addon_sunsynk_multi/test_a_inverter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from unittest.mock import AsyncMock, MagicMock, patch

import pytest
from modbus_connection import ModbusTimeoutError

from ha_addon_sunsynk_multi.a_inverter import AInverter
from ha_addon_sunsynk_multi.a_sensor import MQTT
Expand Down Expand Up @@ -289,3 +290,22 @@ async def test_connect_identity_failure_includes_cause() -> None:
await ist.connect()

assert "solarman://192.168.5.30:50500" in str(raised.value)


async def test_read_identity_retries_dropped_reply(state: InverterState) -> None:
"""READ_ATTEMPTS must cover the identity read (regs 0-7), not only sensor reads."""
unit = MagicMock()
unit.read_holding_registers = AsyncMock(
side_effect=[
ModbusTimeoutError("no reply"),
# device type 6, protocol 1.4, serial "2303218594"
[6, 0, 0x0104, 0x3233, 0x3033, 0x3231, 0x3835, 0x3934],
]
)
inv_opt = InverterOptions(modbus_id=1, ha_prefix="id", serial_nr="2303218594")
ss = Sunsynk(unit=unit, port="/dev/ttyUSB0", state=state, read_attempts=3)

identity = await _ist(inv_opt, ss, state=state).read_identity()

assert unit.read_holding_registers.await_count == 2
assert identity.serial == "2303218594"
12 changes: 12 additions & 0 deletions src/tests/sunsynk/test_sunsynk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from unittest.mock import MagicMock, call, patch

import pytest
from modbus_connection import ModbusProtocolError

from sunsynk import Sunsynk
from sunsynk.rwsensors import NumberRWSensor
Expand Down Expand Up @@ -177,3 +178,14 @@ def rhr_side_effect(start: int, length: int) -> Sequence[int]:
assert state[single] == 5
assert state[pair] == 7 << 16
assert state.registers == {1: 5, 10: 0, 11: 7}


async def test_ss_protocol_error_raises_exception_group() -> None:
"""Protocol errors must reach the ExceptionGroup, not leave it empty."""
unit = MagicMock()
unit.read_holding_registers.side_effect = ModbusProtocolError("desync")
ss = Sunsynk(unit=unit, read_attempts=2) # type: ignore[arg-type]

with pytest.raises(ExceptionGroup) as excinfo:
await ss.read_holding_registers(622, 4)
assert len(excinfo.value.exceptions) == 2