From 919b0a05d3739192f1c05445465c31b9e2aded59 Mon Sep 17 00:00:00 2001 From: fboundy Date: Fri, 3 Jul 2026 21:45:38 +0100 Subject: [PATCH] Fix dual-protocol robustness gaps (#13) Recovery edits now route through the hub so a bad-register-block recovery lands on both protocol controllers instead of being lost when the hub switches; enable_connection/disable_connection likewise apply to both. Duplicate config entries migrating to the same inverter serial are now caught and skipped instead of silently colliding. Overlapping health-tick probes of a dead standby link are now guarded against. Unexpected poll exceptions are logged at warning instead of debug. The number entity's write path no longer fires a fire-and-forget task and optimistically updates regardless of success; it awaits the write like the time entity already did. The binary/switch entity now pushes state immediately on a register update instead of relying on polling. Fixes #13 --- custom_components/solisconnect/__init__.py | 21 +++++- .../solisconnect/data_retrieval.py | 10 ++- .../solisconnect/inverter_hub.py | 65 +++++++++++++------ .../sensors/solis_binary_sensor.py | 2 + .../sensors/solis_number_sensor.py | 7 +- tests/test_inverter_hub.py | 46 +++++++++++++ tests/test_migration.py | 19 ++++++ tests/test_number.py | 4 +- tests/test_register_group_recovery.py | 50 ++++++++++++++ tests/test_s16_encoding.py | 36 +++++----- tests/test_switch_conflicts_requires.py | 34 ++++++++++ 11 files changed, 247 insertions(+), 47 deletions(-) diff --git a/custom_components/solisconnect/__init__.py b/custom_components/solisconnect/__init__.py index f7bd5c2..dab1b3f 100644 --- a/custom_components/solisconnect/__init__.py +++ b/custom_components/solisconnect/__init__.py @@ -487,8 +487,25 @@ def get_old_id(uid, ctrl): # --- CONFIG ENTRY LOGIC --- if entry.unique_id != inverter_serial: - _LOGGER.info("Migrating Config Entry ID from %s to %s", entry.unique_id, inverter_serial) - hass.config_entries.async_update_entry(entry, unique_id=inverter_serial) + # async_update_entry does not enforce unique_id uniqueness the way the config-flow's + # _abort_if_unique_id_configured() does, so a leftover duplicate entry for this same + # inverter (e.g. an old cloud-only entry alongside a new dual-protocol one) could + # otherwise migrate to an identical unique_id and cross-write the shared cache. + existing = next( + (e for e in hass.config_entries.async_entries(DOMAIN) if e.entry_id != entry.entry_id and e.unique_id == inverter_serial), + None, + ) + if existing is not None: + _LOGGER.warning( + "Cannot migrate config entry %s to serial %s: entry %s already uses this serial. " + "Remove the duplicate entry for this inverter before reconfiguring.", + entry.entry_id, + inverter_serial, + existing.entry_id, + ) + else: + _LOGGER.info("Migrating Config Entry ID from %s to %s", entry.unique_id, inverter_serial) + hass.config_entries.async_update_entry(entry, unique_id=inverter_serial) return True diff --git a/custom_components/solisconnect/data_retrieval.py b/custom_components/solisconnect/data_retrieval.py index a538f9b..5123abc 100644 --- a/custom_components/solisconnect/data_retrieval.py +++ b/custom_components/solisconnect/data_retrieval.py @@ -24,9 +24,13 @@ class DataRetrieval: - def __init__(self, hass: HomeAssistant, controller: ModbusController): + def __init__(self, hass: HomeAssistant, controller: ModbusController, hub=None): self._spike_counter = {} self.controller: ModbusController = controller + # When this retrieval is hub-managed, sensor-group recovery edits are applied through + # the hub so they also land on the standby protocol controller (issue #13) instead of + # being lost the next time the hub switches protocols. + self.hub = hub self.hass = hass self.poll_lock = asyncio.Lock() self.connection_check = False @@ -137,7 +141,7 @@ async def _recover_sensor_group_after_modbus_failure( clusters = cluster_sensors_by_contiguous_registers(remaining) new_groups = [SolisSensorGroup.from_sensors(c, sensor_group.poll_speed, sensor_group.identification) for c in clusters] - self.controller.replace_sensor_group(sensor_group, new_groups) + (self.hub or self.controller).replace_sensor_group(sensor_group, new_groups) disabled_names = ", ".join(s.name for s in disabled_sensors) or "(unknown)" _LOGGER.warning( @@ -413,7 +417,7 @@ async def get_modbus_updates(self, groups: list[SolisSensorGroup], speed: PollSp total_duration = time.perf_counter() - total_start_time _LOGGER.debug(f"✅ {speed.name} update completed in {total_duration:.4f}s") except Exception as e: - _LOGGER.debug("exception caught: %s", e) + _LOGGER.warning("(%s.%s) Unexpected error during %s poll: %s", self.controller.host, self.controller.slave, speed.name, e, exc_info=True) finally: del self.poll_updating[speed][group_hash] # ✅ Reset only this group set diff --git a/custom_components/solisconnect/inverter_hub.py b/custom_components/solisconnect/inverter_hub.py index bbdda13..40e2c24 100644 --- a/custom_components/solisconnect/inverter_hub.py +++ b/custom_components/solisconnect/inverter_hub.py @@ -62,6 +62,12 @@ def __init__(self, hass, entry_id: str, inverter_config: InverterConfig, serial_ # Serializes protocol switches: _health_tick, the protocol select entity, and its # restore path can all call async_set_active concurrently. self._switch_lock = asyncio.Lock() + # Guards against overlapping _health_tick runs: a probe of a dead standby link + # (connect() + read) can take longer than HEALTH_CHECK_INTERVAL_SECONDS, and + # async_track_time_interval fires on schedule regardless of whether the previous + # callback finished, so a second tick could otherwise race the first's in-flight + # probe and its (unlocked) _primary_healthy_since/_last_switch bookkeeping. + self._health_tick_running = False # --- lifecycle ----------------------------------------------------------------- @@ -88,7 +94,7 @@ def _start_retrieval(self, protocol: str): if protocol == PROTOCOL_MODBUS: from custom_components.solisconnect.data_retrieval import DataRetrieval - self._retrieval = DataRetrieval(self.hass, self.modbus) + self._retrieval = DataRetrieval(self.hass, self.modbus, hub=self) else: from custom_components.solisconnect.cloud.cloud_retrieval import CloudDataRetrieval @@ -142,23 +148,30 @@ async def _probe_primary(self) -> bool: return False async def _health_tick(self, now=None): - dwell = (datetime.now(UTC) - self._last_switch).total_seconds() - secondary = PROTOCOL_CLOUD if self.failover_primary == PROTOCOL_MODBUS else PROTOCOL_MODBUS - - if self._active_name == self.failover_primary: - if not self._is_active_healthy() and dwell >= MIN_DWELL_SECONDS and self._controller_for(secondary) is not None: - await self.async_set_active(secondary, reason="primary unhealthy") + if self._health_tick_running: + _LOGGER.debug("(%s) Skipping health tick: a previous probe is still running", self.cache_scope) return + self._health_tick_running = True + try: + dwell = (datetime.now(UTC) - self._last_switch).total_seconds() + secondary = PROTOCOL_CLOUD if self.failover_primary == PROTOCOL_MODBUS else PROTOCOL_MODBUS - # Running on the secondary: probe the primary and auto-return with hysteresis - if await self._probe_primary(): - if self._primary_healthy_since is None: - self._primary_healthy_since = datetime.now(UTC) - healthy_for = (datetime.now(UTC) - self._primary_healthy_since).total_seconds() - if healthy_for >= PRIMARY_RECOVERY_SECONDS and dwell >= MIN_DWELL_SECONDS: - await self.async_set_active(self.failover_primary, reason="primary recovered") - else: - self._primary_healthy_since = None + if self._active_name == self.failover_primary: + if not self._is_active_healthy() and dwell >= MIN_DWELL_SECONDS and self._controller_for(secondary) is not None: + await self.async_set_active(secondary, reason="primary unhealthy") + return + + # Running on the secondary: probe the primary and auto-return with hysteresis + if await self._probe_primary(): + if self._primary_healthy_since is None: + self._primary_healthy_since = datetime.now(UTC) + healthy_for = (datetime.now(UTC) - self._primary_healthy_since).total_seconds() + if healthy_for >= PRIMARY_RECOVERY_SECONDS and dwell >= MIN_DWELL_SECONDS: + await self.async_set_active(self.failover_primary, reason="primary recovered") + else: + self._primary_healthy_since = None + finally: + self._health_tick_running = False # --- delegated surface (entities and services talk to the hub) --- @@ -207,13 +220,27 @@ def last_modbus_success(self): return self.active.last_modbus_success def replace_sensor_group(self, old_group, new_groups): - self.active.replace_sensor_group(old_group, new_groups) + """Apply a recovery edit to every configured controller, not just the active one. + + Modbus and cloud controllers are handed the same sensor-group list objects at + setup, but replace_sensor_group reassigns `_sensor_groups` to a new list on + whichever controller instance it's called on — so applying it to only the + active controller silently loses the recovered layout on the standby protocol + once a switch happens. + """ + for controller in (self.modbus, self.cloud): + if controller is not None: + controller.replace_sensor_group(old_group, new_groups) def enable_connection(self): - self.active.enable_connection() + for controller in (self.modbus, self.cloud): + if controller is not None: + controller.enable_connection() def disable_connection(self): - self.active.disable_connection() + for controller in (self.modbus, self.cloud): + if controller is not None: + controller.disable_connection() def close_connection(self): for controller in (self.modbus, self.cloud): diff --git a/custom_components/solisconnect/sensors/solis_binary_sensor.py b/custom_components/solisconnect/sensors/solis_binary_sensor.py index 677b906..73624c3 100644 --- a/custom_components/solisconnect/sensors/solis_binary_sensor.py +++ b/custom_components/solisconnect/sensors/solis_binary_sensor.py @@ -74,6 +74,7 @@ def handle_modbus_update(self, data): if self._register == 5: self._attr_is_on = self._modbus_controller.enabled self._attr_available = True + self.schedule_update_ha_state() return self._attr_is_on value = updated_value @@ -88,6 +89,7 @@ def handle_modbus_update(self, data): if self._on_value is not None: self._attr_is_on = value == self._on_value _LOGGER.debug(f"switch {self.unique_id} set to {self._attr_is_on}, value = {updated_value}") + self.schedule_update_ha_state() @property def is_on(self): diff --git a/custom_components/solisconnect/sensors/solis_number_sensor.py b/custom_components/solisconnect/sensors/solis_number_sensor.py index 2f43dd5..6766921 100644 --- a/custom_components/solisconnect/sensors/solis_number_sensor.py +++ b/custom_components/solisconnect/sensors/solis_number_sensor.py @@ -125,7 +125,7 @@ def handle_modbus_update(self, data): self._attr_native_value = new_value self.schedule_update_ha_state() - def set_native_value(self, value): + async def async_set_native_value(self, value): """Update the current value.""" if self._attr_native_value == value: return @@ -139,8 +139,9 @@ def set_native_value(self, value): register_value = max(-32768, min(32767, register_value)) register_value &= 0xFFFF - # Write to Modbus controller - self._hass.create_task(self.base_sensor.controller.async_write_holding_register(self._write_register, int(register_value))) + # Await the write so a failure raises to the caller instead of silently + # optimistic-updating the entity for a write that never happened. + await self.base_sensor.controller.async_write_holding_register(self._write_register, int(register_value)) self._attr_native_value = value self.schedule_update_ha_state() diff --git a/tests/test_inverter_hub.py b/tests/test_inverter_hub.py index 3bdb369..3960f09 100644 --- a/tests/test_inverter_hub.py +++ b/tests/test_inverter_hub.py @@ -218,3 +218,49 @@ async def test_write_fallback_requires_healthy_other(hass): with pytest.raises(HomeAssistantError, match="no mapping"): await hub.async_write_holding_register(43000, 26) hub.modbus.async_write_holding_register.assert_not_awaited() + + +def test_replace_sensor_group_applies_to_both_controllers(hass): + """Bug #13: a recovery edit applied only to the active controller is lost across a switch.""" + hub = _hub(hass, PROTO_BOTH_MANUAL) + hub.modbus.replace_sensor_group = MagicMock() + hub.cloud.replace_sensor_group = MagicMock() + + old_group, new_groups = object(), [object(), object()] + hub.replace_sensor_group(old_group, new_groups) + + hub.modbus.replace_sensor_group.assert_called_once_with(old_group, new_groups) + hub.cloud.replace_sensor_group.assert_called_once_with(old_group, new_groups) + + +def test_enable_disable_connection_applies_to_both_controllers(hass): + """Bug #13: toggling the connection switch must not leave the standby protocol untouched.""" + hub = _hub(hass, PROTO_BOTH_MANUAL) + + hub.enable_connection() + hub.modbus.enable_connection.assert_called_once() + hub.cloud.enable_connection.assert_called_once() + + hub.disable_connection() + hub.modbus.disable_connection.assert_called_once() + hub.cloud.disable_connection.assert_called_once() + + +async def test_health_tick_skips_when_previous_probe_still_running(hass): + """Bug #13: overlapping timer fires must not race the in-flight probe's state bookkeeping.""" + hub = _hub(hass, PROTO_BOTH_FAILOVER, failover_primary=PROTOCOL_MODBUS) + modbus_patch, cloud_patch = _patch_retrievals() + with modbus_patch as modbus_retrieval, cloud_patch: + modbus_retrieval.return_value.async_stop = AsyncMock() + await hub.async_start() + hub._health_tick_running = True + try: + hub.modbus.last_modbus_success = datetime.now(UTC) - timedelta(seconds=600) + hub._last_switch = datetime.now(UTC) - timedelta(seconds=MIN_DWELL_SECONDS + 1) + await hub._health_tick() + # The tick returned immediately without evaluating failover at all + assert hub.active_protocol == PROTOCOL_MODBUS + finally: + hub._health_tick_running = False + + await hub.async_stop() diff --git a/tests/test_migration.py b/tests/test_migration.py index a32d656..b190fcf 100644 --- a/tests/test_migration.py +++ b/tests/test_migration.py @@ -81,3 +81,22 @@ async def test_missing_serial(self, mock_get_registry): # Should be False because migration is deferred when serial is missing assert result is False + + @patch("homeassistant.helpers.entity_registry.async_get") + async def test_migrate_skips_when_serial_already_used_by_another_entry(self, mock_get_registry): + """Bug #13: migrating to a unique_id already claimed by a different entry must not collide.""" + mock_get_registry.return_value = self.registry + self.registry.async_get_entity_id.return_value = None + + other_entry = MagicMock() + other_entry.entry_id = "other_entry_id" + other_entry.unique_id = "SN123456" # same serial the current entry is migrating to + self.entry.entry_id = "this_entry_id" + self.hass.config_entries.async_entries.return_value = [self.entry, other_entry] + + result = await async_migrate_entry(self.hass, self.entry) + + assert result is True + # version bump still happens, but the unique_id update must be skipped + for call in self.hass.config_entries.async_update_entry.call_args_list: + assert "unique_id" not in call.kwargs diff --git a/tests/test_number.py b/tests/test_number.py index 1ef2ffa..64a241b 100644 --- a/tests/test_number.py +++ b/tests/test_number.py @@ -53,7 +53,7 @@ async def test_solis_number_entity(hass, mock_base_sensor, mock_controller): # Test setting value entity.schedule_update_ha_state = MagicMock() - entity.set_native_value(60) + await entity.async_set_native_value(60) await hass.async_block_till_done() mock_controller.async_write_holding_register.assert_called_with(100, 60) @@ -102,7 +102,7 @@ async def test_solis_number_entity_updates(hass, mock_controller): # Coverage report showed missing lines in write logic mostly. entity.schedule_update_ha_state = MagicMock() - entity.set_native_value(55) + await entity.async_set_native_value(55) await hass.async_block_till_done() # write_register is None, so return. assert not mock_controller.async_write_holding_register.called diff --git a/tests/test_register_group_recovery.py b/tests/test_register_group_recovery.py index 0a904df..763f3e2 100644 --- a/tests/test_register_group_recovery.py +++ b/tests/test_register_group_recovery.py @@ -145,6 +145,56 @@ async def read_blk(start, count, is_holding): self.assertEqual(1, len(values)) +class TestRecoveryRoutesThroughHub(unittest.IsolatedAsyncioTestCase): + """Bug #13: a recovery edit must land on both protocol controllers, not just Modbus's.""" + + async def asyncSetUp(self): + self.hass = MagicMock() + self.hass.is_running = False + self.hass.data = {DOMAIN: {VALUES: {}, SENSOR_ENTITIES: [], NUMBER_ENTITIES: []}} + self.controller = MagicMock() + self.controller.host = "192.168.1.1" + self.controller.slave = 1 + self.controller.device_id = 1 + self.controller.enabled = True + self.controller.connected = MagicMock(return_value=True) + self.hub = MagicMock() + self.dr = DataRetrieval(self.hass, self.controller, hub=self.hub) + + async def test_recovery_calls_hub_not_bare_controller(self): + bad = 302 + s_wide = _mock_sensor([301, 302], name="wide") + s_ok = _mock_sensor([303], name="ok") + group = MagicMock(spec=SolisSensorGroup) + group.sensors = [s_wide, s_ok] + group.poll_speed = PollSpeed.FAST + group.identification = "test" + group.start_register = 301 + group.registrar_count = 3 + + async def detailed(start, count, quiet=False): + regs = range(start, start + count) + if bad in regs: + return None, 2 + return [7] * count, None + + self.controller._async_read_input_register_raw_detailed = AsyncMock(side_effect=detailed) + + async def read_blk(start, count, is_holding): + if start == 303 and count == 1: + return ([42], None) + return (None, 2) + + with ( + patch.object(self.dr, "_read_register_block_with_exception", new=AsyncMock(side_effect=read_blk)), + patch("custom_components.solisconnect.data_retrieval.mark_platform_entities_unavailable_for_base_sensors"), + ): + await self.dr._recover_sensor_group_after_modbus_failure(group, 301, 3, False, []) + + self.hub.replace_sensor_group.assert_called_once() + self.controller.replace_sensor_group.assert_not_called() + + class TestModbusControllerReplaceGroup(unittest.TestCase): def test_replace_sensor_group_preserves_order(self): from custom_components.solisconnect.modbus_controller import ModbusController diff --git a/tests/test_s16_encoding.py b/tests/test_s16_encoding.py index f2baa58..9351bda 100644 --- a/tests/test_s16_encoding.py +++ b/tests/test_s16_encoding.py @@ -50,66 +50,66 @@ def make_entity(data_type, multiplier=10): return entity, controller -class TestS16Encoding(unittest.TestCase): +class TestS16Encoding(unittest.IsolatedAsyncioTestCase): def _written_value(self, controller): args, _ = controller.async_write_holding_register.call_args return args[1] - def test_positive_value_unchanged(self): + async def test_positive_value_unchanged(self): """Positive S16: register_value must not be altered.""" entity, controller = make_entity("S16", multiplier=10) - entity.set_native_value(1000) # → register_value = 100, positive → no change + await entity.async_set_native_value(1000) # → register_value = 100, positive → no change written = self._written_value(controller) self.assertEqual(written, 100) - def test_negative_value_two_complement(self): + async def test_negative_value_two_complement(self): """Negative S16: -1000 W with multiplier 10 → register -100 → 0xFF9C (65436).""" entity, controller = make_entity("S16", multiplier=10) - entity.set_native_value(-1000) + await entity.async_set_native_value(-1000) written = self._written_value(controller) self.assertEqual(written, 65436) # -100 & 0xFFFF - def test_minus_one_unit(self): + async def test_minus_one_unit(self): """-10 W (one step) → register -1 → 65535.""" entity, controller = make_entity("S16", multiplier=10) - entity.set_native_value(-10) + await entity.async_set_native_value(-10) written = self._written_value(controller) self.assertEqual(written, 65535) # -1 & 0xFFFF - def test_zero(self): + async def test_zero(self): """Zero stays zero.""" entity, controller = make_entity("S16", multiplier=10) entity._attr_native_value = 100 # ensure value differs so write is triggered - entity.set_native_value(0) + await entity.async_set_native_value(0) written = self._written_value(controller) self.assertEqual(written, 0) - def test_clamping_below_min(self): + async def test_clamping_below_min(self): """Values below -32768 are clamped before two's complement.""" entity, controller = make_entity("S16", multiplier=1) - entity.set_native_value(-999999) + await entity.async_set_native_value(-999999) written = self._written_value(controller) self.assertEqual(written, 32768) # -32768 & 0xFFFF - def test_clamping_above_max(self): + async def test_clamping_above_max(self): """Values above 32767 are clamped.""" entity, controller = make_entity("S16", multiplier=1) - entity.set_native_value(999999) + await entity.async_set_native_value(999999) written = self._written_value(controller) self.assertEqual(written, 32767) - def test_no_conversion_without_s16(self): + async def test_no_conversion_without_s16(self): """data_type=None: register_value written as-is (positive only registers).""" entity, controller = make_entity(None, multiplier=10) - entity.set_native_value(500) + await entity.async_set_native_value(500) written = self._written_value(controller) self.assertEqual(written, 50) - def test_no_write_when_same_value(self): - """set_native_value does nothing when value is unchanged.""" + async def test_no_write_when_same_value(self): + """async_set_native_value does nothing when value is unchanged.""" entity, controller = make_entity("S16", multiplier=10) entity._attr_native_value = -1000 - entity.set_native_value(-1000) + await entity.async_set_native_value(-1000) controller.async_write_holding_register.assert_not_called() diff --git a/tests/test_switch_conflicts_requires.py b/tests/test_switch_conflicts_requires.py index b101cff..8162c82 100644 --- a/tests/test_switch_conflicts_requires.py +++ b/tests/test_switch_conflicts_requires.py @@ -2,6 +2,7 @@ import pytest +from custom_components.solisconnect.const import CONTROLLER, REGISTER, SLAVE, VALUE from custom_components.solisconnect.sensors.solis_binary_sensor import ( SolisBinaryEntity, set_bit, @@ -144,3 +145,36 @@ async def test_cold_cache_failed_live_read_skips_write(mock_hass, controller): controller.async_read_holding_register.assert_awaited_once_with(43110, 1) controller.async_write_holding_register.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_handle_modbus_update_pushes_state(mock_hass, controller): + """Bug #13: a register update must push entity state immediately, not just rely on polling.""" + entity_def = { + "register": 43110, + "bit_position": 1, + "name": "TOU (Self-Use)", + } + entity = SolisBinaryEntity(mock_hass, controller, entity_def) + entity.schedule_update_ha_state = MagicMock() + + data = {REGISTER: 43110, VALUE: set_bit(0, 1, True), CONTROLLER: str(controller.host), SLAVE: controller.device_id} + entity.handle_modbus_update(data) + + entity.schedule_update_ha_state.assert_called_once() + assert entity.is_on is True + + +@pytest.mark.asyncio +async def test_handle_modbus_update_pushes_state_for_connection_switch(mock_hass, controller): + """Register 5 (connection enable switch) must also push state on update.""" + entity_def = {"register": 5, "name": "Connection Enabled"} + controller.enabled = True + entity = SolisBinaryEntity(mock_hass, controller, entity_def) + entity.schedule_update_ha_state = MagicMock() + + data = {REGISTER: 5, VALUE: 1, CONTROLLER: str(controller.host), SLAVE: controller.device_id} + entity.handle_modbus_update(data) + + entity.schedule_update_ha_state.assert_called_once() + assert entity.is_on is True