pw_rpc: Update packet class to return results

Following the pw_protobuf change updating the encode function to return
a result, this change forwards that through the RPC packet encoder.
The Packet::FromBuffer decoder is also updated to match.

Change-Id: I402e6d5c81b69758ca334960d872ad9639868bc6
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/20840
Commit-Queue: Alexei Frolov <frolv@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>

11 files changed, 68 insertions, 67 deletions

diff --git a/pw_rpc/base_server_writer_test.cc b/pw_rpc/base_server_writer_test.cc
index bb6b8a22a..f95a2dd31 100644
--- a/pw_rpc/base_server_writer_test.cc
+++ b/pw_rpc/base_server_writer_test.cc
@@ -139,12 +139,14 @@ TEST(ServerWriter, Open_SendsPacketWithPayload) {
   ASSERT_EQ(Status::Ok(), writer.Write(data));
 
   byte encoded[64];
-  auto sws = context.packet(data).Encode(encoded);
-  ASSERT_EQ(Status::Ok(), sws.status());
+  auto result = context.packet(data).Encode(encoded);
+  ASSERT_EQ(Status::Ok(), result.status());
 
-  EXPECT_EQ(sws.size(), context.output().sent_data().size());
+  EXPECT_EQ(result.value().size(), context.output().sent_data().size());
   EXPECT_EQ(
-      0, std::memcmp(encoded, context.output().sent_data().data(), sws.size()));
+      0,
+      std::memcmp(
+          encoded, context.output().sent_data().data(), result.value().size()));
 }
 
 TEST(ServerWriter, Closed_IgnoresPacket) {
diff --git a/pw_rpc/channel.cc b/pw_rpc/channel.cc
index 8771109d5..fb5eab2fa 100644
--- a/pw_rpc/channel.cc
+++ b/pw_rpc/channel.cc
@@ -28,7 +28,7 @@ std::span<byte> Channel::OutputBuffer::payload(const Packet& packet) const {
 }
 
 Status Channel::Send(OutputBuffer& buffer, const internal::Packet& packet) {
-  StatusWithSize encoded = packet.Encode(buffer.buffer_);
+  Result encoded = packet.Encode(buffer.buffer_);
   buffer.buffer_ = {};
 
   if (!encoded.ok()) {
@@ -37,7 +37,7 @@ Status Channel::Send(OutputBuffer& buffer, const internal::Packet& packet) {
     return Status::Internal();
   }
 
-  return output().SendAndReleaseBuffer(encoded.size());
+  return output().SendAndReleaseBuffer(encoded.value().size());
 }
 
 }  // namespace pw::rpc::internal
diff --git a/pw_rpc/client.cc b/pw_rpc/client.cc
index 214a2085e..b801e7865 100644
--- a/pw_rpc/client.cc
+++ b/pw_rpc/client.cc
@@ -27,13 +27,14 @@ using internal::PacketType;
 }  // namespace
 
 Status Client::ProcessPacket(ConstByteSpan data) {
-  Packet packet;
-
-  if (Status status = Packet::FromBuffer(data, packet); !status.ok()) {
+  Result<Packet> result = Packet::FromBuffer(data);
+  if (!result.ok()) {
     PW_LOG_WARN("RPC client failed to decode incoming packet");
     return Status::DataLoss();
   }
 
+  Packet& packet = result.value();
+
   if (packet.destination() != Packet::kClient) {
     return Status::InvalidArgument();
   }
diff --git a/pw_rpc/nanopb/nanopb_method_test.cc b/pw_rpc/nanopb/nanopb_method_test.cc
index a98ea509c..05e30fc64 100644
--- a/pw_rpc/nanopb/nanopb_method_test.cc
+++ b/pw_rpc/nanopb/nanopb_method_test.cc
@@ -178,11 +178,11 @@ TEST(NanopbMethod, ServerWriter_SendsResponse) {
   auto encoded = context.packet(payload).Encode(encoded_response);
   ASSERT_EQ(Status::Ok(), encoded.status());
 
-  ASSERT_EQ(encoded.size(), context.output().sent_data().size());
+  ASSERT_EQ(encoded.value().size(), context.output().sent_data().size());
   EXPECT_EQ(0,
-            std::memcmp(encoded_response.data(),
+            std::memcmp(encoded.value().data(),
                         context.output().sent_data().data(),
-                        encoded.size()));
+                        encoded.value().size()));
 }
 
 TEST(NanopbMethod,
@@ -201,7 +201,7 @@ TEST(NanopbMethod,
   std::array<byte, 128> encoded_response = {};
   auto encoded = context.packet({}).Encode(encoded_response);
   ASSERT_EQ(Status::Ok(), encoded.status());
-  ASSERT_EQ(kNoPayloadPacketSize, encoded.size());
+  ASSERT_EQ(kNoPayloadPacketSize, encoded.value().size());
 
   method.Invoke(context.get(), context.packet({}));
 
diff --git a/pw_rpc/nanopb/public/pw_rpc/test_method_context.h b/pw_rpc/nanopb/public/pw_rpc/test_method_context.h
index c30941b73..246d9665a 100644
--- a/pw_rpc/nanopb/public/pw_rpc/test_method_context.h
+++ b/pw_rpc/nanopb/public/pw_rpc/test_method_context.h
@@ -248,18 +248,18 @@ Status MessageOutput<Response>::SendAndReleaseBuffer(size_t size) {
     return Status::Ok();
   }
 
-  internal::Packet packet;
-  PW_CHECK_OK(
-      internal::Packet::FromBuffer(std::span(buffer_.data(), size), packet));
+  Result<internal::Packet> result =
+      internal::Packet::FromBuffer(std::span(buffer_.data(), size));
 
-  last_status_ = packet.status();
+  last_status_ = result.status();
 
-  switch (packet.type()) {
+  switch (result.value().type()) {
     case internal::PacketType::RESPONSE:
       // If we run out of space, the back message is always the most recent.
       responses_.emplace_back();
       responses_.back() = {};
-      PW_CHECK(method_.DecodeResponse(packet.payload(), &responses_.back()));
+      PW_CHECK(
+          method_.DecodeResponse(result.value().payload(), &responses_.back()));
       total_responses_ += 1;
       break;
     case internal::PacketType::SERVER_STREAM_END:
diff --git a/pw_rpc/packet.cc b/pw_rpc/packet.cc
index d99cd8497..e9270cd12 100644
--- a/pw_rpc/packet.cc
+++ b/pw_rpc/packet.cc
@@ -20,12 +20,10 @@ namespace pw::rpc::internal {
 
 using std::byte;
 
-Status Packet::FromBuffer(std::span<const byte> data, Packet& packet) {
-  packet = Packet();
-
-  protobuf::Decoder decoder(data);
-
+Result<Packet> Packet::FromBuffer(ConstByteSpan data) {
+  Packet packet;
   Status status;
+  protobuf::Decoder decoder(data);
 
   while ((status = decoder.Next()).ok()) {
     RpcPacket::Fields field =
@@ -64,10 +62,14 @@ Status Packet::FromBuffer(std::span<const byte> data, Packet& packet) {
     }
   }
 
-  return status == Status::DataLoss() ? Status::DataLoss() : Status::Ok();
+  if (status == Status::DataLoss()) {
+    return status;
+  }
+
+  return packet;
 }
 
-StatusWithSize Packet::Encode(std::span<byte> buffer) const {
+Result<ConstByteSpan> Packet::Encode(ByteSpan buffer) const {
   pw::protobuf::NestedEncoder encoder(buffer);
   RpcPacket::Encoder rpc_packet(&encoder);
 
@@ -80,12 +82,7 @@ StatusWithSize Packet::Encode(std::span<byte> buffer) const {
   rpc_packet.WriteMethodId(method_id_);
   rpc_packet.WriteStatus(status_);
 
-  Result result = encoder.Encode();
-  if (!result.ok()) {
-    return StatusWithSize(result.status(), 0);
-  }
-
-  return StatusWithSize(result.value().size());
+  return encoder.Encode();
 }
 
 size_t Packet::MinEncodedSizeBytes() const {
diff --git a/pw_rpc/packet_test.cc b/pw_rpc/packet_test.cc
index 2fd53a215..9f6d7ea75 100644
--- a/pw_rpc/packet_test.cc
+++ b/pw_rpc/packet_test.cc
@@ -75,8 +75,9 @@ TEST(Packet, Encode) {
 
   Packet packet(PacketType::RESPONSE, 1, 42, 100, kPayload);
 
-  auto sws = packet.Encode(buffer);
-  ASSERT_EQ(kEncoded.size(), sws.size());
+  auto result = packet.Encode(buffer);
+  ASSERT_EQ(Status::Ok(), result.status());
+  ASSERT_EQ(kEncoded.size(), result.value().size());
   EXPECT_EQ(std::memcmp(kEncoded.data(), buffer, kEncoded.size()), 0);
 }
 
@@ -85,15 +86,15 @@ TEST(Packet, Encode_BufferTooSmall) {
 
   Packet packet(PacketType::RESPONSE, 1, 42, 100, kPayload);
 
-  auto sws = packet.Encode(buffer);
-  EXPECT_EQ(0u, sws.size());
-  EXPECT_EQ(Status::ResourceExhausted(), sws.status());
+  auto result = packet.Encode(buffer);
+  EXPECT_EQ(Status::ResourceExhausted(), result.status());
 }
 
 TEST(Packet, Decode_ValidPacket) {
-  Packet packet;
-  ASSERT_EQ(Status::Ok(), Packet::FromBuffer(kEncoded, packet));
+  auto result = Packet::FromBuffer(kEncoded);
+  ASSERT_TRUE(result.ok());
 
+  auto& packet = result.value();
   EXPECT_EQ(PacketType::RESPONSE, packet.type());
   EXPECT_EQ(1u, packet.channel_id());
   EXPECT_EQ(42u, packet.service_id());
@@ -106,9 +107,7 @@ TEST(Packet, Decode_ValidPacket) {
 
 TEST(Packet, Decode_InvalidPacket) {
   byte bad_data[] = {byte{0xFF}, byte{0x00}, byte{0x00}, byte{0xFF}};
-
-  Packet packet;
-  EXPECT_EQ(Status::DataLoss(), Packet::FromBuffer(bad_data, packet));
+  EXPECT_EQ(Status::DataLoss(), Packet::FromBuffer(bad_data).status());
 }
 
 TEST(Packet, EncodeDecode) {
@@ -122,13 +121,14 @@ TEST(Packet, EncodeDecode) {
   packet.set_status(Status::Unavailable());
 
   byte buffer[128];
-  StatusWithSize sws = packet.Encode(buffer);
-  ASSERT_EQ(sws.status(), Status::Ok());
+  Result result = packet.Encode(buffer);
+  ASSERT_EQ(result.status(), Status::Ok());
 
-  std::span<byte> packet_data(buffer, sws.size());
-  Packet decoded;
-  ASSERT_EQ(Status::Ok(), Packet::FromBuffer(packet_data, decoded));
+  std::span<byte> packet_data(buffer, result.value().size());
+  auto decode_result = Packet::FromBuffer(packet_data);
+  ASSERT_TRUE(decode_result.ok());
 
+  auto& decoded = decode_result.value();
   EXPECT_EQ(decoded.type(), packet.type());
   EXPECT_EQ(decoded.channel_id(), packet.channel_id());
   EXPECT_EQ(decoded.service_id(), packet.service_id());
diff --git a/pw_rpc/public/pw_rpc/internal/packet.h b/pw_rpc/public/pw_rpc/internal/packet.h
index db7ce57c1..c67d86ab2 100644
--- a/pw_rpc/public/pw_rpc/internal/packet.h
+++ b/pw_rpc/public/pw_rpc/internal/packet.h
@@ -17,6 +17,7 @@
 #include <cstdint>
 #include <span>
 
+#include "pw_bytes/span.h"
 #include "pw_rpc_protos/packet.pwpb.h"
 #include "pw_status/status_with_size.h"
 
@@ -28,7 +29,7 @@ class Packet {
 
   // Parses a packet from a protobuf message. Missing or malformed fields take
   // their default values.
-  static Status FromBuffer(std::span<const std::byte> data, Packet& packet);
+  static Result<Packet> FromBuffer(ConstByteSpan data);
 
   // Creates an RPC packet with the channel, service, and method ID of the
   // provided packet.
@@ -72,7 +73,7 @@ class Packet {
                    uint32_t channel_id,
                    uint32_t service_id,
                    uint32_t method_id,
-                   std::span<const std::byte> payload = {},
+                   ConstByteSpan payload = {},
                    Status status = Status::Ok())
       : type_(type),
         channel_id_(channel_id),
@@ -82,7 +83,7 @@ class Packet {
         status_(status) {}
 
   // Encodes the packet into its wire format. Returns the encoded size.
-  StatusWithSize Encode(std::span<std::byte> buffer) const;
+  Result<ConstByteSpan> Encode(ByteSpan buffer) const;
 
   // Determines the space required to encode the packet proto fields for a
   // response, excluding the payload. This may be used to split the buffer into
@@ -99,9 +100,7 @@ class Packet {
   constexpr uint32_t channel_id() const { return channel_id_; }
   constexpr uint32_t service_id() const { return service_id_; }
   constexpr uint32_t method_id() const { return method_id_; }
-  constexpr const std::span<const std::byte>& payload() const {
-    return payload_;
-  }
+  constexpr const ConstByteSpan& payload() const { return payload_; }
   constexpr Status status() const { return status_; }
 
   constexpr void set_type(PacketType type) { type_ = type; }
@@ -112,9 +111,7 @@ class Packet {
     service_id_ = service_id;
   }
   constexpr void set_method_id(uint32_t method_id) { method_id_ = method_id; }
-  constexpr void set_payload(std::span<const std::byte> payload) {
-    payload_ = payload;
-  }
+  constexpr void set_payload(ConstByteSpan payload) { payload_ = payload; }
   constexpr void set_status(Status status) { status_ = status; }
 
  private:
@@ -122,7 +119,7 @@ class Packet {
   uint32_t channel_id_;
   uint32_t service_id_;
   uint32_t method_id_;
-  std::span<const std::byte> payload_;
+  ConstByteSpan payload_;
   Status status_;
 };
 
diff --git a/pw_rpc/pw_rpc_private/internal_test_utils.h b/pw_rpc/pw_rpc_private/internal_test_utils.h
index aeb2237ad..530f68110 100644
--- a/pw_rpc/pw_rpc_private/internal_test_utils.h
+++ b/pw_rpc/pw_rpc_private/internal_test_utils.h
@@ -46,8 +46,9 @@ class TestOutput : public ChannelOutput {
 
     packet_count_ += 1;
     sent_data_ = std::span(buffer_.data(), size);
-    Status status = internal::Packet::FromBuffer(sent_data_, sent_packet_);
-    EXPECT_EQ(Status::Ok(), status);
+    Result<internal::Packet> result = internal::Packet::FromBuffer(sent_data_);
+    EXPECT_EQ(Status::Ok(), result.status());
+    sent_packet_ = result.value_or(internal::Packet());
     return send_status_;
   }
 
@@ -149,9 +150,9 @@ class ClientContextForTest {
     internal::Packet packet(
         type, kChannelId, kServiceId, kMethodId, payload, status);
     std::byte buffer[input_buffer_size];
-    StatusWithSize sws = packet.Encode(buffer);
-    EXPECT_EQ(sws.status(), Status::Ok());
-    return client_.ProcessPacket(std::span(buffer, sws.size()));
+    Result result = packet.Encode(buffer);
+    EXPECT_EQ(result.status(), Status::Ok());
+    return client_.ProcessPacket(result.value_or(ConstByteSpan()));
   }
 
   Status SendResponse(Status status, std::span<const std::byte> payload) {
diff --git a/pw_rpc/server.cc b/pw_rpc/server.cc
index e288bfbc1..845a64f8a 100644
--- a/pw_rpc/server.cc
+++ b/pw_rpc/server.cc
@@ -32,11 +32,14 @@ using internal::PacketType;
 bool DecodePacket(ChannelOutput& interface,
                   std::span<const byte> data,
                   Packet& packet) {
-  if (Status status = Packet::FromBuffer(data, packet); !status.ok()) {
+  Result<Packet> result = Packet::FromBuffer(data);
+  if (!result.ok()) {
     PW_LOG_WARN("Failed to decode packet on interface %s", interface.name());
     return false;
   }
 
+  packet = result.value();
+
   // If the packet is malformed, don't try to process it.
   if (packet.channel_id() == Channel::kUnassignedChannelId ||
       packet.service_id() == 0 || packet.method_id() == 0) {
diff --git a/pw_rpc/server_test.cc b/pw_rpc/server_test.cc
index 81c9866e1..a49e24487 100644
--- a/pw_rpc/server_test.cc
+++ b/pw_rpc/server_test.cc
@@ -79,10 +79,10 @@ class BasicServer : public ::testing::Test {
       uint32_t service_id,
       uint32_t method_id,
       std::span<const byte> payload = kDefaultPayload) {
-    auto sws = Packet(type, channel_id, service_id, method_id, payload)
-                   .Encode(request_buffer_);
-    EXPECT_EQ(Status::Ok(), sws.status());
-    return std::span(request_buffer_, sws.size());
+    auto result = Packet(type, channel_id, service_id, method_id, payload)
+                      .Encode(request_buffer_);
+    EXPECT_EQ(Status::Ok(), result.status());
+    return result.value_or(ConstByteSpan());
   }
 
   TestOutput<128> output_;