From 922498d305dd7f42609eaaaabad295d161d3fb47 Mon Sep 17 00:00:00 2001 From: Armando Montanez Date: Thu, 21 Jul 2022 19:43:38 +0000 Subject: [PATCH] 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 Reviewed-by: Wyatt Hepler Commit-Queue: Auto-Submit --- pw_rpc/integration_testing.cc | 4 +++- .../pw_rpc/integration_test_socket_client.h | 7 +++++-- pw_system/hdlc_rpc_server.cc | 13 +++++++++---- pw_system/log.cc | 10 ++++++---- pw_system/public/pw_system/config.h | 9 +++++---- pw_transfer/integration_test/client.cc | 9 +++++---- targets/arduino/system_rpc_server.cc | 17 ++++++++++++----- targets/host/system_rpc_server.cc | 17 ++++++++++++----- targets/mimxrt595_evk/system_rpc_server.cc | 17 ++++++++++++----- targets/stm32f429i_disc1/system_rpc_server.cc | 17 ++++++++++++----- 10 files changed, 81 insertions(+), 39 deletions(-) diff --git a/pw_rpc/integration_testing.cc b/pw_rpc/integration_testing.cc index 576e4458d8..d0b18fadaa 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 c571f0e662..2d4ba0d8be 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 #include +#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 rpc_dispatch_thread_handle_; stream::SocketStream stream_; - hdlc::RpcChannelOutput channel_output_; + hdlc::FixedMtuChannelOutput channel_output_; ChannelOutputWithManipulator channel_output_with_manipulator_; ChannelManipulator* ingress_channel_manipulator_; Channel channel_; @@ -134,7 +135,9 @@ class SocketClientContext { template void SocketClientContext::ProcessPackets() { - std::byte decode_buffer[kMaxTransmissionUnit]; + constexpr size_t kDecoderBufferSize = + hdlc::Decoder::RequiredBufferSizeForFrameSize(kMaxTransmissionUnit); + std::array 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 fe4582651e..dc8ab2f92a 100644 --- a/pw_system/hdlc_rpc_server.cc +++ b/pw_system/hdlc_rpc_server.cc @@ -18,6 +18,7 @@ #include #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 hdlc_channel_output( + GetWriter(), PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS, "HDLC channel"); rpc::Channel channels[] = { rpc::Channel::Create(&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 input_buffer; +std::array input_buffer; hdlc::Decoder decoder(input_buffer); std::array data; diff --git a/pw_system/log.cc b/pw_system/log.cc index 7cfbd828f8..652f3e2b29 100644 --- a/pw_system/log.cc +++ b/pw_system/log.cc @@ -36,7 +36,12 @@ std::array 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 log_decode_buffer PW_GUARDED_BY(drains_mutex); @@ -49,10 +54,7 @@ std::array 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 log_packing_buffer; } // namespace diff --git a/pw_system/public/pw_system/config.h b/pw_system/public/pw_system/config.h index e7021eb03e..e8400d7b10 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 e567cbc442..a0d152edc7 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 @@ 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 index b87e654220..65ef5d2a49 100644 --- a/targets/arduino/system_rpc_server.cc +++ b/targets/arduino/system_rpc_server.cc @@ -14,6 +14,7 @@ #include +#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 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 input_buffer; + std::array 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 7a27e7a59c..4647647210 100644 --- a/targets/host/system_rpc_server.cc +++ b/targets/host/system_rpc_server.cc @@ -17,6 +17,7 @@ #include #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 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 input_buffer; + std::array 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 b87e654220..65ef5d2a49 100644 --- a/targets/mimxrt595_evk/system_rpc_server.cc +++ b/targets/mimxrt595_evk/system_rpc_server.cc @@ -14,6 +14,7 @@ #include +#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 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 input_buffer; + std::array 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 b87e654220..65ef5d2a49 100644 --- a/targets/stm32f429i_disc1/system_rpc_server.cc +++ b/targets/stm32f429i_disc1/system_rpc_server.cc @@ -14,6 +14,7 @@ #include +#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 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 input_buffer; + std::array input_buffer; hdlc::Decoder decoder(input_buffer); while (true) {