pw_rpc: Make Service independent of Method
Rather than depending on the concrete Method implementation, store a
BaseMethod* and the method implementation's size.
Change-Id: Ic52a9a7526f542ab872ed6a14be5c93e93dd95f1
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/18240
Commit-Queue: Wyatt Hepler <hepler@google.com>
Reviewed-by: Alexei Frolov <frolv@google.com>
diff --git a/pw_rpc/BUILD b/pw_rpc/BUILD
index 1d5a7db..0378967 100644
--- a/pw_rpc/BUILD
+++ b/pw_rpc/BUILD
@@ -154,3 +154,15 @@
"//pw_assert",
],
)
+
+pw_cc_test(
+ name = "service_test",
+ srcs = [
+ "service_test.cc",
+ ],
+ deps = [
+ ":common",
+ ":internal_test_utils",
+ "//pw_assert",
+ ],
+)
diff --git a/pw_rpc/BUILD.gn b/pw_rpc/BUILD.gn
index 8796f26..2c7f67e 100644
--- a/pw_rpc/BUILD.gn
+++ b/pw_rpc/BUILD.gn
@@ -79,10 +79,11 @@
# Provides the public RPC service definition (but not implementation). Can be
# used to expose global service registration without depending on the complete
# RPC library.
-pw_source_set("service") {
- public_configs = [ ":default_config" ]
- public_deps = [ "$dir_pw_containers:intrusive_list" ]
- public = [ "public/pw_rpc/service.h" ]
+# TODO(hepler): Remove this after making the RPC implementation classes fully
+# independent of the particular implementation. Targets needing the service
+# should be able to depend on the main RPC library.
+group("service") {
+ deps = [ ":common" ]
}
# Put these dependencies into a group since they need to be shared by the server
@@ -103,12 +104,15 @@
public_configs = [ ":default_config" ]
public_deps = [
":protos_pwpb",
+ "$dir_pw_containers:intrusive_list",
dir_pw_assert,
- dir_pw_log,
dir_pw_span,
dir_pw_status,
]
- public = [ "public/pw_rpc/channel.h" ]
+ public = [
+ "public/pw_rpc/channel.h",
+ "public/pw_rpc/service.h",
+ ]
sources = [
"channel.cc",
"packet.cc",
@@ -118,6 +122,7 @@
"public/pw_rpc/internal/hash.h",
"public/pw_rpc/internal/packet.h",
]
+ deps = [ dir_pw_log ]
friend = [ "./*" ]
}
@@ -166,6 +171,7 @@
":channel_test",
":packet_test",
":server_test",
+ ":service_test",
]
group_deps = [ "nanopb:tests" ]
}
@@ -206,6 +212,16 @@
sources = [ "packet_test.cc" ]
}
+pw_test("service_test") {
+ deps = [
+ ":common",
+ ":protos_pwpb",
+ ":test_server",
+ dir_pw_assert,
+ ]
+ sources = [ "service_test.cc" ]
+}
+
pw_test("server_test") {
deps = [
":internal_test_utils_test_server",
diff --git a/pw_rpc/base_server_writer_test.cc b/pw_rpc/base_server_writer_test.cc
index 6e9d00c..0b4b601 100644
--- a/pw_rpc/base_server_writer_test.cc
+++ b/pw_rpc/base_server_writer_test.cc
@@ -15,6 +15,7 @@
#include "pw_rpc/internal/base_server_writer.h"
#include <algorithm>
+#include <array>
#include <cstdint>
#include <cstring>
@@ -27,8 +28,7 @@
class TestService : public Service {
public:
- constexpr TestService(uint32_t id)
- : Service(id, std::span(&method, 1)), method(8) {}
+ constexpr TestService(uint32_t id) : Service(id, method), method(8) {}
internal::Method method;
};
diff --git a/pw_rpc/nanopb/BUILD.gn b/pw_rpc/nanopb/BUILD.gn
index 223ab01..626e00c 100644
--- a/pw_rpc/nanopb/BUILD.gn
+++ b/pw_rpc/nanopb/BUILD.gn
@@ -34,6 +34,7 @@
public = [ "public_overrides/pw_rpc/internal/method.h" ]
sources = [ "method.cc" ]
public_deps = [ "..:server_library_deps" ]
+ deps = [ dir_pw_log ]
if (dir_pw_third_party_nanopb != "") {
public_deps += [ "$dir_pw_third_party/nanopb" ]
diff --git a/pw_rpc/public/pw_rpc/server.h b/pw_rpc/public/pw_rpc/server.h
index 6f9adb0..e799a0a 100644
--- a/pw_rpc/public/pw_rpc/server.h
+++ b/pw_rpc/public/pw_rpc/server.h
@@ -19,6 +19,7 @@
#include "pw_containers/intrusive_list.h"
#include "pw_rpc/channel.h"
+#include "pw_rpc/internal/base_method.h"
#include "pw_rpc/internal/base_server_writer.h"
#include "pw_rpc/internal/channel.h"
#include "pw_rpc/service.h"
@@ -55,7 +56,7 @@
IntrusiveList<internal::BaseServerWriter>& writers() { return writers_; }
private:
- std::tuple<Service*, const internal::Method*> FindMethod(
+ std::tuple<Service*, const internal::BaseMethod*> FindMethod(
const internal::Packet& packet);
void HandleCancelPacket(const internal::Packet& request,
diff --git a/pw_rpc/public/pw_rpc/service.h b/pw_rpc/public/pw_rpc/service.h
index 2076896..dd05c0c 100644
--- a/pw_rpc/public/pw_rpc/service.h
+++ b/pw_rpc/public/pw_rpc/service.h
@@ -14,37 +14,50 @@
#pragma once
#include <cstdint>
+#include <limits>
#include <span>
-#include <utility>
#include "pw_containers/intrusive_list.h"
+#include "pw_rpc/internal/base_method.h"
namespace pw::rpc {
-namespace internal {
-
-class Method;
-
-} // namespace internal
// Base class for all RPC services. This cannot be instantiated directly; use a
// generated subclass instead.
+//
+// Services store a span of concrete method classes. To support different Method
+// implementations, Service stores as base Method* and the size of the concrete
+// method object.
class Service : public IntrusiveList<Service>::Item {
public:
uint32_t id() const { return id_; }
protected:
+ template <typename T, size_t method_count>
+ constexpr Service(uint32_t id, const std::array<T, method_count>& methods)
+ : id_(id),
+ methods_(methods.data()),
+ method_size_(sizeof(T)),
+ method_count_(static_cast<uint16_t>(method_count)) {
+ static_assert(method_count <= std::numeric_limits<uint16_t>::max());
+ }
+
+ // For use by tests with only one method.
template <typename T>
- constexpr Service(uint32_t id, T&& methods)
- : id_(id), methods_(std::forward<T>(methods)) {}
+ constexpr Service(uint32_t id, const T& method)
+ : id_(id), methods_(&method), method_size_(sizeof(T)), method_count_(1) {}
private:
friend class Server;
+ friend class ServiceTestHelper;
// Finds the method with the provided method_id. Returns nullptr if no match.
- const internal::Method* FindMethod(uint32_t method_id) const;
+ const internal::BaseMethod* FindMethod(uint32_t method_id) const;
- uint32_t id_;
- std::span<const internal::Method> methods_;
+ const uint32_t id_;
+ const internal::BaseMethod* const methods_;
+ const uint16_t method_size_;
+ const uint16_t method_count_;
};
} // namespace pw::rpc
diff --git a/pw_rpc/server.cc b/pw_rpc/server.cc
index dadd955..f08bdd8 100644
--- a/pw_rpc/server.cc
+++ b/pw_rpc/server.cc
@@ -17,7 +17,6 @@
#include <algorithm>
#include "pw_log/log.h"
-#include "pw_rpc/internal/method.h"
#include "pw_rpc/internal/packet.h"
#include "pw_rpc/internal/server.h"
#include "pw_rpc/server_context.h"
@@ -120,7 +119,7 @@
return Status::OK;
}
-std::tuple<Service*, const internal::Method*> Server::FindMethod(
+std::tuple<Service*, const internal::BaseMethod*> Server::FindMethod(
const internal::Packet& packet) {
// Packets always include service and method IDs.
auto service = std::find_if(services_.begin(), services_.end(), [&](auto& s) {
@@ -170,8 +169,4 @@
return channel;
}
-static_assert(std::is_base_of<internal::BaseMethod, internal::Method>(),
- "The Method implementation must be derived from "
- "pw::rpc::internal::BaseMethod");
-
} // namespace pw::rpc
diff --git a/pw_rpc/service.cc b/pw_rpc/service.cc
index 50e0447..4f2dccc 100644
--- a/pw_rpc/service.cc
+++ b/pw_rpc/service.cc
@@ -14,17 +14,21 @@
#include "pw_rpc/service.h"
+#include <cstddef>
#include <type_traits>
-#include "pw_rpc/internal/method.h"
-
namespace pw::rpc {
-const internal::Method* Service::FindMethod(uint32_t method_id) const {
- for (const internal::Method& method : methods_) {
- if (method.id() == method_id) {
- return &method;
+const internal::BaseMethod* Service::FindMethod(uint32_t method_id) const {
+ const internal::BaseMethod* method = methods_;
+
+ for (size_t i = 0; i < method_count_; ++i) {
+ if (method->id() == method_id) {
+ return method;
}
+
+ const auto raw = reinterpret_cast<const std::byte*>(method);
+ method = reinterpret_cast<const internal::BaseMethod*>(raw + method_size_);
}
return nullptr;
diff --git a/pw_rpc/service_test.cc b/pw_rpc/service_test.cc
new file mode 100644
index 0000000..2fadec7
--- /dev/null
+++ b/pw_rpc/service_test.cc
@@ -0,0 +1,85 @@
+// Copyright 2020 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+
+#include "pw_rpc/service.h"
+
+#include "gtest/gtest.h"
+#include "pw_rpc/internal/base_method.h"
+
+namespace pw::rpc {
+
+class ServiceTestHelper {
+ public:
+ static const internal::BaseMethod* FindMethod(Service& service, uint32_t id) {
+ return service.FindMethod(id);
+ }
+};
+
+namespace {
+
+void InvokeIt(const internal::BaseMethod&,
+ internal::ServerCall&,
+ const internal::Packet&) {}
+
+class ServiceTestMethod : public internal::BaseMethod {
+ public:
+ constexpr ServiceTestMethod(uint32_t id, char the_value)
+ : internal::BaseMethod(id, InvokeIt), value(the_value) {}
+
+ char value; // Add a member so the class is larger than the base Method.
+};
+
+class TestService : public Service {
+ public:
+ constexpr TestService() : Service(0xabcd, kMethods) {}
+
+ static constexpr std::array<ServiceTestMethod, 3> kMethods = {{
+ ServiceTestMethod(123, 'a'),
+ ServiceTestMethod(456, 'b'),
+ ServiceTestMethod(789, 'c'),
+ }};
+};
+
+TEST(Service, MultipleMethods_FindMethod_Present) {
+ TestService service;
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 123),
+ &TestService::kMethods[0]);
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 456),
+ &TestService::kMethods[1]);
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 789),
+ &TestService::kMethods[2]);
+}
+
+TEST(Service, MultipleMethods_FindMethod_NotPresent) {
+ TestService service;
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 0), nullptr);
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 457), nullptr);
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 999), nullptr);
+}
+
+class EmptyTestService : public Service {
+ public:
+ constexpr EmptyTestService() : Service(0xabcd, kMethods) {}
+ static constexpr std::array<ServiceTestMethod, 0> kMethods = {{}};
+};
+
+TEST(Service, NoMethods_FindMethod_NotPresent) {
+ EmptyTestService service;
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 123), nullptr);
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 456), nullptr);
+ EXPECT_EQ(ServiceTestHelper::FindMethod(service, 789), nullptr);
+}
+
+} // namespace
+} // namespace pw::rpc
