More sleep fixes

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);

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) {

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
@@ -141,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
@@ -170,23 +170,23 @@ 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 {
                         alarm_id_t id = make_alarm_id(pool->ordered_head, earliest_entry->sequence);
                         delta = earliest_entry->callback(id, earliest_entry->user_data);
                     }
                 } else {
-                    // negative target means cancel alarm
+                    // cancel alarm
                     delta = 0;
                 }
                 if (delta) {
@@ -245,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) {
@@ -257,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;
@@ -270,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
@@ -393,8 +395,8 @@ 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);
+    // 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;
 }
 #endif
@@ -406,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
@@ -416,8 +419,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();
             }
         }
     }

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

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 = [
@@ -13,6 +13,7 @@ cc_binary(
     target_compatible_with = compatible_with_rp2(),
     deps = [
         "//src/common/pico_sync",
+        "//src/rp2_common/pico_multicore",
         "//test/pico_test",
     ] + select({
         "//bazel/constraint:host": ["//src/host/pico_stdlib"],
@@ -24,9 +25,13 @@ 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",
+        "//src/rp2_common/pico_multicore",
         "//test/pico_test",
     ] + select({
         "//bazel/constraint:host": ["//src/host/pico_stdlib"],

test/pico_sync_test/CMakeLists.txt

@@ -1,11 +1,12 @@
-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_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_link_libraries(pico_sync_test_sw PRIVATE pico_test pico_sync)
+    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()
 

test/pico_sync_test/pico_sync_test.c

@@ -6,12 +6,17 @@
 
 #include <stdio.h>
 
-#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!
+#define MAX_WAITS 5
 
 PICOTEST_MODULE_NAME("SYNC", "sync test");
 
@@ -28,42 +33,39 @@ 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) {
-    uint32_t save = spin_lock_blocking(sleep_notifier.spin_lock);
-    lock_internal_spin_unlock_with_notify(&sleep_notifier, save);
+    // note: this implementation is a copy of the code from pico_time/time.c and should be kept in sync
+    __sev();
     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");
+    // 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);
+    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 < 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;
@@ -80,12 +82,11 @@ 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");
+    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);
@@ -98,24 +99,35 @@ 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++;
             }
         }
     }
     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");
+    PICOTEST_CHECK(wait_count < MAX_WAITS, "Expected <= %d waits", MAX_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();
 }