pw_hdlc: Calculate frame size before writing
This updates the WriteUIFrame function to calculate the worst-case
encoded frame size in advance and not write any data if it exceeds
the stream's write limit.
Change-Id: I315a2a94dbf19d6f3d021ecdee66d5329313f1d9
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/29485
Reviewed-by: Wyatt Hepler <hepler@google.com>
Commit-Queue: Alexei Frolov <frolv@google.com>
diff --git a/pw_hdlc/BUILD b/pw_hdlc/BUILD
index 3dbde8c..8316bb8 100644
--- a/pw_hdlc/BUILD
+++ b/pw_hdlc/BUILD
@@ -26,6 +26,7 @@
srcs = [
"decoder.cc",
"encoder.cc",
+ "public/pw_hdlc/internal/encoder.h",
"pw_hdlc_private/protocol.h",
"rpc_packets.cc",
],
diff --git a/pw_hdlc/BUILD.gn b/pw_hdlc/BUILD.gn
index 7522842..18a1946 100644
--- a/pw_hdlc/BUILD.gn
+++ b/pw_hdlc/BUILD.gn
@@ -53,16 +53,17 @@
public = [ "public/pw_hdlc/encoder.h" ]
sources = [
"encoder.cc",
+ "public/pw_hdlc/internal/encoder.h",
"pw_hdlc_private/protocol.h",
]
public_deps = [
"$dir_pw_stream:sys_io_stream",
dir_pw_bytes,
+ dir_pw_checksum,
dir_pw_status,
dir_pw_stream,
dir_pw_sys_io,
]
- deps = [ dir_pw_checksum ]
friend = [ ":*" ]
}
diff --git a/pw_hdlc/encoder.cc b/pw_hdlc/encoder.cc
index ae30f02..b0d209e 100644
--- a/pw_hdlc/encoder.cc
+++ b/pw_hdlc/encoder.cc
@@ -21,13 +21,13 @@
#include <span>
#include "pw_bytes/endian.h"
-#include "pw_checksum/crc32.h"
+#include "pw_hdlc/internal/encoder.h"
#include "pw_hdlc_private/protocol.h"
using std::byte;
namespace pw::hdlc {
-namespace {
+namespace internal {
// Indicates this an information packet with sequence numbers set to 0.
constexpr byte kUnusedControl = byte{0};
@@ -42,31 +42,6 @@
return writer.Write(b);
}
-// Encodes and writes HDLC frames.
-class Encoder {
- public:
- constexpr Encoder(stream::Writer& output) : writer_(output) {}
-
- // Writes the header for an I-frame. After successfully calling
- // StartInformationFrame, WriteData may be called any number of times.
- [[maybe_unused]] Status StartInformationFrame(uint8_t address);
-
- // Writes the header for an U-frame. After successfully calling
- // StartUnnumberedFrame, WriteData may be called any number of times.
- Status StartUnnumberedFrame(uint8_t address);
-
- // Writes data for an ongoing frame. Must only be called after a successful
- // StartInformationFrame call, and prior to a FinishFrame() call.
- Status WriteData(ConstByteSpan data);
-
- // Finishes a frame. Writes the frame check sequence and a terminating flag.
- Status FinishFrame();
-
- private:
- stream::Writer& writer_;
- checksum::Crc32 fcs_;
-};
-
Status Encoder::StartInformationFrame(uint8_t address) {
fcs_.clear();
if (Status status = writer_.Write(kFlag); !status.ok()) {
@@ -117,12 +92,27 @@
return writer_.Write(kFlag);
}
-} // namespace
+size_t Encoder::MaxEncodedSize(uint8_t address, ConstByteSpan payload) {
+ constexpr size_t kFcsMaxSize = 8; // Worst case FCS: 0x7e7e7e7e.
+ size_t encoded_address_size = NeedsEscaping(std::byte{address}) ? 2 : 1;
+ size_t encoded_payload_size =
+ payload.size() +
+ std::count_if(payload.begin(), payload.end(), NeedsEscaping);
+
+ return encoded_address_size + encoded_payload_size + kFcsMaxSize;
+}
+
+} // namespace internal
Status WriteUIFrame(uint8_t address,
ConstByteSpan payload,
stream::Writer& writer) {
- Encoder encoder(writer);
+ if (internal::Encoder::MaxEncodedSize(address, payload) >
+ writer.ConservativeWriteLimit()) {
+ return Status::ResourceExhausted();
+ }
+
+ internal::Encoder encoder(writer);
if (Status status = encoder.StartUnnumberedFrame(address); !status.ok()) {
return status;
diff --git a/pw_hdlc/encoder_test.cc b/pw_hdlc/encoder_test.cc
index f3ce559..ead3bfc 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/internal/encoder.h"
#include "pw_hdlc_private/protocol.h"
#include "pw_stream/memory_stream.h"
@@ -154,10 +155,53 @@
kFlag));
}
-TEST_F(WriteUnnumberedFrame, WriterError) {
+TEST_F(WriteUnnumberedFrame, PayloadTooLarge_WritesNothing) {
constexpr auto data = bytes::Initialized<sizeof(buffer_)>(0x7e);
EXPECT_EQ(Status::ResourceExhausted(), WriteUIFrame(kAddress, data, writer_));
+ EXPECT_EQ(0u, writer_.bytes_written());
+}
+
+class ErrorWriter : public stream::Writer {
+ private:
+ Status DoWrite(ConstByteSpan) override { return Status::Unimplemented(); }
+};
+
+TEST(WriteUnnumberedFrame, WriterError) {
+ ErrorWriter writer;
+ EXPECT_EQ(Status::Unimplemented(),
+ WriteUIFrame(kAddress, bytes::Array<0x01>(), writer));
}
} // namespace
+
+namespace internal {
+namespace {
+
+constexpr uint8_t kEscapeAddress = 0x7d;
+
+TEST(Encoder, MaxEncodedSize_EmptyPayload) {
+ EXPECT_EQ(9u, Encoder::MaxEncodedSize(kAddress, {}));
+ EXPECT_EQ(10u, Encoder::MaxEncodedSize(kEscapeAddress, {}));
+}
+
+TEST(Encoder, MaxEncodedSize_PayloadWithoutEscapes) {
+ constexpr auto data = bytes::Array<0x00, 0x01, 0x02, 0x03>();
+ EXPECT_EQ(13u, Encoder::MaxEncodedSize(kAddress, data));
+ EXPECT_EQ(14u, Encoder::MaxEncodedSize(kEscapeAddress, data));
+}
+
+TEST(Encoder, MaxEncodedSize_PayloadWithOneEscape) {
+ constexpr auto data = bytes::Array<0x00, 0x01, 0x7e, 0x03>();
+ EXPECT_EQ(14u, Encoder::MaxEncodedSize(kAddress, data));
+ EXPECT_EQ(15u, Encoder::MaxEncodedSize(kEscapeAddress, data));
+}
+
+TEST(Encoder, MaxEncodedSize_PayloadWithAllEscapes) {
+ constexpr auto data = bytes::Initialized<8>(0x7e);
+ EXPECT_EQ(25u, Encoder::MaxEncodedSize(kAddress, data));
+ EXPECT_EQ(26u, Encoder::MaxEncodedSize(kEscapeAddress, data));
+}
+
+} // namespace
+} // namespace internal
} // namespace pw::hdlc
diff --git a/pw_hdlc/public/pw_hdlc/internal/encoder.h b/pw_hdlc/public/pw_hdlc/internal/encoder.h
new file mode 100644
index 0000000..a7a61ad
--- /dev/null
+++ b/pw_hdlc/public/pw_hdlc/internal/encoder.h
@@ -0,0 +1,50 @@
+// 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 "pw_checksum/crc32.h"
+#include "pw_stream/stream.h"
+
+namespace pw::hdlc::internal {
+
+// Encodes and writes HDLC frames.
+class Encoder {
+ public:
+ constexpr Encoder(stream::Writer& output) : writer_(output) {}
+
+ // Writes the header for an I-frame. After successfully calling
+ // StartInformationFrame, WriteData may be called any number of times.
+ [[maybe_unused]] Status StartInformationFrame(uint8_t address);
+
+ // Writes the header for an U-frame. After successfully calling
+ // StartUnnumberedFrame, WriteData may be called any number of times.
+ Status StartUnnumberedFrame(uint8_t address);
+
+ // Writes data for an ongoing frame. Must only be called after a successful
+ // StartInformationFrame call, and prior to a FinishFrame() call.
+ Status WriteData(ConstByteSpan data);
+
+ // 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(uint8_t address, ConstByteSpan payload);
+
+ private:
+ stream::Writer& writer_;
+ checksum::Crc32 fcs_;
+};
+
+} // namespace pw::hdlc::internal
