[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 <noreply@anthropic.com>

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<uint8_t[]>(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<uint8_t> 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;

esphome/components/packet_interface/hdlc_reliability.h

@@ -3,7 +3,6 @@
 #include "esphome/core/defines.h"
 #ifdef USE_PACKET_INTERFACE_HDLC
 #include <array>
-#include <memory>
 
 #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<uint8_t[]> 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<uint16_t, MAX_WINDOW> tx_lengths_{};  // indexed like the slots
   uint32_t timeout_ms_{200};
   uint32_t ack_delay_ms_{0};

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,

tests/components/packet_interface/hdlc_reliability.cpp

@@ -5,6 +5,7 @@
 #include <vector>
 #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<bool>{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<Frame>{{1}, {2}}));
+}
+
 }  // namespace esphome::packet_interface::testing

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(