From 3b4897381fd5dd13a11a87febd81b6b958d6368e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 7 Mar 2026 18:25:33 -1000 Subject: [PATCH] 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 4f683995fd..82672217c5 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 c260ad3b7a..d601936498 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 58a8e858de..b2336ad065 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 395fe99b59..64b844bdf2 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 f4cfd613ea..e3e8c8c8da 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 83407a6794..e341d86f53 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