From 32fcbbd7349d13c22a836259cf119aa14e488e5b Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Sat, 18 Jul 2026 16:29:32 -0500 Subject: [PATCH 1/5] So the sleep_until API used to wait for the notifier which was really just relying on a __wfe(), however if the callback happens before the lock_internal_spin_unlock_with_wait() then on RP2350 we will eat the (already happended event) while trying to discard the event from the spin lock acquire/release This doesn't affect other primitives like mutexes as they test some condition and breakout inside the spin lock, rather than using the event itself as a condition The solution (now that any timer event causes an IRQ) is to just use a __wfe() and post a __sev() from the callback (note the IRQ on the same core would cause an event, but the timer irq may be on ther other core) --- src/common/pico_time/time.c | 10 ++++------ test/pico_sync_test/pico_sync_test.c | 6 ++---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/common/pico_time/time.c b/src/common/pico_time/time.c index e2e76c1d5..419f4fd77 100644 --- a/src/common/pico_time/time.c +++ b/src/common/pico_time/time.c @@ -54,7 +54,6 @@ static inline bool default_alarm_pool_initialized(void) { return default_alarm_pool.lock != NULL; } -static lock_core_t sleep_notifier; #endif #include "pico/time_adapter.h" @@ -87,7 +86,6 @@ void __weak runtime_init_default_alarm_pool(void) { PICO_TIME_DEFAULT_ALARM_POOL_HARDWARE_ALARM_NUM, PICO_TIME_DEFAULT_ALARM_POOL_MAX_TIMERS); } - lock_init(&sleep_notifier, PICO_SPINLOCK_ID_TIMER); #endif } #endif @@ -393,8 +391,7 @@ uint alarm_pool_core_num(alarm_pool_t *pool) { #if !PICO_TIME_DEFAULT_ALARM_POOL_DISABLED static int64_t sleep_until_callback(__unused alarm_id_t id, __unused void *user_data) { - uint32_t save = spin_lock_blocking(sleep_notifier.spin_lock); - lock_internal_spin_unlock_with_notify(&sleep_notifier, save); + __sev(); // signal event in case the waiter is on the other core return 0; } #endif @@ -416,8 +413,9 @@ void sleep_until(absolute_time_t t) { if (add_alarm_at(t_before, sleep_until_callback, NULL, false) >= 0) { // able to add alarm for just before the time while (!time_reached(t_before)) { - uint32_t save = spin_lock_blocking(sleep_notifier.spin_lock); - lock_internal_spin_unlock_with_wait(&sleep_notifier, save); + // note __wfe() is sufficient here because the add_alarm always causes an IRQ which calls + // sleep_until_callback() which also does a __sev() - the irq itself will wake us up if on the same core + __wfe(); } } } diff --git a/test/pico_sync_test/pico_sync_test.c b/test/pico_sync_test/pico_sync_test.c index e9fd8f7c3..dda0938bc 100644 --- a/test/pico_sync_test/pico_sync_test.c +++ b/test/pico_sync_test/pico_sync_test.c @@ -31,8 +31,7 @@ int64_t notify_lock_with_flag(__unused alarm_id_t id, void *user_data) { static lock_core_t sleep_notifier; static int64_t sleep_until_callback(__unused alarm_id_t id, __unused void *user_data) { - uint32_t save = spin_lock_blocking(sleep_notifier.spin_lock); - lock_internal_spin_unlock_with_notify(&sleep_notifier, save); + __sev(); return 0; } @@ -98,8 +97,7 @@ int main() { if (add_alarm_at(t_before, sleep_until_callback, NULL, false) >= 0) { // able to add alarm for just before the time while (!time_reached(t_before)) { - uint32_t save = spin_lock_blocking(sleep_notifier.spin_lock); - lock_internal_spin_unlock_with_wait(&sleep_notifier, save); + __wfe(); wait_count++; } } From 057582abfa1e7328f5a0774408ffc5d7b569b029 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Sat, 18 Jul 2026 19:54:59 -0500 Subject: [PATCH 2/5] Update pico_sync_test to be multicore --- test/pico_sync_test/BUILD.bazel | 2 + test/pico_sync_test/CMakeLists.txt | 4 +- test/pico_sync_test/pico_sync_test.c | 114 +++++++++++++++++++-------- test/pico_test/include/pico/test.h | 8 +- 4 files changed, 87 insertions(+), 41 deletions(-) diff --git a/test/pico_sync_test/BUILD.bazel b/test/pico_sync_test/BUILD.bazel index 5705363cb..f1d03de54 100644 --- a/test/pico_sync_test/BUILD.bazel +++ b/test/pico_sync_test/BUILD.bazel @@ -12,6 +12,7 @@ cc_binary( ], target_compatible_with = compatible_with_rp2(), deps = [ + "//src/rp2_common/pico_multicore", "//src/common/pico_sync", "//test/pico_test", ] + select({ @@ -26,6 +27,7 @@ cc_binary( srcs = ["pico_sync_test.c"], target_compatible_with = ["//bazel/constraint:rp2350"], deps = [ + "//src/rp2_common/pico_multicore", "//src/common/pico_sync", "//test/pico_test", ] + select({ diff --git a/test/pico_sync_test/CMakeLists.txt b/test/pico_sync_test/CMakeLists.txt index 1b23e283a..3b843a7b0 100644 --- a/test/pico_sync_test/CMakeLists.txt +++ b/test/pico_sync_test/CMakeLists.txt @@ -1,11 +1,11 @@ add_executable(pico_sync_test_hw pico_sync_test.c) target_compile_definitions(pico_sync_test_hw PRIVATE PICO_USE_SW_SPIN_LOCKS=0) -target_link_libraries(pico_sync_test_hw PRIVATE pico_test pico_sync) +target_link_libraries(pico_sync_test_hw PRIVATE pico_test pico_sync pico_multicore) pico_add_extra_outputs(pico_sync_test_hw) if (NOT PICO_RP2040) add_executable(pico_sync_test_sw pico_sync_test.c) - target_link_libraries(pico_sync_test_sw PRIVATE pico_test pico_sync) + target_link_libraries(pico_sync_test_sw PRIVATE pico_test pico_sync pico_multicore) pico_add_extra_outputs(pico_sync_test_sw) endif() diff --git a/test/pico_sync_test/pico_sync_test.c b/test/pico_sync_test/pico_sync_test.c index dda0938bc..b0466ee37 100644 --- a/test/pico_sync_test/pico_sync_test.c +++ b/test/pico_sync_test/pico_sync_test.c @@ -6,12 +6,16 @@ #include -#include "../../src/common/pico_base_headers/include/pico/types.h" -#include "../../src/common/pico_time/include/pico/time.h" -#include "../../src/rp2_common/hardware_sync_spin_lock/include/hardware/sync/spin_lock.h" +#include "pico/time.h" #include "pico/lock_core.h" +#include "pico/multicore.h" #include "pico/test.h" #include "pico/stdio.h" +#include "pico/sync.h" + +// Note in this test, the main thing is that the processor doesn't spin - the actual number of loops +// when successfully coming to a sleep is between 1 and 5 depending on platform/core etc. and the reasoning +// is left as an exercise for the reader! PICOTEST_MODULE_NAME("SYNC", "sync test"); @@ -35,31 +39,41 @@ static int64_t sleep_until_callback(__unused alarm_id_t id, __unused void *user_ return 0; } -int main() { - stdio_init_all(); +static semaphore_t core1_sem; - PICOTEST_START(); +static int do_test(void) { + int core_num = get_core_num(); + printf("=== Test on core %d ===\n", core_num); PICOTEST_START_SECTION("check low power lock_core wait loop with timeout"); - #if PICO_USE_SW_SPIN_LOCKS - // looping twice (rather than once) is an implementation detail, however we shouldn't loop continuously (i.e. we should __wfe) - #define EXPECTED_TIMEOUT_WAITS 2 - #else - // note that without sw spin locks we wait an extra time (an extra SEV doesn't get eaten) - #define EXPECTED_TIMEOUT_WAITS 3 - #endif - lock_core_t lock; - lock_init(&lock, 0); - absolute_time_t until = make_timeout_time_ms(50); - int wait_count = 0; - do { - uint32_t save = spin_lock_blocking(lock.spin_lock); - wait_count++; - if (lock_internal_spin_unlock_with_best_effort_wait_or_timeout(&lock, save, until)) break; - } while (true); - printf("Waited %d times\n", wait_count); - PICOTEST_CHECK(wait_count == EXPECTED_TIMEOUT_WAITS, "Expected exactly " __XSTRING(EXPECTED_TIMEOUT_WAITS) " waits"); + lock_core_t lock; + lock_init(&lock, 0); + absolute_time_t until = make_timeout_time_ms(50); + int wait_count = 0; + do { + uint32_t save = spin_lock_blocking(lock.spin_lock); + wait_count++; + if (lock_internal_spin_unlock_with_best_effort_wait_or_timeout(&lock, save, until)) break; + } while (true); + printf("Waited %d times\n", wait_count); + + int expected_timeout_waits; +#if PICO_USE_SW_SPIN_LOCKS + // looping twice (rather than once) is an implementation detail, however we shouldn't loop continuously (i.e. we should __wfe) + expected_timeout_waits = 2; +#else + // note that without sw spin locks we wait an extra time (an extra SEV doesn't get eaten) + expected_timeout_waits = 3; +#endif + if (core_num) { +#if PICO_RP2040 || PICO_USE_SW_SPIN_LOCKS + expected_timeout_waits += 2; +#else + expected_timeout_waits += 1; +#endif + } + PICOTEST_CHECK(wait_count == expected_timeout_waits, "Expected exactly %d waits", expected_timeout_waits); PICOTEST_END_SECTION(); PICOTEST_START_SECTION("check low power lock_core wait loop without timeout"); @@ -79,7 +93,15 @@ int main() { lock_internal_spin_unlock_with_wait(&lock_with_flag.lock, save); } while (true); printf("Waited %d times\n", wait_count); - PICOTEST_CHECK(wait_count == 1, "Expected exactly 1 wait"); + int expected_timeout_waits = 1; + if (core_num) { +#if PICO_RP2040 + expected_timeout_waits += 2; +#else + expected_timeout_waits ++; +#endif + } + PICOTEST_CHECK(wait_count == expected_timeout_waits, "Expected exactly %d waits", expected_timeout_waits); PICOTEST_END_SECTION(); PICOTEST_START_SECTION("check low power sleep loop"); @@ -103,17 +125,39 @@ int main() { } } printf("Waited %d times\n", wait_count); - #undef EXPECTED_TIMEOUT_WAITS - #if PICO_USE_SW_SPIN_LOCKS - // looping twice (rather than once) is an implementation detail, however we shouldn't loop continuously (i.e. we should __wfe) - #define EXPECTED_TIMEOUT_WAITS 1 - #else - // note that without sw spin locks we wait an extra time (an extra SEV doesn't get eaten) - #define EXPECTED_TIMEOUT_WAITS 2 - #endif - - PICOTEST_CHECK(wait_count == EXPECTED_TIMEOUT_WAITS, "Expected exactly " __XSTRING(EXPECTED_TIMEOUT_WAITS) " waits"); + + int expected_timeout_waits; + expected_timeout_waits = 2; + if (core_num) { +#if PICO_RP2040 + expected_timeout_waits += 2; +#else + expected_timeout_waits += 1; +#endif + } + PICOTEST_CHECK(wait_count == expected_timeout_waits, "Expected exactly %d waits", expected_timeout_waits); PICOTEST_END_SECTION(); + return picotest_error_code; +} + +int core1_picotest_error_code; +void core1_func() { + core1_picotest_error_code = do_test(); + sem_release(&core1_sem); +} + +int main() { + stdio_init_all(); + + PICOTEST_START(); + + picotest_error_code = do_test(); + if (!picotest_error_code) { + multicore_launch_core1(core1_func); + sem_init(&core1_sem, 0, 1); + sem_acquire_blocking(&core1_sem); + picotest_error_code = core1_picotest_error_code; + } PICOTEST_END_TEST(); } diff --git a/test/pico_test/include/pico/test.h b/test/pico_test/include/pico/test.h index 1352424e3..3dac30874 100644 --- a/test/pico_test/include/pico/test.h +++ b/test/pico_test/include/pico/test.h @@ -29,8 +29,8 @@ but not sure that is implemented yet. printf("Module %s: Section %s : Passed\n", picotest_module, picotest_section_name); \ } -#define PICOTEST_CHECK(COND, MESSAGE) if (!(COND)) { \ - printf("Module %s: %s\n", picotest_module, MESSAGE); \ +#define PICOTEST_CHECK(COND, MESSAGE, ...) if (!(COND)) { \ + printf("Module %s: ", picotest_module); printf(MESSAGE, ## __VA_ARGS__); printf("\n"); \ picotest_error_code = -1; \ } #define PICOTEST_CHECK_CHANNEL(CHANNEL, COND, MESSAGE) if (!(COND)) { \ @@ -38,8 +38,8 @@ but not sure that is implemented yet. picotest_error_code = -1; \ } -#define PICOTEST_CHECK_AND_ABORT(COND, MESSAGE) if (!(COND)) { \ - printf("Module %s: %s\n", picotest_module, MESSAGE); \ +#define PICOTEST_CHECK_AND_ABORT(COND, MESSAGE, ...) if (!(COND)) { \ + printf("Module %s: ", picotest_module); printf(MESSAGE, ## __VA_ARGS__); printf("\n"); \ picotest_error_code = -1; \ return -1; \ } From 8b3c94fdb71a39afe6ad1a67608d326be5874e96 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Sat, 18 Jul 2026 19:56:27 -0500 Subject: [PATCH 3/5] bug wac-o-mole with pico_time_test - we broke the invariant of adding a wakeup in the future if an alarm was created and destroyed before the ta_set_timeout was ever called. we must always set an alarm if there is one --- src/common/pico_time/time.c | 46 +++++++++++++++------------- test/pico_time_test/pico_time_test.c | 2 +- 2 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/common/pico_time/time.c b/src/common/pico_time/time.c index 419f4fd77..43624e059 100644 --- a/src/common/pico_time/time.c +++ b/src/common/pico_time/time.c @@ -139,7 +139,9 @@ static void alarm_pool_irq_handler(void); // marker which we can use in place of handler function to indicate we are a repeating timer -#define repeating_timer_marker ((alarm_callback_t)alarm_pool_irq_handler) +#define repeating_timer_marker ((alarm_callback_t)(uintptr_t)2) +#define deleted_timer_marker ((alarm_callback_t)(uintptr_t)4) + #include "hardware/gpio.h" static void alarm_pool_irq_handler(void) { // This IRQ handler does the main work, as it always (assuming the IRQ hasn't been enabled on both cores @@ -168,15 +170,15 @@ static void alarm_pool_irq_handler(void) { if (earliest_index >= 0) { alarm_pool_entry_t *earliest_entry = &pool->entries[earliest_index]; earliest_target = earliest_entry->target; - if (((int64_t)ta_time_us_64(timer) - earliest_target) >= 0) { - // time to call the callback now (or in the past) - // note that an entry->target of < 0 means the entry has been canceled (not this is set - // by this function, in response to the entry having been queued by the cancel_alarm API - // meaning that we don't need to worry about tearing of the 64 bit value) + // delete_timer flag is set by this function, and then the item is moved to the front + // of the queue for deletion now + bool delete_timer = earliest_entry->callback == deleted_timer_marker; + if (delete_timer || ((int64_t)ta_time_us_64(timer) - earliest_target) >= 0) { int64_t delta; - if (earliest_target >= 0) { - // special case repeating timer without making another function call which adds overhead + if (!delete_timer) { + // time to call the callback now (or in the past) if (earliest_entry->callback == repeating_timer_marker) { + // special case repeating timer without making another function call which adds overhead repeating_timer_t *rpt = (repeating_timer_t *)earliest_entry->user_data; delta = rpt->callback(rpt) ? rpt->delay_us : 0; } else { @@ -184,7 +186,7 @@ static void alarm_pool_irq_handler(void) { delta = earliest_entry->callback(id, earliest_entry->user_data); } } else { - // negative target means cancel alarm + // cancel alarm delta = 0; } if (delta) { @@ -243,6 +245,19 @@ static void alarm_pool_irq_handler(void) { new_entry->next = next; } } + earliest_index = pool->ordered_head; + if (earliest_index < 0) break; + alarm_pool_entry_t *earliest_entry = &pool->entries[earliest_index]; + //printf("NOW EARLIEST INDEX %d timeout %lld\n", earliest_index, earliest_entry->target); + earliest_target = earliest_entry->target; + if (earliest_entry->callback != deleted_timer_marker) { + // we are leaving a timeout every 2^32 microseconds anyway if there is no valid target, so we can choose any value. + // best_effort_wfe_or_timeout now relies on it being the last value set, and arguably this is the + // best value anyway, as it is the furthest away from the last fire. + //printf("SET TIMEOUT %lld\n", earliest_target); + ta_set_timeout(timer, timer_alarm_num, earliest_target); + } + // if we have any canceled alarms, then mark them for removal by setting their due time to -1 (which will // cause them to be handled the next time round and removed) if (pool->has_pending_cancellations) { @@ -255,7 +270,7 @@ static void alarm_pool_irq_handler(void) { int16_t next = entry->next; if ((int16_t)entry->sequence < 0) { // mark for deletion - entry->target = -1; + entry->callback = deleted_timer_marker; if (index != pool->ordered_head) { // move to start of queue *prev = entry->next; @@ -268,17 +283,6 @@ static void alarm_pool_irq_handler(void) { index = next; } } - earliest_index = pool->ordered_head; - if (earliest_index < 0) break; - // need to wait - alarm_pool_entry_t *earliest_entry = &pool->entries[earliest_index]; - earliest_target = earliest_entry->target; - // we are leaving a timeout every 2^32 microseconds anyway if there is no valid target, so we can choose any value. - // best_effort_wfe_or_timeout now relies on it being the last value set, and arguably this is the - // best value anyway, as it is the furthest away from the last fire. - if (earliest_target != -1) { // cancelled alarm has target of -1 - ta_set_timeout(timer, timer_alarm_num, earliest_target); - } // check we haven't now passed the target time; if not we don't want to loop again } while ((earliest_target - (int64_t)ta_time_us_64(timer)) <= 0); // We always want the timer IRQ to wake a WFE so that best_effort_wfe_or_timeout() will wake up. It will wake diff --git a/test/pico_time_test/pico_time_test.c b/test/pico_time_test/pico_time_test.c index b42e16ee7..014b53f65 100644 --- a/test/pico_time_test/pico_time_test.c +++ b/test/pico_time_test/pico_time_test.c @@ -52,7 +52,7 @@ int64_t timer_callback1(alarm_id_t id, void *user_data) { assert(timeout >= timeouts && timeout < (timeouts + NUM_TIMEOUTS)); timeout->fired_at = get_absolute_time(); timeout->fired_count++; -// printf("%d %d %ld\n", timeout->pool, id, to_us_since_boot(timeout->target)); +// printf("%d %d %lld\n", timeout->pool, id, to_us_since_boot(timeout->target)); return 0; } From 21cef58b9b06ea9574d0db47a67f3dd54b1c4aa3 Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Sat, 18 Jul 2026 20:21:35 -0500 Subject: [PATCH 4/5] bazel fixes --- test/pico_sync_test/BUILD.bazel | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/pico_sync_test/BUILD.bazel b/test/pico_sync_test/BUILD.bazel index f1d03de54..a9bffd31f 100644 --- a/test/pico_sync_test/BUILD.bazel +++ b/test/pico_sync_test/BUILD.bazel @@ -12,8 +12,8 @@ cc_binary( ], target_compatible_with = compatible_with_rp2(), deps = [ - "//src/rp2_common/pico_multicore", "//src/common/pico_sync", + "//src/rp2_common/pico_multicore", "//test/pico_test", ] + select({ "//bazel/constraint:host": ["//src/host/pico_stdlib"], @@ -27,8 +27,8 @@ cc_binary( srcs = ["pico_sync_test.c"], target_compatible_with = ["//bazel/constraint:rp2350"], deps = [ - "//src/rp2_common/pico_multicore", "//src/common/pico_sync", + "//src/rp2_common/pico_multicore", "//test/pico_test", ] + select({ "//bazel/constraint:host": ["//src/host/pico_stdlib"], From 7e32ccd25cb8acf4320b2556930108575342298d Mon Sep 17 00:00:00 2001 From: graham sanderson Date: Sun, 19 Jul 2026 18:24:49 -0500 Subject: [PATCH 5/5] new tests and a few comments --- src/common/pico_sync/mutex.c | 3 + src/common/pico_sync/sem.c | 2 + src/common/pico_time/time.c | 2 + test/CMakeLists.txt | 1 + test/pico_sync_test/BUILD.bazel | 5 +- test/pico_sync_test/CMakeLists.txt | 9 +-- test/pico_sync_test/pico_sync_test.c | 50 +++---------- test/pico_time_test/CMakeLists.txt | 15 ++++ test/short_sleep_test/BUILD.bazel | 40 +++++++++++ test/short_sleep_test/CMakeLists.txt | 23 ++++++ test/short_sleep_test/short_sleep_test.c | 91 ++++++++++++++++++++++++ tools/bazel_build.py | 12 +++- 12 files changed, 207 insertions(+), 46 deletions(-) create mode 100644 test/short_sleep_test/BUILD.bazel create mode 100644 test/short_sleep_test/CMakeLists.txt create mode 100644 test/short_sleep_test/short_sleep_test.c diff --git a/src/common/pico_sync/mutex.c b/src/common/pico_sync/mutex.c index 3954da67c..5e0f4c2b0 100644 --- a/src/common/pico_sync/mutex.c +++ b/src/common/pico_sync/mutex.c @@ -66,6 +66,7 @@ void __time_critical_func(mutex_enter_blocking)(mutex_t *mtx) { return; } #endif + // note: if you change the implementation here, please update the similar code in pico_sync_test.c lock_owner_id_t caller = lock_get_caller_owner_id(); do { uint32_t save = spin_lock_blocking(mtx->core.spin_lock); @@ -163,6 +164,7 @@ bool __time_critical_func(mutex_enter_block_until)(mutex_t *mtx, absolute_time_t return recursive_mutex_enter_block_until(mtx, until); } #endif + // note: if you change the implementation here, please update the similar code in pico_sync_test.c assert(mtx->core.spin_lock); lock_owner_id_t caller = lock_get_caller_owner_id(); do { @@ -183,6 +185,7 @@ bool __time_critical_func(mutex_enter_block_until)(mutex_t *mtx, absolute_time_t bool __time_critical_func(recursive_mutex_enter_block_until)(recursive_mutex_t *mtx, absolute_time_t until) { assert(mtx->core.spin_lock); + // note: if you change the implementation here, please update the similar code in pico_sync_test.c lock_owner_id_t caller = lock_get_caller_owner_id(); do { uint32_t save = spin_lock_blocking(mtx->core.spin_lock); diff --git a/src/common/pico_sync/sem.c b/src/common/pico_sync/sem.c index 8fc44580f..7867335fa 100644 --- a/src/common/pico_sync/sem.c +++ b/src/common/pico_sync/sem.c @@ -24,6 +24,7 @@ int __time_critical_func(sem_available)(semaphore_t *sem) { } void __time_critical_func(sem_acquire_blocking)(semaphore_t *sem) { + // note: if you change the implementation here, please update the similar code in pico_sync_test.c do { uint32_t save = spin_lock_blocking(sem->core.spin_lock); if (sem->permits > 0) { @@ -44,6 +45,7 @@ bool __time_critical_func(sem_acquire_timeout_us)(semaphore_t *sem, uint32_t tim } bool __time_critical_func(sem_acquire_block_until)(semaphore_t *sem, absolute_time_t until) { + // note: if you change the implementation here, please update the similar code in pico_sync_test.c do { uint32_t save = spin_lock_blocking(sem->core.spin_lock); if (sem->permits > 0) { diff --git a/src/common/pico_time/time.c b/src/common/pico_time/time.c index 43624e059..dff981f55 100644 --- a/src/common/pico_time/time.c +++ b/src/common/pico_time/time.c @@ -395,6 +395,7 @@ uint alarm_pool_core_num(alarm_pool_t *pool) { #if !PICO_TIME_DEFAULT_ALARM_POOL_DISABLED static int64_t sleep_until_callback(__unused alarm_id_t id, __unused void *user_data) { + // note this implementation is copied in pico_sync_test.c and should be updated if this code is __sev(); // signal event in case the waiter is on the other core return 0; } @@ -407,6 +408,7 @@ void sleep_until(absolute_time_t t) { } #endif #if !PICO_TIME_DEFAULT_ALARM_POOL_DISABLED + // note this implementation is copied in pico_sync_test.c and should be updated if this code is uint64_t t_us = to_us_since_boot(t); uint64_t t_before_us = t_us - PICO_TIME_SLEEP_OVERHEAD_ADJUST_US; // needs to work in the first PICO_TIME_SLEEP_OVERHEAD_ADJUST_US of boot diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e35796619..bc429d387 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -22,6 +22,7 @@ if (PICO_ON_DEVICE) add_subdirectory(pico_low_power_test) add_subdirectory(pico_async_context_test) add_subdirectory(pico_thread_local_test) + add_subdirectory(short_sleep_test) endif() # PICO_TEST_FILE_GENERATOR: Path to the a CMake script to generate test files for ci diff --git a/test/pico_sync_test/BUILD.bazel b/test/pico_sync_test/BUILD.bazel index a9bffd31f..9bc67344d 100644 --- a/test/pico_sync_test/BUILD.bazel +++ b/test/pico_sync_test/BUILD.bazel @@ -4,7 +4,7 @@ load("//bazel:defs.bzl", "compatible_with_rp2") package(default_visibility = ["//visibility:public"]) cc_binary( - name = "pico_sync_test_hw", + name = "pico_sync_test", testonly = True, srcs = ["pico_sync_test.c"], defines = [ @@ -25,6 +25,9 @@ cc_binary( name = "pico_sync_test_sw", testonly = True, srcs = ["pico_sync_test.c"], + defines = [ + "PICO_USE_SW_SPIN_LOCKS=1", + ], target_compatible_with = ["//bazel/constraint:rp2350"], deps = [ "//src/common/pico_sync", diff --git a/test/pico_sync_test/CMakeLists.txt b/test/pico_sync_test/CMakeLists.txt index 3b843a7b0..ea2056de1 100644 --- a/test/pico_sync_test/CMakeLists.txt +++ b/test/pico_sync_test/CMakeLists.txt @@ -1,10 +1,11 @@ -add_executable(pico_sync_test_hw pico_sync_test.c) -target_compile_definitions(pico_sync_test_hw PRIVATE PICO_USE_SW_SPIN_LOCKS=0) -target_link_libraries(pico_sync_test_hw PRIVATE pico_test pico_sync pico_multicore) -pico_add_extra_outputs(pico_sync_test_hw) +add_executable(pico_sync_test pico_sync_test.c) +target_compile_definitions(pico_sync_test PRIVATE PICO_USE_SW_SPIN_LOCKS=0) +target_link_libraries(pico_sync_test PRIVATE pico_test pico_sync pico_multicore) +pico_add_extra_outputs(pico_sync_test) if (NOT PICO_RP2040) add_executable(pico_sync_test_sw pico_sync_test.c) + target_compile_definitions(pico_sync_test_sw PRIVATE PICO_USE_SW_SPIN_LOCKS=1) target_link_libraries(pico_sync_test_sw PRIVATE pico_test pico_sync pico_multicore) pico_add_extra_outputs(pico_sync_test_sw) endif() diff --git a/test/pico_sync_test/pico_sync_test.c b/test/pico_sync_test/pico_sync_test.c index b0466ee37..23895ae42 100644 --- a/test/pico_sync_test/pico_sync_test.c +++ b/test/pico_sync_test/pico_sync_test.c @@ -16,6 +16,7 @@ // Note in this test, the main thing is that the processor doesn't spin - the actual number of loops // when successfully coming to a sleep is between 1 and 5 depending on platform/core etc. and the reasoning // is left as an exercise for the reader! +#define MAX_WAITS 5 PICOTEST_MODULE_NAME("SYNC", "sync test"); @@ -32,9 +33,8 @@ int64_t notify_lock_with_flag(__unused alarm_id_t id, void *user_data) { return 0; } -static lock_core_t sleep_notifier; - static int64_t sleep_until_callback(__unused alarm_id_t id, __unused void *user_data) { + // note: this implementation is a copy of the code from pico_time/time.c and should be kept in sync __sev(); return 0; } @@ -47,6 +47,8 @@ static int do_test(void) { printf("=== Test on core %d ===\n", core_num); PICOTEST_START_SECTION("check low power lock_core wait loop with timeout"); + // note: this implementation is implemented in a similar functions to mutex_enter_block_until(), + // sem_acquire_block_until() etc. and should be updated if they are lock_core_t lock; lock_init(&lock, 0); absolute_time_t until = make_timeout_time_ms(50); @@ -58,25 +60,12 @@ static int do_test(void) { } while (true); printf("Waited %d times\n", wait_count); - int expected_timeout_waits; -#if PICO_USE_SW_SPIN_LOCKS - // looping twice (rather than once) is an implementation detail, however we shouldn't loop continuously (i.e. we should __wfe) - expected_timeout_waits = 2; -#else - // note that without sw spin locks we wait an extra time (an extra SEV doesn't get eaten) - expected_timeout_waits = 3; -#endif - if (core_num) { -#if PICO_RP2040 || PICO_USE_SW_SPIN_LOCKS - expected_timeout_waits += 2; -#else - expected_timeout_waits += 1; -#endif - } - PICOTEST_CHECK(wait_count == expected_timeout_waits, "Expected exactly %d waits", expected_timeout_waits); + PICOTEST_CHECK(wait_count < MAX_WAITS, "Expected <= %d waits", MAX_WAITS); PICOTEST_END_SECTION(); PICOTEST_START_SECTION("check low power lock_core wait loop without timeout"); + // note: this implementation is implemented in a similar functions to mutex_enter_blocking(), + // sem_acquire_blocking() etc. and should be updated if they are lock_with_flag_t lock_with_flag; lock_init(&lock_with_flag.lock, 0); lock_with_flag.flag = false; @@ -93,20 +82,11 @@ static int do_test(void) { lock_internal_spin_unlock_with_wait(&lock_with_flag.lock, save); } while (true); printf("Waited %d times\n", wait_count); - int expected_timeout_waits = 1; - if (core_num) { -#if PICO_RP2040 - expected_timeout_waits += 2; -#else - expected_timeout_waits ++; -#endif - } - PICOTEST_CHECK(wait_count == expected_timeout_waits, "Expected exactly %d waits", expected_timeout_waits); + PICOTEST_CHECK(wait_count < MAX_WAITS, "Expected <= %d waits", MAX_WAITS); PICOTEST_END_SECTION(); PICOTEST_START_SECTION("check low power sleep loop"); - // note sleep implementation taken from pico_ime - lock_init(&sleep_notifier, PICO_SPINLOCK_ID_TIMER); + // note: this sleep implementation is a copy of the code from pico_time/time.c and should be kept in sync int wait_count = 0; absolute_time_t t = make_timeout_time_ms(500); uint64_t t_us = to_us_since_boot(t); @@ -125,17 +105,7 @@ static int do_test(void) { } } printf("Waited %d times\n", wait_count); - - int expected_timeout_waits; - expected_timeout_waits = 2; - if (core_num) { -#if PICO_RP2040 - expected_timeout_waits += 2; -#else - expected_timeout_waits += 1; -#endif - } - PICOTEST_CHECK(wait_count == expected_timeout_waits, "Expected exactly %d waits", expected_timeout_waits); + PICOTEST_CHECK(wait_count < MAX_WAITS, "Expected <= %d waits", MAX_WAITS); PICOTEST_END_SECTION(); return picotest_error_code; } diff --git a/test/pico_time_test/CMakeLists.txt b/test/pico_time_test/CMakeLists.txt index 366325bd5..be969f808 100644 --- a/test/pico_time_test/CMakeLists.txt +++ b/test/pico_time_test/CMakeLists.txt @@ -3,10 +3,25 @@ if (NOT PICO_TIME_NO_ALARM_SUPPORT) set_target_properties(pico_time_test PROPERTIES PICO_TEST_TIMEOUT "20") target_compile_definitions(pico_time_test PRIVATE PICO_TIME_DEFAULT_ALARM_POOL_MAX_TIMERS=250 + PICO_USE_SW_SPIN_LOCKS=0 ) target_link_libraries(pico_time_test PRIVATE pico_test) if (PICO_RP2040) target_link_libraries(pico_time_test PRIVATE pico_aon_timer) endif() pico_add_extra_outputs(pico_time_test) + + if (NOT PICO_RP2040) + add_executable(pico_time_test_sw pico_time_test.c) + set_target_properties(pico_time_test_sw PROPERTIES PICO_TEST_TIMEOUT "20") + target_compile_definitions(pico_time_test_sw PRIVATE + PICO_TIME_DEFAULT_ALARM_POOL_MAX_TIMERS=250 + PICO_USE_SW_SPIN_LOCKS=1 + ) + target_link_libraries(pico_time_test_sw PRIVATE pico_test) + if (PICO_RP2040) + target_link_libraries(pico_time_test_sw PRIVATE pico_aon_timer) + endif() + pico_add_extra_outputs(pico_time_test_sw) + endif() endif() \ No newline at end of file diff --git a/test/short_sleep_test/BUILD.bazel b/test/short_sleep_test/BUILD.bazel new file mode 100644 index 000000000..fcdad5a5d --- /dev/null +++ b/test/short_sleep_test/BUILD.bazel @@ -0,0 +1,40 @@ +load("@rules_cc//cc:cc_binary.bzl", "cc_binary") +load("//bazel:defs.bzl", "compatible_with_rp2") + +package(default_visibility = ["//visibility:public"]) + +cc_binary( + name = "short_sleep_test", + testonly = True, + srcs = ["short_sleep_test.c"], + defines = [ + "PICO_USE_SW_SPIN_LOCKS=0", + ], + target_compatible_with = compatible_with_rp2(), + deps = [ + "//src/common/pico_time", + "//src/rp2_common/pico_multicore", + "//test/pico_test", + ] + select({ + "//bazel/constraint:host": ["//src/host/pico_stdlib"], + "//conditions:default": ["//src/rp2_common/pico_stdlib"], + }), +) + +cc_binary( + name = "short_sleep_test_sw", + testonly = True, + srcs = ["short_sleep_test.c"], + defines = [ + "PICO_USE_SW_SPIN_LOCKS=1", + ], + target_compatible_with = ["//bazel/constraint:rp2350"], + deps = [ + "//src/common/pico_time", + "//src/rp2_common/pico_multicore", + "//test/pico_test", + ] + select({ + "//bazel/constraint:host": ["//src/host/pico_stdlib"], + "//conditions:default": ["//src/rp2_common/pico_stdlib"], + }), +) diff --git a/test/short_sleep_test/CMakeLists.txt b/test/short_sleep_test/CMakeLists.txt new file mode 100644 index 000000000..967ebd95b --- /dev/null +++ b/test/short_sleep_test/CMakeLists.txt @@ -0,0 +1,23 @@ +if (NOT PICO_TIME_NO_ALARM_SUPPORT) + add_executable(short_sleep_test short_sleep_test.c) + set_target_properties(short_sleep_test PROPERTIES PICO_TEST_TIMEOUT "20") + target_compile_definitions(short_sleep_test PRIVATE + PICO_TIME_SLEEP_OVERHEAD_ADJUST_US=0 + PICO_USE_SW_SPIN_LOCKS=0 + ) + target_link_libraries(short_sleep_test PRIVATE pico_test pico_multicore) + pico_add_extra_outputs(short_sleep_test) + pico_set_binary_type(short_sleep_test copy_to_ram) + + if (NOT PICO_RP2040) + add_executable(short_sleep_test_sw short_sleep_test.c) + set_target_properties(short_sleep_test_sw PROPERTIES PICO_TEST_TIMEOUT "20") + target_compile_definitions(short_sleep_test_sw PRIVATE + PICO_TIME_SLEEP_OVERHEAD_ADJUST_US=0 + PICO_USE_SW_SPIN_LOCKS=1 + ) + target_link_libraries(short_sleep_test_sw PRIVATE pico_test pico_multicore) + pico_add_extra_outputs(short_sleep_test_sw) + pico_set_binary_type(short_sleep_test_sw copy_to_ram) + endif() +endif() \ No newline at end of file diff --git a/test/short_sleep_test/short_sleep_test.c b/test/short_sleep_test/short_sleep_test.c new file mode 100644 index 000000000..7d10f68c4 --- /dev/null +++ b/test/short_sleep_test/short_sleep_test.c @@ -0,0 +1,91 @@ +/** +* Copyright (c) 2026 Raspberry Pi (Trading) Ltd. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include + +#include "pico/multicore.h" +#include "pico/stdlib.h" +#include "pico/test.h" + +static_assert(PICO_TIME_SLEEP_OVERHEAD_ADJUST_US == 0, "want to test without sleep busy wait"); + + +#define DELAY_US_MIN 0 +#define DELAY_US_MAX 50 +#define SLEEPS_PER_DELAY 20 +#ifndef NDEBUG +#define MIN_SLEEP_US 20 +#define SLOWER_CORE0_MARGIN_US 5 +#else +#define MIN_SLEEP_US 10 +#define SLOWER_CORE0_MARGIN_US 3 +#endif + +bool allow_slower_core0; + +int do_sleeps() { + uint core_num = get_core_num(); + uint32_t tstart = time_us_32(); + for (int delay = DELAY_US_MIN; delay < DELAY_US_MAX; delay++) { + uint32_t t0 = time_us_32(); + uint32_t t2; + for (int count = 0; count < SLEEPS_PER_DELAY; count++) { + uint32_t t1 = time_us_32(); + sleep_us(delay); + t2 = time_us_32(); + // do minimum time check per sleep as we should never wake up too soon + int tdelta = (int)(t2 - t1); + if (tdelta < delay) { + printf("Core %u: tdelta for sleep %d %d < expected min %d\n", core_num , delay, tdelta, delay); + return -1; + } + } + // do maximum time check per group of sleeps to allow for some jitter + int tdelta = (int)(t2 - t0); + int max_delay = MAX((delay + 2), MIN_SLEEP_US) * SLEEPS_PER_DELAY + 1; + if (allow_slower_core0 && !core_num) max_delay += SLOWER_CORE0_MARGIN_US * SLEEPS_PER_DELAY; + if (tdelta > max_delay) { + printf("Cored %u: cumulative tdelta for sleep %d %d > expected max %d\n", core_num, delay, tdelta, max_delay); + return -1; + } + } + uint32_t tend = time_us_32(); + printf("Core %u: total time %dus\n", core_num, (int)(tend - tstart)); + return 0; +} + +semaphore_t core1_sem; +int core1_picotest_error_code; + +void core1_do_sleeps() { + core1_picotest_error_code = do_sleeps(); + sem_release(&core1_sem); +} + +int main() { + stdio_init_all(); + sem_init(&core1_sem, 0, 1); + + PICOTEST_MODULE_NAME("short_sleep_test", "short_sleep test harness"); + PICOTEST_START(); + PICOTEST_START_SECTION("sleeps core 0"); + picotest_error_code = do_sleeps(); + PICOTEST_END_SECTION(); + PICOTEST_START_SECTION("sleeps core 1"); + multicore_launch_core1(core1_do_sleeps); + sem_acquire_blocking(&core1_sem); + picotest_error_code = core1_picotest_error_code; + PICOTEST_END_SECTION(); + PICOTEST_START_SECTION("sleeps core 0 & 1"); + allow_slower_core0 = true; + multicore_reset_core1(); + multicore_launch_core1(core1_do_sleeps); + picotest_error_code = do_sleeps(); + sem_acquire_blocking(&core1_sem); + if (!picotest_error_code) picotest_error_code = core1_picotest_error_code; + PICOTEST_END_SECTION(); + PICOTEST_END_TEST(); +} \ No newline at end of file diff --git a/tools/bazel_build.py b/tools/bazel_build.py index 4ba94ce26..e68a8ec68 100755 --- a/tools/bazel_build.py +++ b/tools/bazel_build.py @@ -57,14 +57,18 @@ "//test/pico_thread_local_test:pico_thread_local_test", "//test/pico_thread_local_test:pico_thread_local_test_cpp", "//test/pico_time_test:pico_time_test", + "//test/pico_time_test:pico_time_test_sw", + "//test/pico_time_test:pico_time_test_sw", "//test/pico_low_power_test:low_power_test_timers", "//test/pico_low_power_test:low_power_test_gpio", "//test/pico_low_power_test:low_power_test_simple", "//test/pico_low_power_test:external_sleep_timer", "//test/pico_async_context_test:pico_async_context_test", + "//test/pico_sync_test:pico_sync_test", "//test/pico_sync_test:pico_sync_test_sw", - "//test/pico_sync_test:pico_sync_test_hw", "//test/pico_xip_sram_test:pico_critical_xip_sram_test", + "//test/short_sleep_test:short_sleep_test", + "//test/short_sleep_test:short_sleep_test_sw", # Pretty much only Picotool and pioasm build on Windows. "//..." if os.name == "nt" else "", @@ -93,6 +97,8 @@ "//test/kitchen_sink:kitchen_sink_psram", # No SW spinlocks on RP2040 "//test/pico_sync_test:pico_sync_test_sw", + "//test/pico_time_test:pico_time_test_sw", + "//test/short_sleep_test:short_sleep_test_sw", ) ), }, @@ -136,6 +142,8 @@ "//test/kitchen_sink:kitchen_sink_psram", # No SW spinlocks on RP2040 "//test/pico_sync_test:pico_sync_test_sw", + "//test/pico_time_test:pico_time_test_sw", + "//test/short_sleep_test:short_sleep_test_sw", ) ), }, @@ -180,6 +188,8 @@ "//test/kitchen_sink:kitchen_sink_psram", # No SW spinlocks on RP2040 "//test/pico_sync_test:pico_sync_test_sw", + "//test/pico_time_test:pico_time_test_sw", + "//test/short_sleep_test:short_sleep_test_sw", ) ), },