Update screenlogic use asyncio API (#60466)

Co-authored-by: J. Nick Koston <nick@koston.org>

homeassistant/components/screenlogic/__init__.py

@@ -1,5 +1,4 @@
 """The Screenlogic integration."""
-import asyncio
 from datetime import timedelta
 import logging
 
@@ -11,6 +10,7 @@ from screenlogicpy.const import (
     SL_GATEWAY_IP,
     SL_GATEWAY_NAME,
     SL_GATEWAY_PORT,
+    ScreenLogicWarning,
 )
 
 from homeassistant.config_entries import ConfigEntry
@@ -20,7 +20,7 @@ from homeassistant.exceptions import ConfigEntryNotReady
 from homeassistant.helpers import device_registry as dr
 from homeassistant.helpers.debounce import Debouncer
 from homeassistant.helpers.entity import DeviceInfo
-from homeassistant.helpers.typing import ConfigType
+from homeassistant.helpers.event import async_call_later
 from homeassistant.helpers.update_coordinator import (
     CoordinatorEntity,
     DataUpdateCoordinator,
@@ -28,35 +28,35 @@ from homeassistant.helpers.update_coordinator import (
 )
 
 from .config_flow import async_discover_gateways_by_unique_id, name_for_mac
-from .const import DEFAULT_SCAN_INTERVAL, DISCOVERED_GATEWAYS, DOMAIN
+from .const import DEFAULT_SCAN_INTERVAL, DOMAIN
 from .services import async_load_screenlogic_services, async_unload_screenlogic_services
 
 _LOGGER = logging.getLogger(__name__)
 
 
-REQUEST_REFRESH_DELAY = 1
+REQUEST_REFRESH_DELAY = 2
+HEATER_COOLDOWN_DELAY = 6
 
-PLATFORMS = ["switch", "sensor", "binary_sensor", "climate", "light"]
+# These seem to be constant across all controller models
+PRIMARY_CIRCUIT_IDS = [500, 505]  # [Spa, Pool]
 
-
+PLATFORMS = ["binary_sensor", "climate", "light", "sensor", "switch"]
-async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool:
-    """Set up the Screenlogic component."""
-    domain_data = hass.data[DOMAIN] = {}
-    domain_data[DISCOVERED_GATEWAYS] = await async_discover_gateways_by_unique_id(hass)
-    return True
 
 
 async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Set up Screenlogic from a config entry."""
+    connect_info = await async_get_connect_info(hass, entry)
 
-    gateway = await hass.async_add_executor_job(get_new_gateway, hass, entry)
+    gateway = ScreenLogicGateway(**connect_info)
 
-    # The api library uses a shared socket connection and does not handle concurrent
+    try:
-    # requests very well.
+        await gateway.async_connect()
-    api_lock = asyncio.Lock()
+    except ScreenLogicError as ex:
+        _LOGGER.error("Error while connecting to the gateway %s: %s", connect_info, ex)
+        raise ConfigEntryNotReady from ex
 
     coordinator = ScreenlogicDataUpdateCoordinator(
-        hass, config_entry=entry, gateway=gateway, api_lock=api_lock
+        hass, config_entry=entry, gateway=gateway
     )
 
     async_load_screenlogic_services(hass)
@@ -65,7 +65,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
 
     entry.async_on_unload(entry.add_update_listener(async_update_listener))
 
-    hass.data[DOMAIN][entry.entry_id] = coordinator
+    hass.data.setdefault(DOMAIN, {})[entry.entry_id] = coordinator
 
     hass.config_entries.async_setup_platforms(entry, PLATFORMS)
 
@@ -75,8 +75,9 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
 async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Unload a config entry."""
     unload_ok = await hass.config_entries.async_unload_platforms(entry, PLATFORMS)
-    hass.data[DOMAIN][entry.entry_id]["listener"]()
     if unload_ok:
+        coordinator = hass.data[DOMAIN][entry.entry_id]
+        await coordinator.gateway.async_disconnect()
         hass.data[DOMAIN].pop(entry.entry_id)
 
     async_unload_screenlogic_services(hass)
@@ -89,11 +90,11 @@ async def async_update_listener(hass: HomeAssistant, entry: ConfigEntry):
     await hass.config_entries.async_reload(entry.entry_id)
 
 
-def get_connect_info(hass: HomeAssistant, entry: ConfigEntry):
+async def async_get_connect_info(hass: HomeAssistant, entry: ConfigEntry):
     """Construct connect_info from configuration entry and returns it to caller."""
     mac = entry.unique_id
-    # Attempt to re-discover named gateway to follow IP changes
+    # Attempt to rediscover gateway to follow IP changes
-    discovered_gateways = hass.data[DOMAIN][DISCOVERED_GATEWAYS]
+    discovered_gateways = await async_discover_gateways_by_unique_id(hass)
     if mac in discovered_gateways:
         connect_info = discovered_gateways[mac]
     else:
@@ -108,28 +109,13 @@ def get_connect_info(hass: HomeAssistant, entry: ConfigEntry):
     return connect_info
 
 
-def get_new_gateway(hass: HomeAssistant, entry: ConfigEntry):
-    """Instantiate a new ScreenLogicGateway, connect to it and return it to caller."""
-
-    connect_info = get_connect_info(hass, entry)
-
-    try:
-        gateway = ScreenLogicGateway(**connect_info)
-    except ScreenLogicError as ex:
-        _LOGGER.error("Error while connecting to the gateway %s: %s", connect_info, ex)
-        raise ConfigEntryNotReady from ex
-
-    return gateway
-
-
 class ScreenlogicDataUpdateCoordinator(DataUpdateCoordinator):
     """Class to manage the data update for the Screenlogic component."""
 
-    def __init__(self, hass, *, config_entry, gateway, api_lock):
+    def __init__(self, hass, *, config_entry, gateway):
         """Initialize the Screenlogic Data Update Coordinator."""
         self.config_entry = config_entry
         self.gateway = gateway
-        self.api_lock = api_lock
         self.screenlogic_data = {}
 
         interval = timedelta(
@@ -140,41 +126,39 @@ class ScreenlogicDataUpdateCoordinator(DataUpdateCoordinator):
             _LOGGER,
             name=DOMAIN,
             update_interval=interval,
-            # We don't want an immediate refresh since the device
+            # Debounced option since the device takes
-            # takes a moment to reflect the state change
+            # a moment to reflect the knock-on changes
             request_refresh_debouncer=Debouncer(
                 hass, _LOGGER, cooldown=REQUEST_REFRESH_DELAY, immediate=False
             ),
         )
 
-    def reconnect_gateway(self):
-        """Instantiate a new ScreenLogicGateway, connect to it and update. Return new gateway to caller."""
-
-        connect_info = get_connect_info(self.hass, self.config_entry)
-
-        try:
-            gateway = ScreenLogicGateway(**connect_info)
-            gateway.update()
-        except ScreenLogicError as error:
-            raise UpdateFailed(error) from error
-
-        return gateway
-
     async def _async_update_data(self):
         """Fetch data from the Screenlogic gateway."""
         try:
-            async with self.api_lock:
+            await self.gateway.async_update()
-                await self.hass.async_add_executor_job(self.gateway.update)
         except ScreenLogicError as error:
-            _LOGGER.warning("ScreenLogicError - attempting reconnect: %s", error)
+            _LOGGER.warning("Update error - attempting reconnect: %s", error)
-
+            await self._async_reconnect_update_data()
-            async with self.api_lock:
+        except ScreenLogicWarning as warn:
-                self.gateway = await self.hass.async_add_executor_job(
+            raise UpdateFailed(f"Incomplete update: {warn}") from warn
-                    self.reconnect_gateway
-                )
 
         return self.gateway.get_data()
 
+    async def _async_reconnect_update_data(self):
+        """Attempt to reconnect to the gateway and fetch data."""
+        try:
+            # Clean up the previous connection as we're about to create a new one
+            await self.gateway.async_disconnect()
+
+            connect_info = await async_get_connect_info(self.hass, self.config_entry)
+            self.gateway = ScreenLogicGateway(**connect_info)
+
+            await self.gateway.async_update()
+
+        except (ScreenLogicError, ScreenLogicWarning) as ex:
+            raise UpdateFailed(ex) from ex
+
 
 class ScreenlogicEntity(CoordinatorEntity):
     """Base class for all ScreenLogic entities."""
@@ -233,6 +217,17 @@ class ScreenlogicEntity(CoordinatorEntity):
             name=self.gateway_name,
         )
 
+    async def _async_refresh(self):
+        """Refresh the data from the gateway."""
+        await self.coordinator.async_refresh()
+        # Second debounced refresh to catch any secondary
+        # changes in the device
+        await self.coordinator.async_request_refresh()
+
+    async def _async_refresh_timed(self, now):
+        """Refresh from a timed called."""
+        await self.coordinator.async_request_refresh()
+
 
 class ScreenLogicCircuitEntity(ScreenlogicEntity):
     """ScreenLogic circuit entity."""
@@ -255,15 +250,18 @@ class ScreenLogicCircuitEntity(ScreenlogicEntity):
         """Send the OFF command."""
         await self._async_set_circuit(ON_OFF.OFF)
 
-    async def _async_set_circuit(self, circuit_value) -> None:
+        # Turning off spa or pool circuit may require more time for the
-        async with self.coordinator.api_lock:
+        # heater to reflect changes depending on the pool controller,
-            success = await self.hass.async_add_executor_job(
+        # so we schedule an extra refresh a bit farther out
-                self.gateway.set_circuit, self._data_key, circuit_value
+        if self._data_key in PRIMARY_CIRCUIT_IDS:
+            async_call_later(
+                self.hass, HEATER_COOLDOWN_DELAY, self._async_refresh_timed
             )
 
-        if success:
+    async def _async_set_circuit(self, circuit_value) -> None:
+        if await self.gateway.async_set_circuit(self._data_key, circuit_value):
             _LOGGER.debug("Turn %s %s", self._data_key, circuit_value)
-            await self.coordinator.async_request_refresh()
+            await self._async_refresh()
         else:
             _LOGGER.warning(
                 "Failed to set_circuit %s %s", self._data_key, circuit_value

homeassistant/components/screenlogic/climate.py

@@ -138,13 +138,10 @@ class ScreenLogicClimate(ScreenlogicEntity, ClimateEntity, RestoreEntity):
         if (temperature := kwargs.get(ATTR_TEMPERATURE)) is None:
             raise ValueError(f"Expected attribute {ATTR_TEMPERATURE}")
 
-        async with self.coordinator.api_lock:
+        if await self.gateway.async_set_heat_temp(
-            success = await self.hass.async_add_executor_job(
+            int(self._data_key), int(temperature)
-                self.gateway.set_heat_temp, int(self._data_key), int(temperature)
+        ):
-            )
+            await self._async_refresh()
-
-        if success:
-            await self.coordinator.async_request_refresh()
         else:
             raise HomeAssistantError(
                 f"Failed to set_temperature {temperature} on body {self.body['body_type']['value']}"
@@ -157,13 +154,8 @@ class ScreenLogicClimate(ScreenlogicEntity, ClimateEntity, RestoreEntity):
         else:
             mode = HEAT_MODE.NUM_FOR_NAME[self.preset_mode]
 
-        async with self.coordinator.api_lock:
+        if await self.gateway.async_set_heat_mode(int(self._data_key), int(mode)):
-            success = await self.hass.async_add_executor_job(
+            await self._async_refresh()
-                self.gateway.set_heat_mode, int(self._data_key), int(mode)
-            )
-
-        if success:
-            await self.coordinator.async_request_refresh()
         else:
             raise HomeAssistantError(
                 f"Failed to set_hvac_mode {mode} on body {self.body['body_type']['value']}"
@@ -176,13 +168,8 @@ class ScreenLogicClimate(ScreenlogicEntity, ClimateEntity, RestoreEntity):
         if self.hvac_mode == HVAC_MODE_OFF:
             return
 
-        async with self.coordinator.api_lock:
+        if await self.gateway.async_set_heat_mode(int(self._data_key), int(mode)):
-            success = await self.hass.async_add_executor_job(
+            await self._async_refresh()
-                self.gateway.set_heat_mode, int(self._data_key), int(mode)
-            )
-
-        if success:
-            await self.coordinator.async_request_refresh()
         else:
             raise HomeAssistantError(
                 f"Failed to set_preset_mode {mode} on body {self.body['body_type']['value']}"

homeassistant/components/screenlogic/config_flow.py

@@ -56,18 +56,6 @@ def name_for_mac(mac):
     return f"Pentair: {short_mac(mac)}"
 
 
-async def async_get_mac_address(hass, ip_address, port):
-    """Connect to a screenlogic gateway and return the mac address."""
-    connected_socket = await hass.async_add_executor_job(
-        login.create_socket,
-        ip_address,
-        port,
-    )
-    if not connected_socket:
-        raise ScreenLogicError("Unknown socket error")
-    return await hass.async_add_executor_job(login.gateway_connect, connected_socket)
-
-
 class ScreenlogicConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
     """Config flow to setup screen logic devices."""
 
@@ -155,9 +143,7 @@ class ScreenlogicConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
             ip_address = user_input[CONF_IP_ADDRESS]
             port = user_input[CONF_PORT]
             try:
-                mac = format_mac(
+                mac = format_mac(await login.async_get_mac_address(ip_address, port))
-                    await async_get_mac_address(self.hass, ip_address, port)
-                )
             except ScreenLogicError as ex:
                 _LOGGER.debug(ex)
                 errors[CONF_IP_ADDRESS] = "cannot_connect"

homeassistant/components/screenlogic/const.py

@@ -14,5 +14,3 @@ SUPPORTED_COLOR_MODES = {
 }
 
 LIGHT_CIRCUIT_FUNCTIONS = {CIRCUIT_FUNCTION.INTELLIBRITE, CIRCUIT_FUNCTION.LIGHT}
-
-DISCOVERED_GATEWAYS = "_discovered_gateways"

homeassistant/components/screenlogic/manifest.json

@@ -3,7 +3,7 @@
   "name": "Pentair ScreenLogic",
   "config_flow": true,
   "documentation": "https://www.home-assistant.io/integrations/screenlogic",
-  "requirements": ["screenlogicpy==0.4.1"],
+  "requirements": ["screenlogicpy==0.5.3"],
   "codeowners": ["@dieselrabbit"],
   "dhcp": [
     {

homeassistant/components/screenlogic/services.py

@@ -59,13 +59,13 @@ def async_load_screenlogic_services(hass: HomeAssistant):
                 color_num,
             )
             try:
-                async with coordinator.api_lock:
+                if not await coordinator.gateway.async_set_color_lights(color_num):
-                    if not await hass.async_add_executor_job(
+                    raise HomeAssistantError(
-                        coordinator.gateway.set_color_lights, color_num
+                        f"Failed to call service '{SERVICE_SET_COLOR_MODE}'"
-                    ):
+                    )
-                        raise HomeAssistantError(
+                # Debounced refresh to catch any secondary
-                            f"Failed to call service '{SERVICE_SET_COLOR_MODE}'"
+                # changes in the device
-                        )
+                await coordinator.async_request_refresh()
             except ScreenLogicError as error:
                 raise HomeAssistantError(error) from error
 

requirements_all.txt

@@ -2115,7 +2115,7 @@ scapy==2.4.5
 schiene==0.23
 
 # homeassistant.components.screenlogic
-screenlogicpy==0.4.1
+screenlogicpy==0.5.3
 
 # homeassistant.components.scsgate
 scsgate==0.1.0

requirements_test_all.txt

@@ -1257,7 +1257,7 @@ samsungtvws==1.6.0
 scapy==2.4.5
 
 # homeassistant.components.screenlogic
-screenlogicpy==0.4.1
+screenlogicpy==0.5.3
 
 # homeassistant.components.emulated_kasa
 # homeassistant.components.sense

tests/components/screenlogic/test_config_flow.py

@@ -50,8 +50,6 @@ async def test_flow_discovery(hass):
     assert result["step_id"] == "gateway_select"
 
     with patch(
-        "homeassistant.components.screenlogic.async_setup", return_value=True
-    ) as mock_setup, patch(
         "homeassistant.components.screenlogic.async_setup_entry",
         return_value=True,
     ) as mock_setup_entry:
@@ -66,7 +64,6 @@ async def test_flow_discovery(hass):
         CONF_IP_ADDRESS: "1.1.1.1",
         CONF_PORT: 80,
     }
-    assert len(mock_setup.mock_calls) == 1
     assert len(mock_setup_entry.mock_calls) == 1
 
 
@@ -102,15 +99,10 @@ async def test_flow_discover_error(hass):
     assert result["step_id"] == "gateway_entry"
 
     with patch(
-        "homeassistant.components.screenlogic.async_setup", return_value=True
-    ) as mock_setup, patch(
         "homeassistant.components.screenlogic.async_setup_entry",
         return_value=True,
     ) as mock_setup_entry, patch(
-        "homeassistant.components.screenlogic.config_flow.login.create_socket",
+        "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address",
-        return_value=True,
-    ), patch(
-        "homeassistant.components.screenlogic.config_flow.login.gateway_connect",
         return_value="00-C0-33-01-01-01",
     ):
         result3 = await hass.config_entries.flow.async_configure(
@@ -128,7 +120,6 @@ async def test_flow_discover_error(hass):
         CONF_IP_ADDRESS: "1.1.1.1",
         CONF_PORT: 80,
     }
-    assert len(mock_setup.mock_calls) == 1
     assert len(mock_setup_entry.mock_calls) == 1
 
 
@@ -149,15 +140,10 @@ async def test_dhcp(hass):
     assert result["step_id"] == "gateway_entry"
 
     with patch(
-        "homeassistant.components.screenlogic.async_setup", return_value=True
-    ) as mock_setup, patch(
         "homeassistant.components.screenlogic.async_setup_entry",
         return_value=True,
     ) as mock_setup_entry, patch(
-        "homeassistant.components.screenlogic.config_flow.login.create_socket",
+        "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address",
-        return_value=True,
-    ), patch(
-        "homeassistant.components.screenlogic.config_flow.login.gateway_connect",
         return_value="00-C0-33-01-01-01",
     ):
         result3 = await hass.config_entries.flow.async_configure(
@@ -175,7 +161,6 @@ async def test_dhcp(hass):
         CONF_IP_ADDRESS: "1.1.1.1",
         CONF_PORT: 80,
     }
-    assert len(mock_setup.mock_calls) == 1
     assert len(mock_setup_entry.mock_calls) == 1
 
 
@@ -210,15 +195,10 @@ async def test_form_manual_entry(hass):
     assert result2["step_id"] == "gateway_entry"
 
     with patch(
-        "homeassistant.components.screenlogic.async_setup", return_value=True
-    ) as mock_setup, patch(
         "homeassistant.components.screenlogic.async_setup_entry",
         return_value=True,
     ) as mock_setup_entry, patch(
-        "homeassistant.components.screenlogic.config_flow.login.create_socket",
+        "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address",
-        return_value=True,
-    ), patch(
-        "homeassistant.components.screenlogic.config_flow.login.gateway_connect",
         return_value="00-C0-33-01-01-01",
     ):
         result3 = await hass.config_entries.flow.async_configure(
@@ -236,7 +216,6 @@ async def test_form_manual_entry(hass):
         CONF_IP_ADDRESS: "1.1.1.1",
         CONF_PORT: 80,
     }
-    assert len(mock_setup.mock_calls) == 1
     assert len(mock_setup_entry.mock_calls) == 1
 
 
@@ -251,8 +230,8 @@ async def test_form_cannot_connect(hass):
         )
 
     with patch(
-        "homeassistant.components.screenlogic.config_flow.login.create_socket",
+        "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address",
-        return_value=None,
+        side_effect=ScreenLogicError("Failed to connect to host at 1.1.1.1:80"),
     ):
         result2 = await hass.config_entries.flow.async_configure(
             result["flow_id"],
@@ -272,8 +251,6 @@ async def test_option_flow(hass):
     entry.add_to_hass(hass)
 
     with patch(
-        "homeassistant.components.screenlogic.async_setup", return_value=True
-    ), patch(
         "homeassistant.components.screenlogic.async_setup_entry",
         return_value=True,
     ):
@@ -299,8 +276,6 @@ async def test_option_flow_defaults(hass):
     entry.add_to_hass(hass)
 
     with patch(
-        "homeassistant.components.screenlogic.async_setup", return_value=True
-    ), patch(
         "homeassistant.components.screenlogic.async_setup_entry",
         return_value=True,
     ):
@@ -327,8 +302,6 @@ async def test_option_flow_input_floor(hass):
     entry.add_to_hass(hass)
 
     with patch(
-        "homeassistant.components.screenlogic.async_setup", return_value=True
-    ), patch(
         "homeassistant.components.screenlogic.async_setup_entry",
         return_value=True,
     ):