From 56d9d44b4b6ed27346262c83fe1e8fac2d76b8e2 Mon Sep 17 00:00:00 2001 From: David Rees Date: Wed, 8 Jan 2025 14:53:14 -0800 Subject: [PATCH] pw_bluetooth_proxy: Allow Write(MultiBuf) for rfcomm and basic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow clients to use Write(MultiBuf) with RfcommChannel and BasicL2capChannel. Also update all tests to use Write(MultiBuf). Write(span) is deprecated. Later CL will remove it once downstreams have moved to Write(MultiBuf). Bug: 388082771, 379337272 Change-Id: Ia36152adf4ec2e269a7837db643d9ad6495c6f00 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/258712 Presubmit-Verified: CQ Bot Account Lint: Lint 🤖 Commit-Queue: David Rees Docs-Not-Needed: David Rees Reviewed-by: Austin Foxley --- pw_bluetooth_proxy/l2cap_channel.cc | 40 ++++++++++++++++--- pw_bluetooth_proxy/proxy_host_test.cc | 12 ++++-- .../pw_bluetooth_proxy/basic_l2cap_channel.h | 12 +++++- .../internal/l2cap_channel.h | 11 +++++ .../pw_bluetooth_proxy/rfcomm_channel.h | 10 +++++ .../pw_bluetooth_proxy_private/test_utils.h | 2 +- pw_bluetooth_proxy/rfcomm_channel.cc | 3 +- pw_bluetooth_proxy/rfcomm_test.cc | 12 ++++-- 8 files changed, 85 insertions(+), 17 deletions(-) diff --git a/pw_bluetooth_proxy/l2cap_channel.cc b/pw_bluetooth_proxy/l2cap_channel.cc index 92f2148c1..87e9eda41 100644 --- a/pw_bluetooth_proxy/l2cap_channel.cc +++ b/pw_bluetooth_proxy/l2cap_channel.cc @@ -146,14 +146,32 @@ Status L2capChannel::QueuePacket(H4PacketWithH4&& packet) { return status; } +namespace { + +pw::span AsConstUint8Span(ConstByteSpan s) { + return {reinterpret_cast(s.data()), s.size_bytes()}; +} + +} // namespace + StatusWithMultiBuf L2capChannel::Write(multibuf::MultiBuf&& payload) { - if (!UsesPayloadQueue()) { - PW_LOG_ERROR( - "btproxy: Write(MultiBuf) called on class that only supports " - "Write(span)."); - return {Status::Unimplemented(), std::move(payload)}; + if (!payload.IsContiguous()) { + return {Status::InvalidArgument(), std::move(payload)}; + } + + if (state() != State::kRunning) { + return {Status::FailedPrecondition(), std::move(payload)}; } + PW_CHECK(UsesPayloadQueue()); + + return QueuePayload(std::move(payload)); +} + +// TODO: https://pwbug.dev/379337272 - Delete when all channels are +// transitioned to using payload queues. +StatusWithMultiBuf L2capChannel::WriteMultiBufAsSpan( + multibuf::MultiBuf&& payload) { if (!payload.IsContiguous()) { return {Status::InvalidArgument(), std::move(payload)}; } @@ -162,7 +180,17 @@ StatusWithMultiBuf L2capChannel::Write(multibuf::MultiBuf&& payload) { return {Status::FailedPrecondition(), std::move(payload)}; } - return QueuePayload(std::move(payload)); + PW_CHECK(!UsesPayloadQueue()); + + std::optional span = payload.ContiguousSpan(); + PW_CHECK(span.has_value()); + Status status = Write(AsConstUint8Span(span.value())); + + if (!status.ok()) { + return {status, std::move(payload)}; + } + + return {OkStatus(), std::nullopt}; } pw::Status L2capChannel::Write( diff --git a/pw_bluetooth_proxy/proxy_host_test.cc b/pw_bluetooth_proxy/proxy_host_test.cc index f78e03b1f..177419ee1 100644 --- a/pw_bluetooth_proxy/proxy_host_test.cc +++ b/pw_bluetooth_proxy/proxy_host_test.cc @@ -1458,7 +1458,8 @@ TEST_F(NumberOfCompletedPacketsTest, MultipleChannelsDifferentTransports) { RfcommChannel bredr_channel = BuildRfcomm(proxy, RfcommParameters{.handle = 0x456}); - EXPECT_EQ(bredr_channel.Write(capture.payload), PW_STATUS_OK); + PW_TEST_EXPECT_OK( + bredr_channel.Write(MultiBufFromSpan(pw::span(capture.payload))).status); // Send should succeed even though no LE credits available EXPECT_EQ(capture.sends_called, 2); @@ -1476,7 +1477,8 @@ TEST_F(NumberOfCompletedPacketsTest, MultipleChannelsDifferentTransports) { proxy, FlatMap({{{0x456, 1}}}))); // Write again - EXPECT_EQ(bredr_channel.Write(capture.payload), PW_STATUS_OK); + PW_TEST_EXPECT_OK( + bredr_channel.Write(MultiBufFromSpan(pw::span(capture.payload))).status); EXPECT_EQ(capture.sends_called, 4); } @@ -2171,7 +2173,8 @@ TEST_F(BasicL2capChannelTest, BasicWrite) { /*payload_from_controller_fn=*/nullptr, /*event_fn=*/nullptr)); - EXPECT_EQ(channel.Write(capture.payload), PW_STATUS_OK); + PW_TEST_EXPECT_OK( + channel.Write(MultiBufFromSpan(pw::span(capture.payload))).status); EXPECT_EQ(capture.sends_called, 1); } @@ -2202,7 +2205,8 @@ TEST_F(BasicL2capChannelTest, ErrorOnWriteTooLarge) { /*payload_from_controller_fn=*/nullptr, /*event_fn=*/nullptr)); - EXPECT_EQ(channel.Write(span(hci_arr)), PW_STATUS_INVALID_ARGUMENT); + EXPECT_EQ(channel.Write(MultiBufFromSpan(pw::span(hci_arr))).status, + PW_STATUS_INVALID_ARGUMENT); } TEST_F(BasicL2capChannelTest, CannotCreateChannelWithInvalidArgs) { diff --git a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/basic_l2cap_channel.h b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/basic_l2cap_channel.h index 4ac882047..135345907 100644 --- a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/basic_l2cap_channel.h +++ b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/basic_l2cap_channel.h @@ -39,7 +39,17 @@ class BasicL2capChannel : public L2capChannel { BasicL2capChannel& operator=(BasicL2capChannel&& other) = default; ~BasicL2capChannel() override; - pw::Status Write(pw::span payload) override; + // @deprecated + // TODO: https://pwbug.dev/379337272 - Delete this once all downstreams + // have transitioned to Write(MultiBuf) for this channel type. + Status Write(pw::span payload) override; + + // Also allow Write(MultiBuf) during transition from Write(span). + // TODO: https://pwbug.dev/379337272 - Can delete once Write(span) above is + // deleted. + StatusWithMultiBuf Write(multibuf::MultiBuf&& payload) override { + return WriteMultiBufAsSpan(std::move(payload)); + } protected: explicit BasicL2capChannel( diff --git a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h index 54c2516b7..8815583f0 100644 --- a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h +++ b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/internal/l2cap_channel.h @@ -236,6 +236,17 @@ class L2capChannel : public IntrusiveForwardList::Item { // Tx (protected) //---------------- + // Sends a MultiBuf payload to the remote peer using Write(span). + // + // The contents of the MultiBuf are copied during the call and the MultiBuf + // is destroyed. + // + // Used by subclasses during the transition from PDU send_queue_ rather to + // Write(MultiBuf) and payload queues. + // TODO: https://pwbug.dev/379337272 - Delete when all channels are + // transitioned to using payload queues. + StatusWithMultiBuf WriteMultiBufAsSpan(multibuf::MultiBuf&& payload); + // Queue a client `buf` for sending and `ReportPacketsMayBeReadyToSend()`. // Must be a contiguous MultiBuf. // diff --git a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/rfcomm_channel.h b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/rfcomm_channel.h index eea5540d1..7d224dea8 100644 --- a/pw_bluetooth_proxy/public/pw_bluetooth_proxy/rfcomm_channel.h +++ b/pw_bluetooth_proxy/public/pw_bluetooth_proxy/rfcomm_channel.h @@ -87,8 +87,18 @@ class RfcommChannel final : public L2capChannel { Function payload)>&& payload_from_controller_fn, Function&& event_fn); + // @deprecated + // TODO: https://pwbug.dev/379337272 - Delete this once all downstreams + // have transitioned to Write(MultiBuf) for this channel type. Status Write(pw::span payload) override; + // Also allow Write(MultiBuf) during transition from Write(span). + // TODO: https://pwbug.dev/379337272 - Can delete once Write(span) above is + // deleted. + StatusWithMultiBuf Write(multibuf::MultiBuf&& payload) override { + return WriteMultiBufAsSpan(std::move(payload)); + } + Config rx_config() const { return rx_config_; } Config tx_config() const { return tx_config_; } diff --git a/pw_bluetooth_proxy/pw_bluetooth_proxy_private/test_utils.h b/pw_bluetooth_proxy/pw_bluetooth_proxy_private/test_utils.h index 50d4708ec..fa9c6f465 100644 --- a/pw_bluetooth_proxy/pw_bluetooth_proxy_private/test_utils.h +++ b/pw_bluetooth_proxy/pw_bluetooth_proxy_private/test_utils.h @@ -278,7 +278,7 @@ class ProxyHostTest : public testing::Test { private: // MultiBuf allocator for creating objects to pass to the system under // test (e.g. creating test packets to send to proxy host). - pw::multibuf::test::SimpleAllocatorForTest test_multibuf_allocator_{}; diff --git a/pw_bluetooth_proxy/rfcomm_channel.cc b/pw_bluetooth_proxy/rfcomm_channel.cc index 606c652d1..1732c7bb8 100644 --- a/pw_bluetooth_proxy/rfcomm_channel.cc +++ b/pw_bluetooth_proxy/rfcomm_channel.cc @@ -240,7 +240,8 @@ bool RfcommChannel::DoHandlePduFromController(pw::span l2cap_pdu) { if (rx_needs_refill) { // Send credit update with empty payload to refresh remote credit count. - if (const auto status = Write({}); !status.ok()) { + if (const auto status = Write(pw::multibuf::MultiBuf{}).status; + !status.ok()) { PW_LOG_ERROR("Failed to send RFCOMM credits"); } } diff --git a/pw_bluetooth_proxy/rfcomm_test.cc b/pw_bluetooth_proxy/rfcomm_test.cc index 33ab08a52..6b04fd9f7 100644 --- a/pw_bluetooth_proxy/rfcomm_test.cc +++ b/pw_bluetooth_proxy/rfcomm_test.cc @@ -190,7 +190,8 @@ TEST_F(RfcommWriteTest, BasicWrite) { .credits = 10, }}; RfcommChannel channel = BuildRfcomm(proxy, params); - EXPECT_EQ(channel.Write(capture.payload), PW_STATUS_OK); + PW_TEST_EXPECT_OK( + channel.Write(MultiBufFromSpan(pw::span(capture.payload))).status); EXPECT_EQ(capture.sends_called, 1); } @@ -302,7 +303,8 @@ TEST_F(RfcommWriteTest, ExtendedWrite) { .credits = 10, }}; RfcommChannel channel = BuildRfcomm(proxy, params); - EXPECT_EQ(channel.Write(capture.payload), PW_STATUS_OK); + PW_TEST_EXPECT_OK( + channel.Write(MultiBufFromSpan(pw::span(capture.payload))).status); EXPECT_EQ(capture.sends_called, 1); } @@ -346,7 +348,8 @@ TEST_F(RfcommWriteTest, WriteFlowControl) { // Writes while queue has space will return Ok. No RFCOMM credits yet though // so no sends complete. - EXPECT_EQ(channel.Write(capture.payload), PW_STATUS_OK); + PW_TEST_EXPECT_OK( + channel.Write(MultiBufFromSpan(pw::span(capture.payload))).status); EXPECT_EQ(capture.sends_called, 0); EXPECT_EQ(capture.queue_unblocked, 0); @@ -360,7 +363,8 @@ TEST_F(RfcommWriteTest, WriteFlowControl) { // Now fill up queue uint16_t queued = 0; while (true) { - if (const auto status = channel.Write(capture.payload); + if (const auto status = + channel.Write(MultiBufFromSpan(pw::span(capture.payload))).status; status == Status::Unavailable()) { break; }