Merge pull request #111 from RAR/fix/settings-state-loss

fix: settings entities silently report 0/off when the snapshot is lost

Author: zakery292

custom_components/monitormysolar/coordinator.py

@@ -98,6 +98,15 @@ def __init__(
         self._dongle_stale_after = 90.0
         # Don't send more than one recovery snapshot per dongle within this window.
         self._recovery_snapshot_debounce = 30.0
+        # A snapshot request can be lost: the recovery triggers fire while the
+        # dongle is still coming back, so it may not have resubscribed yet. On
+        # FW >= 4.3.0 nothing re-sends hold registers, so a lost request leaves
+        # every setting entity empty until the next reboot. Verify the reply
+        # (<dongle>/snap/hold) actually arrives and retry a bounded number of times.
+        self._snapshot_retry: Dict[str, Any] = {}  # dongle -> cancel callback
+        self._snapshot_retry_attempts: Dict[str, int] = {}
+        self._snapshot_retry_delay = 20.0
+        self._snapshot_max_retries = 3
         self._has_gridboss = entry.data.get("has_gridboss", False)  # Track if GridBoss is enabled
         self._gridboss_dongle = entry.data.get("gridboss_dongle", "")  # Track which dongle is GridBoss
         self._last_fault_warning_data = {}  # Track last fault/warning data to prevent duplicate processing
@@ -202,22 +211,25 @@ def is_ota_in_progress(self, dongle_id: str) -> bool:
         """Whether a dongle is currently running an OTA update."""
         return dongle_id in getattr(self, "_ota_in_progress", ())
 
-    async def request_snapshot(self, dongle_id: str, version: str = "", force: bool = False) -> None:
+    async def request_snapshot(self, dongle_id: str, version: str = "", force: bool = False) -> bool:
         """Ask a dongle for a full /input + /hold snapshot (once per session).
 
         Dongles on FW >= 4.3.0 only publish change-data, so without this the
         entities stay 'unknown' until each value happens to change. Gated to fire
         once per dongle per HA session unless force=True (e.g. a reconnect).
+
+        Returns True only if the request was actually published, so callers can
+        avoid recording a retry/debounce window against a request that never left.
         """
         if self.is_ota_in_progress(dongle_id):
             LOGGER.info(
                 "Suppressing snapshot request for %s: OTA in progress", dongle_id
             )
-            return
+            return False
         if not force and dongle_id in self._snapshot_requested:
-            return
+            return False
         if not self._needs_snapshot(version):
-            return
+            return False
         try:
             await mqtt.async_publish(
                 self.hass,
@@ -226,12 +238,15 @@ async def request_snapshot(self, dongle_id: str, version: str = "", force: bool
                 qos=1,
                 retain=False,
             )
-            self._snapshot_requested.add(dongle_id)
-            LOGGER.info(
-                "Requested full snapshot from %s (version=%s)", dongle_id, version or "unknown"
-            )
         except Exception as e:
             LOGGER.debug(f"Snapshot request publish failed for {dongle_id} (non-fatal): {e}")
+            return False
+        self._snapshot_requested.add(dongle_id)
+        LOGGER.info(
+            "Requested full snapshot from %s (version=%s)", dongle_id, version or "unknown"
+        )
+        self._arm_snapshot_retry(dongle_id, version)
+        return True
 
     async def request_recovery_snapshot(self, dongle_id: str, reason: str) -> None:
         """Force a snapshot after a dongle recovers, debounced per dongle.
@@ -252,11 +267,74 @@ async def request_recovery_snapshot(self, dongle_id: str, reason: str) -> None:
         last = self._last_recovery_snapshot.get(dongle_id, 0.0)
         if now - last < self._recovery_snapshot_debounce:
             return
-        self._last_recovery_snapshot[dongle_id] = now
         LOGGER.info("Recovery snapshot for %s (%s)", dongle_id, reason)
-        await self.request_snapshot(
+        # Only start the debounce window once the request has actually gone out.
+        # Recovery triggers fire while the dongle is rebooting, so a request can
+        # be lost before it is subscribed; stamping first would swallow the
+        # follow-up triggers and leave settings entities empty for the session.
+        if await self.request_snapshot(
             dongle_id, self.current_fw_versions.get(dongle_id, ""), force=True
-        )
+        ):
+            self._last_recovery_snapshot[dongle_id] = now
+
+    def _cancel_snapshot_retry(self, dongle_id: str) -> None:
+        """Drop any armed retry timer for a dongle."""
+        # getattr: test coordinators are built via __new__ and skip __init__.
+        pending = getattr(self, "_snapshot_retry", None)
+        if not pending:
+            return
+        cancel = pending.pop(dongle_id, None)
+        if cancel is not None:
+            cancel()
+
+    def _arm_snapshot_retry(self, dongle_id: str, version: str) -> None:
+        """Re-request the snapshot if its reply doesn't arrive in time.
+
+        A published request is not a delivered one: the recovery triggers fire
+        while the dongle is still reconnecting, so the request can go out before
+        it has resubscribed. Since FW >= 4.3.0 never re-sends hold registers on
+        its own, that single loss would leave every setting entity empty for the
+        rest of the session.
+        """
+        if getattr(self, "_snapshot_retry", None) is None:
+            self._snapshot_retry = {}
+        if getattr(self, "_snapshot_retry_attempts", None) is None:
+            self._snapshot_retry_attempts = {}
+        self._cancel_snapshot_retry(dongle_id)
+        attempts = self._snapshot_retry_attempts.get(dongle_id, 0)
+        max_retries = getattr(self, "_snapshot_max_retries", 3)
+        if attempts >= max_retries:
+            LOGGER.warning(
+                "Snapshot from %s still unanswered after %d retries - its settings "
+                "entities will stay unknown until it reboots or reconnects",
+                dongle_id, attempts,
+            )
+            return
+        delay = getattr(self, "_snapshot_retry_delay", 20.0)
+
+        async def _retry(_now) -> None:
+            self._snapshot_retry.pop(dongle_id, None)
+            if self.is_ota_in_progress(dongle_id):
+                return
+            self._snapshot_retry_attempts[dongle_id] = attempts + 1
+            LOGGER.warning(
+                "No snapshot reply from %s after %.0fs - retrying (%d/%d)",
+                dongle_id, delay, attempts + 1, max_retries,
+            )
+            await self.request_snapshot(dongle_id, version, force=True)
+
+        self._snapshot_retry[dongle_id] = async_call_later(self.hass, delay, _retry)
+
+    def _note_snapshot_delivered(self, dongle_id: str) -> None:
+        """Record that a dongle answered its snapshot request.
+
+        Called when <dongle>/snap/hold arrives — the hold half is what carries
+        the settings, so an /snap/input-only reply deliberately does not count.
+        """
+        self._cancel_snapshot_retry(dongle_id)
+        attempts = getattr(self, "_snapshot_retry_attempts", None)
+        if attempts is not None:
+            attempts.pop(dongle_id, None)
 
     async def mark_dongle_seen(self, dongle_id: str) -> None:
         """Record that a message arrived from a dongle and detect gap recovery.
@@ -1283,6 +1361,10 @@ async def _async_handle_mqtt_message(self, msg) -> None:
             # which on FW >= 4.3.0 (change-data only) may be a long time. The
             # firmware publishes it on <dongle>/snap/input and <dongle>/snap/hold.
             elif topic.endswith("/snap/input") or topic.endswith("/snap/hold"):
+                if topic.endswith("/snap/hold"):
+                    # The hold half carries the settings; an input-only reply
+                    # leaves them empty, so it must not disarm the retry.
+                    self._note_snapshot_delivered(dongle_id)
                 await self.process_message(dongle_id, topic, msg.payload)
                 self.async_set_updated_data(self.entities)
             # Skip other message processing during startup to prevent excessive updates
@@ -1755,6 +1837,10 @@ async def log_ignored_entities(_):
     async def stop_mqtt_subscription(self):
         """Stop all MQTT subscriptions."""
         LOGGER.debug(f"Stopping MQTT subscriptions for all dongles")
+        # Drop armed snapshot retries first: once unsubscribed there is nothing
+        # left to answer them, and a reload would leave the timers orphaned.
+        for dongle_id in list(getattr(self, "_snapshot_retry", {})):
+            self._cancel_snapshot_retry(dongle_id)
         for key, unsubscribe in list(self._mqtt_unsubscribe_callbacks.items()):
             try:
                 unsubscribe()

custom_components/monitormysolar/number.py

@@ -83,7 +83,7 @@ def __init__(self, entity_info, hass, entry: MonitorMySolarEntry, bank_name, don
         self.entity_info = entity_info
         self._attr_name = entity_info["name"]
         self._attr_unique_id = f"{entry.entry_id}_{dongle_id}_{entity_info['unique_id']}".lower()
-        self._attr_native_value = 0
+        self._attr_native_value = None
         self._dongle_id = dongle_id
         self._formatted_dongle_id = self.coordinator.get_formatted_dongle_id(dongle_id)
         self._entity_type = entity_info["unique_id"]
@@ -229,7 +229,7 @@ def __init__(self, entity_info, hass, entry: MonitorMySolarEntry, dongle_ids):
         self.entity_info = entity_info
         self._name = entity_info["name"]
         self._unique_id = f"{entry.entry_id}_{entity_info['unique_id']}".lower()
-        self._attr_native_value = 0
+        self._attr_native_value = None
         self._dongle_ids = dongle_ids
         self._virtual_id = "combined_parallel"
         self._formatted_dongle_id = "combined"

custom_components/monitormysolar/switch.py

@@ -92,7 +92,7 @@ def __init__(self, entity_info, hass, entry: MonitorMySolarEntry, bank_name, don
         self.entity_info = entity_info
         self._name = entity_info["name"]
         self._unique_id = f"{entry.entry_id}_{dongle_id}_{entity_info['unique_id']}".lower()
-        self._state = False
+        self._state = None
         self._dongle_id = dongle_id
         self._formatted_dongle_id = self.coordinator.get_formatted_dongle_id(dongle_id)
         self._entity_type = entity_info["unique_id"]
@@ -231,7 +231,7 @@ def __init__(self, entity_info, hass, entry: MonitorMySolarEntry, dongle_ids):
         self.entity_info = entity_info
         self._name = entity_info["name"]
         self._unique_id = f"{entry.entry_id}_{entity_info['unique_id']}".lower()
-        self._state = False
+        self._state = None
         self._dongle_ids = dongle_ids
         self._virtual_id = "combined_parallel"
         self._formatted_dongle_id = "combined"

tests/test_recovery_snapshot.py

@@ -13,12 +13,15 @@ def _run(coro):
     return asyncio.run(coro)
 
 
-def _prep(coordinator, monkeypatch):
-    # Capture request_snapshot calls without publishing.
+def _prep(coordinator, monkeypatch, published=True):
+    # Capture request_snapshot calls without publishing. `published` models its
+    # return value: True = the request actually went out, False = the publish
+    # failed or was suppressed.
     calls = []
 
     async def fake_request_snapshot(dongle_id, version="", force=False):
         calls.append((dongle_id, force))
+        return published
 
     monkeypatch.setattr(coordinator, "request_snapshot", fake_request_snapshot)
     coordinator.current_fw_versions = {}
@@ -36,6 +39,28 @@ def test_recovery_snapshot_debounced(coordinator, monkeypatch):
     assert calls == [("dongle-A", True)]
 
 
+def test_failed_publish_does_not_burn_debounce(coordinator, monkeypatch):
+    """A request that never went out must not suppress the follow-up triggers.
+
+    Recovery triggers fire while the dongle is still rebooting, so the first
+    attempt can be published before it has resubscribed and is then lost. If
+    the debounce window were stamped regardless, the follow-up triggers would
+    be swallowed and — because FW >= 4.3.0 only streams change-data — the
+    dongle's settings entities would stay empty for the rest of the session.
+
+    This is the same rule the OTA-suppression path already follows; it just
+    also has to apply when the publish itself fails.
+    """
+    calls = _prep(coordinator, monkeypatch, published=False)
+    coordinator._last_recovery_snapshot = {}
+    coordinator._recovery_snapshot_debounce = 30.0
+
+    _run(coordinator.request_recovery_snapshot("dongle-A", "reboot detected"))
+    _run(coordinator.request_recovery_snapshot("dongle-A", "availability online"))
+    assert calls == [("dongle-A", True), ("dongle-A", True)]
+    assert "dongle-A" not in coordinator._last_recovery_snapshot
+
+
 def test_recovery_snapshot_per_dongle(coordinator, monkeypatch):
     calls = _prep(coordinator, monkeypatch)
     coordinator._last_recovery_snapshot = {}

tests/test_settings_default_state.py

@@ -0,0 +1,53 @@
+"""Settings entities must not report a fabricated value before data arrives.
+
+Dongles on FW >= 4.3.0 only stream change-data, so a hold register (a setting)
+reaches HA exactly once, in the connect-time snapshot. If that snapshot is lost
+the entity never receives a value -- and a number seeded with 0 or a switch
+seeded with False is then indistinguishable from a real reading of 0/off.
+Seeding None instead surfaces it as 'unknown', which is what select.py already
+does.
+"""
+from unittest.mock import MagicMock
+
+
+def _entry(entity_id):
+    entry = MagicMock()
+    coord = entry.runtime_data
+    coord.build_entity_id.return_value = entity_id
+    coord.get_formatted_dongle_id.return_value = "dongle_x"
+    coord.get_firmware_code.return_value = "AAAA"
+    return entry
+
+
+def test_number_has_no_value_before_data_arrives():
+    from custom_components.monitormysolar.number import InverterNumber
+
+    entry = _entry("number.dongle_x_acchgsoclimit")
+    n = InverterNumber(
+        {"name": "AC Charge SOC Limit", "unique_id": "acchgsoclimit", "min": 0, "max": 100},
+        MagicMock(), entry, "holdbank1", "dongle-X",
+    )
+    assert n._attr_native_value is None
+
+
+def test_switch_has_no_state_before_data_arrives():
+    from custom_components.monitormysolar.switch import InverterSwitch
+
+    entry = _entry("switch.dongle_x_accharge")
+    s = InverterSwitch(
+        {"name": "AC Charge", "unique_id": "accharge"},
+        MagicMock(), entry, "holdbank1", "dongle-X",
+    )
+    assert s.is_on is None
+
+
+def test_select_already_has_no_state_before_data_arrives():
+    """Baseline: select.py was already correct; number/switch now match it."""
+    from custom_components.monitormysolar.select import InverterSelect
+
+    entry = _entry("select.dongle_x_acchargetype")
+    sel = InverterSelect(
+        {"name": "AC Charge Type", "unique_id": "acchargetype", "options": ["Time", "SOC/Volt"]},
+        MagicMock(), entry, "dongle-X",
+    )
+    assert sel.current_option is None