threadx: fix for_at_least contract to add one tick
Fixes the ThreadX 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 ULONG ticks when
invoking native APIs.
Change-Id: Ia28e0149a5c6426f7363c0467815dc3dcac0a010
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/36923
Reviewed-by: Wyatt Hepler <hepler@google.com>
Pigweed-Auto-Submit: Ewout van Bekkum <ewout@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
diff --git a/pw_sync_threadx/binary_semaphore.cc b/pw_sync_threadx/binary_semaphore.cc
index b902966..5e16fa9 100644
--- a/pw_sync_threadx/binary_semaphore.cc
+++ b/pw_sync_threadx/binary_semaphore.cc
@@ -23,7 +23,6 @@
#include "tx_api.h"
using pw::chrono::SystemClock;
-using pw::chrono::threadx::kMaxTimeout;
namespace pw::sync {
@@ -31,20 +30,27 @@
// Enforce the pw::sync::BinarySemaphore IRQ contract.
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) {
- const UINT result = tx_semaphore_get(&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::threadx::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ const UINT result = tx_semaphore_get(
+ &native_type_, static_cast<ULONG>(kMaxTimeoutMinusOne.count()));
if (result != TX_NO_INSTANCE) {
// If we didn't time out (TX_NO_INSTANCE), then we should have succeeded.
PW_CHECK_UINT_EQ(TX_SUCCESS, result);
return true;
}
- for_at_least -= kMaxTimeout;
+ for_at_least -= kMaxTimeoutMinusOne;
}
-
- const UINT result = tx_semaphore_get(&native_type_, for_at_least.count());
+ const UINT result = tx_semaphore_get(
+ &native_type_, static_cast<ULONG>(for_at_least.count() + 1));
if (result == TX_NO_INSTANCE) {
return false; // We timed out, there's still no token.
}
diff --git a/pw_sync_threadx/counting_semaphore.cc b/pw_sync_threadx/counting_semaphore.cc
index 30184c1..e2e5c86 100644
--- a/pw_sync_threadx/counting_semaphore.cc
+++ b/pw_sync_threadx/counting_semaphore.cc
@@ -23,7 +23,6 @@
#include "tx_api.h"
using pw::chrono::SystemClock;
-using pw::chrono::threadx::kMaxTimeout;
namespace pw::sync {
@@ -31,19 +30,27 @@
// Enforce the pw::sync::CountingSemaphore IRQ contract.
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) {
- const UINT result = tx_semaphore_get(&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::threadx::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ const UINT result = tx_semaphore_get(
+ &native_type_, static_cast<ULONG>(kMaxTimeoutMinusOne.count()));
if (result != TX_NO_INSTANCE) {
// If we didn't time out (TX_NO_INSTANCE), then we should have succeeded.
PW_CHECK_UINT_EQ(TX_SUCCESS, result);
return true;
}
- for_at_least -= kMaxTimeout;
+ for_at_least -= kMaxTimeoutMinusOne;
}
- const UINT result = tx_semaphore_get(&native_type_, for_at_least.count());
+ const UINT result = tx_semaphore_get(
+ &native_type_, static_cast<ULONG>(for_at_least.count() + 1));
if (result == TX_NO_INSTANCE) {
return false; // We timed out, there's still no token.
}
diff --git a/pw_sync_threadx/mutex.cc b/pw_sync_threadx/mutex.cc
index 0e8fab9..ce05399 100644
--- a/pw_sync_threadx/mutex.cc
+++ b/pw_sync_threadx/mutex.cc
@@ -23,7 +23,6 @@
#include "tx_api.h"
using pw::chrono::SystemClock;
-using pw::chrono::threadx::kMaxTimeout;
namespace pw::sync {
@@ -31,18 +30,26 @@
// Enforce the pw::sync::Mutex IRQ contract.
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 or zero length durations.
+ if (for_at_least <= SystemClock::duration::zero()) {
+ return try_lock();
+ }
- while (for_at_least > kMaxTimeout) {
- const UINT result = tx_mutex_get(&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::threadx::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ const UINT result = tx_mutex_get(
+ &native_type_, static_cast<ULONG>(kMaxTimeoutMinusOne.count()));
if (result != TX_NOT_AVAILABLE) {
PW_CHECK_UINT_EQ(TX_SUCCESS, result);
return true;
}
- for_at_least -= kMaxTimeout;
+ for_at_least -= kMaxTimeoutMinusOne;
}
- const UINT result = tx_mutex_get(&native_type_, for_at_least.count());
+ const UINT result =
+ tx_mutex_get(&native_type_, static_cast<ULONG>(for_at_least.count() + 1));
if (result == TX_NOT_AVAILABLE) {
return false;
}
diff --git a/pw_sync_threadx/public/pw_sync_threadx/binary_semaphore_inline.h b/pw_sync_threadx/public/pw_sync_threadx/binary_semaphore_inline.h
index 7fa8dcd..844a6e6 100644
--- a/pw_sync_threadx/public/pw_sync_threadx/binary_semaphore_inline.h
+++ b/pw_sync_threadx/public/pw_sync_threadx/binary_semaphore_inline.h
@@ -61,6 +61,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_threadx/public/pw_sync_threadx/counting_semaphore_inline.h b/pw_sync_threadx/public/pw_sync_threadx/counting_semaphore_inline.h
index 9dfd1ca..7125e01 100644
--- a/pw_sync_threadx/public/pw_sync_threadx/counting_semaphore_inline.h
+++ b/pw_sync_threadx/public/pw_sync_threadx/counting_semaphore_inline.h
@@ -62,6 +62,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_threadx/public/pw_sync_threadx/mutex_inline.h b/pw_sync_threadx/public/pw_sync_threadx/mutex_inline.h
index 388e6b9..af537bf 100644
--- a/pw_sync_threadx/public/pw_sync_threadx/mutex_inline.h
+++ b/pw_sync_threadx/public/pw_sync_threadx/mutex_inline.h
@@ -55,6 +55,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_threadx/public/pw_thread_threadx/sleep_inline.h b/pw_thread_threadx/public/pw_thread_threadx/sleep_inline.h
index 1fc3df6..d03b744 100644
--- a/pw_thread_threadx/public/pw_thread_threadx/sleep_inline.h
+++ b/pw_thread_threadx/public/pw_thread_threadx/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_threadx/sleep.cc b/pw_thread_threadx/sleep.cc
index 48fdcaa..1cb5e5c 100644
--- a/pw_thread_threadx/sleep.cc
+++ b/pw_thread_threadx/sleep.cc
@@ -22,30 +22,31 @@
#include "pw_thread/id.h"
#include "tx_api.h"
-using pw::chrono::threadx::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. ThreadX's tx_thread_sleep does a no-op if 0 is passed,
- // ergo we explicitly check for the yield condition if the duration is 0.
- if (for_at_least == chrono::SystemClock::duration::zero()) {
- tx_thread_relinquish(); // Direct API is used to reduce overhead.
+ // Yield for negative and zero length durations.
+ if (for_at_least <= chrono::SystemClock::duration::zero()) {
+ tx_thread_relinquish();
return;
}
- while (for_at_least > kMaxTimeout) {
- const UINT result = tx_thread_sleep(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::threadx::kMaxTimeout - SystemClock::duration(1);
+ while (for_at_least > kMaxTimeoutMinusOne) {
+ const UINT result =
+ tx_thread_sleep(static_cast<ULONG>(kMaxTimeoutMinusOne.count()));
PW_CHECK_UINT_EQ(TX_SUCCESS, result);
- for_at_least -= kMaxTimeout;
+ for_at_least -= kMaxTimeoutMinusOne;
}
- const UINT result = tx_thread_sleep(for_at_least.count());
+ const UINT result =
+ tx_thread_sleep(static_cast<ULONG>(for_at_least.count() + 1));
PW_CHECK_UINT_EQ(TX_SUCCESS, result);
}
