From dbe3dfb2657709918f332a79e65eabad28b3e28f Mon Sep 17 00:00:00 2001 From: Jonathan Swoboda <154711427+swoboda1337@users.noreply.github.com> Date: Thu, 19 Feb 2026 22:38:33 -0500 Subject: [PATCH] [ld2410/ld2450] Replace header sync with buffer size increase for frame resync Reverts the frame header synchronization added in #14135 and #14136 in favor of a simpler fix: increasing MAX_LINE_LENGTH so that the existing footer-based resynchronization can recover after losing sync. Both components already check for frame footers at every byte position, which naturally resyncs the parser. The problem was that the buffers were sized exactly to fit the largest frame, so a desynced parser's footer could land at the overflow boundary and get discarded. Increasing the buffer by 4 bytes (footer size) ensures the footer always lands inside the buffer. - ld2450: 41 -> 45 (zone query response = 40 bytes + 1 null + 4 footer) - ld2410: 46 -> 50 (engineering data frame = 45 bytes + 1 null + 4 footer) Co-Authored-By: Claude Opus 4.6 --- esphome/components/ld2410/ld2410.cpp | 19 +-- esphome/components/ld2410/ld2410.h | 6 +- esphome/components/ld2450/ld2450.cpp | 19 +-- esphome/components/ld2450/ld2450.h | 8 +- tests/components/ld2450/ld2450_readline.cpp | 166 +++++++++----------- 5 files changed, 81 insertions(+), 137 deletions(-) diff --git a/esphome/components/ld2410/ld2410.cpp b/esphome/components/ld2410/ld2410.cpp index a3c2193d67..f8f782f804 100644 --- a/esphome/components/ld2410/ld2410.cpp +++ b/esphome/components/ld2410/ld2410.cpp @@ -601,28 +601,11 @@ void LD2410Component::readline_(int readch) { return; // No data available } - // Frame header synchronization: verify first 4 bytes match a known frame header. - // This prevents the parser from getting stuck in an overflow loop after losing sync - // (e.g. after module restart or UART noise). - if (this->buffer_pos_ < HEADER_FOOTER_SIZE) { - const uint8_t byte = static_cast(readch); - // Verify header bytes match the frame type established by byte 0 - if (this->buffer_pos_ > 0) { - const uint8_t *expected = (this->buffer_data_[0] == DATA_FRAME_HEADER[0]) ? DATA_FRAME_HEADER : CMD_FRAME_HEADER; - if (byte != expected[this->buffer_pos_]) { - this->buffer_pos_ = 0; // Reset and fall through to check if this byte starts a new frame - } - } - // First byte must match start of a data or command frame header - if (this->buffer_pos_ == 0 && byte != DATA_FRAME_HEADER[0] && byte != CMD_FRAME_HEADER[0]) { - return; - } - } - if (this->buffer_pos_ < MAX_LINE_LENGTH - 1) { this->buffer_data_[this->buffer_pos_++] = readch; this->buffer_data_[this->buffer_pos_] = 0; } else { + // We should never get here, but just in case... ESP_LOGW(TAG, "Max command length exceeded; ignoring"); this->buffer_pos_ = 0; return; diff --git a/esphome/components/ld2410/ld2410.h b/esphome/components/ld2410/ld2410.h index efe585fb76..687ed21d1d 100644 --- a/esphome/components/ld2410/ld2410.h +++ b/esphome/components/ld2410/ld2410.h @@ -33,8 +33,10 @@ namespace esphome::ld2410 { using namespace ld24xx; -static constexpr uint8_t MAX_LINE_LENGTH = 46; // Max characters for serial buffer -static constexpr uint8_t TOTAL_GATES = 9; // Total number of gates supported by the LD2410 +// Engineering data frame is 45 bytes; +1 for null terminator, +4 so that a frame footer always +// lands inside the buffer during footer-based resynchronization after losing sync. +static constexpr uint8_t MAX_LINE_LENGTH = 50; +static constexpr uint8_t TOTAL_GATES = 9; // Total number of gates supported by the LD2410 class LD2410Component : public Component, public uart::UARTDevice { #ifdef USE_BINARY_SENSOR diff --git a/esphome/components/ld2450/ld2450.cpp b/esphome/components/ld2450/ld2450.cpp index 2af45235a3..d30c164769 100644 --- a/esphome/components/ld2450/ld2450.cpp +++ b/esphome/components/ld2450/ld2450.cpp @@ -769,28 +769,11 @@ void LD2450Component::readline_(int readch) { return; // No data available } - // Frame header synchronization: verify first 4 bytes match a known frame header. - // This prevents the parser from accumulating mid-frame data after losing sync - // (e.g. after module restart or UART noise). - if (this->buffer_pos_ < HEADER_FOOTER_SIZE) { - const uint8_t byte = static_cast(readch); - // Verify header bytes match the frame type established by byte 0 - if (this->buffer_pos_ > 0) { - const uint8_t *expected = (this->buffer_data_[0] == DATA_FRAME_HEADER[0]) ? DATA_FRAME_HEADER : CMD_FRAME_HEADER; - if (byte != expected[this->buffer_pos_]) { - this->buffer_pos_ = 0; // Reset and fall through to check if this byte starts a new frame - } - } - // First byte must match start of a data or command frame header - if (this->buffer_pos_ == 0 && byte != DATA_FRAME_HEADER[0] && byte != CMD_FRAME_HEADER[0]) { - return; - } - } - if (this->buffer_pos_ < MAX_LINE_LENGTH - 1) { this->buffer_data_[this->buffer_pos_++] = readch; this->buffer_data_[this->buffer_pos_] = 0; } else { + // We should never get here, but just in case... ESP_LOGW(TAG, "Max command length exceeded; ignoring"); this->buffer_pos_ = 0; return; diff --git a/esphome/components/ld2450/ld2450.h b/esphome/components/ld2450/ld2450.h index 44e5912b2a..30f96c0a9c 100644 --- a/esphome/components/ld2450/ld2450.h +++ b/esphome/components/ld2450/ld2450.h @@ -38,9 +38,11 @@ using namespace ld24xx; // Constants static constexpr uint8_t DEFAULT_PRESENCE_TIMEOUT = 5; // Timeout to reset presense status 5 sec. -static constexpr uint8_t MAX_LINE_LENGTH = 41; // Max characters for serial buffer -static constexpr uint8_t MAX_TARGETS = 3; // Max 3 Targets in LD2450 -static constexpr uint8_t MAX_ZONES = 3; // Max 3 Zones in LD2450 +// Zone query response is 40 bytes; +1 for null terminator, +4 so that a frame footer always +// lands inside the buffer during footer-based resynchronization after losing sync. +static constexpr uint8_t MAX_LINE_LENGTH = 45; +static constexpr uint8_t MAX_TARGETS = 3; // Max 3 Targets in LD2450 +static constexpr uint8_t MAX_ZONES = 3; // Max 3 Zones in LD2450 enum Direction : uint8_t { DIRECTION_APPROACHING = 0, diff --git a/tests/components/ld2450/ld2450_readline.cpp b/tests/components/ld2450/ld2450_readline.cpp index 68b1dd6881..66dec7a36b 100644 --- a/tests/components/ld2450/ld2450_readline.cpp +++ b/tests/components/ld2450/ld2450_readline.cpp @@ -48,19 +48,39 @@ TEST_F(LD2450ReadlineTest, BackToBackMixedFrames) { EXPECT_EQ(this->ld2450_.buffer_pos_, 0); } -// --- Garbage rejection tests --- - -TEST_F(LD2450ReadlineTest, GarbageDiscarded) { - // Feed bytes that don't match any header start byte - std::vector garbage = {0x01, 0x02, 0x03, 0x42, 0x99, 0x00, 0xFF, 0x7F}; - this->ld2450_.feed(garbage); - // Header sync should discard all of these - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); -} +// --- Garbage then valid frame tests --- TEST_F(LD2450ReadlineTest, GarbageThenValidFrame) { + // Garbage bytes accumulate in the buffer but don't match any footer. + // A valid frame follows; its footer resets the buffer and resyncs. std::vector garbage = {0x01, 0x02, 0x03, 0x42, 0x99}; this->ld2450_.feed(garbage); + EXPECT_GT(this->ld2450_.buffer_pos_, 0); // Garbage accumulated + + auto frame = make_periodic_frame(); + this->ld2450_.feed(frame); + // Footer from the valid frame resyncs the parser + EXPECT_EQ(this->ld2450_.buffer_pos_, 0); +} + +// --- Footer-based resynchronization tests --- + +TEST_F(LD2450ReadlineTest, FooterInGarbageResyncs) { + // Garbage containing a periodic frame footer (0x55 0xCC) triggers + // a buffer reset, allowing the next frame to be parsed cleanly. + std::vector garbage_with_footer = {0x01, 0x02, 0x03, 0x04, 0x55, 0xCC}; + this->ld2450_.feed(garbage_with_footer); + EXPECT_EQ(this->ld2450_.buffer_pos_, 0); // Footer reset the buffer + + auto frame = make_periodic_frame(); + this->ld2450_.feed(frame); + EXPECT_EQ(this->ld2450_.buffer_pos_, 0); +} + +TEST_F(LD2450ReadlineTest, CmdFooterInGarbageResyncs) { + // Garbage containing a command frame footer (04 03 02 01) also resyncs. + std::vector garbage_with_footer = {0x10, 0x20, 0x30, 0x40, 0x04, 0x03, 0x02, 0x01}; + this->ld2450_.feed(garbage_with_footer); EXPECT_EQ(this->ld2450_.buffer_pos_, 0); auto frame = make_periodic_frame(); @@ -68,112 +88,66 @@ TEST_F(LD2450ReadlineTest, GarbageThenValidFrame) { EXPECT_EQ(this->ld2450_.buffer_pos_, 0); } -// --- Header synchronization tests --- +// --- Overflow recovery tests --- -TEST_F(LD2450ReadlineTest, PartialDataHeaderThenMismatch) { - // Start of a data frame header, then invalid byte - this->ld2450_.feed({0xAA, 0xFF, 0x42}); // 0x42 doesn't match DATA_FRAME_HEADER[2] (0x03) - // Parser should have reset - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); -} - -TEST_F(LD2450ReadlineTest, PartialCmdHeaderThenMismatch) { - // Start of a command frame header, then invalid byte - this->ld2450_.feed({0xFD, 0xFC, 0xFB, 0x42}); // 0x42 doesn't match CMD_FRAME_HEADER[3] (0xFA) - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); -} - -TEST_F(LD2450ReadlineTest, PartialHeaderThenValidFrame) { - // Partial header that fails, then a complete valid frame - this->ld2450_.feed({0xAA, 0xFF, 0x42}); // Fails at byte 3 - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); - - auto frame = make_periodic_frame(); - this->ld2450_.feed(frame); - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); -} - -TEST_F(LD2450ReadlineTest, HeaderMismatchRecoveryOnNewHeaderByte) { - // Start data header, mismatch at byte 2, but mismatch byte is start of command header - this->ld2450_.feed({0xAA, 0xFF}); - EXPECT_EQ(this->ld2450_.buffer_pos_, 2); // Accumulating header - - this->ld2450_.feed({0xFD}); // Doesn't match DATA_FRAME_HEADER[2]=0x03, but IS CMD_FRAME_HEADER[0] - // Parser should reset and start new frame with 0xFD - EXPECT_EQ(this->ld2450_.buffer_pos_, 1); - EXPECT_EQ(this->ld2450_.buffer_data_[0], 0xFD); -} - -// --- Mid-frame / overflow recovery tests --- - -TEST_F(LD2450ReadlineTest, MidFrameDataRecovery) { - // Simulate starting mid-frame: feed the tail end of a periodic frame (no valid header) - // These bytes would be part of target data in a real frame - std::vector mid_frame = {0x10, 0x20, 0x30, 0x40, 0x55, 0xCC}; - this->ld2450_.feed(mid_frame); - // All discarded (none match header start bytes) - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); - - // Now feed a valid frame - auto frame = make_periodic_frame(); - this->ld2450_.feed(frame); - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); -} - -TEST_F(LD2450ReadlineTest, OverflowRecovery) { - // Feed a valid data frame header followed by enough filler to cause overflow. - // Header (4) + 36 filler = 40 bytes in buffer. The 41st byte triggers overflow. - std::vector overflow_data = {0xAA, 0xFF, 0x03, 0x00}; // Valid header - for (int i = 0; i < 37; i++) { +TEST_F(LD2450ReadlineTest, OverflowResetsBuffer) { + // Fill the buffer to capacity with filler that won't match any footer. + // MAX_LINE_LENGTH is 45, usable is 44. The 45th byte triggers overflow. + std::vector overflow_data; + for (int i = 0; i < MAX_LINE_LENGTH; i++) { overflow_data.push_back(0x11); // Filler that won't match any footer } - // 41 bytes total: 40 stored, 41st triggers overflow and resets buffer_pos_ to 0 this->ld2450_.feed(overflow_data); - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); - - // Feed a valid frame and verify recovery - auto frame = make_periodic_frame(); - this->ld2450_.feed(frame); - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); + // After overflow, buffer_pos_ resets to 0 (via the < 4 early return path) + EXPECT_LT(this->ld2450_.buffer_pos_, 4); } -TEST_F(LD2450ReadlineTest, RepeatedOverflowDoesNotLoop) { - // Simulate the bug scenario: repeated overflows should not prevent recovery. - // Feed 3 rounds of overflow-inducing data. - for (int round = 0; round < 3; round++) { - std::vector overflow_data = {0xAA, 0xFF, 0x03, 0x00}; - for (int i = 0; i < 37; i++) { - overflow_data.push_back(0x22); - } - this->ld2450_.feed(overflow_data); - EXPECT_EQ(this->ld2450_.buffer_pos_, 0) << "Overflow round " << round; +TEST_F(LD2450ReadlineTest, OverflowThenValidFrame) { + // Overflow, then a valid frame should be processed. + std::vector overflow_data; + for (int i = 0; i < MAX_LINE_LENGTH; i++) { + overflow_data.push_back(0x11); } + this->ld2450_.feed(overflow_data); - // Parser should still recover and process a valid frame auto frame = make_periodic_frame(); this->ld2450_.feed(frame); EXPECT_EQ(this->ld2450_.buffer_pos_, 0); } -TEST_F(LD2450ReadlineTest, SimulatedRestartGarbageThenFrames) { - // Simulate LD2450 restart: burst of garbage bytes (partial frames, noise) - // followed by normal periodic data. - // Partial periodic frame (as if we started reading mid-frame), a stale footer, and more garbage +TEST_F(LD2450ReadlineTest, BufferLargeEnoughForDesyncedFooter) { + // The key fix: the buffer (45) is large enough that a desynced periodic frame's + // footer (at most 30 bytes into the stream) will land inside the buffer before overflow. + // Simulate starting 10 bytes into a periodic frame, then a full frame follows. + std::vector mid_frame; + // 10 bytes of "mid-frame" data (no footer match) + for (int i = 0; i < 10; i++) { + mid_frame.push_back(0x30 + i); + } + // Then a complete periodic frame whose footer will land at position 40 (10 + 30), + // well within the buffer size of 45. + auto frame = make_periodic_frame(); + mid_frame.insert(mid_frame.end(), frame.begin(), frame.end()); + + this->ld2450_.feed(mid_frame); + // The footer from the frame should have triggered a reset + EXPECT_EQ(this->ld2450_.buffer_pos_, 0); +} + +TEST_F(LD2450ReadlineTest, SimulatedRestartThenFrames) { + // Simulate LD2450 restart: burst of garbage followed by valid periodic frames. + // The garbage + first frame should fit in the buffer so the footer resyncs. std::vector restart_noise = { - 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, // mid-frame data - 0x55, 0xCC, // stale footer bytes - 0x80, 0x81, 0x82, 0x83, 0x84, 0x85, 0x86, 0x87, 0x88, 0x89, // more garbage + 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, // 8 bytes of mid-frame data }; + auto frame = make_periodic_frame(); + // 8 garbage + 30 frame = 38 bytes, well within buffer of 45 + restart_noise.insert(restart_noise.end(), frame.begin(), frame.end()); this->ld2450_.feed(restart_noise); - // All garbage should be discarded - EXPECT_EQ(this->ld2450_.buffer_pos_, 0); - - // Now the LD2450 starts sending valid frames - auto frame = make_periodic_frame(); - this->ld2450_.feed(frame); EXPECT_EQ(this->ld2450_.buffer_pos_, 0); + // Subsequent frames should work normally this->ld2450_.feed(frame); EXPECT_EQ(this->ld2450_.buffer_pos_, 0); }