From c174d361cd1b8a73b8367f31a8dda4a8f54a1a05 Mon Sep 17 00:00:00 2001 From: clydebarrow <2366188+clydebarrow@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:53:49 +1100 Subject: [PATCH] [packet_interface] Fixes from code review - Reset the link after max_retries even when it is 255, where the retry counter used to wrap - Require a reliability timeout of at least 50 ms - Drop received frames larger than the layer's max_packet_size - Fail setup, instead of aborting, when the send queue cannot be allocated or max_packet_size leaves no room - Test the ack delay timer and a refused REJ Co-Authored-By: Claude Opus 5.5 --- .../packet_interface/hdlc_reliability.cpp | 25 +++++- .../packet_interface/hdlc_reliability.h | 8 +- .../layers/hdlc_reliability.py | 8 +- .../packet_interface/hdlc_reliability.cpp | 76 +++++++++++++++++++ tests/unit_tests/test_packet_interface.py | 7 ++ 5 files changed, 114 insertions(+), 10 deletions(-) diff --git a/esphome/components/packet_interface/hdlc_reliability.cpp b/esphome/components/packet_interface/hdlc_reliability.cpp index cad0ba25b09..9dda0834e96 100644 --- a/esphome/components/packet_interface/hdlc_reliability.cpp +++ b/esphome/components/packet_interface/hdlc_reliability.cpp @@ -30,11 +30,21 @@ static uint16_t frame_crc(uint8_t control, const uint8_t *payload, size_t length HdlcReliability::HdlcReliability(PacketInterface *lower) : lower_(lower) { this->stack_on_(lower, CAPABILITY_RELIABILITY); - this->max_packet_size_ = lower->get_max_packet_size() - FRAME_OVERHEAD; + size_t lower_size = lower->get_max_packet_size(); + this->max_packet_size_ = lower_size > FRAME_OVERHEAD ? lower_size - FRAME_OVERHEAD : 0; } void HdlcReliability::setup() { - this->tx_slots_ = std::make_unique(this->max_queue_length_ * this->max_packet_size_); + if (this->max_packet_size_ == 0) { + this->mark_failed(LOG_STR("max_packet_size too small for the configured layers")); + return; + } + RAMAllocator allocator; + this->tx_slots_ = allocator.allocate(size_t{this->max_queue_length_} * this->max_packet_size_); + if (this->tx_slots_ == nullptr) { + this->mark_failed(LOG_STR("Not enough memory for the send queue")); + return; + } this->lower_->add_packet_interface_listener( [this](const PacketBuffer &data, const PacketMetaData &meta_data) { this->receive_(data, meta_data); }); } @@ -93,6 +103,8 @@ void HdlcReliability::ack_sent_() { void HdlcReliability::request_loop_() { this->enable_loop(); } bool HdlcReliability::send_to_interface(const PacketBuffer &data, const PacketMetaData &meta_data) { + if (this->tx_slots_ == nullptr) + return false; size_t size = data.size(); if (size > this->max_packet_size_) { ESP_LOGW(TAG, "%s: Packet of %zu bytes is larger than %zu", this->name_, size, this->max_packet_size_); @@ -176,11 +188,12 @@ void HdlcReliability::on_timeout_() { } if (this->outstanding_() == 0) return; - if (++this->retries_ > this->max_retries_) { + if (this->retries_ == this->max_retries_) { ESP_LOGW(TAG, "%s: No acknowledgement after %u retries, resetting link", this->name_, this->max_retries_); this->start_reset_(); return; } + this->retries_++; ESP_LOGV(TAG, "%s: Timeout, resending %u frames", this->name_, this->outstanding_()); this->resend_outstanding_(); this->start_timer_(); @@ -255,6 +268,12 @@ void HdlcReliability::receive_(const PacketBuffer &data, const PacketMetaData &m this->note_received_(false); return; } + // The peer's packets may only be larger if it has a larger max_packet_size, which the layers above cannot take + if (length > this->max_packet_size_) { + ESP_LOGV(TAG, "%s: Frame of %zu bytes is larger than %zu, dropped", this->name_, length, this->max_packet_size_); + this->note_received_(false); + return; + } this->note_received_(true); uint8_t command = control & ~POLL_FINAL_BIT; diff --git a/esphome/components/packet_interface/hdlc_reliability.h b/esphome/components/packet_interface/hdlc_reliability.h index 07f741f403e..ccfe16f5d63 100644 --- a/esphome/components/packet_interface/hdlc_reliability.h +++ b/esphome/components/packet_interface/hdlc_reliability.h @@ -3,7 +3,6 @@ #include "esphome/core/defines.h" #ifdef USE_PACKET_INTERFACE_HDLC #include -#include #include "packet_interface.h" @@ -67,11 +66,12 @@ class HdlcReliability : public PacketInterface { void request_loop_(); uint8_t outstanding_() const { return (this->vs_ - this->va_) & 7; } uint8_t slot_index_(uint8_t seq) const { return (this->head_ + ((seq - this->va_) & 7)) % this->max_queue_length_; } - uint8_t *slot_(uint8_t seq) { return this->tx_slots_.get() + this->slot_index_(seq) * this->max_packet_size_; } + uint8_t *slot_(uint8_t seq) { return this->tx_slots_ + this->slot_index_(seq) * this->max_packet_size_; } PacketInterface *lower_; - // max_queue_length_ slots of max_packet_size_ bytes, a ring starting at head_ which holds frame va_ - std::unique_ptr tx_slots_; + // max_queue_length_ slots of max_packet_size_ bytes, a ring starting at head_ which holds frame va_; allocated in + // setup() and never freed + uint8_t *tx_slots_{nullptr}; std::array tx_lengths_{}; // indexed like the slots uint32_t timeout_ms_{200}; uint32_t ack_delay_ms_{0}; diff --git a/esphome/components/packet_interface/layers/hdlc_reliability.py b/esphome/components/packet_interface/layers/hdlc_reliability.py index 22dd942e3bf..fef0d272265 100644 --- a/esphome/components/packet_interface/layers/hdlc_reliability.py +++ b/esphome/components/packet_interface/layers/hdlc_reliability.py @@ -31,9 +31,11 @@ HDLC_SCHEMA = cv.All( cv.Schema( { cv.GenerateID(): cv.declare_id(HdlcReliability), - cv.Optional( - CONF_TIMEOUT, default="200ms" - ): cv.positive_time_period_milliseconds, + # Retransmitting more often than the main loop runs would exhaust the retries at once + cv.Optional(CONF_TIMEOUT, default="200ms"): cv.All( + cv.positive_time_period_milliseconds, + cv.Range(min=cv.TimePeriod(milliseconds=50)), + ), cv.Optional( CONF_ACK_DELAY, default="0ms" ): cv.positive_time_period_milliseconds, diff --git a/tests/components/packet_interface/hdlc_reliability.cpp b/tests/components/packet_interface/hdlc_reliability.cpp index 0d7347c46c9..a22ec423e76 100644 --- a/tests/components/packet_interface/hdlc_reliability.cpp +++ b/tests/components/packet_interface/hdlc_reliability.cpp @@ -5,6 +5,7 @@ #include #include "esphome/components/packet_interface/hdlc_reliability.h" #include "esphome/core/application.h" +#include "esphome/core/hal.h" #include "esphome/core/helpers.h" namespace esphome::packet_interface::testing { @@ -305,4 +306,79 @@ TEST_F(HdlcReliabilityTest, AckDelayLetsReplyCarryAck) { EXPECT_TRUE(b.link.sent.empty()); // the RR is no longer owed } +TEST_F(HdlcReliabilityTest, MaxRetriesOf255ResetsLink) { + Peer a, b; + a.hdlc->set_max_retries(255); + pump(a, b); + a.send({0x01}); + a.link.sent.clear(); + for (int i = 0; i != 255; i++) { + a.hdlc->on_timeout_(); + a.link.sent.clear(); + } + EXPECT_TRUE(a.states.empty()); + a.hdlc->on_timeout_(); + EXPECT_EQ(a.states, std::vector{false}); +} + +TEST_F(HdlcReliabilityTest, OversizedFrameDropped) { + Peer a, b; + pump(a, b); + // An I-frame, N(S) 0 and N(R) 0, one byte larger than this layer's packets + b.link.inject(make_frame(0x00, Frame(b.hdlc->get_max_packet_size() + 1, 0x55))); + EXPECT_TRUE(b.received.empty()); + b.link.inject(make_frame(0x00, Frame(b.hdlc->get_max_packet_size(), 0x55))); + EXPECT_EQ(b.received.size(), 1); +} + +TEST_F(HdlcReliabilityTest, LinkTooSmallFailsSetup) { + MockLink link; + link.set_max_packet_size(HdlcReliability::FRAME_OVERHEAD); + TestHdlc hdlc(&link); + EXPECT_EQ(hdlc.get_max_packet_size(), 0); + hdlc.setup(); + EXPECT_TRUE(hdlc.is_failed()); + Frame packet{1}; + EXPECT_FALSE(hdlc.send_to_interface(PacketBuffer(packet), {})); +} + +TEST_F(HdlcReliabilityTest, AckDelayTimerSendsAck) { + Peer a, b; + b.hdlc->set_ack_delay(5); + pump(a, b); + a.send({1}); + for (auto &frame : a.link.sent) + b.link.inject(frame); + a.link.sent.clear(); + b.hdlc->loop(); + EXPECT_TRUE(b.link.sent.empty()); + delay(10); + App.scheduler.call(millis()); + b.hdlc->loop(); + ASSERT_EQ(b.link.sent.size(), 1); + EXPECT_TRUE(is_ack(b.link.sent[0])); + EXPECT_EQ(b.link.sent[0][0] >> 5, 1); // N(R) acknowledges the frame from a +} + +TEST_F(HdlcReliabilityTest, RefusedRejectFallsBackToAck) { + Peer a, b; + pump(a, b); + a.send({1}); + a.send({2}); + ASSERT_EQ(a.link.sent.size(), 2); + // The first frame is lost, and the REJ for the gap is refused + b.link.budget = 0; + b.link.inject(a.link.sent[1]); + a.link.sent.clear(); + EXPECT_EQ(b.link.refused, 1); + b.link.budget = SIZE_MAX; + b.hdlc->loop(); + ASSERT_EQ(b.link.sent.size(), 1); + EXPECT_TRUE(is_ack(b.link.sent[0])); + EXPECT_EQ(b.link.sent[0][0] >> 5, 0); + a.hdlc->on_timeout_(); + pump(a, b); + EXPECT_EQ(b.received, (std::vector{{1}, {2}})); +} + } // namespace esphome::packet_interface::testing diff --git a/tests/unit_tests/test_packet_interface.py b/tests/unit_tests/test_packet_interface.py index e3343380d96..4e184828424 100644 --- a/tests/unit_tests/test_packet_interface.py +++ b/tests/unit_tests/test_packet_interface.py @@ -64,6 +64,13 @@ def test_reliability_queue_length_limit(setup_core) -> None: ) +def test_reliability_timeout_minimum(setup_core) -> None: + """A timeout shorter than a few loop passes cannot work.""" + uart_packet_interface.CONFIG_SCHEMA({CONF_RELIABILITY: {"timeout": "50ms"}}) + with pytest.raises(cv.Invalid): + uart_packet_interface.CONFIG_SCHEMA({CONF_RELIABILITY: {"timeout": "40ms"}}) + + def test_ack_delay_shorter_than_timeout(setup_core) -> None: """The acknowledgement must not wait longer than the peer's timeout.""" reliability = uart_packet_interface.CONFIG_SCHEMA(