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 <amontanez@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
diff --git a/pw_rpc/integration_testing.cc b/pw_rpc/integration_testing.cc
index 576e445..d0b18fa 100644
--- 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
index c571f0e..2d4ba0d 100644
--- 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 @@
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 @@
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
index fe45826..dc8ab2f 100644
--- 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 @@
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
index 7cfbd82..652f3e2 100644
--- a/pw_system/log.cc
+++ b/pw_system/log.cc
@@ -36,7 +36,12 @@
// 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 @@
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
index e7021eb..e8400d7 100644
--- 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
index e567cbc..a0d152e 100644
--- 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 @@
// 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
index b87e654..65ef5d2 100644
--- 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 @@
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
index 7a27e7a..4647647 100644
--- 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 @@
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
index b87e654..65ef5d2 100644
--- 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 @@
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
index b87e654..65ef5d2 100644
--- 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 @@
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) {
