pw_rpc: Fix RPC MTU allocations

Fixes RPC server channel definitions to properly allocate necessary MTU overhead for RPC/HDLC encoding. Change-Id: I121216ca429879f2ce65c965a48760a7d107ae65 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/101780 Pigweed-Auto-Submit: Armando Montanez <[email protected]> Reviewed-by: Wyatt Hepler <[email protected]> Commit-Queue: Auto-Submit <[email protected]>
google · Jul 21, 2022 · 922498d · 922498d
1 parent 8eeef8a
commit 922498d
Show file tree

Hide file tree

Showing 10 changed files with 81 additions and 39 deletions.
diff --git a/pw_rpc/integration_testing.cc b/pw_rpc/integration_testing.cc
@@ -24,7 +24,9 @@
 namespace pw::rpc::integration_test {
 namespace {
 
-SocketClientContext<512> context;
+// Hard-coded to 1055 bytes, which is enough to fit 512-byte payloads when using
+// HDLC framing.
+SocketClientContext<1055> context;
 unit_test::LoggingEventHandler log_test_events;
 
 }  // namespace

diff --git a/pw_rpc/public/pw_rpc/integration_test_socket_client.h b/pw_rpc/public/pw_rpc/integration_test_socket_client.h
@@ -18,6 +18,7 @@
 #include <optional>
 #include <thread>
 
+#include "pw_hdlc/encoded_size.h"
 #include "pw_hdlc/rpc_channel.h"
 #include "pw_hdlc/rpc_packets.h"
 #include "pw_rpc/integration_testing.h"
@@ -125,7 +126,7 @@ class SocketClientContext {
   std::atomic_flag should_terminate_ = ATOMIC_FLAG_INIT;
   std::optional<std::thread> rpc_dispatch_thread_handle_;
   stream::SocketStream stream_;
-  hdlc::RpcChannelOutput channel_output_;
+  hdlc::FixedMtuChannelOutput<kMaxTransmissionUnit> channel_output_;
   ChannelOutputWithManipulator channel_output_with_manipulator_;
   ChannelManipulator* ingress_channel_manipulator_;
   Channel channel_;
@@ -134,7 +135,9 @@ class SocketClientContext {
 
 template <size_t kMaxTransmissionUnit>
 void SocketClientContext<kMaxTransmissionUnit>::ProcessPackets() {
-  std::byte decode_buffer[kMaxTransmissionUnit];
+  constexpr size_t kDecoderBufferSize =
+      hdlc::Decoder::RequiredBufferSizeForFrameSize(kMaxTransmissionUnit);
+  std::array<std::byte, kDecoderBufferSize> decode_buffer;
   hdlc::Decoder decoder(decode_buffer);
 
   while (true) {

diff --git a/pw_system/hdlc_rpc_server.cc b/pw_system/hdlc_rpc_server.cc
@@ -18,6 +18,7 @@
 #include <cstdio>
 
 #include "pw_assert/check.h"
+#include "pw_hdlc/encoded_size.h"
 #include "pw_hdlc/rpc_channel.h"
 #include "pw_hdlc/rpc_packets.h"
 #include "pw_log/log.h"
@@ -31,15 +32,19 @@ namespace {
 
 constexpr size_t kMaxTransmissionUnit = PW_SYSTEM_MAX_TRANSMISSION_UNIT;
 
-hdlc::RpcChannelOutput hdlc_channel_output(GetWriter(),
-                                           PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS,
-                                           "HDLC channel");
+static_assert(kMaxTransmissionUnit ==
+              hdlc::MaxEncodedFrameSize(rpc::cfg::kEncodingBufferSizeBytes));
+
+hdlc::FixedMtuChannelOutput<kMaxTransmissionUnit> hdlc_channel_output(
+    GetWriter(), PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS, "HDLC channel");
 rpc::Channel channels[] = {
     rpc::Channel::Create<kDefaultRpcChannelId>(&hdlc_channel_output)};
 rpc::Server server(channels);
 
+constexpr size_t kDecoderBufferSize =
+    hdlc::Decoder::RequiredBufferSizeForFrameSize(kMaxTransmissionUnit);
 // Declare a buffer for decoding incoming HDLC frames.
-std::array<std::byte, kMaxTransmissionUnit> input_buffer;
+std::array<std::byte, kDecoderBufferSize> input_buffer;
 hdlc::Decoder decoder(input_buffer);
 
 std::array<std::byte, 1> data;

diff --git a/pw_system/log.cc b/pw_system/log.cc
@@ -36,7 +36,12 @@ std::array<std::byte, PW_SYSTEM_LOG_BUFFER_SIZE> log_buffer;
 // To save RAM, share the mutex and buffer between drains, since drains are
 // flushed sequentially.
 sync::Mutex drains_mutex;
+
 // Buffer to decode and remove entries from log buffer, to send to a drain.
+//
+// TODO(amontanez): pw_log_rpc should provide a helper for this since there's
+// proto encoding overhead unaccounted for here.
+static_assert(rpc::MaxSafePayloadSize() >= PW_SYSTEM_MAX_LOG_ENTRY_SIZE);
 std::array<std::byte, PW_SYSTEM_MAX_LOG_ENTRY_SIZE> log_decode_buffer
     PW_GUARDED_BY(drains_mutex);
 
@@ -49,10 +54,7 @@ std::array<RpcLogDrain, 1> drains{{
 
 log_rpc::RpcLogDrainMap drain_map(drains);
 
-// TODO(amontanez): Is there a helper to subtract RPC overhead?
-constexpr size_t kMaxPackedLogMessagesSize =
-    PW_SYSTEM_MAX_TRANSMISSION_UNIT - 32;
-
+constexpr size_t kMaxPackedLogMessagesSize = rpc::MaxSafePayloadSize();
 std::array<std::byte, kMaxPackedLogMessagesSize> log_packing_buffer;
 
 }  // namespace

diff --git a/pw_system/public/pw_system/config.h b/pw_system/public/pw_system/config.h
@@ -24,16 +24,17 @@
 // PW_SYSTEM_MAX_LOG_ENTRY_SIZE limits the proto-encoded log entry size. This
 // value might depend on a target interface's MTU.
 //
-// Defaults to 512B.
+// Defaults to 256B.
 #ifndef PW_SYSTEM_MAX_LOG_ENTRY_SIZE
-#define PW_SYSTEM_MAX_LOG_ENTRY_SIZE 512
+#define PW_SYSTEM_MAX_LOG_ENTRY_SIZE 256
 #endif  // PW_SYSTEM_MAX_LOG_ENTRY_SIZE
 
 // PW_SYSTEM_MAX_TRANSMISSION_UNIT target's MTU.
 //
-// Defaults to 512B.
+// Defaults to 1055 bytes, which is enough to fit 512-byte payloads when using
+// HDLC framing.
 #ifndef PW_SYSTEM_MAX_TRANSMISSION_UNIT
-#define PW_SYSTEM_MAX_TRANSMISSION_UNIT 512
+#define PW_SYSTEM_MAX_TRANSMISSION_UNIT 1055
 #endif  // PW_SYSTEM_MAX_TRANSMISSION_UNIT
 
 // PW_SYSTEM_DEFAULT_CHANNEL_ID RPC channel ID to host.

diff --git a/pw_transfer/integration_test/client.cc b/pw_transfer/integration_test/client.cc
@@ -29,6 +29,7 @@
 #include "google/protobuf/text_format.h"
 #include "pw_assert/check.h"
 #include "pw_log/log.h"
+#include "pw_rpc/channel.h"
 #include "pw_rpc/integration_testing.h"
 #include "pw_status/status.h"
 #include "pw_status/try.h"
@@ -77,15 +78,15 @@ struct TransferResult {
 
 // Create a pw_transfer client and perform the transfer actions.
 pw::Status PerformTransferActions(const pw::transfer::ClientConfig& config) {
-  std::byte chunk_buffer[512];
-  std::byte encode_buffer[512];
+  constexpr size_t kMaxPayloadSize = rpc::MaxSafePayloadSize();
+  std::byte chunk_buffer[kMaxPayloadSize];
+  std::byte encode_buffer[kMaxPayloadSize];
   transfer::Thread<2, 2> transfer_thread(chunk_buffer, encode_buffer);
   thread::Thread system_thread(TransferThreadOptions(), transfer_thread);
 
   pw::transfer::Client client(rpc::integration_test::client(),
                               rpc::integration_test::kChannelId,
-                              transfer_thread,
-                              /*max_bytes_to_receive=*/256);
+                              transfer_thread);
 
   Status status = pw::OkStatus();
   for (const pw::transfer::TransferAction& action : config.transfer_actions()) {

diff --git a/targets/arduino/system_rpc_server.cc b/targets/arduino/system_rpc_server.cc
@@ -14,6 +14,7 @@
 
 #include <cstddef>
 
+#include "pw_hdlc/encoded_size.h"
 #include "pw_hdlc/rpc_channel.h"
 #include "pw_hdlc/rpc_packets.h"
 #include "pw_log/log.h"
@@ -23,16 +24,20 @@
 namespace pw::rpc::system_server {
 namespace {
 
-constexpr size_t kMaxTransmissionUnit = 256;
+// Hard-coded to 1055 bytes, which is enough to fit 512-byte payloads when using
+// HDLC framing.
+constexpr size_t kMaxTransmissionUnit = 1055;
+
+static_assert(kMaxTransmissionUnit ==
+              hdlc::MaxEncodedFrameSize(rpc::cfg::kEncodingBufferSizeBytes));
 
 // Used to write HDLC data to pw::sys_io.
 stream::SysIoWriter writer;
 stream::SysIoReader reader;
 
 // Set up the output channel for the pw_rpc server to use.
-hdlc::RpcChannelOutput hdlc_channel_output(writer,
-                                           pw::hdlc::kDefaultRpcAddress,
-                                           "HDLC channel");
+hdlc::FixedMtuChannelOutput<kMaxTransmissionUnit> hdlc_channel_output(
+    writer, pw::hdlc::kDefaultRpcAddress, "HDLC channel");
 Channel channels[] = {pw::rpc::Channel::Create<1>(&hdlc_channel_output)};
 rpc::Server server(channels);
 
@@ -49,8 +54,10 @@ void Init() {
 rpc::Server& Server() { return server; }
 
 Status Start() {
+  constexpr size_t kDecoderBufferSize =
+      hdlc::Decoder::RequiredBufferSizeForFrameSize(kMaxTransmissionUnit);
   // Declare a buffer for decoding incoming HDLC frames.
-  std::array<std::byte, kMaxTransmissionUnit> input_buffer;
+  std::array<std::byte, kDecoderBufferSize> input_buffer;
   hdlc::Decoder decoder(input_buffer);
 
   while (true) {

diff --git a/targets/host/system_rpc_server.cc b/targets/host/system_rpc_server.cc
@@ -17,6 +17,7 @@
 #include <cstdio>
 
 #include "pw_assert/check.h"
+#include "pw_hdlc/encoded_size.h"
 #include "pw_hdlc/rpc_channel.h"
 #include "pw_hdlc/rpc_packets.h"
 #include "pw_log/log.h"
@@ -26,14 +27,18 @@
 namespace pw::rpc::system_server {
 namespace {
 
-constexpr size_t kMaxTransmissionUnit = 512;
+// Hard-coded to 1055 bytes, which is enough to fit 512-byte payloads when using
+// HDLC framing.
+constexpr size_t kMaxTransmissionUnit = 1055;
 uint16_t socket_port = 33000;
 
+static_assert(kMaxTransmissionUnit ==
+              hdlc::MaxEncodedFrameSize(rpc::cfg::kEncodingBufferSizeBytes));
+
 stream::SocketStream socket_stream;
 
-hdlc::RpcChannelOutput hdlc_channel_output(socket_stream,
-                                           hdlc::kDefaultRpcAddress,
-                                           "HDLC channel");
+hdlc::FixedMtuChannelOutput<kMaxTransmissionUnit> hdlc_channel_output(
+    socket_stream, hdlc::kDefaultRpcAddress, "HDLC channel");
 Channel channels[] = {rpc::Channel::Create<1>(&hdlc_channel_output)};
 rpc::Server server(channels);
 
@@ -59,8 +64,10 @@ void Init() {
 rpc::Server& Server() { return server; }
 
 Status Start() {
+  constexpr size_t kDecoderBufferSize =
+      hdlc::Decoder::RequiredBufferSizeForFrameSize(kMaxTransmissionUnit);
   // Declare a buffer for decoding incoming HDLC frames.
-  std::array<std::byte, kMaxTransmissionUnit> input_buffer;
+  std::array<std::byte, kDecoderBufferSize> input_buffer;
   hdlc::Decoder decoder(input_buffer);
 
   while (true) {

diff --git a/targets/mimxrt595_evk/system_rpc_server.cc b/targets/mimxrt595_evk/system_rpc_server.cc
@@ -14,6 +14,7 @@
 
 #include <cstddef>
 
+#include "pw_hdlc/encoded_size.h"
 #include "pw_hdlc/rpc_channel.h"
 #include "pw_hdlc/rpc_packets.h"
 #include "pw_log/log.h"
@@ -23,16 +24,20 @@
 namespace pw::rpc::system_server {
 namespace {
 
-constexpr size_t kMaxTransmissionUnit = 256;
+// Hard-coded to 1055 bytes, which is enough to fit 512-byte payloads when using
+// HDLC framing.
+constexpr size_t kMaxTransmissionUnit = 1055;
+
+static_assert(kMaxTransmissionUnit ==
+              hdlc::MaxEncodedFrameSize(rpc::cfg::kEncodingBufferSizeBytes));
 
 // Used to write HDLC data to pw::sys_io.
 stream::SysIoWriter writer;
 stream::SysIoReader reader;
 
 // Set up the output channel for the pw_rpc server to use.
-hdlc::RpcChannelOutput hdlc_channel_output(writer,
-                                           pw::hdlc::kDefaultRpcAddress,
-                                           "HDLC channel");
+hdlc::FixedMtuChannelOutput<kMaxTransmissionUnit> hdlc_channel_output(
+    writer, pw::hdlc::kDefaultRpcAddress, "HDLC channel");
 Channel channels[] = {pw::rpc::Channel::Create<1>(&hdlc_channel_output)};
 rpc::Server server(channels);
 
@@ -49,8 +54,10 @@ void Init() {
 rpc::Server& Server() { return server; }
 
 Status Start() {
+  constexpr size_t kDecoderBufferSize =
+      hdlc::Decoder::RequiredBufferSizeForFrameSize(kMaxTransmissionUnit);
   // Declare a buffer for decoding incoming HDLC frames.
-  std::array<std::byte, kMaxTransmissionUnit> input_buffer;
+  std::array<std::byte, kDecoderBufferSize> input_buffer;
   hdlc::Decoder decoder(input_buffer);
 
   while (true) {

diff --git a/targets/stm32f429i_disc1/system_rpc_server.cc b/targets/stm32f429i_disc1/system_rpc_server.cc
@@ -14,6 +14,7 @@
 
 #include <cstddef>
 
+#include "pw_hdlc/encoded_size.h"
 #include "pw_hdlc/rpc_channel.h"
 #include "pw_hdlc/rpc_packets.h"
 #include "pw_log/log.h"
@@ -23,16 +24,20 @@
 namespace pw::rpc::system_server {
 namespace {
 
-constexpr size_t kMaxTransmissionUnit = 256;
+// Hard-coded to 1055 bytes, which is enough to fit 512-byte payloads when using
+// HDLC framing.
+constexpr size_t kMaxTransmissionUnit = 1055;
+
+static_assert(kMaxTransmissionUnit ==
+              hdlc::MaxEncodedFrameSize(rpc::cfg::kEncodingBufferSizeBytes));
 
 // Used to write HDLC data to pw::sys_io.
 stream::SysIoWriter writer;
 stream::SysIoReader reader;
 
 // Set up the output channel for the pw_rpc server to use.
-hdlc::RpcChannelOutput hdlc_channel_output(writer,
-                                           pw::hdlc::kDefaultRpcAddress,
-                                           "HDLC channel");
+hdlc::FixedMtuChannelOutput<kMaxTransmissionUnit> hdlc_channel_output(
+    writer, pw::hdlc::kDefaultRpcAddress, "HDLC channel");
 Channel channels[] = {pw::rpc::Channel::Create<1>(&hdlc_channel_output)};
 rpc::Server server(channels);
 
@@ -49,8 +54,10 @@ void Init() {
 rpc::Server& Server() { return server; }
 
 Status Start() {
+  constexpr size_t kDecoderBufferSize =
+      hdlc::Decoder::RequiredBufferSizeForFrameSize(kMaxTransmissionUnit);
   // Declare a buffer for decoding incoming HDLC frames.
-  std::array<std::byte, kMaxTransmissionUnit> input_buffer;
+  std::array<std::byte, kDecoderBufferSize> input_buffer;
   hdlc::Decoder decoder(input_buffer);
 
   while (true) {