From d197db4c804b8332d249aa8e281bee53e86fa764 Mon Sep 17 00:00:00 2001 From: Frederik Jensen Kierbye Date: Fri, 4 Oct 2024 13:49:13 +0200 Subject: [PATCH] Discard frames with non-matching sequence numbers (#42) * Discard frames with non-matching sequence numbers During retries it means that we handle the same message multiple times. Frames will still be acknowledged as it's part of the protocol. Signed-off-by: Hans Binderup * handle sequencenumbers in unit test --------- Signed-off-by: Hans Binderup Co-authored-by: Hans Binderup --- include/Hdlcpp.hpp | 26 +++++++++--- test/src/TestHdlcpp.cpp | 88 ++++++++++++++++++++++++----------------- 2 files changed, 71 insertions(+), 43 deletions(-) diff --git a/include/Hdlcpp.hpp b/include/Hdlcpp.hpp index 1890f1f..fc830dc 100644 --- a/include/Hdlcpp.hpp +++ b/include/Hdlcpp.hpp @@ -191,10 +191,19 @@ class Hdlcpp { if (result >= 0) { switch (readFrame) { case FrameData: - if (++readSequenceNumber > 7) - readSequenceNumber = 0; + nextSequenceNumber(readSequenceNumber); writeFrame(address, FrameAck, readSequenceNumber, {}); - return { result, address }; + + // If sequence number matches last sequence number + // it must mean that we're dealing with a retransmission + // We already sent ACK so just ignore the message's data as + // we've already handled it + if (readSequenceNumber == lastReadSequenceNumber) { + return { 0, address }; + } else { + lastReadSequenceNumber = readSequenceNumber; + return { result, address }; + } case FrameAck: case FrameNack: writeResult = readFrame; @@ -221,9 +230,7 @@ class Hdlcpp { std::lock_guard writeLock(writeMutex); - // Sequence number is a 3-bit value - if (++writeSequenceNumber > 7) - writeSequenceNumber = 0; + nextSequenceNumber(writeSequenceNumber); for (uint8_t tries = 0; tries <= writeRetries; tries++) { writeResult = -1; @@ -280,6 +287,12 @@ class Hdlcpp { ControlTypeSelectiveReject, }; + // Helper to increment 3 bit sequence numbers + void nextSequenceNumber(uint8_t& val) const + { + val = (val >= 7 ? 0 : val + 1); + } + template struct span { constexpr span(std::span span) @@ -546,6 +559,7 @@ class Hdlcpp { uint16_t writeTimeout; uint8_t writeRetries; uint8_t readSequenceNumber { 0 }; + uint8_t lastReadSequenceNumber { 0xFF }; // initially something invalid (3 bit value) uint8_t writeSequenceNumber { 0 }; std::atomic writeResult { -1 }; std::atomic stopped { false }; diff --git a/test/src/TestHdlcpp.cpp b/test/src/TestHdlcpp.cpp index e1a6232..28a157c 100644 --- a/test/src/TestHdlcpp.cpp +++ b/test/src/TestHdlcpp.cpp @@ -35,9 +35,11 @@ class HdlcppFixture { return writeBuffer.size(); } - const uint8_t frameAck[6] = { 0x7e, 0xff, 0x41, 0x0a, 0xa3, 0x7e }; - const uint8_t frameNack[6] = { 0x7e, 0xff, 0x29, 0x44, 0x4c, 0x7e }; - const uint8_t frameData[7] = { 0x7e, 0xff, 0x12, 0x55, 0x36, 0xa3, 0x7e }; + const uint8_t frameAck_seq2[6] = { 0x7e, 0xff, 0x41, 0x0a, 0xa3, 0x7e }; + const uint8_t frameAck_seq3[6] = { 0x7e, 0xff, 0x61, 0x08, 0x82, 0x7e }; + const uint8_t frameNack_seq1[6] = { 0x7e, 0xff, 0x29, 0x44, 0x4c, 0x7e }; + const uint8_t frameData_seq1[7] = { 0x7e, 0xff, 0x12, 0x55, 0x36, 0xa3, 0x7e }; + const uint8_t frameData_seq2[7] = { 0x7e, 0xff, 0x14, 0x55, 0xE6, 0xF7, 0x7e }; const uint8_t frameDataInvalid[7] = { 0x7e, 0xff, 0x12, 0x33, 0x67, 0xf8, 0x7e }; const uint8_t frameDataDoubleFlagSequence[9] = { 0x7e, 0x7e, 0xff, 0x12, 0x55, 0x36, 0xa3, 0x7e, 0x7e }; @@ -59,15 +61,15 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") SECTION("Test write with valid 1 byte data input") { - hdlcpp->write(Hdlcpp::AddressBroadcast, { &frameData[3], 1 }); - CHECK(std::memcmp(frameData, writeBuffer.data(), sizeof(frameData)) == 0); + hdlcpp->write(Hdlcpp::AddressBroadcast, { &frameData_seq1[3], 1 }); + CHECK(std::memcmp(frameData_seq1, writeBuffer.data(), sizeof(frameData_seq1)) == 0); } SECTION("Test write/read with FlagSequence as data input") { hdlcpp->write(Hdlcpp::AddressBroadcast, { &hdlcpp->FlagSequence, 1 }); // Size should be 1 byte more compared to a 1 byte data frame due to escape of the value - CHECK(writeBuffer.size() == (sizeof(frameData) + 1)); + CHECK(writeBuffer.size() == (sizeof(frameData_seq1) + 1)); readBuffer = writeBuffer; @@ -79,7 +81,7 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") { hdlcpp->write(Hdlcpp::AddressBroadcast, { &hdlcpp->ControlEscape, 1 }); // Size should be 1 byte more compared to a 1 byte data frame due to escape of the value - CHECK(writeBuffer.size() == (sizeof(frameData) + 1)); + CHECK(writeBuffer.size() == (sizeof(frameData_seq1) + 1)); readBuffer = writeBuffer; @@ -96,79 +98,91 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") readBuffer.assign(frameDataInvalid, frameDataInvalid + sizeof(frameDataInvalid)); CHECK(hdlcpp->read(dataBuffer).size == -EIO); - CHECK(std::memcmp(frameNack, writeBuffer.data(), sizeof(frameNack)) == 0); + CHECK(std::memcmp(frameNack_seq1, writeBuffer.data(), sizeof(frameNack_seq1)) == 0); + } + + SECTION("Test read of duplicate 1 byte data frames, 2nd discarded but ack'ed") + { + readBuffer.assign(frameData_seq1, frameData_seq1 + sizeof(frameData_seq1)); + CHECK(hdlcpp->read(dataBuffer).size == 1); + CHECK(dataBuffer[0] == frameData_seq1[3]); + CHECK(std::memcmp(frameAck_seq2, writeBuffer.data(), sizeof(frameAck_seq2)) == 0); + + CHECK(hdlcpp->read(dataBuffer).size == 0); + CHECK(std::memcmp(frameAck_seq2, writeBuffer.data(), sizeof(frameAck_seq2)) == 0); } SECTION("Test read of two valid 1 byte data frames") { - readBuffer.assign(frameData, frameData + sizeof(frameData)); + readBuffer.assign(frameData_seq1, frameData_seq1 + sizeof(frameData_seq1)); CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq1[3]); + CHECK(std::memcmp(frameAck_seq2, writeBuffer.data(), sizeof(frameAck_seq2)) == 0); + readBuffer.assign(frameData_seq2, frameData_seq2 + sizeof(frameData_seq2)); CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq2[3]); + CHECK(std::memcmp(frameAck_seq3, writeBuffer.data(), sizeof(frameAck_seq3)) == 0); } SECTION("Test read of valid 1 byte data frame in two chunks") { // Add the first 3 bytes to be read - readBuffer.assign(frameData, frameData + 3); + readBuffer.assign(frameData_seq1, frameData_seq1 + 3); CHECK(hdlcpp->read(dataBuffer).size == -ENOMSG); CHECK(writeBuffer.empty()); // Now add the remaining bytes to complete the data frame - readBuffer.assign(frameData + 3, frameData + sizeof(frameData)); + readBuffer.assign(frameData_seq1 + 3, frameData_seq1 + sizeof(frameData_seq1)); CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq1[3]); + CHECK(std::memcmp(frameAck_seq2, writeBuffer.data(), sizeof(frameAck_seq2)) == 0); } SECTION("Test read of valid 1 byte data frame with double flag sequence") { readBuffer.assign(frameDataDoubleFlagSequence, frameDataDoubleFlagSequence + sizeof(frameDataDoubleFlagSequence)); CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq1[3]); + CHECK(std::memcmp(frameAck_seq2, writeBuffer.data(), sizeof(frameAck_seq2)) == 0); } SECTION("Test read of two partial data frames") { - readBuffer.assign(frameData, frameData + sizeof(frameData)); // One complete frame - readBuffer.insert(readBuffer.end(), frameData, frameData + 3); // A partial frame + readBuffer.assign(frameData_seq1, frameData_seq1 + sizeof(frameData_seq1)); // One complete frame + readBuffer.insert(readBuffer.end(), frameData_seq2, frameData_seq2 + 3); // A partial frame CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq1[3]); + CHECK(std::memcmp(frameAck_seq2, writeBuffer.data(), sizeof(frameAck_seq2)) == 0); - readBuffer.assign(frameData + 3, frameData + sizeof(frameData)); + readBuffer.assign(frameData_seq2 + 3, frameData_seq2 + sizeof(frameData_seq2)); CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq2[3]); + CHECK(std::memcmp(frameAck_seq3, writeBuffer.data(), sizeof(frameAck_seq3)) == 0); } SECTION("Test read of two complete data frames") { - readBuffer.assign(frameData, frameData + sizeof(frameData)); - readBuffer.insert(readBuffer.end(), frameData, frameData + sizeof(frameData)); + readBuffer.assign(frameData_seq1, frameData_seq1 + sizeof(frameData_seq1)); + readBuffer.insert(readBuffer.end(), frameData_seq2, frameData_seq2 + sizeof(frameData_seq2)); // Read first frame CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq1[3]); + CHECK(std::memcmp(frameAck_seq2, writeBuffer.data(), sizeof(frameAck_seq2)) == 0); readBuffer.clear(); // This must read the second frame without reading from transport layer CHECK(hdlcpp->read(dataBuffer).size == 1); - CHECK(dataBuffer[0] == frameData[3]); - CHECK(std::memcmp(frameAck, writeBuffer.data(), sizeof(frameAck)) == 0); + CHECK(dataBuffer[0] == frameData_seq2[3]); + CHECK(std::memcmp(frameAck_seq3, writeBuffer.data(), sizeof(frameAck_seq3)) == 0); } SECTION("Test read of ack frame") { hdlcpp->writeSequenceNumber = 1; - readBuffer.assign(frameAck, frameAck + sizeof(frameAck)); + readBuffer.assign(frameAck_seq2, frameAck_seq2 + sizeof(frameAck_seq2)); CHECK(hdlcpp->read(dataBuffer).size == 0); CHECK(hdlcpp->writeResult == Hdlcpp::Hdlcpp::FrameAck); } @@ -176,7 +190,7 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") SECTION("Test read of nack frame") { hdlcpp->writeSequenceNumber = 1; - readBuffer.assign(frameNack, frameNack + sizeof(frameNack)); + readBuffer.assign(frameNack_seq1, frameNack_seq1 + sizeof(frameNack_seq1)); CHECK(hdlcpp->read(dataBuffer).size == 0); CHECK(hdlcpp->writeResult == Hdlcpp::Hdlcpp::FrameNack); } @@ -265,9 +279,9 @@ TEST_CASE_METHOD(HdlcppFixture, "hdlcpp test", "[single-file]") const uint8_t frame1[] = { 0x7e, 0xff, 0x12, 0x12, 0x00, 0x00, 0xaf, 0x7e }; const uint8_t frame2[] = { 0x7e, 0xff, 0x14, 0x4a, 0x07, 0x0a, 0x7e, 0xff, 0x14 }; const uint8_t frame3[] = { 0x4a, 0x07, 0x0a, 0x01, 0x00, 0x10, 0x01, 0x20, 0x64, 0xca, 0x51, 0x7e }; - const uint8_t frame4[] = { 0x7e, 0xff, 0x14 }; - const uint8_t frame5[] = { 0x4a, 0x07, 0x0a, 0x01, 0x00, 0x10, 0x01, 0x20, 0x64, 0xca }; - const uint8_t frame6[] = { 0x51, 0x7e }; + const uint8_t frame4[] = { 0x7e, 0xff, 0x16 }; + const uint8_t frame5[] = { 0x4a, 0x07, 0x0a, 0x01, 0x00, 0x10, 0x01, 0x20, 0x64, 0x84 }; + const uint8_t frame6[] = { 0x09, 0x7e }; const uint8_t frame7[] = { 0x7e, 0xff, 0x21, 0x0c, 0xc0, 0x7e }; readBuffer.assign(frame1, frame1 + sizeof(frame1));