Improved error handling to prevent bad data bricking the connection

custom_components/givenergy_local/coordinator.py

@@ -11,12 +11,14 @@
 from homeassistant.helpers.update_coordinator import DataUpdateCoordinator, UpdateFailed
 
 from .givenergy_modbus.client.client import Client
+from .givenergy_modbus.exceptions import CommunicationError
 from .givenergy_modbus.model.plant import Plant
 from .givenergy_modbus.pdu.transparent import TransparentRequest
 
 _LOGGER = getLogger(__name__)
 _FULL_REFRESH_INTERVAL = timedelta(minutes=5)
 _REFRESH_ATTEMPTS = 3
+_REFRESH_DELAY_BETWEEN_ATTEMPTS = 2.0
 _COMMAND_TIMEOUT = 3.0
 _COMMAND_RETRIES = 3
 
@@ -98,8 +100,9 @@ async def _async_update_data(self) -> Plant:
         # to the coordinator, but sometimes we still get bad values. When that data arrives back
         # here, we perform some quality checks and trigger another attempt if something doesn't
         # look right. If all that fails, then data will show as 'unavailable' in the UI.
-        attempt = 1
-        while attempt <= _REFRESH_ATTEMPTS:
+        attempt = 0
+        while attempt < _REFRESH_ATTEMPTS:
+            attempt += 1
             try:
                 async with asyncio.timeout(10):
                     _LOGGER.info(
@@ -112,13 +115,21 @@ async def _async_update_data(self) -> Plant:
                     plant = await self.client.refresh_plant(
                         full_refresh=self.require_full_refresh, retries=2
                     )
+            except ValueError as err:
+                _LOGGER.warning("Plant refresh failed due to bad data: %s", err)
+                await asyncio.sleep(_REFRESH_DELAY_BETWEEN_ATTEMPTS)
+                continue
+            except CommunicationError as err:
+                _LOGGER.debug("Closing connection due to communication error: %s", err)
+                await self.client.close()
+                raise UpdateFailed() from err
             except Exception as err:
                 _LOGGER.error("Closing connection due to expected error: %s", err)
                 await self.client.close()
                 raise UpdateFailed("Connection closed due to expected error") from err
 
             if not self._is_data_valid(plant):
-                attempt += 1
+                await asyncio.sleep(_REFRESH_DELAY_BETWEEN_ATTEMPTS)
                 continue
 
             if self.require_full_refresh:

custom_components/givenergy_local/givenergy_modbus/client/client.py

@@ -124,6 +124,10 @@ async def refresh_plant(
             full_refresh, self.plant.number_batteries, max_batteries
         )
         await self.execute(reqs, timeout=timeout, retries=retries)
+
+        if full_refresh:
+            self.plant.detect_batteries()
+
         return self.plant
 
     async def watch_plant(
@@ -138,6 +142,7 @@ async def watch_plant(
         """Refresh data about the Plant."""
         await self.connect()
         await self.refresh_plant(True, max_batteries=max_batteries)
+        self.plant.detect_batteries()
         while True:
             if handler:
                 handler()

custom_components/givenergy_local/givenergy_modbus/model/plant.py

@@ -26,6 +26,7 @@ class Plant(GivEnergyBaseModel):
     register_caches: dict[int, RegisterCache] = {}
     inverter_serial_number: str = ""
     data_adapter_serial_number: str = ""
+    number_batteries: int = 0
 
     class Config:  # noqa: D106
         allow_mutation = True
@@ -80,21 +81,25 @@ def update(self, pdu: ClientIncomingMessage):
                     {HR(pdu.register): pdu.value}
                 )
 
-    @property
-    def inverter(self) -> Inverter:
-        """Return Inverter model for the Plant."""
-        return Inverter.from_orm(self.register_caches[0x32])
+    def detect_batteries(self) -> None:
+        """Determine the number of batteries based on whether the register data is valid.
 
-    @property
-    def number_batteries(self) -> int:
-        """Determine the number of batteries connected to the system based on whether the register data is valid."""
+        Since we attempt to decode register data in the process, it's possible for an
+        exception to be raised.
+        """
         i = 0
         for i in range(6):
             try:
                 assert Battery.from_orm(self.register_caches[i + 0x32]).is_valid()
             except (KeyError, AssertionError):
                 break
-        return i
+        _logger.debug("Updating connected battery count to %d", i)
+        self.number_batteries = i
+
+    @property
+    def inverter(self) -> Inverter:
+        """Return Inverter model for the Plant."""
+        return Inverter.from_orm(self.register_caches[0x32])
 
     @property
     def batteries(self) -> list[Battery]: