From f1192a73c6f201d4473b05b09c42a33f38ec343c Mon Sep 17 00:00:00 2001 From: Gary Bressler Date: Sat, 2 Sep 2023 20:36:35 +0000 Subject: [PATCH] pw_system: Multi-channel configuration This change adds a configuration to pw_system that supports separate channels for primary and logging RPC. The immediate motivation is to support a Fuchsia pw_rpc integration prototype which will have separate primary and logging clients. See the docs.rst change for more info. Bug: b/297076185 Change-Id: I9c1aff679a77c24a98c95db3f4d9fb54b75c6408 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/167158 Reviewed-by: Armando Montanez Commit-Queue: Gary Bressler --- pw_system/BUILD.gn | 27 +++++++++++++ pw_system/docs.rst | 18 +++++++++ pw_system/hdlc_rpc_server.cc | 51 ++++++++++++++++++++++++- pw_system/init.cc | 2 +- pw_system/log.cc | 2 +- pw_system/public/pw_system/config.h | 17 +++++++++ pw_system/public/pw_system/rpc_server.h | 7 +++- pw_system/system_target.gni | 16 ++++++++ 8 files changed, 135 insertions(+), 5 deletions(-) diff --git a/pw_system/BUILD.gn b/pw_system/BUILD.gn index e20c9f8d91..72873a922a 100644 --- a/pw_system/BUILD.gn +++ b/pw_system/BUILD.gn @@ -39,6 +39,33 @@ config("public_include_path") { include_dirs = [ "public" ] } +# This config moves RPC logging to a separate RPC channel and HDLC +# address. This does two things: +# * The separate RPC channel allows logging traffic to be treated as +# if it is being sent to a different client via a separate RPC +# channel. This illustrates the ability for an RPC server to +# communicate to multiple clients over multiple physical links. +# * The separate HDLC address completely isolates typical RPC traffic +# from logging traffic by communicating to a different HDLC endpoint +# address. This effectively creates two virtual data pipes over the +# same physical link. +# +# This is mostly to illustrate pw_rpc's capability to route and multiplex +# traffic. +config("multi_endpoint_rpc_overrides") { + defines = [ + "PW_SYSTEM_LOGGING_CHANNEL_ID=10000", + "PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS=10000", + ] +} + +# The Pigweed config pattern requires a pw_source_set to provide the +# configuration defines. This provides the flags in +# multi_endpoint_rpc_overrides. +pw_source_set("multi_endpoint_rpc_config") { + public_configs = [ ":multi_endpoint_rpc_overrides" ] +} + pw_source_set("config") { sources = [ "public/pw_system/config.h" ] public_configs = [ ":public_include_path" ] diff --git a/pw_system/docs.rst b/pw_system/docs.rst index 1d446eea4b..48394e373f 100644 --- a/pw_system/docs.rst +++ b/pw_system/docs.rst @@ -169,3 +169,21 @@ See :ref:`module-pw_system-cli` for detailed CLI usage information. :maxdepth: 1 cli + +------------------- +Multi-endpoint mode +------------------- + +The default configuration serves all its traffic with the same +channel ID and RPC address. There is an alternative mode that assigns a separate +channel ID and address for logging. This can be useful if you want to separate logging and primary RPC to +``pw_system`` among multiple clients. + +To use this mode, add the following to ``gn args out``: + +.. code-block:: + + pw_system_USE_MULTI_ENDPOINT_CONFIG = true + +The settings for the channel ID and address can be found in the target +``//pw_system:multi_endpoint_rpc_overrides``. diff --git a/pw_system/hdlc_rpc_server.cc b/pw_system/hdlc_rpc_server.cc index 5bbcd83ab2..3923d4ff02 100644 --- a/pw_system/hdlc_rpc_server.cc +++ b/pw_system/hdlc_rpc_server.cc @@ -16,17 +16,25 @@ #include #include #include +#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" +#include "pw_rpc/channel.h" #include "pw_sync/mutex.h" #include "pw_system/config.h" #include "pw_system/io.h" #include "pw_system/rpc_server.h" +#if PW_SYSTEM_DEFAULT_CHANNEL_ID != PW_SYSTEM_LOGGING_CHANNEL_ID && \ + PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS == PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS +#error \ + "Default and logging addresses must be different to support multiple channels." +#endif + namespace pw::system { namespace { @@ -35,10 +43,50 @@ constexpr size_t kMaxTransmissionUnit = PW_SYSTEM_MAX_TRANSMISSION_UNIT; static_assert(kMaxTransmissionUnit == hdlc::MaxEncodedFrameSize(rpc::cfg::kEncodingBufferSizeBytes)); +#if PW_SYSTEM_DEFAULT_CHANNEL_ID == PW_SYSTEM_LOGGING_CHANNEL_ID hdlc::FixedMtuChannelOutput hdlc_channel_output( GetWriter(), PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS, "HDLC channel"); rpc::Channel channels[] = { rpc::Channel::Create(&hdlc_channel_output)}; +#else +class SynchronizedChannelOutput : public rpc::ChannelOutput { + public: + SynchronizedChannelOutput(stream::Writer& writer, + uint64_t address, + const char* channel_name) + : rpc::ChannelOutput(channel_name), + inner_(writer, address, channel_name) {} + + Status Send(span buffer) override { + std::lock_guard guard(mtx_); + auto s = inner_.Send(buffer); + return s; + } + + size_t MaximumTransmissionUnit() override { + std::lock_guard guard(mtx_); + auto s = inner_.MaximumTransmissionUnit(); + return s; + } + + private: + sync::Mutex mtx_; + hdlc::FixedMtuChannelOutput inner_ PW_GUARDED_BY(mtx_); +}; + +SynchronizedChannelOutput hdlc_channel_output[] = { + SynchronizedChannelOutput(GetWriter(), + PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS, + "HDLC default channel"), + SynchronizedChannelOutput(GetWriter(), + PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS, + "HDLC logging channel"), +}; +rpc::Channel channels[] = { + rpc::Channel::Create(&hdlc_channel_output[0]), + rpc::Channel::Create(&hdlc_channel_output[1]), +}; +#endif rpc::Server server(channels); constexpr size_t kDecoderBufferSize = @@ -69,7 +117,8 @@ class RpcDispatchThread final : public thread::ThreadCore { for (std::byte byte : ret_val.value()) { if (auto result = decoder.Process(byte); result.ok()) { hdlc::Frame& frame = result.value(); - if (frame.address() == PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS) { + if (frame.address() == PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS || + frame.address() == PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS) { server.ProcessPacket(frame.data()); } } diff --git a/pw_system/init.cc b/pw_system/init.cc index 13c4e1bf6f..4196115d3b 100644 --- a/pw_system/init.cc +++ b/pw_system/init.cc @@ -42,7 +42,7 @@ void InitImpl() { // Setup logging. const Status status = GetLogThread().OpenUnrequestedLogStream( - kDefaultRpcChannelId, GetRpcServer(), GetLogService()); + kLoggingRpcChannelId, GetRpcServer(), GetLogService()); if (!status.ok()) { PW_LOG_ERROR("Error opening unrequested log streams %d", static_cast(status.code())); diff --git a/pw_system/log.cc b/pw_system/log.cc index 652f3e2b29..3a5f0fd8e2 100644 --- a/pw_system/log.cc +++ b/pw_system/log.cc @@ -46,7 +46,7 @@ std::array log_decode_buffer PW_GUARDED_BY(drains_mutex); std::array drains{{ - RpcLogDrain(kDefaultRpcChannelId, + RpcLogDrain(kLoggingRpcChannelId, log_decode_buffer, drains_mutex, RpcLogDrain::LogDrainErrorHandling::kIgnoreWriterErrors), diff --git a/pw_system/public/pw_system/config.h b/pw_system/public/pw_system/config.h index 1dc3c3cbdc..0d04158cbf 100644 --- a/pw_system/public/pw_system/config.h +++ b/pw_system/public/pw_system/config.h @@ -44,6 +44,16 @@ #define PW_SYSTEM_DEFAULT_CHANNEL_ID 1 #endif // PW_SYSTEM_DEFAULT_CHANNEL_ID +// PW_SYSTEM_LOGGING_CHANNEL_ID logging RPC channel ID to host. If this is +// different from PW_SYSTEM_DEFAULT_CHANNEL_ID, then +// PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS must also be different from +// PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS. +// +// Defaults to PW_SYSTEM_DEFAULT_CHANNEL_ID. +#ifndef PW_SYSTEM_LOGGING_CHANNEL_ID +#define PW_SYSTEM_LOGGING_CHANNEL_ID PW_SYSTEM_DEFAULT_CHANNEL_ID +#endif // PW_SYSTEM_LOGGING_CHANNEL_ID + // PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS RPC HDLC default address. // // Defaults to 82. @@ -51,6 +61,13 @@ #define PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS 82 #endif // PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS +// PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS RPC HDLC logging address. +// +// Defaults to PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS. +#ifndef PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS +#define PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS PW_SYSTEM_DEFAULT_RPC_HDLC_ADDRESS +#endif // PW_SYSTEM_LOGGING_RPC_HDLC_ADDRESS + // PW_SYSTEM_ENABLE_THREAD_SNAPSHOT_SERVICE specifies if the thread snapshot // RPC service is enabled. // diff --git a/pw_system/public/pw_system/rpc_server.h b/pw_system/public/pw_system/rpc_server.h index bf05fccdbb..0337570648 100644 --- a/pw_system/public/pw_system/rpc_server.h +++ b/pw_system/public/pw_system/rpc_server.h @@ -23,10 +23,13 @@ namespace pw::system { // This is the default channel used by the pw_system RPC server. Some other -// parts of pw_system (e.g. logging) use this channel ID as the default -// destination for unrequested data streams. +// parts of pw_system use this channel ID as the default destination for +// unrequested data streams. inline constexpr uint32_t kDefaultRpcChannelId = PW_SYSTEM_DEFAULT_CHANNEL_ID; +// This is the channel ID used for logging. +inline constexpr uint32_t kLoggingRpcChannelId = PW_SYSTEM_LOGGING_CHANNEL_ID; + rpc::Server& GetRpcServer(); thread::ThreadCore& GetRpcDispatchThread(); diff --git a/pw_system/system_target.gni b/pw_system/system_target.gni index 4c138e87af..d38ccd30ee 100644 --- a/pw_system/system_target.gni +++ b/pw_system/system_target.gni @@ -57,6 +57,18 @@ PW_SYSTEM_SCHEDULER = { NATIVE = "native" } +declare_args() { + # This argument is intended to be user-facing and should NOT be set by a + # toolchain. This switches ALL pw_system_target toolchains to use the + # multi_endpoint_rpc_config config to illustrate a multi-endpoint mode that + # isolates logging and RPC traffic via HDLC multiplexing. + # + # If you would like to use this in production, it is strongly recommended that + # you instead just add the appropriate defines to your target's toolchain + # definition. + pw_system_USE_MULTI_ENDPOINT_CONFIG = false +} + # Defines a target toolchain, automatically setting many required build # arguments to simplify instantiation of a target. # @@ -113,6 +125,10 @@ template("pw_system_target") { # TODO(amontanez): This should be set to a "$dir_pw_unit_test:rpc_main" # when RPC is working. pw_unit_test_MAIN = "$dir_pw_unit_test:logging_main" + + if (pw_system_USE_MULTI_ENDPOINT_CONFIG) { + pw_system_CONFIG = "$dir_pw_system:multi_endpoint_rpc_config" + } } # Populate architecture-specific build args.