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
This commit is contained in:
J. Nick Koston
2026-03-07 18:25:33 -10:00
parent 5319b8593b
commit 3b4897381f
6 changed files with 7 additions and 6 deletions
+1 -1
View File
@@ -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;
+1 -1
View File
@@ -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};
@@ -27,8 +27,8 @@ template<typename... Ts> class MockUartInjectRXAction : public Action<Ts...>, pu
void play(const Ts &...x) override {
if (this->len_ >= 0) {
// Static mode: use pointer and length
std::vector<uint8_t> data(this->code_.data, this->code_.data + this->len_);
if (this->delay_ms_ > 0) {
std::vector<uint8_t> 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<size_t>(this->len_));
@@ -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<int32_t>(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);
@@ -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,
+1 -1
View File
@@ -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