pw_rpc: Remove PayloadBuffer() from the public API
The PayloadBuffer() function will be removed. ChannelOutput buffers will
only be used internally by the RPC system. This removes PayloadBuffer()
from the public API and exposes it only to its current upstream users.
Bug: 605
Change-Id: I54606d55711e8d24d6d0489a6b9f0c2b7871d7c1
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/79800
Pigweed-Auto-Submit: Wyatt Hepler <hepler@google.com>
Reviewed-by: Alexei Frolov <frolv@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
diff --git a/pw_file/flat_file_system.cc b/pw_file/flat_file_system.cc
index 61ffab8..fc15b4e 100644
--- a/pw_file/flat_file_system.cc
+++ b/pw_file/flat_file_system.cc
@@ -36,6 +36,12 @@
using Entry = FlatFileSystemService::Entry;
+// TODO(pwbug/605): Remove this hack for accessing the PayloadBuffer() API.
+class AccessHiddenFunctions : public rpc::RawServerWriter {
+ public:
+ using RawServerWriter::PayloadBuffer;
+};
+
Status FlatFileSystemService::EnumerateFile(
Entry& entry, pw::file::ListResponse::StreamEncoder& output_encoder) {
StatusWithSize sws = entry.Name(file_name_buffer_);
@@ -58,7 +64,8 @@
for (Entry* entry : entries_) {
PW_DCHECK_NOTNULL(entry);
// For now, don't try to pack entries.
- pw::file::ListResponse::MemoryEncoder encoder(writer.PayloadBuffer());
+ pw::file::ListResponse::MemoryEncoder encoder(
+ static_cast<AccessHiddenFunctions&>(writer).PayloadBuffer());
if (Status status = EnumerateFile(*entry, encoder); !status.ok()) {
if (status != Status::NotFound()) {
PW_LOG_ERROR("Failed to enumerate file (id: %u) with status %d",
@@ -101,7 +108,8 @@
return;
}
- pw::file::ListResponse::MemoryEncoder encoder(writer.PayloadBuffer());
+ pw::file::ListResponse::MemoryEncoder encoder(
+ static_cast<AccessHiddenFunctions&>(writer).PayloadBuffer());
Status proto_encode_status = EnumerateFile(*result.value(), encoder);
if (!proto_encode_status.ok()) {
writer.Finish(proto_encode_status);
diff --git a/pw_log_rpc/rpc_log_drain.cc b/pw_log_rpc/rpc_log_drain.cc
index 2f1003c..52efe6f 100644
--- a/pw_log_rpc/rpc_log_drain.cc
+++ b/pw_log_rpc/rpc_log_drain.cc
@@ -30,6 +30,14 @@
PW_TRY(encoder.status());
return ConstByteSpan(encoder);
}
+
+// TODO(pwbug/605): Remove this hack for accessing the PayloadBuffer() API.
+class AccessHiddenFunctions : public rpc::RawServerWriter {
+ public:
+ using RawServerWriter::PayloadBuffer;
+ using RawServerWriter::ReleaseBuffer;
+};
+
} // namespace
Status RpcLogDrain::Open(rpc::RawServerWriter& writer) {
@@ -53,7 +61,8 @@
if (!server_writer_.active()) {
return Status::Unavailable();
}
- log::LogEntries::MemoryEncoder encoder(server_writer_.PayloadBuffer());
+ log::LogEntries::MemoryEncoder encoder(
+ static_cast<AccessHiddenFunctions&>(server_writer_).PayloadBuffer());
uint32_t packed_entry_count = 0;
log_sink_state = EncodeOutgoingPacket(encoder, packed_entry_count);
// Avoid sending empty packets.
@@ -61,7 +70,7 @@
// Release buffer when still active to keep the writer in a replaceable
// state.
if (server_writer_.active()) {
- server_writer_.ReleaseBuffer();
+ static_cast<AccessHiddenFunctions&>(server_writer_).ReleaseBuffer();
}
continue;
}
diff --git a/pw_rpc/public/pw_rpc/writer.h b/pw_rpc/public/pw_rpc/writer.h
index 847205c..7dc9c2d 100644
--- a/pw_rpc/public/pw_rpc/writer.h
+++ b/pw_rpc/public/pw_rpc/writer.h
@@ -15,6 +15,14 @@
#include "pw_rpc/internal/call.h"
+namespace pw::transfer::internal {
+
+// TODO(pwbug/605): Remove this hack for giving pw_transfer access to the
+// PayloadBuffer() API.
+class Context;
+
+} // namespace pw::transfer::internal
+
namespace pw::rpc {
// The Writer class allows writing requests or responses to a streaming RPC.
@@ -35,11 +43,17 @@
using internal::Call::active;
using internal::Call::channel_id;
- using internal::Call::PayloadBuffer;
- using internal::Call::ReleasePayloadBuffer;
using internal::Call::Write;
+ private:
friend class internal::Call;
+
+ // TODO(pwbug/605): Remove this hack for giving pw_transfer access to the
+ // PayloadBuffer() API.
+ friend class transfer::internal::Context;
+
+ using internal::Call::PayloadBuffer;
+ using internal::Call::ReleasePayloadBuffer;
};
namespace internal {
diff --git a/pw_rpc/raw/codegen_test.cc b/pw_rpc/raw/codegen_test.cc
index 0dcbe1d..9a04ad6 100644
--- a/pw_rpc/raw/codegen_test.cc
+++ b/pw_rpc/raw/codegen_test.cc
@@ -74,10 +74,10 @@
if (request.empty()) {
last_responder_ = std::move(responder);
} else {
- ByteSpan response = responder.PayloadBuffer();
+ std::byte response[32] = {};
StatusWithSize sws = TestUnaryRpc(request, response);
- responder.Finish(response.first(sws.size()), sws.status());
+ responder.Finish(std::span(response).first(sws.size()), sws.status());
}
}
@@ -87,8 +87,7 @@
ASSERT_TRUE(DecodeRequest(request, integer, status));
for (int i = 0; i < integer; ++i) {
- ByteSpan buffer = writer.PayloadBuffer();
-
+ std::byte buffer[32] = {};
TestStreamResponse::MemoryEncoder test_stream_response(buffer);
EXPECT_EQ(OkStatus(), test_stream_response.WriteNumber(i));
EXPECT_EQ(OkStatus(), writer.Write(test_stream_response));
@@ -225,6 +224,12 @@
}
}
+// TODO(pwbug/605): Remove the PayloadBuffer() API.
+class TestResponder : public RawUnaryResponder {
+ public:
+ using RawUnaryResponder::PayloadBuffer;
+};
+
TEST(RawCodegen, Server_HandleErrorWhileHoldingBuffer) {
PW_RAW_TEST_METHOD_CONTEXT(test::TestService, TestAnotherUnaryRpc) ctx;
@@ -232,7 +237,9 @@
ctx.call({});
ASSERT_TRUE(ctx.service().last_responder().active());
- ASSERT_FALSE(ctx.service().last_responder().PayloadBuffer().empty());
+ ASSERT_FALSE(static_cast<TestResponder&>(ctx.service().last_responder())
+ .PayloadBuffer()
+ .empty());
ctx.SendClientError(Status::Unimplemented());
@@ -246,7 +253,9 @@
ctx.call({});
ASSERT_TRUE(ctx.service().last_responder().active());
- ASSERT_FALSE(ctx.service().last_responder().PayloadBuffer().empty());
+ ASSERT_FALSE(static_cast<TestResponder&>(ctx.service().last_responder())
+ .PayloadBuffer()
+ .empty());
ctx.service().last_responder().Finish({});
@@ -260,7 +269,9 @@
RawUnaryResponder call;
ctx.call({});
- ASSERT_FALSE(ctx.service().last_responder().PayloadBuffer().empty());
+ ASSERT_FALSE(static_cast<TestResponder&>(ctx.service().last_responder())
+ .PayloadBuffer()
+ .empty());
call = std::move(ctx.service().last_responder());
}
@@ -274,10 +285,14 @@
ctx_2.set_channel_id(2);
ctx_1.call({});
- ASSERT_FALSE(ctx_1.service().last_responder().PayloadBuffer().empty());
+ ASSERT_FALSE(static_cast<TestResponder&>(ctx_1.service().last_responder())
+ .PayloadBuffer()
+ .empty());
ctx_2.call({});
- ASSERT_FALSE(ctx_2.service().last_responder().PayloadBuffer().empty());
+ ASSERT_FALSE(static_cast<TestResponder&>(ctx_2.service().last_responder())
+ .PayloadBuffer()
+ .empty());
ctx_1.service().last_responder() =
std::move(ctx_2.service().last_responder());
diff --git a/pw_rpc/raw/method_test.cc b/pw_rpc/raw/method_test.cc
index 56a13fe..6ab5cfd 100644
--- a/pw_rpc/raw/method_test.cc
+++ b/pw_rpc/raw/method_test.cc
@@ -266,11 +266,19 @@
encoded.value().size()));
}
+// TODO(pwbug/605): Remove this hack for accessing the PayloadBuffer() API.
+class AccessHiddenFunctions : public rpc::RawServerWriter {
+ public:
+ using RawServerWriter::PayloadBuffer;
+};
+
TEST(RawServerWriter, Write_SendsPreviouslyAcquiredBuffer) {
ServerContextForTest<FakeService> context(kServerStream);
kServerStream.Invoke(context.get(), context.request({}));
- auto buffer = context.service().last_writer.PayloadBuffer();
+ auto buffer =
+ static_cast<AccessHiddenFunctions&>(context.service().last_writer)
+ .PayloadBuffer();
constexpr auto data = bytes::Array<0x0d, 0x06, 0xf0, 0x0d>();
std::memcpy(buffer.data(), data.data(), data.size());
@@ -344,7 +352,7 @@
{
RawServerWriter writer = std::move(context.service().last_writer);
- auto buffer = writer.PayloadBuffer();
+ auto buffer = static_cast<AccessHiddenFunctions&>(writer).PayloadBuffer();
buffer[0] = std::byte{'!'};
// Don't release the buffer.
}
diff --git a/pw_rpc/raw/public/pw_rpc/raw/server_reader_writer.h b/pw_rpc/raw/public/pw_rpc/raw/server_reader_writer.h
index c9c4dc3..baa67a1 100644
--- a/pw_rpc/raw/public/pw_rpc/raw/server_reader_writer.h
+++ b/pw_rpc/raw/public/pw_rpc/raw/server_reader_writer.h
@@ -81,12 +81,6 @@
// external buffer.
using internal::Call::Write;
- // Returns a buffer in which a response payload can be built.
- using internal::Call::PayloadBuffer;
-
- // Releases a buffer acquired from PayloadBuffer() without sending any data.
- void ReleaseBuffer() { ReleasePayloadBuffer(); }
-
Status Finish(Status status = OkStatus()) {
return CloseAndSendResponse(status);
}
@@ -102,6 +96,13 @@
using internal::Call::CloseAndSendResponse;
+ // TODO(pwbug/605): Remove PayloadBuffer() and ReleaseBuffer().
+ // Returns a buffer in which a response payload can be built.
+ using internal::Call::PayloadBuffer;
+
+ // Releases a buffer acquired from PayloadBuffer() without sending any data.
+ void ReleaseBuffer() { ReleasePayloadBuffer(); }
+
private:
friend class internal::RawMethod; // Needed to construct
@@ -141,8 +142,6 @@
using RawServerReaderWriter::set_on_error;
using RawServerReaderWriter::set_on_next;
- using RawServerReaderWriter::PayloadBuffer;
-
Status Finish(ConstByteSpan response, Status status = OkStatus()) {
return CloseAndSendResponse(response, status);
}
@@ -187,14 +186,17 @@
using RawServerReaderWriter::set_on_error;
using RawServerReaderWriter::Finish;
- using RawServerReaderWriter::PayloadBuffer;
- using RawServerReaderWriter::ReleaseBuffer;
using RawServerReaderWriter::Write;
// Allow use as a generic RPC Writer.
using internal::Call::operator Writer&;
using internal::Call::operator const Writer&;
+ protected:
+ // TODO(pwbug/605): Remove PayloadBuffer() and ReleaseBuffer().
+ using RawServerReaderWriter::PayloadBuffer;
+ using RawServerReaderWriter::ReleaseBuffer;
+
private:
template <typename, typename, uint32_t>
friend class internal::test::InvocationContext;
@@ -234,13 +236,14 @@
using RawServerReaderWriter::set_on_error;
- using RawServerReaderWriter::PayloadBuffer;
- using RawServerReaderWriter::ReleaseBuffer;
-
Status Finish(ConstByteSpan response, Status status = OkStatus()) {
return CloseAndSendResponse(response, status);
}
+ protected:
+ // TODO(pwbug/605): Remove PayloadBuffer().
+ using RawServerReaderWriter::PayloadBuffer;
+
private:
template <typename, typename, uint32_t>
friend class internal::test::InvocationContext;
diff --git a/pw_rpc/raw/public/pw_rpc/raw/test_method_context.h b/pw_rpc/raw/public/pw_rpc/raw/test_method_context.h
index a20e677..57f9e33 100644
--- a/pw_rpc/raw/public/pw_rpc/raw/test_method_context.h
+++ b/pw_rpc/raw/public/pw_rpc/raw/test_method_context.h
@@ -137,16 +137,18 @@
UnaryContext(Args&&... args) : Base(std::forward<Args>(args)...) {}
// Invokes the RPC with the provided request. Returns RPC's StatusWithSize.
+ template <size_t kSynchronousResponseBufferSizeBytes = 64>
auto call(ConstByteSpan request) {
if constexpr (MethodTraits<decltype(kMethod)>::kSynchronous) {
Base::output().clear();
auto responder = Base::template GetResponder<RawUnaryResponder>();
- ByteSpan response = responder.PayloadBuffer();
- auto sws =
- CallMethodImplFunction<kMethod>(Base::service(), request, response);
+ std::byte response[kSynchronousResponseBufferSizeBytes] = {};
+ auto sws = CallMethodImplFunction<kMethod>(
+ Base::service(), request, std::span(response));
PW_ASSERT(
- responder.Finish(response.first(sws.size()), sws.status()).ok());
+ responder.Finish(std::span(response).first(sws.size()), sws.status())
+ .ok());
return sws;
} else {
Base::template call<kMethod, RawUnaryResponder>(request);
diff --git a/pw_rpc/raw/server_reader_writer_test.cc b/pw_rpc/raw/server_reader_writer_test.cc
index 6be3174..6d28acb 100644
--- a/pw_rpc/raw/server_reader_writer_test.cc
+++ b/pw_rpc/raw/server_reader_writer_test.cc
@@ -146,13 +146,21 @@
EXPECT_EQ(completions.back(), OkStatus());
}
+// TODO(pwbug/605): Remove the PayloadBuffer() API.
+template <typename T>
+class AccessHidden : public T {
+ public:
+ using T::PayloadBuffer;
+};
+
TEST(RawUnaryResponder, Move_DifferentActiveCalls_ClosesFirstOnly) {
ReaderWriterTestContext ctx;
RawUnaryResponder active_call =
RawUnaryResponder::Open<TestService::TestUnaryRpc>(
ctx.server, ctx.channel.id(), ctx.service);
- std::span buffer = active_call.PayloadBuffer();
+ std::span buffer = static_cast<AccessHidden<RawUnaryResponder>&>(active_call)
+ .PayloadBuffer();
ASSERT_FALSE(buffer.empty());
RawUnaryResponder new_active_call =
@@ -178,7 +186,8 @@
RawUnaryResponder::Open<TestService::TestUnaryRpc>(
ctx.server, ctx.channel.id(), ctx.service);
- std::span buffer = active_call.PayloadBuffer();
+ std::span buffer = static_cast<AccessHidden<RawUnaryResponder>&>(active_call)
+ .PayloadBuffer();
constexpr const char kData[] = "Some data!";
ASSERT_GE(buffer.size(), sizeof(kData));
std::memcpy(buffer.data(), kData, sizeof(kData));
@@ -223,7 +232,10 @@
RawServerWriter::Open<TestService::TestServerStreamRpc>(
ctx.server, ctx.channel.id(), ctx.service);
- EXPECT_GT(active_call.PayloadBuffer().size(), 0u);
+ EXPECT_GT(static_cast<AccessHidden<RawServerWriter>&>(active_call)
+ .PayloadBuffer()
+ .size(),
+ 0u);
RawServerWriter inactive_call;
@@ -241,7 +253,8 @@
RawServerWriter::Open<TestService::TestServerStreamRpc>(
ctx.server, ctx.channel.id(), ctx.service);
- std::span buffer = active_call.PayloadBuffer();
+ std::span buffer =
+ static_cast<AccessHidden<RawServerWriter>&>(active_call).PayloadBuffer();
constexpr const char kData[] = "Some data!";
ASSERT_GE(buffer.size(), sizeof(kData));
std::memcpy(buffer.data(), kData, sizeof(kData));
diff --git a/pw_transfer/client_connection.cc b/pw_transfer/client_connection.cc
index b4425ae..cef565d 100644
--- a/pw_transfer/client_connection.cc
+++ b/pw_transfer/client_connection.cc
@@ -21,6 +21,13 @@
namespace pw::transfer::internal {
+// TODO(pwbug/605): Remove this hack for accessing the PayloadBuffer() API.
+class AccessHiddenFunctions : public rpc::RawServerReaderWriter {
+ public:
+ using RawServerReaderWriter::PayloadBuffer;
+ using RawServerReaderWriter::ReleaseBuffer;
+};
+
void ClientConnection::SendStatusChunk(TransferType type,
uint32_t transfer_id,
Status status) {
@@ -30,13 +37,13 @@
rpc::RawServerReaderWriter& destination = stream(type);
- Result<ConstByteSpan> result =
- internal::EncodeChunk(chunk, destination.PayloadBuffer());
+ Result<ConstByteSpan> result = internal::EncodeChunk(
+ chunk, static_cast<AccessHiddenFunctions&>(destination).PayloadBuffer());
if (!result.ok()) {
PW_LOG_ERROR("Failed to encode final chunk for transfer %u",
static_cast<unsigned>(transfer_id));
- destination.ReleaseBuffer();
+ static_cast<AccessHiddenFunctions&>(destination).ReleaseBuffer();
return;
}
diff --git a/pw_unit_test/public/pw_unit_test/unit_test_service.h b/pw_unit_test/public/pw_unit_test/unit_test_service.h
index 88ab79b..315ad83 100644
--- a/pw_unit_test/public/pw_unit_test/unit_test_service.h
+++ b/pw_unit_test/public/pw_unit_test/unit_test_service.h
@@ -35,7 +35,14 @@
// migrated to it.
template <typename WriteFunction>
void WriteEvent(WriteFunction event_writer) {
- Event::MemoryEncoder event(writer_.PayloadBuffer());
+ // TODO(pwbug/605): Remove use of PayloadBuffer().
+ class AccessHiddenFunction : public rpc::RawServerWriter {
+ public:
+ using RawServerWriter::PayloadBuffer;
+ };
+
+ Event::MemoryEncoder event(
+ static_cast<AccessHiddenFunction&>(writer_).PayloadBuffer());
event_writer(event);
if (event.status().ok()) {
writer_.Write(event)
