pw_hdlc_lite: Update C++ decoder
- Update the C++ decoder to support an address byte and CRC-32 for the
frame check sequence.
- Generate C++ tests from the Python decoder test data.
- Move common encode/decode constants to private protocol.h header.
Change-Id: If96e42b2c36b96a093a35600a47f474726056b49
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/18142
Commit-Queue: Wyatt Hepler <hepler@google.com>
Reviewed-by: Keir Mierle <keir@google.com>
diff --git a/pw_hdlc_lite/BUILD b/pw_hdlc_lite/BUILD
index 26bfad1..a4d0014 100644
--- a/pw_hdlc_lite/BUILD
+++ b/pw_hdlc_lite/BUILD
@@ -23,28 +23,29 @@
pw_cc_library(
name = "pw_hdlc_lite",
- hdrs = [
- "public/pw_hdlc_lite/decoder.h",
- "public/pw_hdlc_lite/encoder.h",
- "public/pw_hdlc_lite/rpc_server_packets.h",
- "public/pw_hdlc_lite/sys_io_stream.h",
- "public/pw_hdlc_lite/hdlc_channel.h",
- ],
srcs = [
- "encoder.cc",
- "decoder.cc",
- "hdlc_channel.cc"
+ "decoder.cc",
+ "encoder.cc",
+ "hdlc_channel.cc",
+ "pw_hdlc_lite_private/protocol.h",
+ ],
+ hdrs = [
+ "public/pw_hdlc_lite/decoder.h",
+ "public/pw_hdlc_lite/encoder.h",
+ "public/pw_hdlc_lite/hdlc_channel.h",
+ "public/pw_hdlc_lite/rpc_server_packets.h",
+ "public/pw_hdlc_lite/sys_io_stream.h",
],
includes = ["public"],
deps = [
+ "//pw_bytes",
+ "//pw_checksum",
"//pw_log",
+ "//pw_result",
+ "//pw_rpc",
"//pw_span",
"//pw_status",
"//pw_stream",
- "//pw_checksum",
- "//pw_bytes",
- "//pw_result",
- "//pw_rpc",
],
)
@@ -54,20 +55,20 @@
"hdlc_server_example.cc",
],
hdrs = [
- "public/pw_hdlc_lite/decoder.h",
- "public/pw_hdlc_lite/rpc_server_packets.h",
- "public/pw_hdlc_lite/hdlc_channel.h",
+ "public/pw_hdlc_lite/decoder.h",
+ "public/pw_hdlc_lite/hdlc_channel.h",
+ "public/pw_hdlc_lite/rpc_server_packets.h",
],
includes = ["public"],
deps = [
+ "//pw_bytes",
+ "//pw_checksum",
"//pw_log",
+ "//pw_result",
+ "//pw_rpc",
"//pw_span",
"//pw_status",
"//pw_stream",
- "//pw_checksum",
- "//pw_bytes",
- "//pw_result",
- "//pw_rpc",
],
)
@@ -86,9 +87,9 @@
srcs = ["decoder_test.cc"],
deps = [
":pw_hdlc_lite",
+ "//pw_result",
"//pw_stream",
"//pw_unit_test",
- "//pw_result",
],
)
diff --git a/pw_hdlc_lite/BUILD.gn b/pw_hdlc_lite/BUILD.gn
index 276f058..9e498c7 100644
--- a/pw_hdlc_lite/BUILD.gn
+++ b/pw_hdlc_lite/BUILD.gn
@@ -32,6 +32,7 @@
sources = [
"decoder.cc",
"encoder.cc",
+ "pw_hdlc_lite_private/protocol.h",
]
public_deps = [
dir_pw_bytes,
@@ -44,6 +45,7 @@
dir_pw_checksum,
dir_pw_log,
]
+ friend = [ ":*" ]
}
pw_source_set("pw_rpc") {
@@ -94,9 +96,18 @@
sources = [ "encoder_test.cc" ]
}
+action("generate_decoder_test") {
+ outputs = [ "$target_gen_dir/generated_decoder_test.cc" ]
+ script = "py/decoder_test.py"
+ args = [ "--generate-cc-decode-test" ] + rebase_path(outputs)
+}
+
pw_test("decoder_test") {
- deps = [ ":pw_hdlc_lite" ]
- sources = [ "decoder_test.cc" ]
+ deps = [
+ ":generate_decoder_test",
+ ":pw_hdlc_lite",
+ ]
+ sources = [ "decoder_test.cc" ] + get_target_outputs(":generate_decoder_test")
}
pw_test("hdlc_channel_test") {
diff --git a/pw_hdlc_lite/decoder.cc b/pw_hdlc_lite/decoder.cc
index fe2a419..37c0023 100644
--- a/pw_hdlc_lite/decoder.cc
+++ b/pw_hdlc_lite/decoder.cc
@@ -14,7 +14,10 @@
#include "pw_hdlc_lite/decoder.h"
-#include "pw_checksum/crc16_ccitt.h"
+#include "pw_assert/assert.h"
+#include "pw_bytes/endian.h"
+#include "pw_checksum/crc32.h"
+#include "pw_hdlc_lite_private/protocol.h"
#include "pw_log/log.h"
using std::byte;
@@ -22,112 +25,114 @@
namespace pw::hdlc_lite {
namespace {
-constexpr byte kHdlcFrameDelimiter = byte{0x7E};
-constexpr byte kHdlcEscape = byte{0x7D};
-constexpr byte kHdlcUnescapingConstant = byte{0x20};
+constexpr byte kUnescapeConstant = byte{0x20};
} // namespace
-Result<ConstByteSpan> Decoder::AddByte(const byte new_byte) {
+Result<Frame> Decoder::Process(const byte new_byte) {
switch (state_) {
- case DecoderState::kPacketActive: {
- if (new_byte != kHdlcFrameDelimiter) {
- // Packet active case
- if (new_byte != kHdlcEscape) {
- return Result<ConstByteSpan>(AddEscapedByte(new_byte));
+ case State::kInterFrame: {
+ if (new_byte == kFlag) {
+ state_ = State::kFrame;
+
+ // Report an error if non-flag bytes were read between frames.
+ if (current_frame_size_ != 0u) {
+ current_frame_size_ = 0;
+ return Status::DATA_LOSS;
}
- state_ = DecoderState::kEscapeNextByte;
- return Result<ConstByteSpan>(Status::UNAVAILABLE);
+ } else {
+ // Count bytes to track how many are discarded.
+ current_frame_size_ += 1;
}
- // Packet complete case
- state_ = DecoderState::kNoPacket;
- Status status = PacketStatus();
- if (status.ok()) {
- // Common case: Happy Packet
- // Returning size_ - 2 is to trim off the 2-byte CRC value.
- return Result<ConstByteSpan>(frame_buffer_.first(size_ - 2));
- }
- if (status == Status::DATA_LOSS && size_ == 0) {
- // Uncommon case: Dropped an ending frame delimiter byte somewhere.
- // This happens if a delimiter byte was lost or corrupted which causes
- // the decoder to fall out of sync with the incoming flow of packets.
- // Recovery is done by switching to active mode assuming that the frame
- // delimiter "close" byte we just saw is actually a start delimiter.
- PW_LOG_ERROR(
- "Detected empty packet. Assuming out of sync; trying recovery");
- clear();
- state_ = DecoderState::kPacketActive;
- return Result<ConstByteSpan>(Status::UNAVAILABLE);
- }
- // Otherwise, forward the status from PacketStatus().
- return Result<ConstByteSpan>(status);
+ return Status::UNAVAILABLE; // Report error when starting a new frame.
}
+ case State::kFrame: {
+ if (new_byte == kFlag) {
+ const Status status = CheckFrame();
- case DecoderState::kEscapeNextByte: {
- byte escaped_byte = new_byte ^ kHdlcUnescapingConstant;
- if (escaped_byte != kHdlcEscape && escaped_byte != kHdlcFrameDelimiter) {
- PW_LOG_WARN(
- "Suspicious escaped byte: 0x%02x; should only need to escape frame "
- "delimiter and escape byte",
- static_cast<int>(escaped_byte));
+ state_ = State::kFrame;
+ const size_t completed_frame_size = current_frame_size_;
+ current_frame_size_ = 0;
+
+ if (status.ok()) {
+ return Frame(buffer_.first(completed_frame_size));
+ }
+ return status;
}
- state_ = DecoderState::kPacketActive;
- return Result<ConstByteSpan>(AddEscapedByte(escaped_byte));
+
+ if (new_byte == kEscape) {
+ state_ = State::kFrameEscape;
+ } else {
+ AppendByte(new_byte);
+ }
+ return Status::UNAVAILABLE;
}
-
- case DecoderState::kNoPacket: {
- if (new_byte != kHdlcFrameDelimiter) {
- PW_LOG_ERROR("Unexpected starting byte to the frame: 0x%02x",
- static_cast<int>(new_byte));
- return Result<ConstByteSpan>(Status::UNAVAILABLE);
+ case State::kFrameEscape: {
+ // The flag character cannot be escaped; return an error.
+ if (new_byte == kFlag) {
+ state_ = State::kFrame;
+ current_frame_size_ = 0;
+ return Status::DATA_LOSS;
}
- clear();
- state_ = DecoderState::kPacketActive;
- return Result<ConstByteSpan>(Status::UNAVAILABLE);
+
+ if (new_byte == kEscape) {
+ // Two escape characters in a row is illegal -- invalidate this frame.
+ // The frame is reported abandoned when the next flag byte appears.
+ state_ = State::kInterFrame;
+
+ // Count the escape byte so that the inter-frame state detects an error.
+ current_frame_size_ += 1;
+ } else {
+ state_ = State::kFrame;
+ AppendByte(new_byte ^ kUnescapeConstant);
+ }
+ return Status::UNAVAILABLE;
}
}
- return Result<ConstByteSpan>(Status::UNAVAILABLE);
+ PW_CRASH("Bad decoder state");
}
-bool Decoder::CheckCrc() const {
- uint16_t expected_crc =
- checksum::Crc16Ccitt::Calculate(frame_buffer_.first(size_ - 2), 0xFFFF);
- uint16_t actual_crc;
- std::memcpy(&actual_crc, (frame_buffer_.data() + size_ - 2), 2);
- return actual_crc == expected_crc;
-}
-
-Status Decoder::AddEscapedByte(const byte new_byte) {
- if (size_ >= max_size()) {
- // Increasing the size to flag the overflow case when the packet is complete
- size_++;
- return Status::RESOURCE_EXHAUSTED;
+void Decoder::AppendByte(byte new_byte) {
+ if (current_frame_size_ < max_size()) {
+ buffer_[current_frame_size_] = new_byte;
}
- frame_buffer_[size_++] = new_byte;
- return Status::UNAVAILABLE;
+
+ // Always increase size: if it is larger than the buffer, overflow occurred.
+ current_frame_size_ += 1;
}
-Status Decoder::PacketStatus() const {
- if (size_ < 2) {
- PW_LOG_ERROR(
- "Received %d-byte packet; packets must at least have 2 CRC bytes",
- static_cast<int>(size_));
+Status Decoder::CheckFrame() const {
+ // Empty frames are not an error; repeated flag characters are okay.
+ if (current_frame_size_ == 0u) {
+ return Status::UNAVAILABLE;
+ }
+
+ if (current_frame_size_ < Frame::kMinSizeBytes) {
+ PW_LOG_ERROR("Received %lu-byte frame; frame must be at least 6 bytes",
+ static_cast<unsigned long>(current_frame_size_));
return Status::DATA_LOSS;
}
- if (size_ > max_size()) {
- PW_LOG_ERROR("Packet size [%zu] exceeds the maximum buffer size [%zu]",
- size_,
- max_size());
+ if (current_frame_size_ > max_size()) {
+ PW_LOG_ERROR("Frame size [%lu] exceeds the maximum buffer size [%lu]",
+ static_cast<unsigned long>(current_frame_size_),
+ static_cast<unsigned long>(max_size()));
return Status::RESOURCE_EXHAUSTED;
}
- if (!CheckCrc()) {
- PW_LOG_ERROR("CRC verification failed for packet");
+ if (!VerifyFrameCheckSequence()) {
+ PW_LOG_ERROR("Frame check sequence verification failed");
return Status::DATA_LOSS;
}
return Status::OK;
}
+bool Decoder::VerifyFrameCheckSequence() const {
+ uint32_t fcs = bytes::ReadInOrder<uint32_t>(
+ std::endian::little, buffer_.data() + current_frame_size_ - sizeof(fcs));
+ return fcs == checksum::Crc32::Calculate(
+ buffer_.first(current_frame_size_ - sizeof(fcs)));
+}
+
} // namespace pw::hdlc_lite
diff --git a/pw_hdlc_lite/decoder_test.cc b/pw_hdlc_lite/decoder_test.cc
index 0373727..787fbf1 100644
--- a/pw_hdlc_lite/decoder_test.cc
+++ b/pw_hdlc_lite/decoder_test.cc
@@ -19,306 +19,109 @@
#include "gtest/gtest.h"
#include "pw_bytes/array.h"
-#include "pw_result/result.h"
-#include "pw_stream/memory_stream.h"
-
-using std::byte;
+#include "pw_hdlc_lite_private/protocol.h"
namespace pw::hdlc_lite {
namespace {
-TEST(DecoderBuffer, 1BytePayload) {
- std::array<byte, 1> expected_payload = bytes::Array<0x41>();
- std::array<byte, 5> data_frame = bytes::Array<0x7E, 0x41, 0x15, 0xB9, 0x7E>();
- DecoderBuffer<10> decoder;
+using std::byte;
- for (size_t i = 0; i < data_frame.size() - 1; i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- Result<ConstByteSpan> result =
- decoder.AddByte(data_frame[data_frame.size() - 1]);
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
+TEST(Frame, Fields) {
+ static constexpr auto kFrameData = bytes::String("1234\xa3\xe0\xe3\x9b");
+ constexpr Frame frame(kFrameData);
+
+ static_assert(frame.address() == unsigned{'1'});
+ static_assert(frame.control() == byte{'2'});
+
+ static_assert(frame.data().size() == 2u);
+ static_assert(frame.data()[0] == byte{'3'});
+ static_assert(frame.data()[1] == byte{'4'});
}
-TEST(DecoderBuffer, 0BytePayload) {
- std::array<byte, 4> data_frame = bytes::Array<0x7E, 0xFF, 0xFF, 0x7E>();
- DecoderBuffer<10> decoder;
+TEST(Decoder, Clear) {
+ DecoderBuffer<8> decoder;
- for (size_t i = 0; i < data_frame.size() - 1; i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- Result<ConstByteSpan> result =
- decoder.AddByte(data_frame[data_frame.size() - 1]);
- EXPECT_TRUE(result.ok());
- EXPECT_TRUE(result.value().empty());
+ // Process a partial packet
+ decoder.Process(bytes::String("~1234abcd"),
+ [](const Result<Frame>&) { FAIL(); });
+
+ decoder.clear();
+ Status status = Status::UNKNOWN;
+
+ decoder.Process(
+ bytes::String("~1234\xa3\xe0\xe3\x9b~"),
+ [&status](const Result<Frame>& result) { status = result.status(); });
+
+ EXPECT_EQ(Status::OK, status);
}
-TEST(DecoderBuffer, 9BytePayload) {
- std::array<byte, 9> expected_payload =
- bytes::Array<0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39>();
- std::array<byte, 13> data_frame = bytes::Array<0x7E,
- 0x31,
- 0x32,
- 0x33,
- 0x34,
- 0x35,
- 0x36,
- 0x37,
- 0x38,
- 0x39,
- 0xB1,
- 0x29,
- 0x7E>();
- DecoderBuffer<15> decoder;
+TEST(Decoder, ExactFit) {
+ DecoderBuffer<8> decoder;
- for (size_t i = 0; i < data_frame.size() - 1; i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
+ for (byte b : bytes::String("~1234\xa3\xe0\xe3\x9b")) {
+ EXPECT_EQ(Status::UNAVAILABLE, decoder.Process(b).status());
}
- Result<ConstByteSpan> result =
- decoder.AddByte(data_frame[data_frame.size() - 1]);
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
+ auto result = decoder.Process(kFlag);
+ ASSERT_EQ(Status::OK, result.status());
+ ASSERT_EQ(result.value().data().size(), 2u);
+ ASSERT_EQ(result.value().data()[0], byte{'3'});
+ ASSERT_EQ(result.value().data()[1], byte{'4'});
}
-TEST(DecoderBuffer, MultiFrameDecoding) {
- std::array<byte, 1> expected_payload = bytes::Array<0x41>();
- std::array<byte, 10> multiple_data_frames = bytes::
- Array<0x7E, 0x41, 0x15, 0xB9, 0x7E, 0x7E, 0x41, 0x15, 0xB9, 0x7E>();
- DecoderBuffer<12> decoder;
+TEST(Decoder, MinimumSizedBuffer) {
+ DecoderBuffer<6> decoder;
- for (size_t i = 0; i < multiple_data_frames.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(multiple_data_frames[i]);
- if (i == 4u || i == 9u) {
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
+ for (byte b : bytes::String("~12\xcd\x44\x53\x4f")) {
+ EXPECT_EQ(Status::UNAVAILABLE, decoder.Process(b).status());
}
+
+ auto result = decoder.Process(kFlag);
+ ASSERT_EQ(Status::OK, result.status());
+ EXPECT_EQ(result.value().data().size(), 0u);
}
-TEST(DecoderBuffer, UnescapingDataFrame_0x7D) {
- std::array<byte, 1> expected_payload = bytes::Array<0x7D>();
- std::array<byte, 6> data_frame =
- bytes::Array<0x7E, 0x7D, 0x5D, 0xCA, 0x4E, 0x7E>();
- DecoderBuffer<10> decoder;
+TEST(Decoder, TooLargeForBuffer_ReportsResourceExhausted) {
+ DecoderBuffer<8> decoder;
- for (size_t i = 0; i < data_frame.size() - 1; i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
+ for (byte b : bytes::String("~123456789")) {
+ EXPECT_EQ(Status::UNAVAILABLE, decoder.Process(b).status());
}
- Result<ConstByteSpan> result =
- decoder.AddByte(data_frame[data_frame.size() - 1]);
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
+ EXPECT_EQ(Status::RESOURCE_EXHAUSTED, decoder.Process(kFlag).status());
+
+ for (byte b : bytes::String("~123456789012345678901234567890")) {
+ EXPECT_EQ(Status::UNAVAILABLE, decoder.Process(b).status());
+ }
+ EXPECT_EQ(Status::RESOURCE_EXHAUSTED, decoder.Process(kFlag).status());
}
-TEST(DecoderBuffer, UnescapingDataFrame_0x7E) {
- std::array<byte, 1> expected_payload = bytes::Array<0x7E>();
- std::array<byte, 7> data_frame =
- bytes::Array<0x7E, 0x7D, 0x5E, 0xA9, 0x7D, 0x5E, 0x7E>();
- DecoderBuffer<10> decoder;
+TEST(Decoder, TooLargeForBuffer_StaysWithinBufferBoundaries) {
+ std::array<byte, 16> buffer = bytes::Initialized<16>('?');
- for (size_t i = 0; i < data_frame.size() - 1; i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
+ Decoder decoder(std::span(buffer.data(), 8));
+
+ for (byte b : bytes::String("~1234567890123456789012345678901234567890")) {
+ EXPECT_EQ(Status::UNAVAILABLE, decoder.Process(b).status());
}
- Result<ConstByteSpan> result =
- decoder.AddByte(data_frame[data_frame.size() - 1]);
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
+
+ for (size_t i = 8; i < buffer.size(); ++i) {
+ ASSERT_EQ(byte{'?'}, buffer[i]);
+ }
+
+ EXPECT_EQ(Status::RESOURCE_EXHAUSTED, decoder.Process(kFlag).status());
}
-TEST(DecoderBuffer, UnescapingDataFrame_Mix) {
- std::array<byte, 7> expected_payload =
- bytes::Array<0x7E, 0x7B, 0x61, 0x62, 0x63, 0x7D, 0x7E>();
- ;
- std::array<byte, 14> data_frame = bytes::Array<0x7E,
- 0x7D,
- 0x5E,
- 0x7B,
- 0x61,
- 0x62,
- 0x63,
- 0x7D,
- 0x5D,
- 0x7D,
- 0x5E,
- 0x49,
- 0xE5,
- 0x7E>();
- DecoderBuffer<15> decoder;
+TEST(Decoder, TooLargeForBuffer_DecodesNextFrame) {
+ DecoderBuffer<8> decoder;
- for (size_t i = 0; i < data_frame.size() - 1; i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
+ for (byte b : bytes::String("~123456789012345678901234567890")) {
+ EXPECT_EQ(Status::UNAVAILABLE, decoder.Process(b).status());
}
- Result<ConstByteSpan> result =
- decoder.AddByte(data_frame[data_frame.size() - 1]);
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
-}
+ EXPECT_EQ(Status::RESOURCE_EXHAUSTED, decoder.Process(kFlag).status());
-TEST(DecoderBuffer, IncorrectCRC) {
- std::array<byte, 1> expected_payload = bytes::Array<0x41>();
- std::array<byte, 5> incorrect_data_frame =
- bytes::Array<0x7E, 0x41, 0x15, 0xB8, 0x7E>();
- std::array<byte, 5> correct_data_frame =
- bytes::Array<0x7E, 0x41, 0x15, 0xB9, 0x7E>();
- DecoderBuffer<10> decoder;
-
- for (size_t i = 0; i < incorrect_data_frame.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(incorrect_data_frame[i]);
- if (i == incorrect_data_frame.size() - 1) {
- EXPECT_EQ(result.status(), Status::DATA_LOSS);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
+ for (byte b : bytes::String("1234\xa3\xe0\xe3\x9b")) {
+ EXPECT_EQ(Status::UNAVAILABLE, decoder.Process(b).status());
}
-
- for (size_t i = 0; i < correct_data_frame.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(correct_data_frame[i]);
- if (i == 4u) {
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- }
-}
-
-TEST(DecoderBuffer, BufferSizeLessThan2EdgeCase) {
- std::array<byte, 3> data_frame = bytes::Array<0x7E, 0xFF, 0x7E>();
- DecoderBuffer<10> decoder;
-
- for (size_t i = 0; i < data_frame.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- if (i == data_frame.size() - 1) {
- EXPECT_EQ(result.status(), Status::DATA_LOSS);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- }
-}
-
-TEST(DecoderBuffer, BufferOutOfSpace_SkipsRestOfFrame) {
- std::array<byte, 5> data_frame = bytes::Array<0x7E, 0x41, 0x15, 0xB9, 0x7E>();
- DecoderBuffer<2> decoder;
-
- for (size_t i = 0; i < data_frame.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frame[i]);
- if (i >= 3u) {
- EXPECT_EQ(result.status(), Status::RESOURCE_EXHAUSTED);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- }
-}
-
-TEST(DecoderBuffer, BufferOutOfSpace_SkipsRestOfFrameAndDecodesNext) {
- std::array<byte, 1> expected_payload = bytes::Array<0x41>();
- std::array<byte, 11> data_frames = bytes::
- Array<0x7E, 0x41, 0x42, 0x15, 0xB9, 0x7E, 0x7E, 0x41, 0x15, 0xB9, 0x7E>();
- DecoderBuffer<3> decoder;
-
- for (size_t i = 0; i < data_frames.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frames[i]);
- if (i == 4u || i == 5u) {
- EXPECT_EQ(result.status(), Status::RESOURCE_EXHAUSTED);
- } else if (i == 10u) {
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- }
-}
-
-TEST(DecoderBuffer, UnexpectedStartingByte) {
- std::array<byte, 1> expected_payload = bytes::Array<0x41>();
- std::array<byte, 12> data_frames = bytes::Array<0x7E,
- 0x41,
- 0x15,
- 0xB9,
- 0x7E, // End of 1st Packet
- 0xAA, // Garbage bytes
- 0xAA, // Garbage bytes
- 0x7E,
- 0x41,
- 0x15,
- 0xB9,
- 0x7E>(); // End of 2nd Packet
- DecoderBuffer<10> decoder;
-
- for (size_t i = 0; i < data_frames.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frames[i]);
- if (i == 4u || i == 11u) {
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- }
-}
-
-TEST(DecoderBuffer, RecoveringMissingFrameDelimiterCase) {
- std::array<byte, 1> expected_payload = bytes::Array<0x41>();
- std::array<byte, 12> data_frames =
- bytes::Array<0x7E,
- 0x41,
- 0x15,
- 0xB9,
- 0x7E, // End of 1st Packet
- 0x41,
- 0x7E, // End of Packet with missing start frame delimiter
- 0x7E,
- 0x41,
- 0x15,
- 0xB9,
- 0x7E>(); // End of 2nd Packet
- DecoderBuffer<10> decoder;
-
- for (size_t i = 0; i < data_frames.size(); i++) {
- Result<ConstByteSpan> result = decoder.AddByte(data_frames[i]);
- if (i == 4u || i == 11u) {
- EXPECT_TRUE(result.ok());
- EXPECT_EQ(std::memcmp(result.value().data(),
- expected_payload.data(),
- expected_payload.size()),
- 0);
- } else {
- EXPECT_EQ(result.status(), Status::UNAVAILABLE);
- }
- }
+ EXPECT_EQ(Status::OK, decoder.Process(kFlag).status());
}
} // namespace
diff --git a/pw_hdlc_lite/encoder.cc b/pw_hdlc_lite/encoder.cc
index d47fd04..dde2556 100644
--- a/pw_hdlc_lite/encoder.cc
+++ b/pw_hdlc_lite/encoder.cc
@@ -22,33 +22,26 @@
#include "pw_bytes/endian.h"
#include "pw_checksum/crc32.h"
+#include "pw_hdlc_lite_private/protocol.h"
using std::byte;
namespace pw::hdlc_lite {
namespace {
-constexpr byte kHdlcFlag = byte{0x7E};
-constexpr byte kHdlcEscape = byte{0x7D};
-
// Indicates this an information packet with sequence numbers set to 0.
constexpr byte kUnusedControl = byte{0};
-constexpr std::array<byte, 2> kEscapedFlag = {byte{0x7D}, byte{0x5E}};
-constexpr std::array<byte, 2> kEscapedEscape = {byte{0x7D}, byte{0x5D}};
-
Status EscapeAndWrite(const byte b, stream::Writer& writer) {
- if (b == kHdlcFlag) {
+ if (b == kFlag) {
return writer.Write(kEscapedFlag);
}
- if (b == kHdlcEscape) {
+ if (b == kEscape) {
return writer.Write(kEscapedEscape);
}
return writer.Write(b);
}
-bool NeedsEscaping(byte b) { return (b == kHdlcFlag || b == kHdlcEscape); }
-
// Encodes and writes HDLC frames.
class Encoder {
public:
@@ -72,7 +65,7 @@
Status Encoder::StartInformationFrame(uint8_t address) {
fcs_.clear();
- if (Status status = writer_.Write(kHdlcFlag); !status.ok()) {
+ if (Status status = writer_.Write(kFlag); !status.ok()) {
return status;
}
@@ -105,7 +98,7 @@
!status.ok()) {
return status;
}
- return writer_.Write(kHdlcFlag);
+ return writer_.Write(kFlag);
}
} // namespace
diff --git a/pw_hdlc_lite/encoder_test.cc b/pw_hdlc_lite/encoder_test.cc
index d5d0848..0321c86 100644
--- a/pw_hdlc_lite/encoder_test.cc
+++ b/pw_hdlc_lite/encoder_test.cc
@@ -20,6 +20,7 @@
#include "gtest/gtest.h"
#include "pw_bytes/array.h"
+#include "pw_hdlc_lite_private/protocol.h"
#include "pw_stream/memory_stream.h"
using std::byte;
@@ -27,8 +28,6 @@
namespace pw::hdlc_lite {
namespace {
-constexpr byte kFlag = byte{0x7E};
-constexpr byte kEscape = byte{0x7D};
constexpr uint8_t kAddress = 0x7B; // 123
constexpr byte kControl = byte{0};
diff --git a/pw_hdlc_lite/public/pw_hdlc_lite/decoder.h b/pw_hdlc_lite/public/pw_hdlc_lite/decoder.h
index 5c03977..1b88bb5 100644
--- a/pw_hdlc_lite/public/pw_hdlc_lite/decoder.h
+++ b/pw_hdlc_lite/public/pw_hdlc_lite/decoder.h
@@ -17,6 +17,7 @@
#include <array>
#include <cstddef>
#include <cstring>
+#include <functional> // std::invoke
#include "pw_bytes/span.h"
#include "pw_result/result.h"
@@ -24,6 +25,45 @@
namespace pw::hdlc_lite {
+// Represents the contents of an HDLC frame -- the unescaped data between two
+// flag bytes. Instances of Frame are only created when a full, valid frame has
+// been read.
+//
+// For now, the Frame class assumes single-byte address and control fields and a
+// 32-bit frame check sequence (FCS).
+class Frame {
+ private:
+ static constexpr size_t kAddressSize = 1;
+ static constexpr size_t kControlSize = 1;
+ static constexpr size_t kFcsSize = sizeof(uint32_t);
+
+ public:
+ // The minimum size of a frame, excluding control bytes (flag or escape).
+ static constexpr size_t kMinSizeBytes =
+ kAddressSize + kControlSize + kFcsSize;
+
+ // Creates a Frame with the specified data. The data MUST be valid frame data
+ // with a verified frame check sequence.
+ explicit constexpr Frame(ConstByteSpan data) : frame_(data) {
+ // TODO(pwbug/246): Use PW_DASSERT when available.
+ // PW_DASSERT(data.size() >= kMinSizeBytes);
+ }
+
+ constexpr unsigned address() const {
+ return std::to_integer<unsigned>(frame_[0]);
+ }
+
+ constexpr std::byte control() const { return frame_[kAddressSize]; }
+
+ constexpr ConstByteSpan data() const {
+ return frame_.subspan(kAddressSize + kControlSize,
+ frame_.size() - kMinSizeBytes);
+ }
+
+ private:
+ ConstByteSpan frame_;
+};
+
// The Decoder class facilitates decoding of data frames using the HDLC-Lite
// protocol, by returning packets as they are decoded and storing incomplete
// data frames in a buffer.
@@ -34,80 +74,64 @@
class Decoder {
public:
constexpr Decoder(ByteSpan buffer)
- : frame_buffer_(buffer), state_(DecoderState::kNoPacket), size_(0) {}
+ : buffer_(buffer), current_frame_size_(0), state_(State::kInterFrame) {}
- // Parse a single byte of a HDLC stream. Returns a result object with the
- // complete packet if the latest byte finishes a frame, or a variety of
- // statuses in other cases, as follows:
- //
- // OK - If the end of the data-frame was found and the packet was decoded
- // successfully. The value of this Result<ConstByteSpan> object will
- // be the payload.
- // RESOURCE_EXHAUSTED - If the number of bytes added to the Decoder is
- // greater than the buffer size. This function call will clear the
- // decoder before returning.
- // UNAVAILABLE - If the byte has been successfully escaped and added to
- // the buffer, but we havent reached the end of the data-frame.
- // DATA_LOSS - If the CRC verification process fails after seeing the
- // ending Frame Delimiter byte (0x7E). Additionally happens when the
- // FCS is not in the payload or when the data-frame does not start
- // with the initial Frame Delimiter byte (0x7E).
- //
- Result<ConstByteSpan> AddByte(const std::byte b);
-
- // Returns the number of bytes of the active packet that are added to the
- // frame buffer.
- size_t size() const { return size_; }
-
- // Returns the maximum size of the Decoder's frame buffer.
- size_t max_size() const { return frame_buffer_.size(); }
-
- // Clears the frame buffer at the beginning of decoding the next packet.
- void clear() { size_ = 0; };
-
- // Indicates if the decoder is currently in the process of decoding a packet.
- bool IsPacketActive() { return state_ != DecoderState::kNoPacket; }
-
- private:
- // DecoderState enum class is used to make the Decoder a finite state machine.
- enum class DecoderState {
- kNoPacket,
- kPacketActive,
- kEscapeNextByte,
- };
-
- // Disallow Copy and Assign.
Decoder(const Decoder&) = delete;
-
Decoder& operator=(const Decoder&) = delete;
- // Will return true if the CRC is successfully verified.
- bool CheckCrc() const;
-
- // Attempts to write the escaped byte to the buffer and returns a Status
- // object accordingly:
+ // Parses a single byte of an HDLC stream. Returns a Result with the complete
+ // frame if the byte completes a frame. The status is the following:
//
- // RESOURCE_EXHAUSTED - If the buffer is out of space.
- // UNAVAILABLE - If the byte has been successfully added to the buffer.
+ // OK - A frame was successfully decoded. The Result contains the Frame,
+ // which is invalidated by the next Process call.
+ // UNAVAILABLE - No frame is available.
+ // RESOURCE_EXHAUSTED - A frame completed, but it was too large to fit in
+ // the decoder's buffer.
+ // DATA_LOSS - A frame completed, but it was invalid. The frame was
+ // incomplete or the frame check sequence verification failed.
//
- Status AddEscapedByte(std::byte new_byte);
+ Result<Frame> Process(std::byte b);
- // Ensures the packet is correctly decoded and returns a status object
- // indicating if the packet can be returned. The three checks it does are:
- // 1. Checks if there are packet meets the minimum size of a HDLC-Lite
- // packet.
- // 2. Checks that the frame buffer wasnt overflowed.
- // 3. Verifies if the CRC is correct
- // Will log errors accordingly.
- //
- Status PacketStatus() const;
+ // Calls the provided callback with each frame or error.
+ template <typename F, typename... Args>
+ void Process(ConstByteSpan data, F&& callback, Args&&... args) {
+ for (std::byte b : data) {
+ auto result = Process(b);
+ if (result.status() != Status::UNAVAILABLE) {
+ std::invoke(
+ std::forward<F>(callback), std::forward<Args>(args)..., result);
+ }
+ }
+ }
- ByteSpan frame_buffer_;
- DecoderState state_;
+ // Returns the maximum size of the Decoder's frame buffer.
+ size_t max_size() const { return buffer_.size(); }
- // The size_ variable represents the number of decoded bytes of the current
- // active packet.
- size_t size_;
+ // Clears and resets the decoder.
+ void clear() {
+ current_frame_size_ = 0;
+ state_ = State::kInterFrame;
+ };
+
+ private:
+ // State enum class is used to make the Decoder a finite state machine.
+ enum class State {
+ kInterFrame,
+ kFrame,
+ kFrameEscape,
+ };
+
+ void AppendByte(std::byte new_byte);
+
+ Status CheckFrame() const;
+
+ bool VerifyFrameCheckSequence() const;
+
+ const ByteSpan buffer_;
+
+ size_t current_frame_size_;
+
+ State state_;
};
// DecoderBuffers declare a buffer along with a Decoder.
@@ -121,6 +145,8 @@
static constexpr size_t max_size() { return size_bytes; }
private:
+ static_assert(size_bytes >= Frame::kMinSizeBytes);
+
std::array<std::byte, size_bytes> frame_buffer_;
};
diff --git a/pw_hdlc_lite/pw_hdlc_lite_private/protocol.h b/pw_hdlc_lite/pw_hdlc_lite_private/protocol.h
new file mode 100644
index 0000000..25159dbc
--- /dev/null
+++ b/pw_hdlc_lite/pw_hdlc_lite_private/protocol.h
@@ -0,0 +1,32 @@
+// Copyright 2020 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+#pragma once
+
+#include <cstddef>
+
+namespace pw::hdlc_lite {
+
+inline constexpr std::byte kFlag = std::byte{0x7E};
+inline constexpr std::byte kEscape = std::byte{0x7D};
+
+inline constexpr std::array<std::byte, 2> kEscapedFlag = {kEscape,
+ std::byte{0x5E}};
+inline constexpr std::array<std::byte, 2> kEscapedEscape = {kEscape,
+ std::byte{0x5D}};
+
+constexpr bool NeedsEscaping(std::byte b) {
+ return (b == kFlag || b == kEscape);
+}
+
+} // namespace pw::hdlc_lite
diff --git a/pw_hdlc_lite/py/decoder_test.py b/pw_hdlc_lite/py/decoder_test.py
index df801eb..ec1e52c 100755
--- a/pw_hdlc_lite/py/decoder_test.py
+++ b/pw_hdlc_lite/py/decoder_test.py
@@ -15,10 +15,13 @@
"""Contains the Python decoder tests and generates C++ decoder tests."""
from collections import defaultdict
-from typing import Any, Callable, Dict, List, NamedTuple, Tuple, Union
+from pathlib import Path
+from typing import Any, Callable, Dict, Iterator, List, NamedTuple, Union
+from typing import Optional, TextIO, Tuple
import unittest
+import sys
-from pw_hdlc_lite.decoder import FrameDecoder, FrameStatus, NO_ADDRESS
+from pw_hdlc_lite.decoder import Frame, FrameDecoder, FrameStatus, NO_ADDRESS
from pw_hdlc_lite.protocol import frame_check_sequence as fcs
@@ -54,7 +57,6 @@
TestCase = Tuple[bytes, List[Expected]]
TestCases = Tuple[Union[str, TestCase], ...]
-
TEST_CASES: TestCases = (
'Empty payload',
(_encode(0, 0, b''), [Expected(0, b'\0', b'')]),
@@ -137,6 +139,15 @@
[Expected(1, b'\2', b'', FrameStatus.INCOMPLETE)]),
(b'\x7e\1\2abcd1234\x7d\x7e',
[Expected(1, b'\2', b'abcd', FrameStatus.INCOMPLETE)]),
+ 'Inter-frame data is only escapes',
+ (b'\x7e\x7d\x7e\x7d\x7e', [
+ Expected(NO_ADDRESS, b'', b'', FrameStatus.INCOMPLETE),
+ Expected(NO_ADDRESS, b'', b'', FrameStatus.INCOMPLETE),
+ ]),
+ (b'\x7e\x7d\x7d\x7e\x7d\x7d\x7e', [
+ Expected(NO_ADDRESS, b'', b'', FrameStatus.INVALID_ESCAPE),
+ Expected(NO_ADDRESS, b'', b'', FrameStatus.INVALID_ESCAPE),
+ ]),
'Data before first flag',
(b'\0\1' + fcs(b'\0\1'), []),
(b'\0\1' + fcs(b'\0\1') + b'\x7e',
@@ -161,6 +172,129 @@
return cases
+def _expected(frames: List[Frame]) -> Iterator[str]:
+ for i, frame in enumerate(frames, 1):
+ if frame.ok():
+ yield f' Frame(kDecodedFrame{i:02}),'
+ else:
+ yield f' Status::DATA_LOSS, // Frame {i}'
+
+
+_CPP_HEADER = f"""\
+// Copyright 2020 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+
+// AUTOGENERATED by {Path(__file__).name} - DO NOT EDIT
+
+#include "pw_hdlc_lite/decoder.h"
+
+#include <array>
+#include <cstddef>
+#include <variant>
+
+#include "gtest/gtest.h"
+#include "pw_bytes/array.h"
+
+namespace pw::hdlc_lite {{
+namespace {{
+
+// clang-format off
+"""
+
+_CPP_FOOTER = """\
+// clang-format on
+
+} // namespace
+} // namespace pw::hdlc_lite
+"""
+
+
+def _cpp_test(group: str, count: Optional[int], data: bytes) -> Iterator[str]:
+ """Generates a C++ test for the provided test data."""
+ frames = list(FrameDecoder().process(data))
+ data_bytes = ''.join(rf'\x{byte:02x}' for byte in data)
+
+ name = ''.join(w.capitalize() for w in group.replace('-', ' ').split(' '))
+ name = ''.join(c if c.isalnum() else '_' for c in name)
+ name = name if count is None else name + f'_{count}'
+
+ yield f'TEST(Decoder, {name}) {{'
+ yield f' static constexpr auto kData = bytes::String("{data_bytes}");\n'
+
+ for i, frame in enumerate(frames, 1):
+ if frame.status is FrameStatus.OK:
+ frame_bytes = ''.join(rf'\x{byte:02x}' for byte in frame.raw)
+ yield (f' static constexpr auto kDecodedFrame{i:02} = '
+ f'bytes::String("{frame_bytes}");')
+ else:
+ yield f' // Frame {i}: {frame.status.value}'
+
+ yield ''
+
+ expected = '\n'.join(_expected(frames)) or ' // No frames'
+ decoder_size = max(len(data), 8) # Make sure large enough for a frame
+
+ yield f"""\
+ DecoderBuffer<{decoder_size}> decoder;
+
+ static constexpr std::array<std::variant<Frame, Status>, {len(frames)}> kExpected = {{
+{expected}
+ }};
+
+ size_t decoded_frames = 0;
+
+ decoder.Process(kData, [&](const Result<Frame>& result) {{
+ ASSERT_LT(decoded_frames++, kExpected.size());
+ auto& expected = kExpected[decoded_frames - 1];
+
+ if (std::holds_alternative<Status>(expected)) {{
+ EXPECT_EQ(Status::DATA_LOSS, result.status());
+ }} else {{
+ ASSERT_EQ(Status::OK, result.status());
+
+ const Frame& decoded_frame = result.value();
+ const Frame& expected_frame = std::get<Frame>(expected);
+ EXPECT_EQ(expected_frame.address(), decoded_frame.address());
+ EXPECT_EQ(expected_frame.control(), decoded_frame.control());
+ ASSERT_EQ(expected_frame.data().size(), decoded_frame.data().size());
+ EXPECT_EQ(std::memcmp(expected_frame.data().data(),
+ decoded_frame.data().data(),
+ expected_frame.data().size()),
+ 0);
+ }}
+ }});
+
+ EXPECT_EQ(decoded_frames, kExpected.size());
+}}"""
+
+
+def _define_cc_tests(test_cases: TestCases, output: TextIO) -> None:
+ """Writes C++ tests for all test cases."""
+ output.write(_CPP_HEADER)
+
+ for group, test_list in _sort_test_cases(test_cases).items():
+ for i, (data, _) in enumerate(test_list, 1):
+ count = i if len(test_list) > 1 else None
+ for line in _cpp_test(group, count, data):
+ output.write(line)
+ output.write('\n')
+
+ output.write('\n')
+
+ output.write(_CPP_FOOTER)
+
+
def _define_py_test(group: str,
data: bytes,
expected_frames: List[Expected],
@@ -205,4 +339,9 @@
DecoderTest = type('DecoderTest', (unittest.TestCase, ), _define_py_tests())
if __name__ == '__main__':
- unittest.main()
+ # If --generate-cc-decode-test is provided, generate the C++ test file.
+ if len(sys.argv) >= 2 and sys.argv[1] == '--generate-cc-decode-test':
+ with Path(sys.argv[2]).open('w') as file:
+ _define_cc_tests(TEST_CASES, file)
+ else: # Otherwise, run the unit tests.
+ unittest.main()
