embos: fix for_at_least contract to add one tick
Fixes the embOS backends for pw::sync::Mutex,
pw::sync::BinarySemaphore, pw::sync::CountingSemaphore, and
pw::this_thread::sleep_for to add one tick when invoking the native
API to comply with the for_at_least contract as we do not know how
far we are into the current tick.
Note this is not observable without the use of an independent clock.
This also adds explicit downcasting from int64_t to OS_TIME ticks
when invoking native APIs.
Change-Id: I113cbdfc1a88795df87223117e65a763ae050772
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/37280
Reviewed-by: Wyatt Hepler <hepler@google.com>
Commit-Queue: Ewout van Bekkum <ewout@google.com>
diff --git a/pw_sync_embos/binary_semaphore.cc b/pw_sync_embos/binary_semaphore.cc
index 379dbd4..7f77afe 100644
--- a/pw_sync_embos/binary_semaphore.cc
+++ b/pw_sync_embos/binary_semaphore.cc
@@ -23,23 +23,30 @@
#include "pw_interrupt/context.h"
using pw::chrono::SystemClock;
-using pw::chrono::embos::kMaxTimeout;
namespace pw::sync {
bool BinarySemaphore::try_acquire_for(SystemClock::duration for_at_least) {
PW_DCHECK(!interrupt::InInterruptContext());
- // Clamp negative durations to be 0 which maps to non-blocking.
- for_at_least = std::max(for_at_least, SystemClock::duration::zero());
+ // Use non-blocking try_acquire for negative and zero length durations.
+ if (for_at_least <= SystemClock::duration::zero()) {
+ return try_acquire();
+ }
- while (for_at_least > kMaxTimeout) {
- if (OS_WaitCSemaTimed(&native_type_, kMaxTimeout.count())) {
+ // On a tick based kernel we cannot tell how far along we are on the current
+ // tick, ergo we add one whole tick to the final duration.
+ constexpr SystemClock::duration kMaxTimeoutMinusOne =
+ pw::chrono::embos::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ if (OS_WaitCSemaTimed(&native_type_,
+ static_cast<OS_TIME>(kMaxTimeoutMinusOne.count()))) {
return true;
}
- for_at_least -= kMaxTimeout;
+ for_at_least -= kMaxTimeoutMinusOne;
}
- return OS_WaitCSemaTimed(&native_type_, for_at_least.count());
+ return OS_WaitCSemaTimed(&native_type_,
+ static_cast<OS_TIME>(for_at_least.count() + 1));
}
} // namespace pw::sync
diff --git a/pw_sync_embos/counting_semaphore.cc b/pw_sync_embos/counting_semaphore.cc
index e46a36b..a2dd40e 100644
--- a/pw_sync_embos/counting_semaphore.cc
+++ b/pw_sync_embos/counting_semaphore.cc
@@ -23,7 +23,6 @@
#include "pw_interrupt/context.h"
using pw::chrono::SystemClock;
-using pw::chrono::embos::kMaxTimeout;
namespace pw::sync {
@@ -44,16 +43,24 @@
bool CountingSemaphore::try_acquire_for(SystemClock::duration for_at_least) {
PW_DCHECK(!interrupt::InInterruptContext());
- // Clamp negative durations to be 0 which maps to non-blocking.
- for_at_least = std::max(for_at_least, SystemClock::duration::zero());
+ // Use non-blocking try_acquire for negative and zero length durations.
+ if (for_at_least <= SystemClock::duration::zero()) {
+ return try_acquire();
+ }
- while (for_at_least > kMaxTimeout) {
- if (OS_WaitCSemaTimed(&native_type_, kMaxTimeout.count())) {
+ // On a tick based kernel we cannot tell how far along we are on the current
+ // tick, ergo we add one whole tick to the final duration.
+ constexpr SystemClock::duration kMaxTimeoutMinusOne =
+ pw::chrono::embos::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ if (OS_WaitCSemaTimed(&native_type_,
+ static_cast<OS_TIME>(kMaxTimeoutMinusOne.count()))) {
return true;
}
- for_at_least -= kMaxTimeout;
+ for_at_least -= kMaxTimeoutMinusOne;
}
- return OS_WaitCSemaTimed(&native_type_, for_at_least.count());
+ return OS_WaitCSemaTimed(&native_type_,
+ static_cast<OS_TIME>(for_at_least.count() + 1));
}
} // namespace pw::sync
diff --git a/pw_sync_embos/mutex.cc b/pw_sync_embos/mutex.cc
index 8686039..fb33d9f 100644
--- a/pw_sync_embos/mutex.cc
+++ b/pw_sync_embos/mutex.cc
@@ -23,25 +23,32 @@
#include "pw_interrupt/context.h"
using pw::chrono::SystemClock;
-using pw::chrono::embos::kMaxTimeout;
namespace pw::sync {
bool Mutex::try_lock_for(SystemClock::duration for_at_least) {
PW_DCHECK(!interrupt::InInterruptContext());
- // Clamp negative durations to be 0 which maps to non-blocking.
- for_at_least = std::max(for_at_least, SystemClock::duration::zero());
+ // Use non-blocking try_lock for negative and zero length durations.
+ if (for_at_least <= SystemClock::duration::zero()) {
+ return try_lock();
+ }
- while (for_at_least > kMaxTimeout) {
- const int lock_count = OS_UseTimed(&native_type_, kMaxTimeout.count());
+ // On a tick based kernel we cannot tell how far along we are on the current
+ // tick, ergo we add one whole tick to the final duration.
+ constexpr SystemClock::duration kMaxTimeoutMinusOne =
+ pw::chrono::embos::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ const int lock_count = OS_UseTimed(
+ &native_type_, static_cast<OS_TIME>(kMaxTimeoutMinusOne.count()));
if (lock_count != 0) {
PW_CHECK_UINT_EQ(1, lock_count, "Recursive locking is not permitted");
return true;
}
- for_at_least -= kMaxTimeout;
+ for_at_least -= kMaxTimeoutMinusOne;
}
- const int lock_count = OS_UseTimed(&native_type_, for_at_least.count());
+ const int lock_count = OS_UseTimed(
+ &native_type_, static_cast<OS_TIME>(for_at_least.count() + 1));
PW_CHECK_UINT_LE(1, lock_count, "Recursive locking is not permitted");
return lock_count == 1;
}
diff --git a/pw_sync_embos/public/pw_sync_embos/binary_semaphore_inline.h b/pw_sync_embos/public/pw_sync_embos/binary_semaphore_inline.h
index 3996851..d8abf72 100644
--- a/pw_sync_embos/public/pw_sync_embos/binary_semaphore_inline.h
+++ b/pw_sync_embos/public/pw_sync_embos/binary_semaphore_inline.h
@@ -41,6 +41,8 @@
inline bool BinarySemaphore::try_acquire_until(
chrono::SystemClock::time_point until_at_least) {
+ // Note that if this deadline is in the future, it will get rounded up by
+ // one whole tick due to how try_acquire_for is implemented.
return try_acquire_for(until_at_least - chrono::SystemClock::now());
}
diff --git a/pw_sync_embos/public/pw_sync_embos/counting_semaphore_inline.h b/pw_sync_embos/public/pw_sync_embos/counting_semaphore_inline.h
index d0b1a8c..55ad150 100644
--- a/pw_sync_embos/public/pw_sync_embos/counting_semaphore_inline.h
+++ b/pw_sync_embos/public/pw_sync_embos/counting_semaphore_inline.h
@@ -41,6 +41,8 @@
inline bool CountingSemaphore::try_acquire_until(
chrono::SystemClock::time_point until_at_least) {
+ // Note that if this deadline is in the future, it will get rounded up by
+ // one whole tick due to how try_acquire_for is implemented.
return try_acquire_for(until_at_least - chrono::SystemClock::now());
}
diff --git a/pw_sync_embos/public/pw_sync_embos/mutex_inline.h b/pw_sync_embos/public/pw_sync_embos/mutex_inline.h
index a8d6862..a8dd26e 100644
--- a/pw_sync_embos/public/pw_sync_embos/mutex_inline.h
+++ b/pw_sync_embos/public/pw_sync_embos/mutex_inline.h
@@ -39,6 +39,8 @@
inline bool Mutex::try_lock_until(
chrono::SystemClock::time_point until_at_least) {
+ // Note that if this deadline is in the future, it will get rounded up by
+ // one whole tick due to how try_lock_for is implemented.
return try_lock_for(until_at_least - chrono::SystemClock::now());
}
diff --git a/pw_thread_embos/public/pw_thread_embos/sleep_inline.h b/pw_thread_embos/public/pw_thread_embos/sleep_inline.h
index e39ad17..5988e7e 100644
--- a/pw_thread_embos/public/pw_thread_embos/sleep_inline.h
+++ b/pw_thread_embos/public/pw_thread_embos/sleep_inline.h
@@ -18,6 +18,8 @@
namespace pw::this_thread {
inline void sleep_until(chrono::SystemClock::time_point until_at_least) {
+ // Note that if this deadline is in the future, it will get rounded up by
+ // one whole tick due to how sleep_for is implemented.
return sleep_for(until_at_least - chrono::SystemClock::now());
}
diff --git a/pw_thread_embos/sleep.cc b/pw_thread_embos/sleep.cc
index 410967c..03ca25f 100644
--- a/pw_thread_embos/sleep.cc
+++ b/pw_thread_embos/sleep.cc
@@ -22,29 +22,28 @@
#include "pw_chrono_embos/system_clock_constants.h"
#include "pw_thread/id.h"
-using pw::chrono::embos::kMaxTimeout;
+using pw::chrono::SystemClock;
namespace pw::this_thread {
void sleep_for(chrono::SystemClock::duration for_at_least) {
PW_DCHECK(get_id() != thread::Id());
- // Clamp negative durations to be 0 which maps to non-blocking.
- for_at_least = std::max(for_at_least, chrono::SystemClock::duration::zero());
-
- // The pw::sleep_{for,until} API contract is to yield if we attempt to sleep
- // for a duration of 0. The embOS delay does not explicitly yield if 0 is
- // passed, ergo we explicitly check for the yield condition.
- if (for_at_least == chrono::SystemClock::duration::zero()) {
- OS_Yield(); // Direct API is used to reduce overhead.
+ // Yield for negative and zero length durations.
+ if (for_at_least <= SystemClock::duration::zero()) {
+ OS_Yield();
return;
}
- while (for_at_least > kMaxTimeout) {
- OS_Delay(kMaxTimeout.count());
- for_at_least -= kMaxTimeout;
+ // On a tick based kernel we cannot tell how far along we are on the current
+ // tick, ergo we add one whole tick to the final duration.
+ constexpr SystemClock::duration kMaxTimeoutMinusOne =
+ pw::chrono::embos::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ OS_Delay(static_cast<OS_TIME>(kMaxTimeoutMinusOne.count()));
+ for_at_least -= kMaxTimeoutMinusOne;
}
- OS_Delay(for_at_least.count());
+ OS_Delay(static_cast<OS_TIME>(for_at_least.count()));
}
} // namespace pw::this_thread
