[modbus] Fix timing bugs and better adhere to spec (#8032)

Co-authored-by: brambo123 <52667932+brambo123@users.noreply.github.com>
Co-authored-by: Keith Burzinski <kbx81x@gmail.com>
Co-authored-by: J. Nick Koston <nick@koston.org>
Co-authored-by: J. Nick Koston <nick+github@koston.org>
Co-authored-by: J. Nick Koston <nick@home-assistant.io>

tests/components/modbus/common.yaml

@@ -1,3 +1,5 @@
 modbus:
   id: mod_bus1
   flow_control_pin: ${flow_control_pin}
+  send_wait_time: 500ms
+  turnaround_time: 100ms

tests/integration/fixtures/external_components/uart_mock/__init__.py

@@ -71,6 +71,7 @@ RESPONSE_SCHEMA = cv.Schema(
     {
         cv.Required(CONF_EXPECT_TX): [cv.hex_uint8_t],
         cv.Required(CONF_INJECT_RX): [cv.hex_uint8_t],
+        cv.Optional(CONF_DELAY, default="0ms"): cv.positive_time_period_milliseconds,
     }
 )
 
@@ -151,7 +152,8 @@ async def to_code(config):
     for response in config[CONF_RESPONSES]:
         tx_data = response[CONF_EXPECT_TX]
         rx_data = response[CONF_INJECT_RX]
-        cg.add(var.add_response(tx_data, rx_data))
+        delay_ms = response[CONF_DELAY]
+        cg.add(var.add_response(tx_data, rx_data, delay_ms))
 
     for periodic in config[CONF_PERIODIC_RX]:
         data = periodic[CONF_DATA]

tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp

@@ -36,8 +36,8 @@ void MockUartComponent::loop() {
   // component (e.g., LD2410) a chance to process each batch independently.
   if (this->injection_index_ < this->injections_.size()) {
     auto &injection = this->injections_[this->injection_index_];
-    uint32_t target_time = this->scenario_start_ms_ + this->cumulative_delay_ms_ + injection.delay_ms;
-    if (now >= target_time) {
+    uint32_t total_delay = this->cumulative_delay_ms_ + injection.delay_ms;
+    if (now - this->scenario_start_ms_ >= total_delay) {
       ESP_LOGD(TAG, "Injecting %zu RX bytes (injection %u)", injection.rx_data.size(), this->injection_index_);
       this->inject_to_rx_buffer(injection.rx_data);
       this->cumulative_delay_ms_ += injection.delay_ms;
@@ -52,6 +52,15 @@ void MockUartComponent::loop() {
       periodic.last_inject_ms = now;
     }
   }
+
+  // Process delayed responses
+  for (auto &response : this->responses_) {
+    if (response.delay_ms > 0 && response.last_match_ms > 0 && now - response.last_match_ms >= response.delay_ms) {
+      ESP_LOGD(TAG, "Injecting %zu RX bytes for delayed response", response.inject_rx.size());
+      this->inject_to_rx_buffer(response.inject_rx);
+      response.last_match_ms = 0;  // Reset to prevent repeated injection
+    }
+  }
 }
 
 void MockUartComponent::start_scenario() {
@@ -149,8 +158,9 @@ void MockUartComponent::add_injection(const std::vector<uint8_t> &rx_data, uint3
   this->injections_.push_back({rx_data, delay_ms});
 }
 
-void MockUartComponent::add_response(const std::vector<uint8_t> &expect_tx, const std::vector<uint8_t> &inject_rx) {
-  this->responses_.push_back({expect_tx, inject_rx});
+void MockUartComponent::add_response(const std::vector<uint8_t> &expect_tx, const std::vector<uint8_t> &inject_rx,
+                                     uint32_t delay_ms) {
+  this->responses_.push_back({expect_tx, inject_rx, delay_ms, 0});
 }
 
 void MockUartComponent::add_periodic_rx(const std::vector<uint8_t> &data, uint32_t interval_ms) {
@@ -166,7 +176,13 @@ void MockUartComponent::try_match_response_() {
     size_t offset = this->tx_buffer_.size() - response.expect_tx.size();
     if (std::equal(response.expect_tx.begin(), response.expect_tx.end(), this->tx_buffer_.begin() + offset)) {
       ESP_LOGD(TAG, "TX match found, injecting %zu RX bytes", response.inject_rx.size());
-      this->inject_to_rx_buffer(response.inject_rx);
+      if (response.delay_ms > 0) {
+        ESP_LOGD(TAG, "Delaying response by %u ms", response.delay_ms);
+        // Schedule the response injection as a future injection
+        response.last_match_ms = App.get_loop_component_start_time();
+      } else {
+        this->inject_to_rx_buffer(response.inject_rx);
+      }
       this->tx_buffer_.clear();
       return;
     }

tests/integration/fixtures/external_components/uart_mock/uart_mock.h

@@ -34,7 +34,8 @@ class MockUartComponent : public uart::UARTComponent, public Component {
 
   // Scenario configuration - called from generated code
   void add_injection(const std::vector<uint8_t> &rx_data, uint32_t delay_ms);
-  void add_response(const std::vector<uint8_t> &expect_tx, const std::vector<uint8_t> &inject_rx);
+  void add_response(const std::vector<uint8_t> &expect_tx, const std::vector<uint8_t> &inject_rx,
+                    uint32_t delay_ms = 0);
   void add_periodic_rx(const std::vector<uint8_t> &data, uint32_t interval_ms);
 
   void start_scenario();
@@ -64,6 +65,8 @@ class MockUartComponent : public uart::UARTComponent, public Component {
   struct Response {
     std::vector<uint8_t> expect_tx;
     std::vector<uint8_t> inject_rx;
+    uint32_t delay_ms;
+    uint32_t last_match_ms{0};
   };
   std::vector<Response> responses_;
   std::vector<uint8_t> tx_buffer_;

tests/integration/fixtures/uart_mock_modbus.yaml

@@ -25,20 +25,64 @@ uart_mock:
     auto_start: false
     debug:
     responses:
-      - expect_tx: [0x01, 0x03, 0x00, 0x03, 0x00, 0x01, 0x74, 0x0A] # Read holding register 1 on device 1
+      - expect_tx: [0x01, 0x03, 0x00, 0x03, 0x00, 0x01, 0x74, 0x0A] # Read holding register 3 on device 1 (basic_register)
         inject_rx: [0x01, 0x03, 0x02, 0x01, 0x03, 0xF9, 0xD5] # Return value 0x0103 (hex) = 259 (dec)
+      - expect_tx: [0x01, 0x03, 0x00, 0x05, 0x00, 0x01, 0x94, 0x0B] # Read holding register 5 on device 1 (delayed_response)
+        delay: 100ms # Shorter than modbus send_wait_time of 200ms, should succeed
+        inject_rx: [0x01, 0x03, 0x02, 0x00, 0xFF, 0xF8, 0x04] # Return value 0x00FF (hex) = 255 (dec)
+      - expect_tx: [0x02, 0x03, 0x00, 0x07, 0x00, 0x01, 0x35, 0xF8] # Read holding register 7 on device 2 (late_response)
+        delay: 300ms # Longer than modbus send_wait_time of 200ms, should cause timeout
+        inject_rx: [0x02, 0x03, 0x02, 0x00, 0xF0, 0xFC, 0x00] # Return value 0x00F0 (hex) = 240 (dec)
+      - expect_tx: [0x03, 0x03, 0x00, 0x09, 0x00, 0x01, 0x55, 0xEA] # Read holding register 9 on device 3 (no_response)
+        inject_rx: [] # No response, should cause timeout
+      - expect_tx: [0x01, 0x03, 0x00, 0x0A, 0x00, 0x01, 0xA4, 0x08] # Read holding register A on device 1 (exception_response)
+        inject_rx: [0x01, 0x83, 0x02, 0xC0, 0xF1] # Exception response with code 2 (illegal data address)
 
 modbus:
   uart_id: virtual_uart_dev
+  send_wait_time: 200ms
+  turnaround_time: 10ms
 
 modbus_controller:
-  address: 1
+  - address: 1
+    id: modbus_controller_ok
+    max_cmd_retries: 0
+    update_interval: 1s
+  - address: 2
+    id: modbus_controller_slow
+    max_cmd_retries: 0
+    update_interval: 1s
+  - address: 3
+    id: modbus_controller_offline
+    max_cmd_retries: 0
+    update_interval: 1s
 
 sensor:
   - platform: modbus_controller
     name: "basic_register"
     address: 0x03
     register_type: holding
+    modbus_controller_id: modbus_controller_ok
+  - platform: modbus_controller
+    name: "delayed_response"
+    address: 0x05
+    register_type: holding
+    modbus_controller_id: modbus_controller_ok
+  - platform: modbus_controller
+    name: "late_response"
+    address: 0x07
+    register_type: holding
+    modbus_controller_id: modbus_controller_slow
+  - platform: modbus_controller
+    name: "no_response"
+    address: 0x09
+    register_type: holding
+    modbus_controller_id: modbus_controller_offline
+  - platform: modbus_controller
+    name: "exception_response"
+    address: 0x0A
+    register_type: holding
+    modbus_controller_id: modbus_controller_ok
 
 button:
   - platform: template

tests/integration/fixtures/uart_mock_modbus_timing.yaml

@@ -46,10 +46,12 @@ uart_mock:
 
 modbus:
   uart_id: virtual_uart_dev
+  turnaround_time: 10ms
 
 sensor:
   - platform: sdm_meter
     address: 2
+    update_interval: 1s
     phase_a:
       voltage:
         name: sdm_voltage

tests/integration/test_uart_mock_modbus.py

@@ -39,9 +39,17 @@ async def test_uart_mock_modbus(
     # Track sensor state updates (after initial state is swallowed)
     sensor_states: dict[str, list[float]] = {
         "basic_register": [],
+        "delayed_response": [],
+        "late_response": [],
+        "no_response": [],
+        "exception_response": [],
     }
 
     basic_register_changed = loop.create_future()
+    delayed_response_changed = loop.create_future()
+    late_response_changed = loop.create_future()
+    no_response_changed = loop.create_future()
+    exception_response_changed = loop.create_future()
 
     def on_state(state: EntityState) -> None:
         if isinstance(state, SensorState) and not state.missing_state:
@@ -54,6 +62,23 @@ async def test_uart_mock_modbus(
                     and not basic_register_changed.done()
                 ):
                     basic_register_changed.set_result(True)
+                elif (
+                    sensor_name == "delayed_response"
+                    and state.state == 255.0
+                    and not delayed_response_changed.done()
+                ):
+                    delayed_response_changed.set_result(True)
+                elif (
+                    sensor_name == "late_response" and not late_response_changed.done()
+                ):
+                    late_response_changed.set_result(True)
+                elif sensor_name == "no_response" and not no_response_changed.done():
+                    no_response_changed.set_result(True)
+                elif (
+                    sensor_name == "exception_response"
+                    and not exception_response_changed.done()
+                ):
+                    exception_response_changed.set_result(True)
 
     async with (
         run_compiled(yaml_config),
@@ -79,20 +104,52 @@ async def test_uart_mock_modbus(
         assert start_btn is not None, "Start Scenario button not found"
         client.button_command(start_btn.key)
 
+        try:
+            await asyncio.wait_for(delayed_response_changed, timeout=2.0)
+        except TimeoutError:
+            pytest.fail(
+                f"Timeout waiting for delayed_response change. Received sensor states:\n"
+                f"  delayed_response: {sensor_states['delayed_response']}\n"
+            )
+
+        try:
+            await asyncio.wait_for(late_response_changed, timeout=2.0)
+            pytest.fail(
+                f"late_response change should not have been triggered, but was. Received sensor states:\n"
+                f"  late_response: {sensor_states['late_response']}\n"
+            )
+        except TimeoutError:
+            pass  # Expected timeout since we never inject a response for late_response
+
+        try:
+            await asyncio.wait_for(no_response_changed, timeout=2.0)
+            pytest.fail(
+                f"no_response change should not have been triggered, but was. Received sensor states:\n"
+                f"  no_response: {sensor_states['no_response']}\n"
+            )
+        except TimeoutError:
+            pass  # Expected timeout since we never inject a response for no_response
+
         # Wait for basic register to be updated with successful parse
         try:
-            await asyncio.wait_for(basic_register_changed, timeout=15.0)
+            await asyncio.wait_for(basic_register_changed, timeout=2.0)
         except TimeoutError:
             pytest.fail(
                 f"Timeout waiting for Basic Register change. Received sensor states:\n"
                 f"  basic_register: {sensor_states['basic_register']}\n"
             )
 
+        try:
+            await asyncio.wait_for(exception_response_changed, timeout=2.0)
+            pytest.fail(
+                f"exception_response change should not have been triggered, but was. Received sensor states:\n"
+                f"  exception_response: {sensor_states['exception_response']}\n"
+            )
+        except TimeoutError:
+            pass
+
 
 @pytest.mark.asyncio
-@pytest.mark.xfail(
-    reason="There is a bug in UART which will timeout for long responses."
-)
 async def test_uart_mock_modbus_timing(
     yaml_config: str,
     run_compiled: RunCompiledFunction,
@@ -155,7 +212,7 @@ async def test_uart_mock_modbus_timing(
 
         # Wait for voltage to be updated with successful parse
         try:
-            await asyncio.wait_for(voltage_changed, timeout=15.0)
+            await asyncio.wait_for(voltage_changed, timeout=2.0)
         except TimeoutError:
             pytest.fail(
                 f"Timeout waiting for SDM voltage change. Received sensor states:\n"