pw_bluetooth_proxy: Allow Write(MultiBuf) for rfcomm and basic

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 <[email protected]> Lint: Lint 🤖 <[email protected]> Commit-Queue: David Rees <[email protected]> Docs-Not-Needed: David Rees <[email protected]> Reviewed-by: Austin Foxley <[email protected]>
google · Jan 8, 2025 · 56d9d44 · 56d9d44
1 parent b129282
commit 56d9d44
Show file tree

Hide file tree

Showing 8 changed files with 85 additions and 17 deletions.
diff --git 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<const uint8_t> AsConstUint8Span(ConstByteSpan s) {
+  return {reinterpret_cast<const uint8_t*>(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<ByteSpan> 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
@@ -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<uint16_t, uint16_t, 1>({{{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
@@ -39,7 +39,17 @@ class BasicL2capChannel : public L2capChannel {
   BasicL2capChannel& operator=(BasicL2capChannel&& other) = default;
   ~BasicL2capChannel() override;
 
-  pw::Status Write(pw::span<const uint8_t> 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<const uint8_t> 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
@@ -236,6 +236,17 @@ class L2capChannel : public IntrusiveForwardList<L2capChannel>::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
@@ -87,8 +87,18 @@ class RfcommChannel final : public L2capChannel {
       Function<void(pw::span<uint8_t> payload)>&& payload_from_controller_fn,
       Function<void(L2capChannelEvent event)>&& 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<const uint8_t> 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
@@ -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</*kDataSizeBytes=*/512,
+  pw::multibuf::test::SimpleAllocatorForTest</*kDataSizeBytes=*/2048,
                                              /*kMetaSizeBytes=*/512>
       test_multibuf_allocator_{};
 

diff --git a/pw_bluetooth_proxy/rfcomm_channel.cc b/pw_bluetooth_proxy/rfcomm_channel.cc
@@ -240,7 +240,8 @@ bool RfcommChannel::DoHandlePduFromController(pw::span<uint8_t> 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
@@ -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;
     }