From b8afb7dcfe3d7be28a8165df1a571c3b294fbb07 Mon Sep 17 00:00:00 2001 From: jan iversen Date: Sat, 5 Jun 2021 14:39:09 +0200 Subject: [PATCH] Check initial connect() worked in modbus (#51470) Co-authored-by: Martin Hjelmare Co-authored-by: Franck Nijhof Co-authored-by: Franck Nijhof --- homeassistant/components/modbus/modbus.py | 29 +++++--- tests/components/modbus/test_init.py | 82 ++++++++++++++++++++--- 2 files changed, 92 insertions(+), 19 deletions(-) diff --git a/homeassistant/components/modbus/modbus.py b/homeassistant/components/modbus/modbus.py index 4a02f019238..20e9978c0e8 100644 --- a/homeassistant/components/modbus/modbus.py +++ b/homeassistant/components/modbus/modbus.py @@ -65,7 +65,8 @@ async def async_modbus_setup( # modbus needs to be activated before components are loaded # to avoid a racing problem - await my_hub.async_setup() + if not await my_hub.async_setup(): + return False # load platforms for component, conf_key in PLATFORMS: @@ -195,8 +196,8 @@ class ModbusHub: }, } - def _log_error(self, exception_error: ModbusException, error_state=True): - log_text = "Pymodbus: " + str(exception_error) + def _log_error(self, text: str, error_state=True): + log_text = f"Pymodbus: {text}" if self._in_error: _LOGGER.debug(log_text) else: @@ -241,11 +242,13 @@ class ModbusHub: reset_socket=self._config_reset_socket, ) except ModbusException as exception_error: - self._log_error(exception_error, error_state=False) - return + self._log_error(str(exception_error), error_state=False) + return False async with self._lock: - await self.hass.async_add_executor_job(self._pymodbus_connect) + if not await self.hass.async_add_executor_job(self._pymodbus_connect): + self._log_error("initial connect failed, no retry", error_state=False) + return False self._call_type[CALL_TYPE_COIL][ENTRY_FUNC] = self._client.read_coils self._call_type[CALL_TYPE_DISCRETE][ @@ -271,6 +274,7 @@ class ModbusHub: self._async_cancel_listener = async_call_later( self.hass, self._config_delay, self.async_end_delay ) + return True @callback def async_end_delay(self, args): @@ -284,7 +288,7 @@ class ModbusHub: try: self._client.close() except ModbusException as exception_error: - self._log_error(exception_error) + self._log_error(str(exception_error)) self._client = None async def async_close(self): @@ -299,9 +303,10 @@ class ModbusHub: def _pymodbus_connect(self): """Connect client.""" try: - self._client.connect() + return self._client.connect() except ModbusException as exception_error: - self._log_error(exception_error, error_state=False) + self._log_error(str(exception_error), error_state=False) + return False def _pymodbus_call(self, unit, address, value, use_call): """Call sync. pymodbus.""" @@ -309,10 +314,10 @@ class ModbusHub: try: result = self._call_type[use_call][ENTRY_FUNC](address, value, **kwargs) except ModbusException as exception_error: - self._log_error(exception_error) + self._log_error(str(exception_error)) result = exception_error if not hasattr(result, self._call_type[use_call][ENTRY_ATTR]): - self._log_error(result) + self._log_error(str(result)) return None self._in_error = False return result @@ -321,6 +326,8 @@ class ModbusHub: """Convert async to sync pymodbus call.""" if self._config_delay: return None + if not self._client.is_socket_open(): + return None async with self._lock: return await self.hass.async_add_executor_job( self._pymodbus_call, unit, address, value, use_call diff --git a/tests/components/modbus/test_init.py b/tests/components/modbus/test_init.py index 37cc8ae8766..5f311ab57b0 100644 --- a/tests/components/modbus/test_init.py +++ b/tests/components/modbus/test_init.py @@ -511,15 +511,24 @@ async def test_pymodbus_constructor_fail(hass, caplog): ) as mock_pb: caplog.set_level(logging.ERROR) mock_pb.side_effect = ModbusException("test no class") - assert await async_setup_component(hass, DOMAIN, config) is True + assert await async_setup_component(hass, DOMAIN, config) is False await hass.async_block_till_done() - assert len(caplog.records) == 1 + assert caplog.messages[0].startswith("Pymodbus: Modbus Error: test") assert caplog.records[0].levelname == "ERROR" assert mock_pb.called -async def test_pymodbus_connect_fail(hass, caplog, mock_pymodbus): - """Run test for failing pymodbus constructor.""" +@pytest.mark.parametrize( + "do_connect,do_exception,do_text", + [ + [False, None, "initial connect failed, no retry"], + [True, ModbusException("no connect"), "Modbus Error: no connect"], + ], +) +async def test_pymodbus_connect_fail( + hass, do_connect, do_exception, do_text, caplog, mock_pymodbus +): + """Run test for failing pymodbus connect.""" config = { DOMAIN: [ { @@ -530,12 +539,69 @@ async def test_pymodbus_connect_fail(hass, caplog, mock_pymodbus): ] } caplog.set_level(logging.ERROR) - mock_pymodbus.connect.side_effect = ModbusException("test connect fail") - mock_pymodbus.close.side_effect = ModbusException("test connect fail") + mock_pymodbus.connect.return_value = do_connect + mock_pymodbus.connect.side_effect = do_exception + assert await async_setup_component(hass, DOMAIN, config) is False + await hass.async_block_till_done() + assert caplog.messages[0].startswith(f"Pymodbus: {do_text}") + assert caplog.records[0].levelname == "ERROR" + + +async def test_pymodbus_close_fail(hass, caplog, mock_pymodbus): + """Run test for failing pymodbus close.""" + config = { + DOMAIN: [ + { + CONF_TYPE: "tcp", + CONF_HOST: TEST_HOST, + CONF_PORT: 5501, + } + ] + } + caplog.set_level(logging.ERROR) + mock_pymodbus.connect.return_value = True + mock_pymodbus.close.side_effect = ModbusException("close fail") assert await async_setup_component(hass, DOMAIN, config) is True await hass.async_block_till_done() - assert len(caplog.records) == 1 - assert caplog.records[0].levelname == "ERROR" + # Close() is called as part of teardown + + +async def test_disconnect(hass, mock_pymodbus): + """Run test for startup delay.""" + + # the purpose of this test is to test a device disconnect + # We "hijiack" a binary_sensor to make a proper blackbox test. + entity_id = f"{BINARY_SENSOR_DOMAIN}.{TEST_SENSOR_NAME}" + config = { + DOMAIN: [ + { + CONF_TYPE: "tcp", + CONF_HOST: TEST_HOST, + CONF_PORT: 5501, + CONF_NAME: TEST_MODBUS_NAME, + CONF_BINARY_SENSORS: [ + { + CONF_INPUT_TYPE: CALL_TYPE_COIL, + CONF_NAME: f"{TEST_SENSOR_NAME}", + CONF_ADDRESS: 52, + }, + ], + } + ] + } + mock_pymodbus.read_coils.return_value = ReadResult([0x01]) + mock_pymodbus.is_socket_open.return_value = False + 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() + + # pass first scan_interval + now = now + timedelta(seconds=20) + 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 async def test_delay(hass, mock_pymodbus):