From d13e87b5b5497c98060ab9c336346b8282ac0548 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Mar 2026 17:37:57 -1000 Subject: [PATCH 1/6] [modbus] Fix timeout for non-hardware UARTs (e.g., USB UART) The rx_full_threshold is only meaningful for ESP32 native UARTs where it controls the hardware FIFO interrupt threshold. On other platforms (USB UART, Arduino, etc.) it was left at the default of 1, causing the long_rx_buffer_delay_ms calculation to produce a tiny value (~1-2ms). This caused false timeouts on partial responses when data arrives in USB packets with inherent USB-level latency, leading to cascading CRC failures as the remaining bytes were parsed as garbage. Change the default rx_full_threshold to 0 (unset sentinel) and use 50ms when unset, matching the previous hardcoded timeout behavior. --- esphome/components/modbus/modbus.cpp | 10 ++++++++-- esphome/components/uart/uart_component.h | 5 ++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index 28e26e307e3..da5bfa5ef85 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -21,8 +21,14 @@ void Modbus::setup() { // 3.5 characters * 11 bits per character * 1000ms/sec / (bits/sec) (Standard modbus frame delay) (uint16_t) (3.5 * 11 * 1000 / this->parent_->get_baud_rate()) + 1); - this->long_rx_buffer_delay_ms_ = - (this->parent_->get_rx_full_threshold() * 11 * 1000 / this->parent_->get_baud_rate()) + 1; + // When rx_full_threshold is configured (non-zero), the UART has a hardware FIFO with a + // meaningful threshold (e.g., ESP32 native UART), so we can calculate a precise delay. + // Otherwise (e.g., USB UART), use 50ms to handle data arriving in chunks. + static constexpr uint16_t DEFAULT_LONG_RX_BUFFER_DELAY_MS = 50; + size_t rx_threshold = this->parent_->get_rx_full_threshold(); + this->long_rx_buffer_delay_ms_ = rx_threshold != uart::UARTComponent::RX_FULL_THRESHOLD_UNSET + ? (rx_threshold * 11 * 1000 / this->parent_->get_baud_rate()) + 1 + : DEFAULT_LONG_RX_BUFFER_DELAY_MS; } void Modbus::loop() { diff --git a/esphome/components/uart/uart_component.h b/esphome/components/uart/uart_component.h index 078ce64b30f..3d2a848462e 100644 --- a/esphome/components/uart/uart_component.h +++ b/esphome/components/uart/uart_component.h @@ -187,7 +187,10 @@ class UARTComponent { InternalGPIOPin *rx_pin_{}; InternalGPIOPin *flow_control_pin_{}; size_t rx_buffer_size_{}; - size_t rx_full_threshold_{1}; + static constexpr size_t RX_FULL_THRESHOLD_UNSET = 0; + // ESP-IDF always sets this at codegen time via set_rx_full_threshold(). + // Other platforms (USB UART, Arduino, etc.) leave it unset. + size_t rx_full_threshold_{RX_FULL_THRESHOLD_UNSET}; size_t rx_timeout_{0}; uint32_t baud_rate_{0}; uint8_t stop_bits_{0}; From c4b26fd3f54d1d1015ab3a21d575baf2ca2430cd Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Mar 2026 17:41:56 -1000 Subject: [PATCH 2/6] Add integration test for non-hardware UART and extract constants - Add test fixture simulating USB UART (no rx_full_threshold set) with a 20ms gap between response chunks - Make rx_full_threshold optional in uart_mock - Extract MODBUS_BITS_PER_CHAR and MS_PER_SEC constexprs --- esphome/components/modbus/modbus.cpp | 16 ++-- .../external_components/uart_mock/__init__.py | 5 +- .../uart_mock_modbus_no_threshold.yaml | 64 +++++++++++++++ tests/integration/test_uart_mock_modbus.py | 79 +++++++++++++++++++ 4 files changed, 157 insertions(+), 7 deletions(-) create mode 100644 tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index da5bfa5ef85..4f683995fd9 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -11,6 +11,11 @@ static const char *const TAG = "modbus"; // Maximum bytes to log for Modbus frames (truncated if larger) static constexpr size_t MODBUS_MAX_LOG_BYTES = 64; +// Bits per character on the wire: 1 start + 8 data + 1 parity + 1 stop +static constexpr uint32_t MODBUS_BITS_PER_CHAR = 11; +// Milliseconds per second +static constexpr uint32_t MS_PER_SEC = 1000; + void Modbus::setup() { if (this->flow_control_pin_ != nullptr) { this->flow_control_pin_->setup(); @@ -19,16 +24,17 @@ void Modbus::setup() { this->frame_delay_ms_ = std::max(2, // 1750us minimum per spec - rounded up to 2ms. // 3.5 characters * 11 bits per character * 1000ms/sec / (bits/sec) (Standard modbus frame delay) - (uint16_t) (3.5 * 11 * 1000 / this->parent_->get_baud_rate()) + 1); + (uint16_t) (3.5 * MODBUS_BITS_PER_CHAR * MS_PER_SEC / this->parent_->get_baud_rate()) + 1); // When rx_full_threshold is configured (non-zero), the UART has a hardware FIFO with a // meaningful threshold (e.g., ESP32 native UART), so we can calculate a precise delay. // Otherwise (e.g., USB UART), use 50ms to handle data arriving in chunks. static constexpr uint16_t DEFAULT_LONG_RX_BUFFER_DELAY_MS = 50; size_t rx_threshold = this->parent_->get_rx_full_threshold(); - this->long_rx_buffer_delay_ms_ = rx_threshold != uart::UARTComponent::RX_FULL_THRESHOLD_UNSET - ? (rx_threshold * 11 * 1000 / this->parent_->get_baud_rate()) + 1 - : DEFAULT_LONG_RX_BUFFER_DELAY_MS; + this->long_rx_buffer_delay_ms_ = + rx_threshold != uart::UARTComponent::RX_FULL_THRESHOLD_UNSET + ? (rx_threshold * MODBUS_BITS_PER_CHAR * MS_PER_SEC / this->parent_->get_baud_rate()) + 1 + : DEFAULT_LONG_RX_BUFFER_DELAY_MS; } void Modbus::loop() { @@ -296,7 +302,7 @@ void Modbus::send_next_frame_() { this->last_send_tx_offset_ = 0; } else { this->write_array(frame.data.get(), frame.size); - this->last_send_tx_offset_ = frame.size * 11 * 1000 / this->parent_->get_baud_rate() + 1; + this->last_send_tx_offset_ = frame.size * MODBUS_BITS_PER_CHAR * MS_PER_SEC / this->parent_->get_baud_rate() + 1; } #if ESPHOME_LOG_LEVEL >= ESPHOME_LOG_LEVEL_VERBOSE diff --git a/tests/integration/fixtures/external_components/uart_mock/__init__.py b/tests/integration/fixtures/external_components/uart_mock/__init__.py index c10d73354e2..6b9d0e24971 100644 --- a/tests/integration/fixtures/external_components/uart_mock/__init__.py +++ b/tests/integration/fixtures/external_components/uart_mock/__init__.py @@ -87,7 +87,7 @@ CONFIG_SCHEMA = cv.Schema( cv.GenerateID(): cv.declare_id(MockUartComponent), cv.Required(CONF_BAUD_RATE): cv.int_range(min=1), cv.Optional(CONF_RX_BUFFER_SIZE, default=256): cv.validate_bytes, - cv.Optional(CONF_RX_FULL_THRESHOLD, default=10): cv.int_range(min=1, max=120), + cv.Optional(CONF_RX_FULL_THRESHOLD): cv.int_range(min=1, max=120), cv.Optional(CONF_RX_TIMEOUT, default=2): cv.int_range(min=0, max=92), cv.Optional(CONF_STOP_BITS, default=1): cv.one_of(1, 2, int=True), cv.Optional(CONF_DATA_BITS, default=8): cv.int_range(min=5, max=8), @@ -135,7 +135,8 @@ async def to_code(config): cg.add(var.set_baud_rate(config[CONF_BAUD_RATE])) cg.add(var.set_rx_buffer_size(config[CONF_RX_BUFFER_SIZE])) - cg.add(var.set_rx_full_threshold(config[CONF_RX_FULL_THRESHOLD])) + if CONF_RX_FULL_THRESHOLD in config: + cg.add(var.set_rx_full_threshold(config[CONF_RX_FULL_THRESHOLD])) cg.add(var.set_rx_timeout(config[CONF_RX_TIMEOUT])) cg.add(var.set_stop_bits(config[CONF_STOP_BITS])) cg.add(var.set_data_bits(config[CONF_DATA_BITS])) diff --git a/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml new file mode 100644 index 00000000000..407e1c858a1 --- /dev/null +++ b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml @@ -0,0 +1,64 @@ +esphome: + name: uart-mock-modbus-no-threshold-test + +host: +api: +logger: + level: VERBOSE + +external_components: + - source: + type: local + path: EXTERNAL_COMPONENT_PATH + +# Dummy uart entry to satisfy modbus's DEPENDENCIES = ["uart"] +# The actual UART bus used is the uart_mock component below +uart: + baud_rate: 115200 + port: /dev/null + +# Simulate a non-hardware UART (e.g., USB UART) by not setting rx_full_threshold. +# This leaves it at the default sentinel value (0), triggering the 50ms fallback timeout. +uart_mock: + - id: virtual_uart_dev + baud_rate: 9600 + auto_start: false + debug: + on_tx: + - then: + - if: + condition: #Read 80 input registers on device 2, starting at address 0 (SDM meter request) + lambda: "return data == std::vector({0x02,0x04,0x00,0x00,0x00,0x50,0xF0,0x05});" + then: + - uart_mock.inject_rx: # Good response from SDM meter with CRC. Voltage is 243V + !lambda return {0x02,0x04,0xA0,0x43,0x73,0x19,0x9A,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x3F,0x80,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}; + - delay: 20ms # Simulate USB packet gap - would fail with old ~2ms timeout + - uart_mock.inject_rx: # Rest of response (including CRC) + !lambda return{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x42,0x6F,0xCC,0xCD,0x43,0x7C,0xB8,0x10,0x3D,0x38,0x51,0xEC, + 0x43,0x81,0x1B,0xE7,0x3B,0x03,0x12,0x6F,0x50,0x1B}; + +modbus: + uart_id: virtual_uart_dev + turnaround_time: 10ms + +sensor: + - platform: sdm_meter + address: 2 + update_interval: 1s + phase_a: + voltage: + name: sdm_voltage + +button: + - platform: template + name: "Start Scenario" + id: start_scenario_btn + on_press: + - lambda: 'id(virtual_uart_dev).start_scenario();' diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index 6901dc27fe1..83407a6794e 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/tests/integration/test_uart_mock_modbus.py @@ -5,6 +5,10 @@ test_uart_mock_modbus : 1. Read a single register and parse successfully (basic_register) 2. Read multiple registers from SDM meter and parse successfully (sdm_voltage), with some intermediate delay to simulate UART buffer time. +test_uart_mock_modbus_no_threshold : + Test modbus with no rx_full_threshold set (simulating USB UART / non-hardware UART). + Verifies the 50ms fallback timeout handles chunked data with USB packet gaps. + """ from __future__ import annotations @@ -218,3 +222,78 @@ async def test_uart_mock_modbus_timing( f"Timeout waiting for SDM voltage change. Received sensor states:\n" f" sdm_voltage: {sensor_states['sdm_voltage']}\n" ) + + +@pytest.mark.asyncio +async def test_uart_mock_modbus_no_threshold( + yaml_config: str, + run_compiled: RunCompiledFunction, + api_client_connected: APIClientConnectedFactory, +) -> None: + """Test modbus with no rx_full_threshold (simulating USB UART). + + Without the 50ms fallback timeout, the chunked response with a 20ms gap + between USB packets would cause a false timeout and CRC failure cascade. + """ + # Replace external component path placeholder + external_components_path = str( + Path(__file__).parent / "fixtures" / "external_components" + ) + yaml_config = yaml_config.replace( + "EXTERNAL_COMPONENT_PATH", external_components_path + ) + + loop = asyncio.get_running_loop() + + # Track sensor state updates (after initial state is swallowed) + sensor_states: dict[str, list[float]] = { + "sdm_voltage": [], + } + + voltage_changed = loop.create_future() + + def on_state(state: EntityState) -> None: + if isinstance(state, SensorState) and not state.missing_state: + sensor_name = key_to_sensor.get(state.key) + if sensor_name and sensor_name in sensor_states: + sensor_states[sensor_name].append(state.state) + # Check if this is a good voltage reading (243V) + if ( + sensor_name == "sdm_voltage" + and state.state > 200.0 + and not voltage_changed.done() + ): + voltage_changed.set_result(True) + + async with ( + run_compiled(yaml_config), + api_client_connected() as client, + ): + entities, _ = await client.list_entities_services() + + # Build key mappings for all sensor types + all_names = list(sensor_states.keys()) + key_to_sensor = build_key_to_entity_mapping(entities, all_names) + + # Set up initial state helper + initial_state_helper = InitialStateHelper(entities) + client.subscribe_states(initial_state_helper.on_state_wrapper(on_state)) + + try: + await initial_state_helper.wait_for_initial_states() + except TimeoutError: + pytest.fail("Timeout waiting for initial states") + + # Start the UART mock scenario now that we're subscribed + start_btn = find_entity(entities, "start_scenario", ButtonInfo) + assert start_btn is not None, "Start Scenario button not found" + client.button_command(start_btn.key) + + # Wait for voltage to be updated with successful parse + try: + await asyncio.wait_for(voltage_changed, timeout=2.0) + except TimeoutError: + pytest.fail( + f"Timeout waiting for SDM voltage change. Received sensor states:\n" + f" sdm_voltage: {sensor_states['sdm_voltage']}\n" + ) From 7d9a081a8409dbebc566522ec1a7d5fc75158e76 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Mar 2026 17:49:01 -1000 Subject: [PATCH 3/6] Make RX_FULL_THRESHOLD_UNSET public Needed since modbus accesses it from outside the class. --- esphome/components/uart/uart_component.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/esphome/components/uart/uart_component.h b/esphome/components/uart/uart_component.h index 3d2a848462e..c260ad3b7a1 100644 --- a/esphome/components/uart/uart_component.h +++ b/esphome/components/uart/uart_component.h @@ -31,6 +31,8 @@ const LogString *parity_to_str(UARTParityOptions parity); class UARTComponent { public: + static constexpr size_t RX_FULL_THRESHOLD_UNSET = 0; + // Writes an array of bytes to the UART bus. // @param data A vector of bytes to be written. void write_array(const std::vector &data) { this->write_array(&data[0], data.size()); } @@ -187,7 +189,6 @@ class UARTComponent { InternalGPIOPin *rx_pin_{}; InternalGPIOPin *flow_control_pin_{}; size_t rx_buffer_size_{}; - static constexpr size_t RX_FULL_THRESHOLD_UNSET = 0; // ESP-IDF always sets this at codegen time via set_rx_full_threshold(). // Other platforms (USB UART, Arduino, etc.) leave it unset. size_t rx_full_threshold_{RX_FULL_THRESHOLD_UNSET}; From ab08aa7553884973f7f0536326df696c5819c382 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Mar 2026 17:51:56 -1000 Subject: [PATCH 4/6] fix hostname len --- tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml index 407e1c858a1..1beb01e9faa 100644 --- a/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml +++ b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml @@ -1,5 +1,5 @@ esphome: - name: uart-mock-modbus-no-threshold-test + name: uart-mock-modbus-no-thresh host: api: From 5319b8593b352f314b1baa8d9cc32017e17e1105 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Mar 2026 18:07:18 -1000 Subject: [PATCH 5/6] Add latency simulation to uart_mock for USB UART test Add inject_to_rx_buffer_delayed() to uart_mock which stages bytes that aren't visible to available() until the delay elapses. This simulates USB packet delivery latency. The test uses a 40ms delay which is: - Greater than the old ~2ms timeout (fails without fix) - Less than the new 50ms fallback timeout (passes with fix) --- .../external_components/uart_mock/__init__.py | 3 +++ .../uart_mock/automation.h | 16 ++++++++++++++-- .../uart_mock/uart_mock.cpp | 19 +++++++++++++++++++ .../external_components/uart_mock/uart_mock.h | 10 ++++++++++ .../uart_mock_modbus_no_threshold.yaml | 12 ++++++------ 5 files changed, 52 insertions(+), 8 deletions(-) diff --git a/tests/integration/fixtures/external_components/uart_mock/__init__.py b/tests/integration/fixtures/external_components/uart_mock/__init__.py index 6b9d0e24971..fdd481b3977 100644 --- a/tests/integration/fixtures/external_components/uart_mock/__init__.py +++ b/tests/integration/fixtures/external_components/uart_mock/__init__.py @@ -62,6 +62,7 @@ CONFIG_INJECT_RX_SCHEMA = cv.maybe_simple_value( { cv.GenerateID(): cv.use_id(MockUartComponent), cv.Required("data"): cv.templatable(validate_raw_data), + cv.Optional(CONF_DELAY): cv.positive_time_period_milliseconds, }, key=CONF_DATA, ) @@ -126,6 +127,8 @@ async def inject_rx_to_code(config, action_id, template_arg, args): arr_id = ID(f"{action_id}_data", is_declaration=True, type=cg.uint8) arr = cg.static_const_array(arr_id, cg.ArrayInitializer(*data)) cg.add(var.set_data_static(arr, len(data))) + if CONF_DELAY in config: + cg.add(var.set_delay(config[CONF_DELAY])) return var diff --git a/tests/integration/fixtures/external_components/uart_mock/automation.h b/tests/integration/fixtures/external_components/uart_mock/automation.h index 83a057d3a05..58a8e858de3 100644 --- a/tests/integration/fixtures/external_components/uart_mock/automation.h +++ b/tests/integration/fixtures/external_components/uart_mock/automation.h @@ -22,18 +22,30 @@ template class MockUartInjectRXAction : public Action, pu this->len_ = len; // Length >= 0 indicates static mode } + void set_delay(uint32_t delay_ms) { this->delay_ms_ = delay_ms; } + void play(const Ts &...x) override { if (this->len_ >= 0) { // Static mode: use pointer and length - this->parent_->inject_to_rx_buffer(this->code_.data, static_cast(this->len_)); + std::vector data(this->code_.data, this->code_.data + this->len_); + if (this->delay_ms_ > 0) { + this->parent_->inject_to_rx_buffer_delayed(data, this->delay_ms_); + } else { + this->parent_->inject_to_rx_buffer(this->code_.data, static_cast(this->len_)); + } } else { // Template mode: call function auto val = this->code_.func(x...); - this->parent_->inject_to_rx_buffer(val); + if (this->delay_ms_ > 0) { + this->parent_->inject_to_rx_buffer_delayed(val, this->delay_ms_); + } else { + this->parent_->inject_to_rx_buffer(val); + } } } protected: + uint32_t delay_ms_{0}; ssize_t len_{-1}; // -1 = template mode, >=0 = static mode with length union Code { std::vector (*func)(Ts...); // Function pointer (stateless lambdas) diff --git a/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp b/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp index e8c07d632de..395fe99b593 100644 --- a/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp +++ b/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp @@ -53,6 +53,14 @@ void MockUartComponent::loop() { } } + // Process staged RX - deliver bytes whose delay has elapsed + while (!this->staged_rx_.empty() && millis() >= this->staged_rx_.front().available_at_ms) { + auto &staged = this->staged_rx_.front(); + ESP_LOGD(TAG, "Delivering %zu staged RX bytes", staged.data.size()); + this->inject_to_rx_buffer(staged.data); + this->staged_rx_.pop_front(); + } + // 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) { @@ -208,4 +216,15 @@ void MockUartComponent::inject_to_rx_buffer(const std::vector &data) { } } +void MockUartComponent::inject_to_rx_buffer_delayed(const std::vector &data, uint32_t delay_ms) { + if (!data.empty() && data.size() <= 64) { + char hex_buf[format_hex_pretty_size(64)]; + ESP_LOGD(TAG, "Staging %zu RX bytes with %ums delay: %s", data.size(), delay_ms, + format_hex_pretty_to(hex_buf, sizeof(hex_buf), data.data(), data.size())); + } else if (data.size() > 64) { + ESP_LOGD(TAG, "Staging %zu RX bytes with %ums delay (too large to log inline)", data.size(), delay_ms); + } + this->staged_rx_.push_back({data, millis() + delay_ms}); +} + } // namespace esphome::uart_mock diff --git a/tests/integration/fixtures/external_components/uart_mock/uart_mock.h b/tests/integration/fixtures/external_components/uart_mock/uart_mock.h index 901e371dec4..32517bffba0 100644 --- a/tests/integration/fixtures/external_components/uart_mock/uart_mock.h +++ b/tests/integration/fixtures/external_components/uart_mock/uart_mock.h @@ -43,6 +43,8 @@ class MockUartComponent : public uart::UARTComponent, public Component { void set_tx_hook(std::function &)> &&cb) { this->tx_hook_ = std::move(cb); } void inject_to_rx_buffer(const std::vector &data); void inject_to_rx_buffer(const uint8_t *data, size_t len); + // Stage bytes for delayed delivery - simulates transport-level latency (e.g., USB packets) + void inject_to_rx_buffer_delayed(const std::vector &data, uint32_t delay_ms); protected: void check_logger_conflict() override {} @@ -82,6 +84,14 @@ class MockUartComponent : public uart::UARTComponent, public Component { }; std::vector periodic_rx_; + // Staged RX - bytes that are pending delivery after a delay + // Simulates transport-level latency (e.g., USB packet delivery) + struct StagedRx { + std::vector data; + uint32_t available_at_ms; // millis() time when bytes become available + }; + std::deque staged_rx_; + // Observability uint32_t tx_count_{0}; uint32_t rx_count_{0}; diff --git a/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml index 1beb01e9faa..f4cfd613ea4 100644 --- a/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml +++ b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml @@ -30,7 +30,7 @@ uart_mock: condition: #Read 80 input registers on device 2, starting at address 0 (SDM meter request) lambda: "return data == std::vector({0x02,0x04,0x00,0x00,0x00,0x50,0xF0,0x05});" then: - - uart_mock.inject_rx: # Good response from SDM meter with CRC. Voltage is 243V + - uart_mock.inject_rx: # First USB packet: SDM meter response part 1 !lambda return {0x02,0x04,0xA0,0x43,0x73,0x19,0x9A,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, @@ -38,11 +38,11 @@ uart_mock: 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}; - - delay: 20ms # Simulate USB packet gap - would fail with old ~2ms timeout - - uart_mock.inject_rx: # Rest of response (including CRC) - !lambda return{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, - 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x42,0x6F,0xCC,0xCD,0x43,0x7C,0xB8,0x10,0x3D,0x38,0x51,0xEC, - 0x43,0x81,0x1B,0xE7,0x3B,0x03,0x12,0x6F,0x50,0x1B}; + - uart_mock.inject_rx: # Second USB packet: rest of response (staged with 20ms latency) + delay: 40ms + data: !lambda return{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x42,0x6F,0xCC,0xCD,0x43,0x7C,0xB8,0x10,0x3D,0x38,0x51,0xEC, + 0x43,0x81,0x1B,0xE7,0x3B,0x03,0x12,0x6F,0x50,0x1B}; modbus: uart_id: virtual_uart_dev From 3b4897381fd5dd13a11a87febd81b6b958d6368e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Mar 2026 18:25:33 -1000 Subject: [PATCH 6/6] Address review feedback - Avoid unnecessary vector construction in static mode path - Use wraparound-safe millis comparison for staged RX - Fix stale comments (20ms -> 40ms, ESP-IDF -> ESP32) - Clarify MODBUS_BITS_PER_CHAR is approximate --- esphome/components/modbus/modbus.cpp | 2 +- esphome/components/uart/uart_component.h | 2 +- .../fixtures/external_components/uart_mock/automation.h | 2 +- .../fixtures/external_components/uart_mock/uart_mock.cpp | 3 ++- tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml | 2 +- tests/integration/test_uart_mock_modbus.py | 2 +- 6 files changed, 7 insertions(+), 6 deletions(-) diff --git a/esphome/components/modbus/modbus.cpp b/esphome/components/modbus/modbus.cpp index 4f683995fd9..82672217c56 100644 --- a/esphome/components/modbus/modbus.cpp +++ b/esphome/components/modbus/modbus.cpp @@ -11,7 +11,7 @@ static const char *const TAG = "modbus"; // Maximum bytes to log for Modbus frames (truncated if larger) static constexpr size_t MODBUS_MAX_LOG_BYTES = 64; -// Bits per character on the wire: 1 start + 8 data + 1 parity + 1 stop +// Approximate bits per character on the wire (depends on parity/stop bit config) static constexpr uint32_t MODBUS_BITS_PER_CHAR = 11; // Milliseconds per second static constexpr uint32_t MS_PER_SEC = 1000; diff --git a/esphome/components/uart/uart_component.h b/esphome/components/uart/uart_component.h index c260ad3b7a1..d6019364985 100644 --- a/esphome/components/uart/uart_component.h +++ b/esphome/components/uart/uart_component.h @@ -189,7 +189,7 @@ class UARTComponent { InternalGPIOPin *rx_pin_{}; InternalGPIOPin *flow_control_pin_{}; size_t rx_buffer_size_{}; - // ESP-IDF always sets this at codegen time via set_rx_full_threshold(). + // ESP32 (both Arduino and ESP-IDF) always sets this at codegen time via set_rx_full_threshold(). // Other platforms (USB UART, Arduino, etc.) leave it unset. size_t rx_full_threshold_{RX_FULL_THRESHOLD_UNSET}; size_t rx_timeout_{0}; diff --git a/tests/integration/fixtures/external_components/uart_mock/automation.h b/tests/integration/fixtures/external_components/uart_mock/automation.h index 58a8e858de3..b2336ad0656 100644 --- a/tests/integration/fixtures/external_components/uart_mock/automation.h +++ b/tests/integration/fixtures/external_components/uart_mock/automation.h @@ -27,8 +27,8 @@ template class MockUartInjectRXAction : public Action, pu void play(const Ts &...x) override { if (this->len_ >= 0) { // Static mode: use pointer and length - std::vector data(this->code_.data, this->code_.data + this->len_); if (this->delay_ms_ > 0) { + std::vector data(this->code_.data, this->code_.data + this->len_); this->parent_->inject_to_rx_buffer_delayed(data, this->delay_ms_); } else { this->parent_->inject_to_rx_buffer(this->code_.data, static_cast(this->len_)); diff --git a/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp b/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp index 395fe99b593..64b844bdf20 100644 --- a/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp +++ b/tests/integration/fixtures/external_components/uart_mock/uart_mock.cpp @@ -54,7 +54,8 @@ void MockUartComponent::loop() { } // Process staged RX - deliver bytes whose delay has elapsed - while (!this->staged_rx_.empty() && millis() >= this->staged_rx_.front().available_at_ms) { + uint32_t now_ms = millis(); + while (!this->staged_rx_.empty() && (static_cast(now_ms - this->staged_rx_.front().available_at_ms) >= 0)) { auto &staged = this->staged_rx_.front(); ESP_LOGD(TAG, "Delivering %zu staged RX bytes", staged.data.size()); this->inject_to_rx_buffer(staged.data); diff --git a/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml index f4cfd613ea4..e3e8c8c8da7 100644 --- a/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml +++ b/tests/integration/fixtures/uart_mock_modbus_no_threshold.yaml @@ -38,7 +38,7 @@ uart_mock: 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}; - - uart_mock.inject_rx: # Second USB packet: rest of response (staged with 20ms latency) + - uart_mock.inject_rx: # Second USB packet: rest of response (staged with 40ms latency) delay: 40ms data: !lambda return{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x42,0x6F,0xCC,0xCD,0x43,0x7C,0xB8,0x10,0x3D,0x38,0x51,0xEC, diff --git a/tests/integration/test_uart_mock_modbus.py b/tests/integration/test_uart_mock_modbus.py index 83407a6794e..e341d86f53f 100644 --- a/tests/integration/test_uart_mock_modbus.py +++ b/tests/integration/test_uart_mock_modbus.py @@ -232,7 +232,7 @@ async def test_uart_mock_modbus_no_threshold( ) -> None: """Test modbus with no rx_full_threshold (simulating USB UART). - Without the 50ms fallback timeout, the chunked response with a 20ms gap + Without the 50ms fallback timeout, the chunked response with a 40ms gap between USB packets would cause a false timeout and CRC failure cascade. """ # Replace external component path placeholder