Check initial connect() worked in modbus (#51470)

Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Co-authored-by: Franck Nijhof <frenck@frenck.nl>
Co-authored-by: Franck Nijhof <git@frenck.dev>

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

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):