pw_sync: Add GenericBasicLockable
Lock types that derive from pw::sync::VirtualBasicLockable need to
implement DoLockOperation() for the values of Operation::kLock and
Operation::kUnlock. For most lock types, this translates to simply
calling lock() or unlock() on the underlying lock instance,
respectively. This CL deduplicates code from Mutex, TimedMutex, and
InterruptSpinLock by providing a common implementation templated on the
lock type.
Change-Id: I962e329b47aeb50918ed5667520d0d3417fdb8ce
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/165930
Commit-Queue: Aaron Green <aarongreen@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
diff --git a/pw_sync/docs.rst b/pw_sync/docs.rst
index 9e0df4c..5cc7996 100644
--- a/pw_sync/docs.rst
+++ b/pw_sync/docs.rst
@@ -731,9 +731,9 @@
virtual lock interface could be used here to minimize the code-size cost that
would occur otherwise if the flash driver were templated.
-VirtualBasicLock
-----------------
-The ``VirtualBasicLock`` interface meets the
+VirtualBasicLockable
+--------------------
+The ``VirtualBasicLockable`` interface meets the
`BasicLockable <https://en.cppreference.com/w/cpp/named_req/BasicLockable>`_ C++
named requirement. Our critical section lock primitives offer optional virtual
versions, including:
@@ -742,9 +742,22 @@
* :cpp:func:`pw::sync::VirtualTimedMutex`
* :cpp:func:`pw::sync::VirtualInterruptSpinLock`
+GenericBasicLockable
+--------------------
+``GenericBasicLockable`` is a helper construct that can be used to declare
+virtual versions of a critical section lock primitive that meets the
+`BasicLockable <https://en.cppreference.com/w/cpp/named_req/BasicLockable>`_
+C++ named requirement. For example, given a ``Mutex`` type with ``lock()`` and
+``unlock()`` methods, a ``VirtualMutex`` type that derives from
+``VirtualBasicLockable`` can be declared as follows:
+
+.. code-block:: cpp
+
+ class VirtualMutex : public GenericBasicLockable<Mutex> {};
+
Borrowable
==========
-The Borrowable is a helper construct that enables callers to borrow an object
+``Borrowable`` is a helper construct that enables callers to borrow an object
which is guarded by a lock, enabling a containerized style of external locking.
Users who need access to the guarded object can ask to acquire a
diff --git a/pw_sync/mutex_facade_test.cc b/pw_sync/mutex_facade_test.cc
index 2b7926e..bf4e61d 100644
--- a/pw_sync/mutex_facade_test.cc
+++ b/pw_sync/mutex_facade_test.cc
@@ -33,7 +33,7 @@
// TODO(b/235284163): Add real concurrency tests once we have pw::thread.
TEST(Mutex, LockUnlock) {
- pw::sync::Mutex mutex;
+ Mutex mutex;
mutex.lock();
// TODO(b/235284163): Ensure it fails to lock when already held.
// EXPECT_FALSE(mutex.try_lock());
@@ -49,7 +49,7 @@
}
TEST(Mutex, TryLockUnlock) {
- pw::sync::Mutex mutex;
+ Mutex mutex;
const bool locked = mutex.try_lock();
EXPECT_TRUE(locked);
if (locked) {
@@ -62,7 +62,7 @@
PW_SYNC_ADD_BORROWABLE_LOCK_NAMED_TESTS(BorrowableMutex, Mutex);
TEST(VirtualMutex, LockUnlock) {
- pw::sync::VirtualMutex mutex;
+ VirtualMutex mutex;
mutex.lock();
// TODO(b/235284163): Ensure it fails to lock when already held.
// EXPECT_FALSE(mutex.try_lock());
@@ -77,16 +77,25 @@
static_virtual_mutex.unlock();
}
+TEST(VirtualMutex, LockUnlockExternal) {
+ VirtualMutex virtual_mutex;
+ auto& mutex = virtual_mutex.mutex();
+ mutex.lock();
+ // TODO(b/235284163): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(mutex.try_lock());
+ mutex.unlock();
+}
+
PW_SYNC_ADD_BORROWABLE_LOCK_NAMED_TESTS(BorrowableVirtualMutex, VirtualMutex);
TEST(Mutex, LockUnlockInC) {
- pw::sync::Mutex mutex;
+ Mutex mutex;
pw_sync_Mutex_CallLock(&mutex);
pw_sync_Mutex_CallUnlock(&mutex);
}
TEST(Mutex, TryLockUnlockInC) {
- pw::sync::Mutex mutex;
+ Mutex mutex;
ASSERT_TRUE(pw_sync_Mutex_CallTryLock(&mutex));
// TODO(b/235284163): Ensure it fails to lock when already held.
// EXPECT_FALSE(pw_sync_Mutex_CallTryLock(&mutex));
diff --git a/pw_sync/public/pw_sync/interrupt_spin_lock.h b/pw_sync/public/pw_sync/interrupt_spin_lock.h
index 7726c92..996ad6d 100644
--- a/pw_sync/public/pw_sync/interrupt_spin_lock.h
+++ b/pw_sync/public/pw_sync/interrupt_spin_lock.h
@@ -81,32 +81,8 @@
};
class PW_LOCKABLE("pw::sync::VirtualInterruptSpinLock")
- VirtualInterruptSpinLock final : public VirtualBasicLockable {
- public:
- VirtualInterruptSpinLock() = default;
-
- VirtualInterruptSpinLock(const VirtualInterruptSpinLock&) = delete;
- VirtualInterruptSpinLock(VirtualInterruptSpinLock&&) = delete;
- VirtualInterruptSpinLock& operator=(const VirtualInterruptSpinLock&) = delete;
- VirtualInterruptSpinLock& operator=(VirtualInterruptSpinLock&&) = delete;
-
- InterruptSpinLock& interrupt_spin_lock() { return interrupt_spin_lock_; }
-
- private:
- void DoLockOperation(Operation operation) override
- PW_NO_LOCK_SAFETY_ANALYSIS {
- switch (operation) {
- case Operation::kLock:
- return interrupt_spin_lock_.lock();
-
- case Operation::kUnlock:
- default:
- return interrupt_spin_lock_.unlock();
- }
- }
-
- InterruptSpinLock interrupt_spin_lock_;
-};
+ VirtualInterruptSpinLock final
+ : public GenericBasicLockable<InterruptSpinLock> {};
} // namespace pw::sync
diff --git a/pw_sync/public/pw_sync/mutex.h b/pw_sync/public/pw_sync/mutex.h
index 5b42468..18729f5 100644
--- a/pw_sync/public/pw_sync/mutex.h
+++ b/pw_sync/public/pw_sync/mutex.h
@@ -86,31 +86,9 @@
};
class PW_LOCKABLE("pw::sync::VirtualMutex") VirtualMutex final
- : public VirtualBasicLockable {
+ : public GenericBasicLockable<Mutex> {
public:
- VirtualMutex() = default;
-
- VirtualMutex(const VirtualMutex&) = delete;
- VirtualMutex(VirtualMutex&&) = delete;
- VirtualMutex& operator=(const VirtualMutex&) = delete;
- VirtualMutex& operator=(VirtualMutex&&) = delete;
-
- Mutex& mutex() { return mutex_; }
-
- private:
- void DoLockOperation(Operation operation) override
- PW_NO_LOCK_SAFETY_ANALYSIS {
- switch (operation) {
- case Operation::kLock:
- return mutex_.lock();
-
- case Operation::kUnlock:
- default:
- return mutex_.unlock();
- }
- }
-
- Mutex mutex_;
+ Mutex& mutex() { return impl(); }
};
} // namespace pw::sync
diff --git a/pw_sync/public/pw_sync/timed_mutex.h b/pw_sync/public/pw_sync/timed_mutex.h
index abb48c1..396f9b6 100644
--- a/pw_sync/public/pw_sync/timed_mutex.h
+++ b/pw_sync/public/pw_sync/timed_mutex.h
@@ -71,31 +71,9 @@
};
class PW_LOCKABLE("pw::sync::VirtualTimedMutex") VirtualTimedMutex final
- : public VirtualBasicLockable {
+ : public GenericBasicLockable<TimedMutex> {
public:
- VirtualTimedMutex() = default;
-
- VirtualTimedMutex(const VirtualTimedMutex&) = delete;
- VirtualTimedMutex(VirtualTimedMutex&&) = delete;
- VirtualTimedMutex& operator=(const VirtualTimedMutex&) = delete;
- VirtualTimedMutex& operator=(VirtualTimedMutex&&) = delete;
-
- TimedMutex& timed_mutex() { return timed_mutex_; }
-
- private:
- void DoLockOperation(Operation operation) override
- PW_NO_LOCK_SAFETY_ANALYSIS {
- switch (operation) {
- case Operation::kLock:
- return timed_mutex_.lock();
-
- case Operation::kUnlock:
- default:
- return timed_mutex_.unlock();
- }
- }
-
- TimedMutex timed_mutex_;
+ TimedMutex& timed_mutex() { return impl(); }
};
} // namespace pw::sync
diff --git a/pw_sync/public/pw_sync/virtual_basic_lockable.h b/pw_sync/public/pw_sync/virtual_basic_lockable.h
index 5010365..c41f39b 100644
--- a/pw_sync/public/pw_sync/virtual_basic_lockable.h
+++ b/pw_sync/public/pw_sync/virtual_basic_lockable.h
@@ -70,4 +70,34 @@
void DoLockOperation(Operation) override {}
};
+/// Templated base class to facilitate making "Virtual{LockType}" from a
+/// "LockType" class that provides `lock()` and `unlock()` methods.
+/// The resulting classes will derive from `VirtualBasicLockable`.
+///
+/// Example:
+/// class VirtualMutex : public GenericBasicLockable<Mutex> {};
+template <typename LockType>
+class GenericBasicLockable : public VirtualBasicLockable {
+ public:
+ virtual ~GenericBasicLockable() = default;
+
+ protected:
+ LockType& impl() { return impl_; }
+
+ private:
+ void DoLockOperation(Operation operation) override
+ PW_NO_LOCK_SAFETY_ANALYSIS {
+ switch (operation) {
+ case Operation::kLock:
+ return impl_.lock();
+
+ case Operation::kUnlock:
+ default:
+ return impl_.unlock();
+ }
+ }
+
+ LockType impl_;
+};
+
} // namespace pw::sync
diff --git a/pw_sync/timed_mutex_facade_test.cc b/pw_sync/timed_mutex_facade_test.cc
index b74c569..26a3b47 100644
--- a/pw_sync/timed_mutex_facade_test.cc
+++ b/pw_sync/timed_mutex_facade_test.cc
@@ -49,7 +49,7 @@
// TODO(b/235284163): Add real concurrency tests once we have pw::thread.
TEST(TimedMutex, LockUnlock) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
mutex.lock();
mutex.unlock();
// TODO(b/235284163): Ensure it fails to lock when already held by someone
@@ -67,7 +67,7 @@
}
TEST(TimedMutex, TryLockUnlock) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
const bool locked = mutex.try_lock();
EXPECT_TRUE(locked);
if (locked) {
@@ -79,7 +79,7 @@
}
TEST(TimedMutex, TryLockUnlockFor) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
SystemClock::time_point before = SystemClock::now();
const bool locked = mutex.try_lock_for(kRoundedArbitraryDuration);
@@ -98,7 +98,7 @@
}
TEST(TimedMutex, TryLockUnlockUntil) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
const SystemClock::time_point deadline =
SystemClock::now() + kRoundedArbitraryDuration;
@@ -121,7 +121,7 @@
chrono::SystemClock);
TEST(VirtualTimedMutex, LockUnlock) {
- pw::sync::VirtualTimedMutex mutex;
+ VirtualTimedMutex mutex;
mutex.lock();
// TODO(b/235284163): Ensure it fails to lock when already held by someone
// else.
@@ -138,18 +138,27 @@
static_virtual_mutex.unlock();
}
+TEST(VirtualMutex, LockUnlockExternal) {
+ VirtualTimedMutex virtual_timed_mutex;
+ auto& mutex = virtual_timed_mutex.timed_mutex();
+ mutex.lock();
+ // TODO(b/235284163): Ensure it fails to lock when already held.
+ // EXPECT_FALSE(mutex.try_lock());
+ mutex.unlock();
+}
+
PW_SYNC_ADD_BORROWABLE_TIMED_LOCK_NAMED_TESTS(BorrowableVirtualTimedMutex,
VirtualTimedMutex,
chrono::SystemClock);
TEST(TimedMutex, LockUnlockInC) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
pw_sync_TimedMutex_CallLock(&mutex);
pw_sync_TimedMutex_CallUnlock(&mutex);
}
TEST(TimedMutex, TryLockUnlockInC) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
ASSERT_TRUE(pw_sync_TimedMutex_CallTryLock(&mutex));
// TODO(b/235284163): Ensure it fails to lock when already held by someone
// else.
@@ -158,7 +167,7 @@
}
TEST(TimedMutex, TryLockUnlockForInC) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
pw_chrono_SystemClock_TimePoint before = pw_chrono_SystemClock_Now();
ASSERT_TRUE(
@@ -176,7 +185,7 @@
}
TEST(TimedMutex, TryLockUnlockUntilInC) {
- pw::sync::TimedMutex mutex;
+ TimedMutex mutex;
pw_chrono_SystemClock_TimePoint deadline;
deadline.duration_since_epoch.ticks =
pw_chrono_SystemClock_Now().duration_since_epoch.ticks +
