From 98c3b6c86b38e38ffb0e79dc3c6e0d05f8e69c2f Mon Sep 17 00:00:00 2001 From: jan Iversen Date: Tue, 11 May 2021 17:57:29 +0200 Subject: [PATCH 1/7] Convert modbus platforms to async, but keep pymodbus sync. Make single sync function to do pymodbus calls. Add async version of pymodbus calls. Convert platform to async. Update/simplify test_delay. Change Lock() from threading. to asyncio. Add thread block test. --- homeassistant/components/modbus/__init__.py | 6 +- .../components/modbus/binary_sensor.py | 18 +- homeassistant/components/modbus/climate.py | 30 +- homeassistant/components/modbus/cover.py | 52 ++-- homeassistant/components/modbus/modbus.py | 293 ++++++++---------- homeassistant/components/modbus/sensor.py | 16 +- homeassistant/components/modbus/switch.py | 52 ++-- tests/components/modbus/test_init.py | 140 ++++----- 8 files changed, 276 insertions(+), 331 deletions(-) diff --git a/homeassistant/components/modbus/__init__.py b/homeassistant/components/modbus/__init__.py index 8e0d92207183f6..13b8c90908d98c 100644 --- a/homeassistant/components/modbus/__init__.py +++ b/homeassistant/components/modbus/__init__.py @@ -100,7 +100,7 @@ MODBUS_DOMAIN as DOMAIN, PLATFORMS, ) -from .modbus import modbus_setup +from .modbus import async_modbus_setup _LOGGER = logging.getLogger(__name__) @@ -348,8 +348,8 @@ def control_scan_interval(config: dict) -> dict: ) -def setup(hass, config): +async def async_setup(hass, config): """Set up Modbus component.""" - return modbus_setup( + return await async_modbus_setup( hass, config, SERVICE_WRITE_REGISTER_SCHEMA, SERVICE_WRITE_COIL_SCHEMA ) diff --git a/homeassistant/components/modbus/binary_sensor.py b/homeassistant/components/modbus/binary_sensor.py index 82b1db6dd1ac2c..ed59081150717b 100644 --- a/homeassistant/components/modbus/binary_sensor.py +++ b/homeassistant/components/modbus/binary_sensor.py @@ -114,9 +114,7 @@ def __init__(self, hub, hass, entry): async def async_added_to_hass(self): """Handle entity which will be added.""" - async_track_time_interval( - self._hass, lambda arg: self.update(), self._scan_interval - ) + async_track_time_interval(self._hass, self.async_update, self._scan_interval) @property def name(self): @@ -148,17 +146,21 @@ def available(self) -> bool: """Return True if entity is available.""" return self._available - def update(self): + async def async_update(self, now=None): """Update the state of the sensor.""" + # remark "now" is a dummy parameter to avoid problems with + # async_track_time_interval if self._input_type == CALL_TYPE_COIL: - result = self._hub.read_coils(self._slave, self._address, 1) + result = await self._hub.async_read_coils(self._slave, self._address, 1) else: - result = self._hub.read_discrete_inputs(self._slave, self._address, 1) + result = await self._hub.async_read_discrete_inputs( + self._slave, self._address, 1 + ) if result is None: self._available = False - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() return self._value = result.bits[0] & 1 self._available = True - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() diff --git a/homeassistant/components/modbus/climate.py b/homeassistant/components/modbus/climate.py index cc8f74577c782f..24a6038c68c37b 100644 --- a/homeassistant/components/modbus/climate.py +++ b/homeassistant/components/modbus/climate.py @@ -132,9 +132,7 @@ def __init__( async def async_added_to_hass(self): """Handle entity which will be added.""" - async_track_time_interval( - self.hass, lambda arg: self.update(), self._scan_interval - ) + async_track_time_interval(self.hass, self.async_update, self._scan_interval) @property def should_poll(self): @@ -160,7 +158,7 @@ def hvac_modes(self): """Return the possible HVAC modes.""" return [HVAC_MODE_AUTO] - def set_hvac_mode(self, hvac_mode: str) -> None: + async def async_set_hvac_mode(self, hvac_mode: str) -> None: """Set new target hvac mode.""" # Home Assistant expects this method. # We'll keep it here to avoid getting exceptions. @@ -200,7 +198,7 @@ def target_temperature_step(self): """Return the supported step of target temperature.""" return self._temp_step - def set_temperature(self, **kwargs): + async def async_set_temperature(self, **kwargs): """Set new target temperature.""" if ATTR_TEMPERATURE not in kwargs: return @@ -209,35 +207,39 @@ def set_temperature(self, **kwargs): ) byte_string = struct.pack(self._structure, target_temperature) register_value = struct.unpack(">h", byte_string[0:2])[0] - self._available = self._hub.write_registers( + self._available = await self._hub.async_write_registers( self._slave, self._target_temperature_register, register_value, ) - self.update() + self.async_update() @property def available(self) -> bool: """Return True if entity is available.""" return self._available - def update(self): + async def async_update(self, now=None): """Update Target & Current Temperature.""" - self._target_temperature = self._read_register( + # remark "now" is a dummy parameter to avoid problems with + # async_track_time_interval + self._target_temperature = await self._async_read_register( CALL_TYPE_REGISTER_HOLDING, self._target_temperature_register ) - self._current_temperature = self._read_register( + self._current_temperature = await self._async_read_register( self._current_temperature_register_type, self._current_temperature_register ) - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() - def _read_register(self, register_type, register) -> float | None: + async def _async_read_register(self, register_type, register) -> float | None: """Read register using the Modbus hub slave.""" if register_type == CALL_TYPE_REGISTER_INPUT: - result = self._hub.read_input_registers(self._slave, register, self._count) + result = await self._hub.async_read_input_registers( + self._slave, register, self._count + ) else: - result = self._hub.read_holding_registers( + result = await self._hub.async_read_holding_registers( self._slave, register, self._count ) if result is None: diff --git a/homeassistant/components/modbus/cover.py b/homeassistant/components/modbus/cover.py index 7d9bf9e2e45f72..ddc44d6edcfcc4 100644 --- a/homeassistant/components/modbus/cover.py +++ b/homeassistant/components/modbus/cover.py @@ -106,9 +106,7 @@ async def async_added_to_hass(self): if state: self._value = state.state - async_track_time_interval( - self.hass, lambda arg: self.update(), self._scan_interval - ) + async_track_time_interval(self.hass, self.async_update, self._scan_interval) @property def device_class(self) -> str | None: @@ -154,41 +152,43 @@ def should_poll(self): # Handle polling directly in this entity return False - def open_cover(self, **kwargs: Any) -> None: + async def async_open_cover(self, **kwargs: Any) -> None: """Open cover.""" if self._coil is not None: - self._write_coil(True) + await self._async_write_coil(True) else: - self._write_register(self._state_open) + await self._async_write_register(self._state_open) - self.update() + self.async_update() - def close_cover(self, **kwargs: Any) -> None: + async def async_close_cover(self, **kwargs: Any) -> None: """Close cover.""" if self._coil is not None: - self._write_coil(False) + await self._async_write_coil(False) else: - self._write_register(self._state_closed) + await self._async_write_register(self._state_closed) - self.update() + self.async_update() - def update(self): + async def async_update(self, now=None): """Update the state of the cover.""" + # remark "now" is a dummy parameter to avoid problems with + # async_track_time_interval if self._coil is not None and self._status_register is None: - self._value = self._read_coil() + self._value = await self._async_read_coil() else: - self._value = self._read_status_register() + self._value = await self._async_read_status_register() - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() - def _read_status_register(self) -> int | None: + async def _async_read_status_register(self) -> int | None: """Read status register using the Modbus hub slave.""" if self._status_register_type == CALL_TYPE_REGISTER_INPUT: - result = self._hub.read_input_registers( + result = await self._hub.async_read_input_registers( self._slave, self._status_register, 1 ) else: - result = self._hub.read_holding_registers( + result = await self._hub.async_read_holding_registers( self._slave, self._status_register, 1 ) if result is None: @@ -200,13 +200,15 @@ def _read_status_register(self) -> int | None: return value - def _write_register(self, value): + async def _async_write_register(self, value): """Write holding register using the Modbus hub slave.""" - self._available = self._hub.write_register(self._slave, self._register, value) + self._available = await self._hub.async_write_register( + self._slave, self._register, value + ) - def _read_coil(self) -> bool | None: + async def _async_read_coil(self) -> bool | None: """Read coil using the Modbus hub slave.""" - result = self._hub.read_coils(self._slave, self._coil, 1) + result = await self._hub.async_read_coils(self._slave, self._coil, 1) if result is None: self._available = False return None @@ -214,6 +216,8 @@ def _read_coil(self) -> bool | None: value = bool(result.bits[0] & 1) return value - def _write_coil(self, value): + async def _async_write_coil(self, value): """Write coil using the Modbus hub slave.""" - self._available = self._hub.write_coil(self._slave, self._coil, value) + self._available = await self._hub.async_write_coil( + self._slave, self._coil, value + ) diff --git a/homeassistant/components/modbus/modbus.py b/homeassistant/components/modbus/modbus.py index d9db6cf980c5a6..3dd4eeb7f6adb3 100644 --- a/homeassistant/components/modbus/modbus.py +++ b/homeassistant/components/modbus/modbus.py @@ -1,6 +1,6 @@ """Support for Modbus.""" +import asyncio import logging -import threading from pymodbus.client.sync import ModbusSerialClient, ModbusTcpClient, ModbusUdpClient from pymodbus.constants import Defaults @@ -17,8 +17,8 @@ CONF_TYPE, EVENT_HOMEASSISTANT_STOP, ) -from homeassistant.helpers.discovery import load_platform -from homeassistant.helpers.event import call_later +from homeassistant.helpers.discovery import async_load_platform +from homeassistant.helpers.event import async_call_later from .const import ( ATTR_ADDRESS, @@ -40,32 +40,35 @@ _LOGGER = logging.getLogger(__name__) -def modbus_setup( +async def async_modbus_setup( hass, config, service_write_register_schema, service_write_coil_schema ): """Set up Modbus component.""" hass.data[DOMAIN] = hub_collect = {} for conf_hub in config[DOMAIN]: - hub_collect[conf_hub[CONF_NAME]] = ModbusHub(conf_hub) + myHub = ModbusHub(hass, conf_hub) + hub_collect[conf_hub[CONF_NAME]] = myHub # modbus needs to be activated before components are loaded # to avoid a racing problem - hub_collect[conf_hub[CONF_NAME]].setup(hass) + await myHub.async_setup() # load platforms for component, conf_key in PLATFORMS: if conf_key in conf_hub: - load_platform(hass, component, DOMAIN, conf_hub, config) + await async_load_platform(hass, component, DOMAIN, conf_hub, config) - def stop_modbus(event): + async def async_stop_modbus(event): """Stop Modbus service.""" for client in hub_collect.values(): - client.close() + await client.async_close() del client - def write_register(service): + hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, async_stop_modbus) + + async def async_write_register(service): """Write Modbus registers.""" unit = int(float(service.data[ATTR_UNIT])) address = int(float(service.data[ATTR_ADDRESS])) @@ -74,13 +77,22 @@ def write_register(service): service.data[ATTR_HUB] if ATTR_HUB in service.data else DEFAULT_HUB ) if isinstance(value, list): - hub_collect[client_name].write_registers( + await hub_collect[client_name].async_write_registers( unit, address, [int(float(i)) for i in value] ) else: - hub_collect[client_name].write_register(unit, address, int(float(value))) + await hub_collect[client_name].async_write_register( + unit, address, int(float(value)) + ) + + hass.services.async_register( + DOMAIN, + SERVICE_WRITE_REGISTER, + async_write_register, + schema=service_write_register_schema, + ) - def write_coil(service): + async def async_write_coil(service): """Write Modbus coil.""" unit = service.data[ATTR_UNIT] address = service.data[ATTR_ADDRESS] @@ -89,22 +101,12 @@ def write_coil(service): service.data[ATTR_HUB] if ATTR_HUB in service.data else DEFAULT_HUB ) if isinstance(state, list): - hub_collect[client_name].write_coils(unit, address, state) + await hub_collect[client_name].async_write_coils(unit, address, state) else: - hub_collect[client_name].write_coil(unit, address, state) - - # register function to gracefully stop modbus - hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, stop_modbus) + await hub_collect[client_name].async_write_coil(unit, address, state) - # Register services for modbus - hass.services.register( - DOMAIN, - SERVICE_WRITE_REGISTER, - write_register, - schema=service_write_register_schema, - ) - hass.services.register( - DOMAIN, SERVICE_WRITE_COIL, write_coil, schema=service_write_coil_schema + hass.services.async_register( + DOMAIN, SERVICE_WRITE_COIL, async_write_coil, schema=service_write_coil_schema ) return True @@ -112,14 +114,15 @@ def write_coil(service): class ModbusHub: """Thread safe wrapper class for pymodbus.""" - def __init__(self, client_config): + def __init__(self, hass, client_config): """Initialize the Modbus hub.""" # generic configuration self._client = None - self._cancel_listener = None + self._async_cancel_listener = None self._in_error = False - self._lock = threading.Lock() + self._lock = asyncio.Lock() + self.hass = hass self._config_name = client_config[CONF_NAME] self._config_type = client_config[CONF_TYPE] self._config_port = client_config[CONF_PORT] @@ -150,7 +153,7 @@ def _log_error(self, exception_error: ModbusException, error_state=True): _LOGGER.error(log_text) self._in_error = error_state - def setup(self, hass): + async def async_setup(self): """Set up pymodbus client.""" try: if self._config_type == "serial": @@ -187,166 +190,112 @@ def setup(self, hass): self._log_error(exception_error, error_state=False) return - # Connect device - self.connect() + async with self._lock: + await self.hass.async_add_executor_job(self._pymodbus_connect) # Start counting down to allow modbus requests. if self._config_delay: - self._cancel_listener = call_later(hass, self._config_delay, self.end_delay) + self._async_cancel_listener = async_call_later( + self.hass, self._config_delay, self.async_end_delay + ) - def end_delay(self, args): + async def async_end_delay(self, args): """End startup delay.""" - self._cancel_listener = None + self._async_cancel_listener = None self._config_delay = 0 - def close(self): - """Disconnect client.""" - if self._cancel_listener: - self._cancel_listener() - self._cancel_listener = None - with self._lock: + def _pymodbus_close(self): + """Close sync. pymodbus.""" + if self._client: try: - if self._client: - self._client.close() - self._client = None + self._client.close() except ModbusException as exception_error: self._log_error(exception_error) - return + self._client = None - def connect(self): + async def async_close(self): + """Disconnect client.""" + if self._async_cancel_listener: + self._async_cancel_listener() + self._async_cancel_listener = None + + async with self._lock: + return await self.hass.async_add_executor_job(self._pymodbus_close) + + def _pymodbus_connect(self): """Connect client.""" - with self._lock: - try: - self._client.connect() - except ModbusException as exception_error: - self._log_error(exception_error, error_state=False) - return + try: + self._client.connect() + except ModbusException as exception_error: + self._log_error(exception_error, error_state=False) - def read_coils(self, unit, address, count): - """Read coils.""" - if self._config_delay: + def _pymodbus_call(self, unit, address, value, check_attr, func): + """Call sync. pymodbus.""" + kwargs = {"unit": unit} if unit else {} + try: + result = func(address, value, **kwargs) + except ModbusException as exception_error: + self._log_error(exception_error) + result = exception_error + if not hasattr(result, check_attr): + self._log_error(result) return None - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.read_coils(address, count, **kwargs) - except ModbusException as exception_error: - self._log_error(exception_error) - result = exception_error - if not hasattr(result, "bits"): - self._log_error(result) - return None - self._in_error = False - return result - - def read_discrete_inputs(self, unit, address, count): - """Read discrete inputs.""" + self._in_error = False + return result + + async def async_pymodbus_call(self, unit, address, value, check_attr, func): + """Convert async to sync pymodbus call.""" if self._config_delay: return None - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.read_discrete_inputs(address, count, **kwargs) - except ModbusException as exception_error: - result = exception_error - if not hasattr(result, "bits"): - self._log_error(result) - return None - self._in_error = False - return result - - def read_input_registers(self, unit, address, count): + async with self._lock: + return await self.hass.async_add_executor_job( + lambda: self._pymodbus_call(unit, address, value, check_attr, func) + ) + + async def async_read_coils(self, unit, address, count): + """Read coils.""" + return await self.async_pymodbus_call( + unit, address, count, "bits", self._client.read_coils + ) + + async def async_read_discrete_inputs(self, unit, address, count): + """Read discrete inputs.""" + return await self.async_pymodbus_call( + unit, address, count, "bits", self._client.read_discrete_inputs + ) + + async def async_read_input_registers(self, unit, address, count): """Read input registers.""" - if self._config_delay: - return None - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.read_input_registers(address, count, **kwargs) - except ModbusException as exception_error: - result = exception_error - if not hasattr(result, "registers"): - self._log_error(result) - return None - self._in_error = False - return result - - def read_holding_registers(self, unit, address, count): + return await self.async_pymodbus_call( + unit, address, count, "registers", self._client.read_input_registers + ) + + async def async_read_holding_registers(self, unit, address, count): """Read holding registers.""" - if self._config_delay: - return None - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.read_holding_registers(address, count, **kwargs) - except ModbusException as exception_error: - result = exception_error - if not hasattr(result, "registers"): - self._log_error(result) - return None - self._in_error = False - return result - - def write_coil(self, unit, address, value) -> bool: + return await self.async_pymodbus_call( + unit, address, count, "registers", self._client.read_holding_registers + ) + + async def async_write_coil(self, unit, address, value) -> bool: """Write coil.""" - if self._config_delay: - return False - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.write_coil(address, value, **kwargs) - except ModbusException as exception_error: - result = exception_error - if not hasattr(result, "value"): - self._log_error(result) - return False - self._in_error = False - return True - - def write_coils(self, unit, address, values) -> bool: + return await self.async_pymodbus_call( + unit, address, value, "value", self._client.write_coil + ) + + async def async_write_coils(self, unit, address, values) -> bool: """Write coil.""" - if self._config_delay: - return False - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.write_coils(address, values, **kwargs) - except ModbusException as exception_error: - result = exception_error - if not hasattr(result, "count"): - self._log_error(result) - return False - self._in_error = False - return True - - def write_register(self, unit, address, value) -> bool: + return await self.async_pymodbus_call( + unit, address, values, "count", self._client.write_coils + ) + + async def async_write_register(self, unit, address, value) -> bool: """Write register.""" - if self._config_delay: - return False - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.write_register(address, value, **kwargs) - except ModbusException as exception_error: - result = exception_error - if not hasattr(result, "value"): - self._log_error(result) - return False - self._in_error = False - return True - - def write_registers(self, unit, address, values) -> bool: + return await self.async_pymodbus_call( + unit, address, value, "value", self._client.write_register + ) + + async def async_write_registers(self, unit, address, values) -> bool: """Write registers.""" - if self._config_delay: - return False - with self._lock: - kwargs = {"unit": unit} if unit else {} - try: - result = self._client.write_registers(address, values, **kwargs) - except ModbusException as exception_error: - result = exception_error - if not hasattr(result, "count"): - self._log_error(result) - return False - self._in_error = False - return True + return await self.async_pymodbus_call( + unit, address, values, "count", self._client.write_registers + ) diff --git a/homeassistant/components/modbus/sensor.py b/homeassistant/components/modbus/sensor.py index 91f80864f73b78..09d0ebf8c74e81 100644 --- a/homeassistant/components/modbus/sensor.py +++ b/homeassistant/components/modbus/sensor.py @@ -226,9 +226,7 @@ async def async_added_to_hass(self): if state: self._value = state.state - async_track_time_interval( - self.hass, lambda arg: self.update(), self._scan_interval - ) + async_track_time_interval(self.hass, self.async_update, self._scan_interval) @property def state(self): @@ -280,19 +278,21 @@ def _swap_registers(self, registers): registers.reverse() return registers - def update(self): + async def async_update(self, now=None): """Update the state of the sensor.""" + # remark "now" is a dummy parameter to avoid problems with + # async_track_time_interval if self._register_type == CALL_TYPE_REGISTER_INPUT: - result = self._hub.read_input_registers( + result = await self._hub.async_read_input_registers( self._slave, self._register, self._count ) else: - result = self._hub.read_holding_registers( + result = await self._hub.async_read_holding_registers( self._slave, self._register, self._count ) if result is None: self._available = False - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() return registers = self._swap_registers(result.registers) @@ -332,4 +332,4 @@ def update(self): self._value = f"{float(val):.{self._precision}f}" self._available = True - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() diff --git a/homeassistant/components/modbus/switch.py b/homeassistant/components/modbus/switch.py index c449c25bb228aa..f7d44105897991 100644 --- a/homeassistant/components/modbus/switch.py +++ b/homeassistant/components/modbus/switch.py @@ -62,11 +62,11 @@ def __init__(self, hub: ModbusHub, config: dict): self._scan_interval = timedelta(seconds=config[CONF_SCAN_INTERVAL]) self._address = config[CONF_ADDRESS] if config[CONF_WRITE_TYPE] == CALL_TYPE_COIL: - self._write_func = self._hub.write_coil + self._async_write_func = self._hub.async_write_coil self._command_on = 0x01 self._command_off = 0x00 else: - self._write_func = self._hub.write_register + self._async_write_func = self._hub.async_write_register self._command_on = config[CONF_COMMAND_ON] self._command_off = config[CONF_COMMAND_OFF] if CONF_VERIFY in config: @@ -83,13 +83,13 @@ def __init__(self, hub: ModbusHub, config: dict): self._state_off = config[CONF_VERIFY].get(CONF_STATE_OFF, self._command_off) if self._verify_type == CALL_TYPE_REGISTER_HOLDING: - self._read_func = self._hub.read_holding_registers + self._async_read_func = self._hub.async_read_holding_registers elif self._verify_type == CALL_TYPE_DISCRETE: - self._read_func = self._hub.read_discrete_inputs + self._async_read_func = self._hub.async_read_discrete_inputs elif self._verify_type == CALL_TYPE_REGISTER_INPUT: - self._read_func = self._hub.read_input_registers + self._async_read_func = self._hub.async_read_input_registers else: # self._verify_type == CALL_TYPE_COIL: - self._read_func = self._hub.read_coils + self._async_read_func = self._hub.async_read_coils else: self._verify_active = False @@ -99,9 +99,7 @@ async def async_added_to_hass(self): if state: self._is_on = state.state == STATE_ON - async_track_time_interval( - self.hass, lambda arg: self.update(), self._scan_interval - ) + async_track_time_interval(self.hass, self.async_update, self._scan_interval) @property def is_on(self): @@ -123,46 +121,52 @@ def available(self) -> bool: """Return True if entity is available.""" return self._available - def turn_on(self, **kwargs): + async def async_turn_on(self, **kwargs): """Set switch on.""" - result = self._write_func(self._slave, self._address, self._command_on) + result = await self._async_write_func( + self._slave, self._address, self._command_on + ) if result is False: self._available = False - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() else: self._available = True if self._verify_active: - self.update() + self.async_update() else: self._is_on = True - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() - def turn_off(self, **kwargs): + async def async_turn_off(self, **kwargs): """Set switch off.""" - result = self._write_func(self._slave, self._address, self._command_off) + result = await self._async_write_func( + self._slave, self._address, self._command_off + ) if result is False: self._available = False - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() else: self._available = True if self._verify_active: - self.update() + self.async_update() else: self._is_on = False - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() - def update(self): + async def async_update(self, now=None): """Update the entity state.""" + # remark "now" is a dummy parameter to avoid problems with + # async_track_time_interval if not self._verify_active: self._available = True - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() return - result = self._read_func(self._slave, self._verify_address, 1) + result = await self._async_read_func(self._slave, self._verify_address, 1) if result is None: self._available = False - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() return self._available = True @@ -182,4 +186,4 @@ def update(self): self._verify_address, value, ) - self.schedule_update_ha_state() + self.async_schedule_update_ha_state() diff --git a/tests/components/modbus/test_init.py b/tests/components/modbus/test_init.py index 7c0c7453abbdb1..23538772c90a5d 100644 --- a/tests/components/modbus/test_init.py +++ b/tests/components/modbus/test_init.py @@ -480,11 +480,13 @@ async def test_pymodbus_connect_fail(hass, caplog, mock_pymodbus): async def test_delay(hass, mock_pymodbus): - """Run test for different read.""" + """Run test for startup delay.""" # the purpose of this test is to test startup delay - # We "hijiack" binary_sensor and sensor in order - # to make a proper blackbox test. + # We "hijiack" a binary_sensor to make a proper blackbox test. + test_delay = 15 + test_scan_interval = 5 + entity_id = f"{BINARY_SENSOR_DOMAIN}.{TEST_SENSOR_NAME}" config = { DOMAIN: [ { @@ -492,101 +494,83 @@ async def test_delay(hass, mock_pymodbus): CONF_HOST: "modbusTestHost", CONF_PORT: 5501, CONF_NAME: TEST_MODBUS_NAME, - CONF_DELAY: 15, + CONF_DELAY: test_delay, CONF_BINARY_SENSORS: [ { CONF_INPUT_TYPE: CALL_TYPE_COIL, - CONF_NAME: f"{TEST_SENSOR_NAME}_2", + CONF_NAME: f"{TEST_SENSOR_NAME}", CONF_ADDRESS: 52, - CONF_SCAN_INTERVAL: 5, - }, - { - CONF_INPUT_TYPE: CALL_TYPE_DISCRETE, - CONF_NAME: f"{TEST_SENSOR_NAME}_1", - CONF_ADDRESS: 51, - CONF_SCAN_INTERVAL: 5, - }, - ], - CONF_SENSORS: [ - { - CONF_INPUT_TYPE: CALL_TYPE_REGISTER_HOLDING, - CONF_NAME: f"{TEST_SENSOR_NAME}_3", - CONF_ADDRESS: 53, - CONF_SCAN_INTERVAL: 5, - }, - { - CONF_INPUT_TYPE: CALL_TYPE_REGISTER_INPUT, - CONF_NAME: f"{TEST_SENSOR_NAME}_4", - CONF_ADDRESS: 54, - CONF_SCAN_INTERVAL: 5, + CONF_SCAN_INTERVAL: test_scan_interval, }, ], } ] } mock_pymodbus.read_coils.return_value = ReadResult([0x01]) - mock_pymodbus.read_discrete_inputs.return_value = ReadResult([0x01]) - mock_pymodbus.read_holding_registers.return_value = ReadResult([7]) - mock_pymodbus.read_input_registers.return_value = ReadResult([7]) now = dt_util.utcnow() with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): assert await async_setup_component(hass, DOMAIN, config) is True await hass.async_block_till_done() - now = now + timedelta(seconds=10) + # pass first scan_interval + start_time = now + now = now + timedelta(seconds=test_scan_interval) with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): async_fire_time_changed(hass, now) await hass.async_block_till_done() - - # Check states - entity_id = f"{BINARY_SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_1" - assert hass.states.get(entity_id).state == STATE_UNAVAILABLE - entity_id = f"{BINARY_SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_2" - assert hass.states.get(entity_id).state == STATE_UNAVAILABLE - entity_id = f"{SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_3" - assert hass.states.get(entity_id).state == STATE_UNAVAILABLE - entity_id = f"{SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_4" - assert hass.states.get(entity_id).state == STATE_UNAVAILABLE - - mock_pymodbus.reset_mock() - data = { - ATTR_HUB: TEST_MODBUS_NAME, - ATTR_UNIT: 17, - ATTR_ADDRESS: 16, - ATTR_STATE: False, - } - await hass.services.async_call(DOMAIN, SERVICE_WRITE_COIL, data, blocking=True) - assert not mock_pymodbus.write_coil.called - await hass.services.async_call(DOMAIN, SERVICE_WRITE_COIL, data, blocking=True) - assert not mock_pymodbus.write_coil.called - data[ATTR_STATE] = [True, False, True] - await hass.services.async_call(DOMAIN, SERVICE_WRITE_COIL, data, blocking=True) - assert not mock_pymodbus.write_coils.called - - del data[ATTR_STATE] - data[ATTR_VALUE] = 15 - await hass.services.async_call(DOMAIN, SERVICE_WRITE_REGISTER, data, blocking=True) - assert not mock_pymodbus.write_register.called - data[ATTR_VALUE] = [1, 2, 3] - await hass.services.async_call(DOMAIN, SERVICE_WRITE_REGISTER, data, blocking=True) - assert not mock_pymodbus.write_registers.called - - # 2 times fire_changed is needed to secure "normal" update is called. - now = now + timedelta(seconds=6) + assert hass.states.get(entity_id).state == STATE_UNAVAILABLE + + stop_time = start_time + timedelta(seconds=test_delay) + step_timedelta = timedelta(seconds=1) + while now < stop_time: + now = now + step_timedelta + with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): + async_fire_time_changed(hass, now) + await hass.async_block_till_done() + assert hass.states.get(entity_id).state == STATE_UNAVAILABLE + now = now + step_timedelta + step_timedelta with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): async_fire_time_changed(hass, now) await hass.async_block_till_done() - now = now + timedelta(seconds=10) + assert hass.states.get(entity_id).state == STATE_ON + + +async def test_thread_lock(hass, mock_pymodbus): + """Run test for block of threads.""" + + # the purpose of this test is to test the threads are not being blocked + # We "hijiack" a binary_sensor to make a proper blackbox test. + test_scan_interval = 5 + sensors = [] + for i in range(200): + sensors.append( + { + CONF_INPUT_TYPE: CALL_TYPE_COIL, + CONF_NAME: f"{TEST_SENSOR_NAME}_{i}", + CONF_ADDRESS: 52 + i, + CONF_SCAN_INTERVAL: test_scan_interval, + } + ) + config = { + DOMAIN: [ + { + CONF_TYPE: "tcp", + CONF_HOST: "modbusTestHost", + CONF_PORT: 5501, + CONF_NAME: TEST_MODBUS_NAME, + CONF_BINARY_SENSORS: sensors, + } + ] + } + mock_pymodbus.read_coils.return_value = ReadResult([0x01]) + now = dt_util.utcnow() with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): - async_fire_time_changed(hass, now) + assert await async_setup_component(hass, DOMAIN, config) is True await hass.async_block_till_done() - - # Check states - entity_id = f"{BINARY_SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_1" - assert not hass.states.get(entity_id).state == STATE_UNAVAILABLE - entity_id = f"{BINARY_SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_2" - assert not hass.states.get(entity_id).state == STATE_UNAVAILABLE - entity_id = f"{SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_3" - assert not hass.states.get(entity_id).state == STATE_UNAVAILABLE - entity_id = f"{SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_4" - assert not hass.states.get(entity_id).state == STATE_UNAVAILABLE + stop_time = now + timedelta(seconds=10) + step_timedelta = timedelta(seconds=1) + while now < stop_time: + now = now + step_timedelta + with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): + async_fire_time_changed(hass, now) + await hass.async_block_till_done() From e34cba53124631e3e8d9def7207f62dcb6a6bc09 Mon Sep 17 00:00:00 2001 From: jan Iversen Date: Fri, 14 May 2021 20:09:46 +0200 Subject: [PATCH 2/7] Please pylint. --- homeassistant/components/modbus/modbus.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/homeassistant/components/modbus/modbus.py b/homeassistant/components/modbus/modbus.py index 3dd4eeb7f6adb3..4e6fd8c0605560 100644 --- a/homeassistant/components/modbus/modbus.py +++ b/homeassistant/components/modbus/modbus.py @@ -47,12 +47,12 @@ async def async_modbus_setup( hass.data[DOMAIN] = hub_collect = {} for conf_hub in config[DOMAIN]: - myHub = ModbusHub(hass, conf_hub) - hub_collect[conf_hub[CONF_NAME]] = myHub + my_hub = ModbusHub(hass, conf_hub) + hub_collect[conf_hub[CONF_NAME]] = my_hub # modbus needs to be activated before components are loaded # to avoid a racing problem - await myHub.async_setup() + await my_hub.async_setup() # load platforms for component, conf_key in PLATFORMS: From 483342ccc693fc53cd45ecde2f1f0457dc9eb2e5 Mon Sep 17 00:00:00 2001 From: jan Iversen Date: Sat, 15 May 2021 07:14:50 +0200 Subject: [PATCH 3/7] Review comments. --- homeassistant/components/modbus/binary_sensor.py | 4 ++-- homeassistant/components/modbus/climate.py | 2 +- homeassistant/components/modbus/cover.py | 2 +- homeassistant/components/modbus/modbus.py | 6 ++++-- homeassistant/components/modbus/sensor.py | 4 ++-- homeassistant/components/modbus/switch.py | 14 +++++++------- tests/components/modbus/test_init.py | 9 ++++++--- 7 files changed, 23 insertions(+), 18 deletions(-) diff --git a/homeassistant/components/modbus/binary_sensor.py b/homeassistant/components/modbus/binary_sensor.py index ed59081150717b..9b6094b6dfbe07 100644 --- a/homeassistant/components/modbus/binary_sensor.py +++ b/homeassistant/components/modbus/binary_sensor.py @@ -158,9 +158,9 @@ async def async_update(self, now=None): ) if result is None: self._available = False - self.async_schedule_update_ha_state() + self.async_write_ha_state() return self._value = result.bits[0] & 1 self._available = True - self.async_schedule_update_ha_state() + self.async_write_ha_state() diff --git a/homeassistant/components/modbus/climate.py b/homeassistant/components/modbus/climate.py index 24a6038c68c37b..176afadafb1dc9 100644 --- a/homeassistant/components/modbus/climate.py +++ b/homeassistant/components/modbus/climate.py @@ -230,7 +230,7 @@ async def async_update(self, now=None): self._current_temperature_register_type, self._current_temperature_register ) - self.async_schedule_update_ha_state() + self.async_write_ha_state() async def _async_read_register(self, register_type, register) -> float | None: """Read register using the Modbus hub slave.""" diff --git a/homeassistant/components/modbus/cover.py b/homeassistant/components/modbus/cover.py index ddc44d6edcfcc4..ac785e25ef4f4b 100644 --- a/homeassistant/components/modbus/cover.py +++ b/homeassistant/components/modbus/cover.py @@ -179,7 +179,7 @@ async def async_update(self, now=None): else: self._value = await self._async_read_status_register() - self.async_schedule_update_ha_state() + self.async_write_ha_state() async def _async_read_status_register(self) -> int | None: """Read status register using the Modbus hub slave.""" diff --git a/homeassistant/components/modbus/modbus.py b/homeassistant/components/modbus/modbus.py index 4e6fd8c0605560..a3b44fa9400363 100644 --- a/homeassistant/components/modbus/modbus.py +++ b/homeassistant/components/modbus/modbus.py @@ -57,7 +57,9 @@ async def async_modbus_setup( # load platforms for component, conf_key in PLATFORMS: if conf_key in conf_hub: - await async_load_platform(hass, component, DOMAIN, conf_hub, config) + await hass.async_create_task( + async_load_platform(hass, component, DOMAIN, conf_hub, config) + ) async def async_stop_modbus(event): """Stop Modbus service.""" @@ -249,7 +251,7 @@ async def async_pymodbus_call(self, unit, address, value, check_attr, func): return None async with self._lock: return await self.hass.async_add_executor_job( - lambda: self._pymodbus_call(unit, address, value, check_attr, func) + self._pymodbus_call, unit, address, value, check_attr, func ) async def async_read_coils(self, unit, address, count): diff --git a/homeassistant/components/modbus/sensor.py b/homeassistant/components/modbus/sensor.py index 09d0ebf8c74e81..4197e0a8e10563 100644 --- a/homeassistant/components/modbus/sensor.py +++ b/homeassistant/components/modbus/sensor.py @@ -292,7 +292,7 @@ async def async_update(self, now=None): ) if result is None: self._available = False - self.async_schedule_update_ha_state() + self.async_write_ha_state() return registers = self._swap_registers(result.registers) @@ -332,4 +332,4 @@ async def async_update(self, now=None): self._value = f"{float(val):.{self._precision}f}" self._available = True - self.async_schedule_update_ha_state() + self.async_write_ha_state() diff --git a/homeassistant/components/modbus/switch.py b/homeassistant/components/modbus/switch.py index f7d44105897991..c220e76c8d12b7 100644 --- a/homeassistant/components/modbus/switch.py +++ b/homeassistant/components/modbus/switch.py @@ -129,14 +129,14 @@ async def async_turn_on(self, **kwargs): ) if result is False: self._available = False - self.async_schedule_update_ha_state() + self.async_write_ha_state() else: self._available = True if self._verify_active: self.async_update() else: self._is_on = True - self.async_schedule_update_ha_state() + self.async_write_ha_state() async def async_turn_off(self, **kwargs): """Set switch off.""" @@ -145,14 +145,14 @@ async def async_turn_off(self, **kwargs): ) if result is False: self._available = False - self.async_schedule_update_ha_state() + self.async_write_ha_state() else: self._available = True if self._verify_active: self.async_update() else: self._is_on = False - self.async_schedule_update_ha_state() + self.async_write_ha_state() async def async_update(self, now=None): """Update the entity state.""" @@ -160,13 +160,13 @@ async def async_update(self, now=None): # async_track_time_interval if not self._verify_active: self._available = True - self.async_schedule_update_ha_state() + self.async_write_ha_state() return result = await self._async_read_func(self._slave, self._verify_address, 1) if result is None: self._available = False - self.async_schedule_update_ha_state() + self.async_write_ha_state() return self._available = True @@ -186,4 +186,4 @@ async def async_update(self, now=None): self._verify_address, value, ) - self.async_schedule_update_ha_state() + self.async_write_ha_state() diff --git a/tests/components/modbus/test_init.py b/tests/components/modbus/test_init.py index 23538772c90a5d..8959fe82319aba 100644 --- a/tests/components/modbus/test_init.py +++ b/tests/components/modbus/test_init.py @@ -514,13 +514,13 @@ async def test_delay(hass, mock_pymodbus): # pass first scan_interval start_time = now - now = now + timedelta(seconds=test_scan_interval) + now = now + timedelta(seconds=(test_scan_interval + 1)) with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): async_fire_time_changed(hass, now) await hass.async_block_till_done() assert hass.states.get(entity_id).state == STATE_UNAVAILABLE - stop_time = start_time + timedelta(seconds=test_delay) + stop_time = start_time + timedelta(seconds=(test_delay + 1)) step_timedelta = timedelta(seconds=1) while now < stop_time: now = now + step_timedelta @@ -528,7 +528,7 @@ async def test_delay(hass, mock_pymodbus): async_fire_time_changed(hass, now) await hass.async_block_till_done() assert hass.states.get(entity_id).state == STATE_UNAVAILABLE - now = now + step_timedelta + step_timedelta + now = now + step_timedelta + timedelta(seconds=2) with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): async_fire_time_changed(hass, now) await hass.async_block_till_done() @@ -574,3 +574,6 @@ async def test_thread_lock(hass, mock_pymodbus): with mock.patch("homeassistant.helpers.event.dt_util.utcnow", return_value=now): async_fire_time_changed(hass, now) await hass.async_block_till_done() + for i in range(200): + entity_id = f"{BINARY_SENSOR_DOMAIN}.{TEST_SENSOR_NAME}_{i}" + assert hass.states.get(entity_id).state == STATE_ON From 20c6528a0e41c004494b4fd664907e3967d98ea8 Mon Sep 17 00:00:00 2001 From: jan Iversen Date: Sat, 15 May 2021 07:56:24 +0200 Subject: [PATCH 4/7] PARALLEL_UPDATE=1 --- homeassistant/components/modbus/binary_sensor.py | 1 + homeassistant/components/modbus/climate.py | 1 + homeassistant/components/modbus/cover.py | 1 + homeassistant/components/modbus/sensor.py | 1 + homeassistant/components/modbus/switch.py | 1 + 5 files changed, 5 insertions(+) diff --git a/homeassistant/components/modbus/binary_sensor.py b/homeassistant/components/modbus/binary_sensor.py index 9b6094b6dfbe07..14d01535c5b9b2 100644 --- a/homeassistant/components/modbus/binary_sensor.py +++ b/homeassistant/components/modbus/binary_sensor.py @@ -36,6 +36,7 @@ MODBUS_DOMAIN, ) +PARALLEL_UPDATES = 1 _LOGGER = logging.getLogger(__name__) diff --git a/homeassistant/components/modbus/climate.py b/homeassistant/components/modbus/climate.py index 176afadafb1dc9..43c0f0d05db085 100644 --- a/homeassistant/components/modbus/climate.py +++ b/homeassistant/components/modbus/climate.py @@ -46,6 +46,7 @@ ) from .modbus import ModbusHub +PARALLEL_UPDATES = 1 _LOGGER = logging.getLogger(__name__) diff --git a/homeassistant/components/modbus/cover.py b/homeassistant/components/modbus/cover.py index ac785e25ef4f4b..edb81ae7eb39e3 100644 --- a/homeassistant/components/modbus/cover.py +++ b/homeassistant/components/modbus/cover.py @@ -33,6 +33,7 @@ ) from .modbus import ModbusHub +PARALLEL_UPDATES = 1 _LOGGER = logging.getLogger(__name__) diff --git a/homeassistant/components/modbus/sensor.py b/homeassistant/components/modbus/sensor.py index 4197e0a8e10563..7aeb142d1e225b 100644 --- a/homeassistant/components/modbus/sensor.py +++ b/homeassistant/components/modbus/sensor.py @@ -59,6 +59,7 @@ MODBUS_DOMAIN, ) +PARALLEL_UPDATES = 1 _LOGGER = logging.getLogger(__name__) diff --git a/homeassistant/components/modbus/switch.py b/homeassistant/components/modbus/switch.py index c220e76c8d12b7..4495a72c63a398 100644 --- a/homeassistant/components/modbus/switch.py +++ b/homeassistant/components/modbus/switch.py @@ -34,6 +34,7 @@ ) from .modbus import ModbusHub +PARALLEL_UPDATES = 1 _LOGGER = logging.getLogger(__name__) From eb093bd9b3f646e4e0f17d3f9eb717559f8818e3 Mon Sep 17 00:00:00 2001 From: jan iversen Date: Sat, 15 May 2021 08:20:17 +0200 Subject: [PATCH 5/7] Update homeassistant/components/modbus/modbus.py Co-authored-by: Martin Hjelmare --- homeassistant/components/modbus/modbus.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/components/modbus/modbus.py b/homeassistant/components/modbus/modbus.py index a3b44fa9400363..2d9b3a9baecd1d 100644 --- a/homeassistant/components/modbus/modbus.py +++ b/homeassistant/components/modbus/modbus.py @@ -57,7 +57,7 @@ async def async_modbus_setup( # load platforms for component, conf_key in PLATFORMS: if conf_key in conf_hub: - await hass.async_create_task( + hass.async_create_task( async_load_platform(hass, component, DOMAIN, conf_hub, config) ) From cafa20cc6f56a7d7dc0965b197de52ec4eb4091d Mon Sep 17 00:00:00 2001 From: jan Iversen Date: Sat, 15 May 2021 15:34:58 +0200 Subject: [PATCH 6/7] Review2. --- homeassistant/components/modbus/modbus.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/modbus/modbus.py b/homeassistant/components/modbus/modbus.py index 2d9b3a9baecd1d..5d8e6a9d984b53 100644 --- a/homeassistant/components/modbus/modbus.py +++ b/homeassistant/components/modbus/modbus.py @@ -17,6 +17,7 @@ CONF_TYPE, EVENT_HOMEASSISTANT_STOP, ) +from homeassistant.core import callback from homeassistant.helpers.discovery import async_load_platform from homeassistant.helpers.event import async_call_later @@ -198,10 +199,11 @@ async def async_setup(self): # Start counting down to allow modbus requests. if self._config_delay: self._async_cancel_listener = async_call_later( - self.hass, self._config_delay, self.async_end_delay + self.hass, self._config_delay, self.callback_end_delay ) - async def async_end_delay(self, args): + @callback + def callback_end_delay(self, args): """End startup delay.""" self._async_cancel_listener = None self._config_delay = 0 From f49bb8dce52352777a109c51b9951cd47964538b Mon Sep 17 00:00:00 2001 From: jan Iversen Date: Sat, 15 May 2021 17:09:43 +0200 Subject: [PATCH 7/7] Rename end_delay. --- homeassistant/components/modbus/modbus.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/homeassistant/components/modbus/modbus.py b/homeassistant/components/modbus/modbus.py index 5d8e6a9d984b53..f755b29b67a945 100644 --- a/homeassistant/components/modbus/modbus.py +++ b/homeassistant/components/modbus/modbus.py @@ -199,11 +199,11 @@ async def async_setup(self): # Start counting down to allow modbus requests. if self._config_delay: self._async_cancel_listener = async_call_later( - self.hass, self._config_delay, self.callback_end_delay + self.hass, self._config_delay, self.async_end_delay ) @callback - def callback_end_delay(self, args): + def async_end_delay(self, args): """End startup delay.""" self._async_cancel_listener = None self._config_delay = 0