From 194997727e2b6601a69e0c0b5112d47af45a4b99 Mon Sep 17 00:00:00 2001 From: darkrain-nl Date: Fri, 25 Sep 2026 07:17:25 +0200 Subject: [PATCH] Tune the Sofar link from what its own polls measure (#182229) --- homeassistant/components/sofar/__init__.py | 9 +- homeassistant/components/sofar/coordinator.py | 45 ++++- homeassistant/components/sofar/diagnostics.py | 8 +- .../sofar/snapshots/test_diagnostics.ambr | 17 ++ tests/components/sofar/test_diagnostics.py | 7 +- tests/components/sofar/test_tuning.py | 189 ++++++++++++++++++ 6 files changed, 267 insertions(+), 8 deletions(-) create mode 100644 tests/components/sofar/test_tuning.py diff --git a/homeassistant/components/sofar/__init__.py b/homeassistant/components/sofar/__init__.py index e1c77f8154a0..df50630c259d 100644 --- a/homeassistant/components/sofar/__init__.py +++ b/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 ) diff --git a/homeassistant/components/sofar/coordinator.py b/homeassistant/components/sofar/coordinator.py index 93e45962c8aa..04bc54c9eb75 100644 --- a/homeassistant/components/sofar/coordinator.py +++ b/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 diff --git a/homeassistant/components/sofar/diagnostics.py b/homeassistant/components/sofar/diagnostics.py index e421bc2e70ee..697edac6bb93 100644 --- a/homeassistant/components/sofar/diagnostics.py +++ b/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, diff --git a/tests/components/sofar/snapshots/test_diagnostics.ambr b/tests/components/sofar/snapshots/test_diagnostics.ambr index 544a685691b3..f676db3dda70 100644 --- a/tests/components/sofar/snapshots/test_diagnostics.ambr +++ b/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({ diff --git a/tests/components/sofar/test_diagnostics.py b/tests/components/sofar/test_diagnostics.py index e1a0d7588b16..92ed3a9b7d9a 100644 --- a/tests/components/sofar/test_diagnostics.py +++ b/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( diff --git a/tests/components/sofar/test_tuning.py b/tests/components/sofar/test_tuning.py new file mode 100644 index 000000000000..8424ebfd7bec --- /dev/null +++ b/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