Tune the Sofar link from what its own polls measure (#182229)

homeassistant/components/sofar/__init__.py

@@ -6,6 +6,7 @@ from typing import TYPE_CHECKING
 
 from modbus_connection import ModbusError, ModbusTcpParams
 from sofar_modbus.modern.device import SofarInverter, identify
+from sofar_modbus.tuning import LinkTuner, TimedUnit
 
 from homeassistant.components.modbus import async_get_unit
 from homeassistant.components.sensor import (
@@ -143,8 +144,10 @@ async def async_setup_entry(hass: HomeAssistant, entry: SofarConfigEntry) -> boo
         entry.data[CONF_UNIT_ID],
     )
 
+    link = TimedUnit(unit)
+    tuner = LinkTuner(link)
     device = SofarInverter(
-        unit,
+        link,
         serial_number=serial,
         model=model,
         inverter_type=inverter_type,
@@ -157,6 +160,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: SofarConfigEntry) -> boo
         device,
         device.async_update_readings,
         timedelta(seconds=SCAN_INTERVAL),
+        tuner,
     )
     settings = SofarDataUpdateCoordinator(
         hass,
@@ -164,6 +168,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: SofarConfigEntry) -> boo
         device,
         device.async_update_settings,
         timedelta(seconds=SETTINGS_SCAN_INTERVAL),
+        tuner,
     )
     await readings.async_config_entry_first_refresh()
     await settings.async_refresh()
@@ -175,7 +180,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: SofarConfigEntry) -> boo
     inverter = dr.async_get(hass).async_get_or_create(
         config_entry_id=entry.entry_id, **readings.device_info
     )
-    entry.runtime_data = SofarRuntimeData(readings, settings, inverter.id)
+    entry.runtime_data = SofarRuntimeData(readings, settings, inverter.id, link, tuner)
     _async_remove_denied_meter_energy(
         hass, serial, entry.runtime_data.served_components
     )

homeassistant/components/sofar/coordinator.py

@@ -1,15 +1,16 @@
 """Data update coordinator for Sofar devices."""
 
-from collections.abc import Awaitable, Callable
+from collections.abc import Awaitable, Callable, Mapping
 from dataclasses import dataclass, field
 from datetime import timedelta
 import logging
 from typing import override
 
-from modbus_connection import ModbusConnectionError, ModbusError
+from modbus_connection import ModbusConnectionError, ModbusError, ModbusTimeoutError
 from propcache.api import cached_property
 from sofar_modbus.model import UpdateReport
 from sofar_modbus.modern.device import SofarInverter
+from sofar_modbus.tuning import LinkTuner, TimedUnit
 
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.core import HomeAssistant
@@ -34,6 +35,7 @@ class SofarDataUpdateCoordinator(DataUpdateCoordinator[UpdateReport]):
         device: SofarInverter,
         poll: Callable[[], Awaitable[UpdateReport]],
         interval: timedelta,
+        tuner: LinkTuner,
     ) -> None:
         """Initialize the coordinator."""
         super().__init__(
@@ -45,6 +47,7 @@ class SofarDataUpdateCoordinator(DataUpdateCoordinator[UpdateReport]):
         )
         self.device = device
         self._poll = poll
+        self._tuner = tuner
         self._consecutive_failures: dict[str, int] = {}
 
     @cached_property
@@ -65,8 +68,7 @@ class SofarDataUpdateCoordinator(DataUpdateCoordinator[UpdateReport]):
     @override
     async def _async_update_data(self) -> UpdateReport:
         try:
-            report = await self._poll()
-            report = await self._retry_failed(report)
+            report = await self._async_observed_poll()
             if not report.updated:
                 errors = list(report.failed.values())
                 if not errors:
@@ -88,6 +90,21 @@ class SofarDataUpdateCoordinator(DataUpdateCoordinator[UpdateReport]):
         else:
             return report
 
+    async def _async_observed_poll(self) -> UpdateReport:
+        """Poll once; a timeout either attempt hit must reach the tuner."""
+        attempted: dict[str, ModbusError] = {}
+        try:
+            report = await self._poll()
+            attempted = dict(report.failed)
+            report = await self._retry_failed(report)
+        except ModbusError as err:
+            self._tuner.observe_failure(_timed_out(attempted) or err)
+            raise
+        self._tuner.observe(
+            UpdateReport(report.updated, _both_attempts(attempted, report.failed))
+        )
+        return report
+
     async def _retry_failed(self, report: UpdateReport) -> UpdateReport:
         """Retry failures once; skip if none answered, to avoid doubling timeout."""
         if report.failed and report.updated:
@@ -121,6 +138,24 @@ class SofarDataUpdateCoordinator(DataUpdateCoordinator[UpdateReport]):
         return report
 
 
+def _timed_out(failures: Mapping[str, ModbusError]) -> ModbusTimeoutError | None:
+    """Whichever of these failures timed out, if any of them did."""
+    return next(
+        (err for err in failures.values() if isinstance(err, ModbusTimeoutError)), None
+    )
+
+
+def _both_attempts(
+    attempted: Mapping[str, ModbusError], retried: Mapping[str, ModbusError]
+) -> dict[str, ModbusError]:
+    """Both attempts' failures, a timeout outranking any other error."""
+    failures = dict(attempted)
+    for name, err in retried.items():
+        if not isinstance(failures.get(name), ModbusTimeoutError):
+            failures[name] = err
+    return failures
+
+
 @dataclass
 class SofarRuntimeData:
     """Class to hold runtime data."""
@@ -128,6 +163,8 @@ class SofarRuntimeData:
     readings: SofarDataUpdateCoordinator
     settings: SofarDataUpdateCoordinator
     inverter_device_id: str
+    link: TimedUnit
+    tuner: LinkTuner
     wired_packs: set[int] = field(default_factory=set)
 
     @property

homeassistant/components/sofar/diagnostics.py

@@ -1,5 +1,6 @@
 """Diagnostics support for Sofar."""
 
+from dataclasses import asdict
 from typing import Any
 
 from homeassistant.components.diagnostics import async_redact_data
@@ -16,7 +17,8 @@ async def async_get_config_entry_diagnostics(
     hass: HomeAssistant, entry: SofarConfigEntry
 ) -> dict[str, Any]:
     """Return diagnostics for a config entry."""
-    device = entry.runtime_data.readings.device
+    runtime_data = entry.runtime_data
+    device = runtime_data.readings.device
     raw = await device.async_read_raw()
     if (holding := raw.get("holding")) is not None:
         for address in _SERIAL_NUMBER_REGISTERS:
@@ -31,6 +33,10 @@ async def async_get_config_entry_diagnostics(
             "settings_components": device.settings_components,
             "active_faults": sorted(fault.key for fault in device.state.active_faults),
             "address_masks": await device.async_read_masks(),
+            "link": {
+                "tuning": asdict(runtime_data.tuner.tuning),
+                "stats": asdict(runtime_data.link.stats),
+            },
             "raw": raw,
         },
         TO_REDACT,

tests/components/sofar/snapshots/test_diagnostics.ambr

@@ -20,6 +20,23 @@
       '4480': 0,
     }),
     'inverter_type': 1537,
+    'link': dict({
+      'stats': dict({
+        'answered': 43,
+        'failures': dict({
+        }),
+        'median': float,
+        'p95': float,
+        'requests': 43,
+        'slowest': float,
+      }),
+      'tuning': dict({
+        'connect_delay': 0.0,
+        'spacing': 0.0,
+        'timeout': None,
+        'withdrawals': 0,
+      }),
+    }),
     'model': '4.4 KTLX-G3',
     'raw': dict({
       'holding': dict({

tests/components/sofar/test_diagnostics.py

@@ -4,6 +4,7 @@ from unittest.mock import patch
 
 from modbus_connection.mock import MockModbusConnection
 from syrupy.assertion import SnapshotAssertion
+from syrupy.matchers import path_type
 
 from homeassistant.core import HomeAssistant
 
@@ -21,7 +22,11 @@ async def test_diagnostics(
     """Test generating diagnostics for a config entry."""
     diag = await get_diagnostics_for_config_entry(hass, hass_client, init_integration)
 
-    assert diag == snapshot
+    assert diag == snapshot(
+        matcher=path_type(
+            {r"^link\.stats\.(median|p95|slowest)$": (float,)}, regex=True
+        )
+    )
 
 
 async def test_diagnostics_includes_active_faults(

tests/components/sofar/test_tuning.py

@@ -0,0 +1,189 @@
+"""Test the Sofar Inverter Modbus link tuning."""
+
+from datetime import timedelta
+
+from freezegun.api import FrozenDateTimeFactory
+from modbus_connection import (
+    ModbusConnectionError,
+    ModbusTimeoutError,
+    ServerDeviceBusyError,
+)
+from modbus_connection.mock import MockModbusConnection
+import pytest
+from sofar_modbus.model import UpdateReport
+
+from homeassistant.components.sofar.const import SCAN_INTERVAL
+from homeassistant.core import HomeAssistant
+
+from tests.common import MockConfigEntry, async_fire_time_changed
+from tests.components.diagnostics import get_diagnostics_for_config_entry
+from tests.typing import ClientSessionGenerator
+
+GRID_REGISTER = 0x0484
+
+CLEAN_POLLS = 5
+"""Polls the tuner wants before it acts on what it measured."""
+
+EARNED_TIMEOUT = 0.5
+"""The shortest ask the tuner makes, which a mocked link always earns."""
+
+
+async def _poll(
+    hass: HomeAssistant, freezer: FrozenDateTimeFactory, count: int
+) -> None:
+    """Run the readings coordinator ``count`` times."""
+    for _ in range(count):
+        freezer.tick(timedelta(seconds=SCAN_INTERVAL))
+        async_fire_time_changed(hass)
+        await hass.async_block_till_done()
+
+
+@pytest.mark.usefixtures("init_integration")
+async def test_tuner_lowers_the_link_timeout(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+) -> None:
+    """Test a link that answers cleanly is asked for a shorter timeout."""
+    unit = mock_connection.for_unit(1)
+    assert unit.required_timeout is None
+
+    await _poll(hass, freezer, CLEAN_POLLS)
+
+    assert unit.required_timeout == EARNED_TIMEOUT
+
+
+@pytest.mark.usefixtures("init_integration")
+async def test_tuner_withdraws_the_ask_after_a_timeout(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+) -> None:
+    """Test a timed-out poll hands the link its own timeout back."""
+    unit = mock_connection.for_unit(1)
+    await _poll(hass, freezer, CLEAN_POLLS)
+    assert unit.required_timeout == EARNED_TIMEOUT
+
+    unit.fail_read(GRID_REGISTER, ModbusTimeoutError("stuck"))
+    await _poll(hass, freezer, 1)
+
+    assert unit.required_timeout is None
+
+
+@pytest.mark.usefixtures("init_integration")
+async def test_tuner_withdraws_the_ask_when_the_poll_raises(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+) -> None:
+    """Test a poll that times out before anything answers is still heard."""
+    unit = mock_connection.for_unit(1)
+    await _poll(hass, freezer, CLEAN_POLLS)
+    assert unit.required_timeout == EARNED_TIMEOUT
+
+    unit.fail_requests(ModbusTimeoutError("link gone slow"))
+    await _poll(hass, freezer, 1)
+
+    assert unit.required_timeout is None
+
+
+async def test_tuner_hears_a_timeout_only_the_retry_hit(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+    init_integration: MockConfigEntry,
+) -> None:
+    """Test a component that times out only on the retry still withdraws it."""
+    unit = mock_connection.for_unit(1)
+    await _poll(hass, freezer, CLEAN_POLLS)
+    assert unit.required_timeout == EARNED_TIMEOUT
+
+    async def busy_grid() -> UpdateReport:
+        """A first attempt that failed without timing out."""
+        return UpdateReport({"state"}, {"grid": ServerDeviceBusyError("busy")})
+
+    init_integration.runtime_data.readings._poll = busy_grid
+    unit.fail_read(GRID_REGISTER, ModbusTimeoutError("stuck on retry"))
+    await _poll(hass, freezer, 1)
+
+    assert unit.required_timeout is None
+
+
+async def test_tuner_hears_a_timeout_the_retry_recovered(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+    init_integration: MockConfigEntry,
+) -> None:
+    """Test a timeout that answered on the second attempt still withdraws it."""
+    unit = mock_connection.for_unit(1)
+    await _poll(hass, freezer, CLEAN_POLLS)
+    assert unit.required_timeout == EARNED_TIMEOUT
+
+    async def timed_out_grid() -> UpdateReport:
+        """A first attempt that timed out; the retry finds the unit healthy."""
+        return UpdateReport({"state"}, {"grid": ModbusTimeoutError("slow")})
+
+    init_integration.runtime_data.readings._poll = timed_out_grid
+    await _poll(hass, freezer, 1)
+
+    assert unit.required_timeout is None
+
+
+async def test_tuner_hears_a_timeout_the_retry_reported_otherwise(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+    init_integration: MockConfigEntry,
+) -> None:
+    """Test a retry's own error does not bury the timeout that preceded it."""
+    unit = mock_connection.for_unit(1)
+    await _poll(hass, freezer, CLEAN_POLLS)
+    assert unit.required_timeout == EARNED_TIMEOUT
+
+    async def timed_out_grid() -> UpdateReport:
+        """A first attempt that timed out."""
+        return UpdateReport({"state"}, {"grid": ModbusTimeoutError("slow")})
+
+    init_integration.runtime_data.readings._poll = timed_out_grid
+    unit.fail_read(GRID_REGISTER, ServerDeviceBusyError("busy on retry"))
+    await _poll(hass, freezer, 1)
+
+    assert unit.required_timeout is None
+
+
+async def test_tuner_hears_a_timeout_when_the_retry_loses_the_link(
+    hass: HomeAssistant,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+    init_integration: MockConfigEntry,
+) -> None:
+    """Test a link dying during the retry does not bury the earlier timeout."""
+    unit = mock_connection.for_unit(1)
+    await _poll(hass, freezer, CLEAN_POLLS)
+    assert unit.required_timeout == EARNED_TIMEOUT
+
+    async def timed_out_grid() -> UpdateReport:
+        """A first attempt that timed out."""
+        return UpdateReport({"state"}, {"grid": ModbusTimeoutError("slow")})
+
+    init_integration.runtime_data.readings._poll = timed_out_grid
+    unit.fail_read(GRID_REGISTER, ModbusConnectionError("link gone"))
+    await _poll(hass, freezer, 1)
+
+    assert unit.required_timeout is None
+
+
+async def test_diagnostics_report_what_the_tuner_asked(
+    hass: HomeAssistant,
+    hass_client: ClientSessionGenerator,
+    freezer: FrozenDateTimeFactory,
+    mock_connection: MockModbusConnection,
+    init_integration: MockConfigEntry,
+) -> None:
+    """Test the tuning a link settled on reaches the diagnostics dump."""
+    await _poll(hass, freezer, CLEAN_POLLS)
+
+    diag = await get_diagnostics_for_config_entry(hass, hass_client, init_integration)
+
+    assert diag["link"]["tuning"]["timeout"] == EARNED_TIMEOUT