pw_rpc: Add function for yielding the RPC lock in C++
Upcoming CLs will require threads to yield the RPC lock while waiting
for a different thread to finish executing an RPC callback.
- Add a config option for selecting how to yield (busy loop,
pw::this_thread::sleep_for(), pw::this_thread::yield()).
- Add dependencies on pw_thread if backends are defined.
- Have the pw_rpc:disable_global_mutex config select the busy loop yield
approach, since platforms with the mutex disabled either do not
support threading or exclusively use RPC from one thread.
- Disable the RPC lock on a few platforms with no RTOS.
Change-Id: I94517d66e04b51b95a911adac560426ee3683388
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/126751
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Reviewed-by: Alexei Frolov <frolv@google.com>
Pigweed-Auto-Submit: Wyatt Hepler <hepler@google.com>
diff --git a/pw_rpc/BUILD.bazel b/pw_rpc/BUILD.bazel
index f669214..004c60d 100644
--- a/pw_rpc/BUILD.bazel
+++ b/pw_rpc/BUILD.bazel
@@ -122,6 +122,7 @@
"//pw_status",
"//pw_sync:lock_annotations",
"//pw_sync:mutex",
+ "//pw_thread:sleep",
"//pw_toolchain:no_destructor",
],
)
diff --git a/pw_rpc/BUILD.gn b/pw_rpc/BUILD.gn
index c0af29f..1c7bbdb 100644
--- a/pw_rpc/BUILD.gn
+++ b/pw_rpc/BUILD.gn
@@ -35,7 +35,10 @@
}
config("disable_global_mutex_config") {
- defines = [ "PW_RPC_USE_GLOBAL_MUTEX=0" ]
+ defines = [
+ "PW_RPC_USE_GLOBAL_MUTEX=0",
+ "PW_RPC_YIELD_MODE=PW_RPC_YIELD_MODE_BUSY_LOOP",
+ ]
visibility = [ ":*" ]
}
@@ -168,6 +171,16 @@
":log_config",
dir_pw_log,
]
+
+ # pw_rpc needs a way to yield the current thread. Depending on its
+ # configuration, it may need either pw_thread:sleep or pw_thread:yield.
+ if (pw_thread_SLEEP_BACKEND != "") {
+ deps += [ "$dir_pw_thread:sleep" ]
+ }
+ if (pw_thread_YIELD_BACKEND != "") {
+ deps += [ "$dir_pw_thread:yield" ]
+ }
+
public = [
"public/pw_rpc/channel.h",
"public/pw_rpc/method_id.h",
diff --git a/pw_rpc/CMakeLists.txt b/pw_rpc/CMakeLists.txt
index ba4c97c..2d55a68 100644
--- a/pw_rpc/CMakeLists.txt
+++ b/pw_rpc/CMakeLists.txt
@@ -156,9 +156,18 @@
pw_log
pw_rpc.log_config
)
-if (NOT "${pw_sync.mutex_BACKEND}" STREQUAL "")
+if(NOT "${pw_sync.mutex_BACKEND}" STREQUAL "")
pw_target_link_targets(pw_rpc.common PUBLIC pw_sync.mutex)
endif()
+
+if(NOT "${pw_thread.sleep_BACKEND}" STREQUAL "")
+ pw_target_link_targets(pw_rpc.common PUBLIC pw_thread.sleep)
+endif()
+
+if(NOT "${pw_thread.yield_BACKEND}" STREQUAL "")
+ pw_target_link_targets(pw_rpc.common PUBLIC pw_thread.yield)
+endif()
+
if(Zephyr_FOUND AND CONFIG_PIGWEED_RPC_COMMON)
zephyr_link_libraries(pw_rpc.common)
endif()
diff --git a/pw_rpc/docs.rst b/pw_rpc/docs.rst
index f8f4598..636ad95 100644
--- a/pw_rpc/docs.rst
+++ b/pw_rpc/docs.rst
@@ -1464,6 +1464,22 @@
This is enabled by default.
+.. c:macro:: PW_RPC_YIELD_MODE
+
+ pw_rpc must yield the current thread when waiting for a callback to complete
+ in a different thread. ``PW_RPC_YIELD_MODE`` determines how to yield. There
+ are three supported settings:
+
+ - ``PW_RPC_YIELD_MODE_BUSY_LOOP``. Do nothing. Release and reacquire the RPC
+ lock in a busy loop. :c:macro:`PW_RPC_USE_GLOBAL_MUTEX` must be 0 as well.
+ - ``PW_RPC_YIELD_MODE_SLEEP``. Yield with 1-tick calls to
+ :cpp:func:`pw::this_thread::sleep_for`. A backend must be configured for
+ pw_thread:sleep.
+ - ``PW_RPC_YIELD_MODE_YIELD``. Yield with :cpp:func:`pw::this_thread::yield`.
+ A backend must be configured for pw_thread:yield. IMPORTANT: On some
+ platforms, :cpp:func:`pw::this_thread::yield` does not yield to lower
+ priority tasks and should not be used here.
+
.. c:macro:: PW_RPC_DYNAMIC_ALLOCATION
Whether pw_rpc should use dynamic memory allocation internally. If enabled,
diff --git a/pw_rpc/endpoint.cc b/pw_rpc/endpoint.cc
index 9a70f0d..a3d922c 100644
--- a/pw_rpc/endpoint.cc
+++ b/pw_rpc/endpoint.cc
@@ -22,6 +22,54 @@
#include "pw_rpc/internal/lock.h"
#include "pw_toolchain/no_destructor.h"
+#if PW_RPC_YIELD_MODE == PW_RPC_YIELD_MODE_BUSY_LOOP
+
+static_assert(
+ PW_RPC_USE_GLOBAL_MUTEX == 0,
+ "The RPC global mutex is enabled, but no pw_rpc yield mode is selected! "
+ "Because the global mutex is in use, pw_rpc may be used from multiple "
+ "threads. This could result in thread starvation. To fix this, set "
+ "PW_RPC_YIELD to PW_RPC_YIELD_MODE_SLEEP and add a dependency on "
+ "pw_thread:sleep.");
+
+#elif PW_RPC_YIELD_MODE == PW_RPC_YIELD_MODE_SLEEP
+
+#if !__has_include("pw_thread/sleep.h")
+
+static_assert(false,
+ "PW_RPC_YIELD_MODE is PW_RPC_YIELD_MODE_SLEEP "
+ "(pw::this_thread::sleep_for()), but no backend is set for "
+ "pw_thread:sleep. Set a pw_thread:sleep backend or use a "
+ "different PW_RPC_YIELD_MODE setting.");
+
+#endif // !__has_include("pw_thread/sleep.h")
+
+#include "pw_thread/sleep.h"
+
+#elif PW_RPC_YIELD_MODE == PW_RPC_YIELD_MODE_YIELD
+
+#if !__has_include("pw_thread/yield.h")
+
+static_assert(false,
+ "PW_RPC_YIELD_MODE is PW_RPC_YIELD_MODE_YIELD "
+ "(pw::this_thread::yield()), but no backend is set for "
+ "pw_thread:yield. Set a pw_thread:yield backend or use a "
+ "different PW_RPC_YIELD_MODE setting.");
+
+#endif // !__has_include("pw_thread/yield.h")
+
+#include "pw_thread/yield.h"
+
+#else
+
+static_assert(
+ false,
+ "PW_RPC_YIELD_MODE macro must be set to PW_RPC_YIELD_MODE_BUSY_LOOP, "
+ "PW_RPC_YIELD_MODE_SLEEP (pw::this_thread::sleep_for()), or "
+ "PW_RPC_YIELD_MODE_YIELD (pw::this_thread::yield())");
+
+#endif // PW_RPC_YIELD_MODE
+
namespace pw::rpc::internal {
RpcLock& rpc_lock() {
@@ -29,6 +77,16 @@
return *lock;
}
+void YieldRpcLock() {
+ rpc_lock().unlock();
+#if PW_RPC_YIELD_MODE == PW_RPC_YIELD_MODE_SLEEP
+ this_thread::sleep_for(chrono::SystemClock::duration(1));
+#elif PW_RPC_YIELD_MODE == PW_RPC_YIELD_MODE_YIELD
+ this_thread::yield();
+#endif // PW_RPC_YIELD_MODE
+ rpc_lock().lock();
+}
+
Endpoint::~Endpoint() {
// Since the calls remove themselves from the Endpoint in
// CloseAndSendResponse(), close responders until no responders remain.
diff --git a/pw_rpc/public/pw_rpc/internal/config.h b/pw_rpc/public/pw_rpc/internal/config.h
index d29c24e..8b4f6ba 100644
--- a/pw_rpc/public/pw_rpc/internal/config.h
+++ b/pw_rpc/public/pw_rpc/internal/config.h
@@ -49,6 +49,28 @@
#define PW_RPC_USE_GLOBAL_MUTEX 1
#endif // PW_RPC_USE_GLOBAL_MUTEX
+// pw_rpc must yield the current thread when waiting for a callback to complete
+// in a different thread. PW_RPC_YIELD_MODE determines how to yield. There are
+// three supported settings:
+//
+// PW_RPC_YIELD_MODE_BUSY_LOOP - Do nothing. Release and reacquire the RPC
+// lock in a busy loop. PW_RPC_USE_GLOBAL_MUTEX must be 0.
+// PW_RPC_YIELD_MODE_SLEEP - Yield with 1-tick calls to
+// pw::this_thread::sleep_for(). A backend must be configured for
+// pw_thread:sleep.
+// PW_RPC_YIELD_MODE_YIELD - Yield with pw::this_thread::yield(). A backend
+// must be configured for pw_thread:yield. IMPORTANT: On some platforms,
+// pw::this_thread::yield() does not yield to lower priority tasks and
+// should not be used here.
+//
+#define PW_RPC_YIELD_MODE_BUSY_LOOP 100
+#define PW_RPC_YIELD_MODE_SLEEP 101
+#define PW_RPC_YIELD_MODE_YIELD 102
+
+#ifndef PW_RPC_YIELD_MODE
+#define PW_RPC_YIELD_MODE PW_RPC_YIELD_MODE_SLEEP
+#endif // PW_RPC_YIELD_MODE
+
// Whether pw_rpc should use dynamic memory allocation internally. If enabled,
// pw_rpc dynamically allocates channels and its encoding buffers. RPC users may
// use dynamic allocation independently of this option (e.g. to allocate pw_rpc
diff --git a/pw_rpc/public/pw_rpc/internal/lock.h b/pw_rpc/public/pw_rpc/internal/lock.h
index 50c53d5..e0bf67b 100644
--- a/pw_rpc/public/pw_rpc/internal/lock.h
+++ b/pw_rpc/public/pw_rpc/internal/lock.h
@@ -53,4 +53,7 @@
RpcLock& rpc_lock();
+// Releases the RPC lock, yields, and reacquires it.
+void YieldRpcLock() PW_EXCLUSIVE_LOCKS_REQUIRED(rpc_lock());
+
} // namespace pw::rpc::internal
diff --git a/targets/arduino/target_toolchains.gni b/targets/arduino/target_toolchains.gni
index e5120c0..3b74b8b 100644
--- a/targets/arduino/target_toolchains.gni
+++ b/targets/arduino/target_toolchains.gni
@@ -47,6 +47,7 @@
pw_sync_INTERRUPT_SPIN_LOCK_BACKEND =
"$dir_pw_sync_baremetal:interrupt_spin_lock"
pw_sync_MUTEX_BACKEND = "$dir_pw_sync_baremetal:mutex"
+ pw_rpc_CONFIG = "$dir_pw_rpc:disable_global_mutex"
pw_sys_io_BACKEND = dir_pw_sys_io_arduino
pw_rpc_system_server_BACKEND = "$dir_pw_hdlc:hdlc_sys_io_system_server"
pw_arduino_build_INIT_BACKEND = "$dir_pigweed/targets/arduino:pre_init"
diff --git a/targets/lm3s6965evb_qemu/target_toolchains.gni b/targets/lm3s6965evb_qemu/target_toolchains.gni
index 334aef8..25779b8 100644
--- a/targets/lm3s6965evb_qemu/target_toolchains.gni
+++ b/targets/lm3s6965evb_qemu/target_toolchains.gni
@@ -44,6 +44,7 @@
pw_sync_INTERRUPT_SPIN_LOCK_BACKEND =
"$dir_pw_sync_baremetal:interrupt_spin_lock"
pw_sync_MUTEX_BACKEND = "$dir_pw_sync_baremetal:mutex"
+ pw_rpc_CONFIG = "$dir_pw_rpc:disable_global_mutex"
# pw_cpu_exception_armv7m tests do not work as expected in QEMU. It does not
# appear the divide-by-zero traps as expected when enabled, which prevents the
diff --git a/targets/rp2040/BUILD.gn b/targets/rp2040/BUILD.gn
index 44f5350..329d95e 100644
--- a/targets/rp2040/BUILD.gn
+++ b/targets/rp2040/BUILD.gn
@@ -55,6 +55,7 @@
pw_sync_INTERRUPT_SPIN_LOCK_BACKEND =
"$dir_pw_sync_baremetal:interrupt_spin_lock"
pw_sync_MUTEX_BACKEND = "$dir_pw_sync_baremetal:mutex"
+ pw_rpc_CONFIG = "$dir_pw_rpc:disable_global_mutex"
# Silence GN variable overwrite warning.
pw_build_LINK_DEPS = []
diff --git a/targets/stm32f429i_disc1/target_toolchains.gni b/targets/stm32f429i_disc1/target_toolchains.gni
index 8903ec0..d7c0fdf 100644
--- a/targets/stm32f429i_disc1/target_toolchains.gni
+++ b/targets/stm32f429i_disc1/target_toolchains.gni
@@ -60,6 +60,7 @@
pw_sync_INTERRUPT_SPIN_LOCK_BACKEND =
"$dir_pw_sync_baremetal:interrupt_spin_lock"
pw_sync_MUTEX_BACKEND = "$dir_pw_sync_baremetal:mutex"
+ pw_rpc_CONFIG = "$dir_pw_rpc:disable_global_mutex"
pw_log_BACKEND = dir_pw_log_basic
pw_sys_io_BACKEND = dir_pw_sys_io_baremetal_stm32f429
pw_rpc_system_server_BACKEND = "$dir_pw_hdlc:hdlc_sys_io_system_server"
