From 8240b8c72e9add4e85ca037852f57f1cc00a5f0b Mon Sep 17 00:00:00 2001 From: Kevin Worrel <37058192+dieselrabbit@users.noreply.github.com> Date: Wed, 1 Dec 2021 00:19:01 -0800 Subject: [PATCH] Update screenlogic use asyncio API (#60466) Co-authored-by: J. Nick Koston --- .../components/screenlogic/__init__.py | 130 +++++++++--------- .../components/screenlogic/climate.py | 29 ++-- .../components/screenlogic/config_flow.py | 16 +-- homeassistant/components/screenlogic/const.py | 2 - .../components/screenlogic/manifest.json | 2 +- .../components/screenlogic/services.py | 14 +- requirements_all.txt | 2 +- requirements_test_all.txt | 2 +- .../screenlogic/test_config_flow.py | 37 +---- 9 files changed, 88 insertions(+), 146 deletions(-) diff --git a/homeassistant/components/screenlogic/__init__.py b/homeassistant/components/screenlogic/__init__.py index 894a8a5e527..bc7761857d5 100644 --- a/homeassistant/components/screenlogic/__init__.py +++ b/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] - -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 +PLATFORMS = ["binary_sensor", "climate", "light", "sensor", "switch"] 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 - # requests very well. - api_lock = asyncio.Lock() + try: + await gateway.async_connect() + 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 - discovered_gateways = hass.data[DOMAIN][DISCOVERED_GATEWAYS] + # Attempt to rediscover gateway to follow IP changes + 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 - # takes a moment to reflect the state change + # Debounced option since the device takes + # 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.hass.async_add_executor_job(self.gateway.update) + await self.gateway.async_update() except ScreenLogicError as error: - _LOGGER.warning("ScreenLogicError - attempting reconnect: %s", error) - - async with self.api_lock: - self.gateway = await self.hass.async_add_executor_job( - self.reconnect_gateway - ) + _LOGGER.warning("Update error - attempting reconnect: %s", error) + await self._async_reconnect_update_data() + except ScreenLogicWarning as warn: + raise UpdateFailed(f"Incomplete update: {warn}") from warn 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: - async with self.coordinator.api_lock: - success = await self.hass.async_add_executor_job( - self.gateway.set_circuit, self._data_key, circuit_value + # Turning off spa or pool circuit may require more time for the + # heater to reflect changes depending on the pool controller, + # so we schedule an extra refresh a bit farther out + 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 diff --git a/homeassistant/components/screenlogic/climate.py b/homeassistant/components/screenlogic/climate.py index dc9e185ca9f..65200bb7dda 100644 --- a/homeassistant/components/screenlogic/climate.py +++ b/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: - success = await self.hass.async_add_executor_job( - self.gateway.set_heat_temp, int(self._data_key), int(temperature) - ) - - if success: - await self.coordinator.async_request_refresh() + if await self.gateway.async_set_heat_temp( + int(self._data_key), int(temperature) + ): + await self._async_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: - success = await self.hass.async_add_executor_job( - self.gateway.set_heat_mode, int(self._data_key), int(mode) - ) - - if success: - await self.coordinator.async_request_refresh() + if await self.gateway.async_set_heat_mode(int(self._data_key), int(mode)): + await self._async_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: - success = await self.hass.async_add_executor_job( - self.gateway.set_heat_mode, int(self._data_key), int(mode) - ) - - if success: - await self.coordinator.async_request_refresh() + if await self.gateway.async_set_heat_mode(int(self._data_key), int(mode)): + await self._async_refresh() else: raise HomeAssistantError( f"Failed to set_preset_mode {mode} on body {self.body['body_type']['value']}" diff --git a/homeassistant/components/screenlogic/config_flow.py b/homeassistant/components/screenlogic/config_flow.py index e8f2fca1be5..ccb2dd38bb8 100644 --- a/homeassistant/components/screenlogic/config_flow.py +++ b/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( - await async_get_mac_address(self.hass, ip_address, port) - ) + mac = format_mac(await login.async_get_mac_address(ip_address, port)) except ScreenLogicError as ex: _LOGGER.debug(ex) errors[CONF_IP_ADDRESS] = "cannot_connect" diff --git a/homeassistant/components/screenlogic/const.py b/homeassistant/components/screenlogic/const.py index 2a1a3c23d2e..bfa9d09cab8 100644 --- a/homeassistant/components/screenlogic/const.py +++ b/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" diff --git a/homeassistant/components/screenlogic/manifest.json b/homeassistant/components/screenlogic/manifest.json index abef9ec99ed..b6134216049 100644 --- a/homeassistant/components/screenlogic/manifest.json +++ b/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": [ { diff --git a/homeassistant/components/screenlogic/services.py b/homeassistant/components/screenlogic/services.py index d5edda12abb..c046a4478fe 100644 --- a/homeassistant/components/screenlogic/services.py +++ b/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 hass.async_add_executor_job( - coordinator.gateway.set_color_lights, color_num - ): - raise HomeAssistantError( - f"Failed to call service '{SERVICE_SET_COLOR_MODE}'" - ) + if not await coordinator.gateway.async_set_color_lights(color_num): + raise HomeAssistantError( + f"Failed to call service '{SERVICE_SET_COLOR_MODE}'" + ) + # Debounced refresh to catch any secondary + # changes in the device + await coordinator.async_request_refresh() except ScreenLogicError as error: raise HomeAssistantError(error) from error diff --git a/requirements_all.txt b/requirements_all.txt index 86a88c3b915..21e951ed1db 100644 --- a/requirements_all.txt +++ b/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 diff --git a/requirements_test_all.txt b/requirements_test_all.txt index 00b958e67a5..3978378f604 100644 --- a/requirements_test_all.txt +++ b/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 diff --git a/tests/components/screenlogic/test_config_flow.py b/tests/components/screenlogic/test_config_flow.py index 375bcddac24..1bb441f145c 100644 --- a/tests/components/screenlogic/test_config_flow.py +++ b/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", - return_value=True, - ), patch( - "homeassistant.components.screenlogic.config_flow.login.gateway_connect", + "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address", 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", - return_value=True, - ), patch( - "homeassistant.components.screenlogic.config_flow.login.gateway_connect", + "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address", 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", - return_value=True, - ), patch( - "homeassistant.components.screenlogic.config_flow.login.gateway_connect", + "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address", 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", - return_value=None, + "homeassistant.components.screenlogic.config_flow.login.async_get_mac_address", + 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, ):