From 1cab1b440ae6c3412772a7f4e00c1e569b9834c6 Mon Sep 17 00:00:00 2001 From: Armando Montanez Date: Fri, 15 Jul 2022 01:11:19 +0000 Subject: [PATCH] pw_hdlc: Helpers for buffer sizing Introduces helpers for sizing buffers that account for HDLC encoding overhead. Change-Id: Idfd153a836fedf3835e8961eb168067eee6db2f4 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/101500 Commit-Queue: Auto-Submit Reviewed-by: Wyatt Hepler Pigweed-Auto-Submit: Armando Montanez --- pw_hdlc/BUILD.bazel | 14 + pw_hdlc/BUILD.gn | 25 ++ pw_hdlc/decoder.cc | 2 +- pw_hdlc/docs.rst | 42 +++ pw_hdlc/encoded_size_test.cc | 313 +++++++++++++++++++++ pw_hdlc/encoder.cc | 15 +- pw_hdlc/encoder_test.cc | 35 +-- pw_hdlc/public/pw_hdlc/decoder.h | 26 +- pw_hdlc/public/pw_hdlc/encoded_size.h | 91 ++++++ pw_hdlc/public/pw_hdlc/encoder.h | 4 +- pw_hdlc/public/pw_hdlc/internal/encoder.h | 9 +- pw_hdlc/public/pw_hdlc/internal/protocol.h | 31 ++ pw_hdlc/rpc_channel_test.cc | 6 +- pw_hdlc/wire_packet_parser.cc | 2 +- 14 files changed, 549 insertions(+), 66 deletions(-) create mode 100644 pw_hdlc/encoded_size_test.cc create mode 100644 pw_hdlc/public/pw_hdlc/encoded_size.h diff --git a/pw_hdlc/BUILD.bazel b/pw_hdlc/BUILD.bazel index 99d2031f2b..9c859a00b6 100644 --- a/pw_hdlc/BUILD.bazel +++ b/pw_hdlc/BUILD.bazel @@ -31,6 +31,7 @@ pw_cc_library( ], hdrs = [ "public/pw_hdlc/decoder.h", + "public/pw_hdlc/encoded_size.h", "public/pw_hdlc/encoder.h", ], includes = ["public"], @@ -102,6 +103,19 @@ cc_test( ], ) +cc_test( + name = "encoded_size_test", + srcs = ["encoded_size_test.cc"], + deps = [ + ":pw_hdlc", + "//pw_bytes", + "//pw_result", + "//pw_stream", + "//pw_unit_test", + "//pw_varint", + ], +) + cc_test( name = "wire_packet_parser_test", srcs = ["wire_packet_parser_test.cc"], diff --git a/pw_hdlc/BUILD.gn b/pw_hdlc/BUILD.gn index f1a2e51832..d1cf35c1e7 100644 --- a/pw_hdlc/BUILD.gn +++ b/pw_hdlc/BUILD.gn @@ -26,6 +26,7 @@ config("default_config") { group("pw_hdlc") { public_deps = [ ":decoder", + ":encoded_size", ":encoder", ] } @@ -37,6 +38,17 @@ pw_source_set("common") { visibility = [ ":*" ] } +pw_source_set("encoded_size") { + public_configs = [ ":default_config" ] + public = [ "public/pw_hdlc/encoded_size.h" ] + public_deps = [ + ":common", + "$dir_pw_bytes", + "$dir_pw_span", + "$dir_pw_varint", + ] +} + pw_source_set("decoder") { public_configs = [ ":default_config" ] public = [ "public/pw_hdlc/decoder.h" ] @@ -68,6 +80,7 @@ pw_source_set("encoder") { dir_pw_status, dir_pw_stream, ] + deps = [ ":encoded_size" ] friend = [ ":*" ] } @@ -111,6 +124,7 @@ pw_test_group("tests") { ":encoder_test", ":decoder_test", ":rpc_channel_test", + ":encoded_size_test", ":wire_packet_parser_test", ] group_deps = [ @@ -120,6 +134,17 @@ pw_test_group("tests") { ] } +pw_test("encoded_size_test") { + deps = [ + ":pw_hdlc", + "$dir_pw_bytes", + "$dir_pw_result", + "$dir_pw_stream", + "$dir_pw_varint", + ] + sources = [ "encoded_size_test.cc" ] +} + pw_test("encoder_test") { deps = [ ":pw_hdlc" ] sources = [ "encoder_test.cc" ] diff --git a/pw_hdlc/decoder.cc b/pw_hdlc/decoder.cc index e8296a09e5..82452b2fb1 100644 --- a/pw_hdlc/decoder.cc +++ b/pw_hdlc/decoder.cc @@ -123,7 +123,7 @@ Status Decoder::CheckFrame() const { return Status::Unavailable(); } - if (current_frame_size_ < Frame::kMinSizeBytes) { + if (current_frame_size_ < Frame::kMinContentSizeBytes) { PW_LOG_ERROR("Received %lu-byte frame; frame must be at least 6 bytes", static_cast(current_frame_size_)); return Status::DataLoss(); diff --git a/pw_hdlc/docs.rst b/pw_hdlc/docs.rst index 3004891a30..90d1dc0d74 100644 --- a/pw_hdlc/docs.rst +++ b/pw_hdlc/docs.rst @@ -239,6 +239,48 @@ Decodes one or more HDLC frames from a stream of data. :param Uint8Array data: bytes to be decoded. :yields: Valid HDLC frames, logging any errors. +Allocating buffers +------------------ +Since HDLC's encoding overhead changes with payload size and what data is being +encoded, this module provides helper functions that are useful for determining +the size of buffers by providing worst-case sizes of frames given a certain +payload size and vice-versa. + +.. code-block:: cpp + + #include "pw_assert/check.h" + #include "pw_bytes/span.h" + #include "pw_hdlc/encoder" + #include "pw_hdlc/encoded_size.h" + #include "pw_status/status.h" + + // The max on-the-wire size in bytes of a single HDLC frame after encoding. + constexpr size_t kMtu = 512; + constexpr size_t kRpcEncodeBufferSize = pw::hdlc::MaxSafePayloadSize(kMtu); + std::array rpc_encode_buffer; + + // Any data encoded to this buffer is guaranteed to fit in the MTU after + // HDLC encoding. + pw::ConstByteSpan GetRpcEncodeBuffer() { + return rpc_encode_buffer; + } + +The HDLC ``Decoder`` has its own helper for allocating a buffer since it doesn't +need the entire escaped frame in-memory to decode, and therefore has slightly +lower overhead. + +.. code-block:: cpp + + #include "pw_hdlc/decoder.h" + + // The max on-the-wire size in bytes of a single HDLC frame after encoding. + constexpr size_t kMtu = 512; + + // Create a decoder given the MTU constraint. + constexpr size_t kDecoderBufferSize = + pw::hdlc::Decoder::RequiredBufferSizeForFrameSize(kMtu); + pw::hdlc::DecoderBuffer decoder; + Additional features =================== diff --git a/pw_hdlc/encoded_size_test.cc b/pw_hdlc/encoded_size_test.cc new file mode 100644 index 0000000000..6dde795027 --- /dev/null +++ b/pw_hdlc/encoded_size_test.cc @@ -0,0 +1,313 @@ +// Copyright 2022 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. + +#include "pw_hdlc/encoded_size.h" + +#include +#include +#include + +#include "gtest/gtest.h" +#include "pw_bytes/array.h" +#include "pw_hdlc/decoder.h" +#include "pw_hdlc/encoder.h" +#include "pw_hdlc/internal/encoder.h" +#include "pw_result/result.h" +#include "pw_stream/memory_stream.h" +#include "pw_varint/varint.h" + +namespace pw::hdlc { +namespace { + +// The varint-encoded address that represents the value that will result in the +// largest on-the-wire address after HDLC escaping. +constexpr auto kWidestVarintAddress = + bytes::String("\x7e\x7e\x7e\x7e\x7e\x7e\x7e\x7e\x7e\x03"); + +// This is the decoded varint value of kWidestVarintAddress. This is +// pre-calculated as a constant to simplify tests. +constexpr uint64_t kWidestAddress = 0xbf7efdfbf7efdfbf; + +// UI frames created by WriteUIFrame() will never be have an escaped control +// field, but it's technically possible for other HDLC frame types to produce +// control bytes that would need to be escaped. +constexpr size_t kEscapedControlCost = kControlSize; + +// UI frames created by WriteUIFrame() will never have an escaped control +// field, but it's technically possible for other HDLC frame types to produce +// control bytes that would need to be escaped. +constexpr size_t kEscapedFcsCost = kMaxEscapedFcsSize - kFcsSize; + +// Due to API limitations, the worst case buffer calculations used by the HDLC +// encoder/decoder can't be fully saturated. This constexpr value accounts for +// this by expressing the delta between the constant largest testable HDLC frame +// and the calculated worst-case-scenario. +constexpr size_t kTestLimitationsOverhead = + kEscapedControlCost + kEscapedFcsCost; + +// A payload only containing bytes that need to be escaped. +constexpr auto kFullyEscapedPayload = + bytes::String("\x7e\x7e\x7e\x7e\x7e\x7e\x7e\x7e"); + +constexpr uint8_t kEscapeAddress = static_cast(kFlag); +constexpr uint8_t kNoEscapeAddress = 6; + +TEST(EncodedSize, Constants_WidestAddress) { + uint64_t address = 0; + size_t address_size = + varint::Decode(kWidestVarintAddress, &address, kAddressFormat); + EXPECT_EQ(address_size, 10u); + EXPECT_EQ(address_size, kMaxAddressSize); + EXPECT_EQ(kMaxEscapedVarintAddressSize, 19u); + EXPECT_EQ(EscapedSize(kWidestVarintAddress), kMaxEscapedVarintAddressSize); + EXPECT_EQ(address, kWidestAddress); + EXPECT_EQ(varint::EncodedSize(kWidestAddress), 10u); +} + +TEST(EncodedSize, EscapedSize_AllEscapeBytes) { + EXPECT_EQ(EscapedSize(kFullyEscapedPayload), kFullyEscapedPayload.size() * 2); +} + +TEST(EncodedSize, EscapedSize_NoEscapeBytes) { + constexpr auto kData = bytes::String("\x01\x23\x45\x67\x89\xab\xcd\xef"); + EXPECT_EQ(EscapedSize(kData), kData.size()); +} + +TEST(EncodedSize, EscapedSize_SomeEscapeBytes) { + constexpr auto kData = bytes::String("\x7epabu\x7d"); + EXPECT_EQ(EscapedSize(kData), kData.size() + 2); +} + +TEST(EncodedSize, EscapedSize_Address) { + EXPECT_EQ(EscapedSize(kWidestVarintAddress), + varint::EncodedSize(kWidestAddress) * 2 - 1); +} + +TEST(EncodedSize, MaxEncodedSize_Overload) { + EXPECT_EQ(MaxEncodedFrameSize(kFullyEscapedPayload.size()), + MaxEncodedFrameSize(kWidestAddress, kFullyEscapedPayload)); +} + +TEST(EncodedSize, MaxEncodedSize_EmptyPayload) { + EXPECT_EQ(14u, MaxEncodedFrameSize(kNoEscapeAddress, {})); + EXPECT_EQ(14u, MaxEncodedFrameSize(kEscapeAddress, {})); +} + +TEST(EncodedSize, MaxEncodedSize_PayloadWithoutEscapes) { + constexpr auto data = bytes::Array<0x00, 0x01, 0x02, 0x03>(); + EXPECT_EQ(18u, MaxEncodedFrameSize(kNoEscapeAddress, data)); + EXPECT_EQ(18u, MaxEncodedFrameSize(kEscapeAddress, data)); +} + +TEST(EncodedSize, MaxEncodedSize_PayloadWithOneEscape) { + constexpr auto data = bytes::Array<0x00, 0x01, 0x7e, 0x03>(); + EXPECT_EQ(19u, MaxEncodedFrameSize(kNoEscapeAddress, data)); + EXPECT_EQ(19u, MaxEncodedFrameSize(kEscapeAddress, data)); +} + +TEST(EncodedSize, MaxEncodedSize_PayloadWithAllEscapes) { + constexpr auto data = bytes::Initialized<8>(0x7e); + EXPECT_EQ(30u, MaxEncodedFrameSize(kNoEscapeAddress, data)); + EXPECT_EQ(30u, MaxEncodedFrameSize(kEscapeAddress, data)); +} + +TEST(EncodedSize, MaxPayload_UndersizedFrame) { + EXPECT_EQ(MaxSafePayloadSize(4), 0u); +} + +TEST(EncodedSize, MaxPayload_SmallFrame) { + EXPECT_EQ(MaxSafePayloadSize(128), 48u); +} + +TEST(EncodedSize, MaxPayload_MediumFrame) { + EXPECT_EQ(MaxSafePayloadSize(512), 240u); +} + +TEST(EncodedSize, FrameToPayloadInversion_Odd) { + static constexpr size_t kIntendedPayloadSize = 1234567891; + EXPECT_EQ(MaxSafePayloadSize(MaxEncodedFrameSize(kIntendedPayloadSize)), + kIntendedPayloadSize); +} + +TEST(EncodedSize, PayloadToFrameInversion_Odd) { + static constexpr size_t kIntendedFrameSize = 1234567891; + EXPECT_EQ(MaxEncodedFrameSize(MaxSafePayloadSize(kIntendedFrameSize)), + kIntendedFrameSize); +} + +TEST(EncodedSize, FrameToPayloadInversion_Even) { + static constexpr size_t kIntendedPayloadSize = 42; + EXPECT_EQ(MaxSafePayloadSize(MaxEncodedFrameSize(kIntendedPayloadSize)), + kIntendedPayloadSize); +} + +TEST(EncodedSize, PayloadToFrameInversion_Even) { + static constexpr size_t kIntendedFrameSize = 42; + // Because of HDLC encoding overhead requirements, the last byte of the + // intended frame size is wasted because it doesn't allow sufficient space for + // another byte since said additional byte could require escaping, therefore + // requiring a second byte to increase the safe payload size by one. + const size_t max_frame_usage = + MaxEncodedFrameSize(MaxSafePayloadSize(kIntendedFrameSize)); + EXPECT_EQ(max_frame_usage, kIntendedFrameSize - 1); + + // There's no further change if the inversion is done again since the frame + // size is aligned to the reduced bounds. + EXPECT_EQ(MaxEncodedFrameSize(MaxSafePayloadSize(max_frame_usage)), + kIntendedFrameSize - 1); +} + +TEST(EncodedSize, MostlyEscaped) { + constexpr auto kMostlyEscapedPayload = + bytes::String(":)\x7e\x7e\x7e\x7e\x7e\x7e\x7e\x7e"); + constexpr size_t kUnescapedBytes = + 2 * kMostlyEscapedPayload.size() - EscapedSize(kMostlyEscapedPayload); + // Subtracting 2 should still leave enough space since two bytes won't need + // to be escaped. + constexpr size_t kExpectedMaxFrameSize = + MaxEncodedFrameSize(kMostlyEscapedPayload.size()) - kUnescapedBytes; + std::array dest_buffer; + stream::MemoryWriter writer(dest_buffer); + EXPECT_EQ(kUnescapedBytes, 2u); + EXPECT_EQ(OkStatus(), + WriteUIFrame(kWidestAddress, kFullyEscapedPayload, writer)); + EXPECT_EQ(writer.size(), + kExpectedMaxFrameSize - kTestLimitationsOverhead - kUnescapedBytes); +} + +TEST(EncodedSize, BigAddress_SaturatedPayload) { + constexpr size_t kExpectedMaxFrameSize = + MaxEncodedFrameSize(kFullyEscapedPayload.size()); + std::array dest_buffer; + stream::MemoryWriter writer(dest_buffer); + EXPECT_EQ(OkStatus(), + WriteUIFrame(kWidestAddress, kFullyEscapedPayload, writer)); + EXPECT_EQ(writer.size(), kExpectedMaxFrameSize - kTestLimitationsOverhead); +} + +TEST(EncodedSize, BigAddress_OffByOne) { + constexpr size_t kExpectedMaxFrameSize = + MaxEncodedFrameSize(kFullyEscapedPayload.size()) - 1; + std::array dest_buffer; + stream::MemoryWriter writer(dest_buffer); + EXPECT_EQ(Status::ResourceExhausted(), + WriteUIFrame(kWidestAddress, kFullyEscapedPayload, writer)); +} + +TEST(EncodedSize, SmallAddress_SaturatedPayload) { + constexpr auto kSmallerEscapedAddress = bytes::String("\x7e\x7d"); + // varint::Decode() is not constexpr, so this is a hard-coded and then runtime + // validated. + constexpr size_t kVarintDecodedAddress = 7999; + constexpr size_t kExpectedMaxFrameSize = + MaxEncodedFrameSize(kVarintDecodedAddress, kFullyEscapedPayload); + std::array dest_buffer; + stream::MemoryWriter writer(dest_buffer); + + uint64_t address = 0; + size_t address_size = + varint::Decode(kSmallerEscapedAddress, &address, kAddressFormat); + EXPECT_EQ(address, kVarintDecodedAddress); + EXPECT_EQ(address_size, 2u); + + EXPECT_EQ(OkStatus(), WriteUIFrame(address, kFullyEscapedPayload, writer)); + EXPECT_EQ(writer.size(), kExpectedMaxFrameSize - kTestLimitationsOverhead); +} + +TEST(EncodedSize, SmallAddress_OffByOne) { + constexpr auto kSmallerEscapedAddress = bytes::String("\x7e\x7d"); + // varint::Decode() is not constexpr, so this is a hard-coded and then runtime + // validated. + constexpr size_t kVarintDecodedAddress = 7999; + constexpr size_t kExpectedMaxFrameSize = + MaxEncodedFrameSize(kVarintDecodedAddress, kFullyEscapedPayload); + std::array dest_buffer; + stream::MemoryWriter writer(dest_buffer); + + uint64_t address = 0; + size_t address_size = + varint::Decode(kSmallerEscapedAddress, &address, kAddressFormat); + EXPECT_EQ(address, kVarintDecodedAddress); + EXPECT_EQ(address_size, 2u); + + EXPECT_EQ(Status::ResourceExhausted(), + WriteUIFrame(address, kFullyEscapedPayload, writer)); +} + +TEST(DecodedSize, BigAddress_SaturatedPayload) { + constexpr auto kNoEscapePayload = + bytes::String("The decoder needs the most space when there's no escapes"); + constexpr size_t kExpectedMaxFrameSize = + MaxEncodedFrameSize(kNoEscapePayload.size()); + std::array dest_buffer; + stream::MemoryWriter writer(dest_buffer); + EXPECT_EQ(OkStatus(), + WriteUIFrame(kNoEscapeAddress, kNoEscapePayload, writer)); + + // Allocate at least enough real buffer space. + constexpr size_t kDecoderBufferSize = + Decoder::RequiredBufferSizeForFrameSize(kExpectedMaxFrameSize); + std::array buffer; + + // Pretend the supported frame size is whatever the final size of the encoded + // frame was. + const size_t max_frame_size = + Decoder::RequiredBufferSizeForFrameSize(writer.size()); + + Decoder decoder(ByteSpan(buffer).first(max_frame_size)); + for (const std::byte b : writer.WrittenData()) { + Result frame = decoder.Process(b); + if (frame.ok()) { + EXPECT_EQ(frame->address(), kNoEscapeAddress); + EXPECT_EQ(frame->data().size(), kNoEscapePayload.size()); + EXPECT_TRUE(std::memcmp(frame->data().begin(), + kNoEscapePayload.begin(), + kNoEscapePayload.size()) == 0); + } + } +} + +TEST(DecodedSize, BigAddress_OffByOne) { + constexpr auto kNoEscapePayload = + bytes::String("The decoder needs the most space when there's no escapes"); + constexpr size_t kExpectedMaxFrameSize = + MaxEncodedFrameSize(kNoEscapePayload.size()); + std::array dest_buffer; + stream::MemoryWriter writer(dest_buffer); + EXPECT_EQ(OkStatus(), + WriteUIFrame(kNoEscapeAddress, kNoEscapePayload, writer)); + + // Allocate at least enough real buffer space. + constexpr size_t kDecoderBufferSize = + Decoder::RequiredBufferSizeForFrameSize(kExpectedMaxFrameSize); + std::array buffer; + + // Pretend the supported frame size is whatever the final size of the encoded + // frame was. + const size_t max_frame_size = + Decoder::RequiredBufferSizeForFrameSize(writer.size()); + + Decoder decoder(ByteSpan(buffer).first(max_frame_size - 1)); + for (size_t i = 0; i < writer.size(); i++) { + Result frame = decoder.Process(writer[i]); + if (i < writer.size() - 1) { + EXPECT_EQ(frame.status(), Status::Unavailable()); + } else { + EXPECT_EQ(frame.status(), Status::ResourceExhausted()); + } + } +} + +} // namespace +} // namespace pw::hdlc diff --git a/pw_hdlc/encoder.cc b/pw_hdlc/encoder.cc index 84b1811d0d..4361255477 100644 --- a/pw_hdlc/encoder.cc +++ b/pw_hdlc/encoder.cc @@ -20,6 +20,7 @@ #include #include "pw_bytes/endian.h" +#include "pw_hdlc/encoded_size.h" #include "pw_hdlc/internal/encoder.h" #include "pw_span/span.h" #include "pw_varint/varint.h" @@ -67,17 +68,6 @@ Status Encoder::FinishFrame() { return writer_.Write(kFlag); } -size_t Encoder::MaxEncodedSize(uint64_t address, ConstByteSpan payload) { - constexpr size_t kFcsMaxSize = 8; // Worst case FCS: 0x7e7e7e7e. - size_t max_encoded_address_size = varint::EncodedSize(address) * 2; - size_t encoded_payload_size = - payload.size() + - std::count_if(payload.begin(), payload.end(), NeedsEscaping); - - return max_encoded_address_size + sizeof(kUnusedControl) + - encoded_payload_size + kFcsMaxSize; -} - Status Encoder::StartFrame(uint64_t address, std::byte control) { fcs_.clear(); if (Status status = writer_.Write(kFlag); !status.ok()) { @@ -100,8 +90,7 @@ Status Encoder::StartFrame(uint64_t address, std::byte control) { Status WriteUIFrame(uint64_t address, ConstByteSpan payload, stream::Writer& writer) { - if (internal::Encoder::MaxEncodedSize(address, payload) > - writer.ConservativeWriteLimit()) { + if (MaxEncodedFrameSize(address, payload) > writer.ConservativeWriteLimit()) { return Status::ResourceExhausted(); } diff --git a/pw_hdlc/encoder_test.cc b/pw_hdlc/encoder_test.cc index 11eabeee99..d428433916 100644 --- a/pw_hdlc/encoder_test.cc +++ b/pw_hdlc/encoder_test.cc @@ -20,6 +20,7 @@ #include "gtest/gtest.h" #include "pw_bytes/array.h" +#include "pw_hdlc/encoded_size.h" #include "pw_hdlc/internal/encoder.h" #include "pw_hdlc/internal/protocol.h" #include "pw_stream/memory_stream.h" @@ -47,7 +48,8 @@ class WriteUnnumberedFrame : public ::testing::Test { WriteUnnumberedFrame() : writer_(buffer_) {} stream::MemoryWriter writer_; - std::array buffer_; + // Allocate a buffer that will fit any 7-byte payload. + std::array buffer_; }; constexpr byte kUnnumberedControl = byte{0x3}; @@ -193,35 +195,4 @@ TEST(WriteUIFrame, WriterError) { } } // namespace - -namespace internal { -namespace { - -constexpr uint8_t kEscapeAddress = 0x7d; - -TEST(Encoder, MaxEncodedSize_EmptyPayload) { - EXPECT_EQ(11u, Encoder::MaxEncodedSize(kAddress, {})); - EXPECT_EQ(11u, Encoder::MaxEncodedSize(kEscapeAddress, {})); -} - -TEST(Encoder, MaxEncodedSize_PayloadWithoutEscapes) { - constexpr auto data = bytes::Array<0x00, 0x01, 0x02, 0x03>(); - EXPECT_EQ(15u, Encoder::MaxEncodedSize(kAddress, data)); - EXPECT_EQ(15u, Encoder::MaxEncodedSize(kEscapeAddress, data)); -} - -TEST(Encoder, MaxEncodedSize_PayloadWithOneEscape) { - constexpr auto data = bytes::Array<0x00, 0x01, 0x7e, 0x03>(); - EXPECT_EQ(16u, Encoder::MaxEncodedSize(kAddress, data)); - EXPECT_EQ(16u, Encoder::MaxEncodedSize(kEscapeAddress, data)); -} - -TEST(Encoder, MaxEncodedSize_PayloadWithAllEscapes) { - constexpr auto data = bytes::Initialized<8>(0x7e); - EXPECT_EQ(27u, Encoder::MaxEncodedSize(kAddress, data)); - EXPECT_EQ(27u, Encoder::MaxEncodedSize(kEscapeAddress, data)); -} - -} // namespace -} // namespace internal } // namespace pw::hdlc diff --git a/pw_hdlc/public/pw_hdlc/decoder.h b/pw_hdlc/public/pw_hdlc/decoder.h index 0c6e14ef97..0290a2ad87 100644 --- a/pw_hdlc/public/pw_hdlc/decoder.h +++ b/pw_hdlc/public/pw_hdlc/decoder.h @@ -22,6 +22,7 @@ #include "pw_assert/assert.h" #include "pw_bytes/span.h" #include "pw_checksum/crc32.h" +#include "pw_hdlc/internal/protocol.h" #include "pw_result/result.h" #include "pw_status/status.h" @@ -30,19 +31,11 @@ namespace pw::hdlc { // 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 a single-byte control field and a 32-bit -// frame check sequence (FCS). class Frame { - private: - static constexpr size_t kMinimumAddressSize = 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 = - kMinimumAddressSize + kControlSize + kFcsSize; + static constexpr size_t kMinContentSizeBytes = + kMinAddressSize + kControlSize + kFcsSize; static Result Parse(ConstByteSpan frame); @@ -95,6 +88,17 @@ class Decoder { // Result Process(std::byte new_byte); + // Returns the buffer space required for a `Decoder` to successfully decode a + // frame whose on-the-wire HDLC encoded size does not exceed `max_frame_size`. + static constexpr size_t RequiredBufferSizeForFrameSize( + size_t max_frame_size) { + // Flag bytes aren't stored in the internal buffer, so we can save a couple + // bytes. + return max_frame_size < Frame::kMinContentSizeBytes + ? Frame::kMinContentSizeBytes + : max_frame_size - 2; + } + // Processes a span of data and calls the provided callback with each frame or // error. template @@ -165,7 +169,7 @@ class DecoderBuffer : public Decoder { static constexpr size_t max_size() { return kSizeBytes; } private: - static_assert(kSizeBytes >= Frame::kMinSizeBytes); + static_assert(kSizeBytes >= Frame::kMinContentSizeBytes); std::array frame_buffer_; }; diff --git a/pw_hdlc/public/pw_hdlc/encoded_size.h b/pw_hdlc/public/pw_hdlc/encoded_size.h new file mode 100644 index 0000000000..ed51cca301 --- /dev/null +++ b/pw_hdlc/public/pw_hdlc/encoded_size.h @@ -0,0 +1,91 @@ +// Copyright 2022 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 +#include +#include + +#include "pw_bytes/span.h" +#include "pw_hdlc/internal/protocol.h" +#include "pw_span/span.h" +#include "pw_varint/varint.h" + +namespace pw::hdlc { + +// Calculates the size of a series of bytes after HDLC escaping. +constexpr size_t EscapedSize(ConstByteSpan data) { + size_t count = 0; + for (std::byte b : data) { + count += NeedsEscaping(b) ? 2 : 1; + } + return count; +} + +template ::value>> +constexpr size_t EscapedSize(T val) { + return EscapedSize(as_bytes(span(&val, 1))); +} + +// Calculate the buffer space required for encoding an HDLC frame with a given +// payload size. This uses worst-case escaping cost as part of the calculation, +// which is extremely expensive but guarantees the payload will always fit in +// the frame AND the value can be evaluated at compile-time. +// +// This is NOT a perfect inverse of MaxSafePayloadSize()! This is because +// increasing the frame size by one doesn't mean another payload byte can safely +// fit since it might need to be escaped. +constexpr size_t MaxEncodedFrameSize(size_t max_payload_size) { + return 2 * sizeof(kFlag) + kMaxEscapedVarintAddressSize + kMaxEscapedFcsSize + + kMaxEscapedControlSize + max_payload_size * 2; +} + +// Calculates a maximum the on-the-wire encoded size for a given payload +// destined for the provided address. Because payload escaping and some of the +// address is accounted for, there's significantly less wasted space when +// compared to MaxEncodedFrameSize(). However, because the checksum, address, +// and control fields are not precisely calculated, there's up to 17 bytes of +// potentially unused overhead left over by this estimation. This is done to +// improve the speed of this calculation, since precisely calculating all of +// this information isn't nearly as efficient. +constexpr size_t MaxEncodedFrameSize(uint64_t address, ConstByteSpan payload) { + // The largest on-the-wire escaped varint will never exceed + // kMaxEscapedVarintAddressSize since the 10th varint byte can never be an + // byte that needs escaping. + const size_t max_encoded_address_size = + std::min(varint::EncodedSize(address) * 2, kMaxEscapedVarintAddressSize); + return 2 * sizeof(kFlag) + max_encoded_address_size + kMaxEscapedFcsSize + + kMaxEscapedControlSize + EscapedSize(payload); +} + +// Calculates the maximum safe payload size of an HDLC-encoded frame. As long as +// a payload does not exceed this value, it will always be safe to encode it +// as an HDLC frame in a buffer of size max_frame_size. +// +// When max_frame_size is too small to safely fit any payload data, this +// function returns zero. +// +// This is NOT a perfect inverse of MaxEncodedFrameSize()! This is because +// increasing the frame size by one doesn't mean another payload byte can safely +// fit since it might need to be escaped. +constexpr size_t MaxSafePayloadSize(size_t max_frame_size) { + constexpr size_t kMaxConstantOverhead = + 2 * sizeof(kFlag) + kMaxEscapedVarintAddressSize + kMaxEscapedFcsSize + + kMaxEscapedControlSize; + return max_frame_size < kMaxConstantOverhead + ? 0 + : (max_frame_size - kMaxConstantOverhead) / 2; +} + +} // namespace pw::hdlc diff --git a/pw_hdlc/public/pw_hdlc/encoder.h b/pw_hdlc/public/pw_hdlc/encoder.h index 8dcf66fb7b..8cd58379a4 100644 --- a/pw_hdlc/public/pw_hdlc/encoder.h +++ b/pw_hdlc/public/pw_hdlc/encoder.h @@ -23,10 +23,10 @@ namespace pw::hdlc { // writer. The complete frame contains the following: // // - HDLC flag byte (0x7e) -// - Address +// - Address (variable length, up to 10 bytes) // - UI-frame control (metadata) byte // - Payload (0 or more bytes) -// - Frame check sequence (CRC-32) +// - Frame check sequence (CRC-32, 4 bytes) // - HDLC flag byte (0x7e) // Status WriteUIFrame(uint64_t address, diff --git a/pw_hdlc/public/pw_hdlc/internal/encoder.h b/pw_hdlc/public/pw_hdlc/internal/encoder.h index 2f3b4b0a66..9fce722552 100644 --- a/pw_hdlc/public/pw_hdlc/internal/encoder.h +++ b/pw_hdlc/public/pw_hdlc/internal/encoder.h @@ -13,8 +13,13 @@ // the License. #pragma once +#include +#include + +#include "pw_bytes/span.h" #include "pw_checksum/crc32.h" #include "pw_hdlc/internal/protocol.h" +#include "pw_status/status.h" #include "pw_stream/stream.h" namespace pw::hdlc::internal { @@ -43,10 +48,6 @@ class Encoder { // Finishes a frame. Writes the frame check sequence and a terminating flag. Status FinishFrame(); - // Runs a pass through a payload, returning the worst-case encoded size for a - // frame containing it. Does not calculate CRC to improve efficiency. - static size_t MaxEncodedSize(uint64_t address, ConstByteSpan payload); - private: // Indicates this an information packet with sequence numbers set to 0. static constexpr std::byte kUnusedControl = std::byte{0}; diff --git a/pw_hdlc/public/pw_hdlc/internal/protocol.h b/pw_hdlc/public/pw_hdlc/internal/protocol.h index f11b34fabf..e471c9140d 100644 --- a/pw_hdlc/public/pw_hdlc/internal/protocol.h +++ b/pw_hdlc/public/pw_hdlc/internal/protocol.h @@ -28,6 +28,37 @@ inline constexpr std::array kEscapedFlag = {kEscape, inline constexpr std::array kEscapedEscape = {kEscape, std::byte{0x5D}}; +// This implementation also only supports addresses up to the maximum value of a +// 64-bit unsigned integer. +inline constexpr size_t kMinAddressSize = 1; +inline constexpr size_t kMaxAddressSize = + varint::EncodedSize(std::numeric_limits::max()); + +// The biggest on-the-wire size of a FCS after HDLC escaping. If, for example, +// the FCS is 0x7e7e7e7e the encoded value will be: +// [ 0x7e, 0x5e, 0x7e, 0x5e, 0x7e, 0x5e, 0x7e, 0x5e ] +inline constexpr size_t kMaxEscapedFcsSize = 8; + +// The biggest on-the-wire size of an HDLC address after escaping. This number +// is not just kMaxAddressSize * 2 because the kFlag and kEscape bytes are not +// valid terminating bytes of a one-terminated least significant varint. This +// means that the practical biggest varint BEFORE escaping looks like this: +// [ 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x7e, 0x03 ] +// The first nine bytes will be escaped, but the tenth never will be escaped +// since any values other than 0x03 would cause a uint64_t to overflow. +inline constexpr size_t kMaxEscapedVarintAddressSize = + (kMaxAddressSize * 2) - 1; + +// This implementation does not support extended control fields. +inline constexpr size_t kControlSize = 1; + +// The control byte may need to be escaped. When it is, this is its maximum +// size. +inline constexpr size_t kMaxEscapedControlSize = 2; + +// This implementation only supports a 32-bit CRC-32 Frame Check Sequence (FCS). +inline constexpr size_t kFcsSize = sizeof(uint32_t); + inline constexpr varint::Format kAddressFormat = varint::Format::kOneTerminatedLeastSignificant; diff --git a/pw_hdlc/rpc_channel_test.cc b/pw_hdlc/rpc_channel_test.cc index eedf12008f..a02268dbd7 100644 --- a/pw_hdlc/rpc_channel_test.cc +++ b/pw_hdlc/rpc_channel_test.cc @@ -34,6 +34,7 @@ #include "gtest/gtest.h" #include "pw_bytes/array.h" +#include "pw_hdlc/encoded_size.h" #include "pw_stream/memory_stream.h" using std::byte; @@ -46,8 +47,9 @@ constexpr uint8_t kAddress = 0x7b; // 123 constexpr uint8_t kEncodedAddress = (kAddress << 1) | 1; constexpr byte kControl = byte{0x3}; // UI-frame control sequence. -// Size of the in-memory buffer to use for this test. -constexpr size_t kSinkBufferSize = 15; +// Size of the in-memory buffer to use for this test. All tests send a one-byte +// payload. +constexpr size_t kSinkBufferSize = MaxEncodedFrameSize(1); TEST(RpcChannelOutput, 1BytePayload) { stream::MemoryWriterBuffer memory_writer; diff --git a/pw_hdlc/wire_packet_parser.cc b/pw_hdlc/wire_packet_parser.cc index 992dfdee4a..d867180957 100644 --- a/pw_hdlc/wire_packet_parser.cc +++ b/pw_hdlc/wire_packet_parser.cc @@ -22,7 +22,7 @@ namespace pw::hdlc { bool WirePacketParser::Parse(ConstByteSpan packet) { - if (packet.size_bytes() < Frame::kMinSizeBytes) { + if (packet.size_bytes() < Frame::kMinContentSizeBytes) { return false; }