Merge pull request #22 from fboundy/patch

Fix dual-protocol robustness gaps (#13)

Author: fboundy

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
 

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
 

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):

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):

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()

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()

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

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

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