More sleep fixes #3079

2026-07-18T20:20:40-05:00

kilograham commented

2026-07-18 20:20:40 -05:00

(Migrated from github.com)

fixes #3078

peterharperuk (Migrated from github.com) reviewed

2026-07-18 20:20:40 -05:00

will-v-pi (Migrated from github.com) reviewed

2026-07-18 20:20:40 -05:00

lurch (Migrated from github.com) reviewed

2026-07-19 10:57:27 -05:00

test/pico_sync_test/pico_sync_test.c

					
				@ -28,42 +33,45 @@ int64_t notify_lock_with_flag(__unused alarm_id_t id, void *user_data) {

				    return 0;

				}

I don't claim to understand what this code is doing, but does the callback function now always need to do a __sev(), or is that just an "implementation detail" of this particular test?

If the __sev() isn't required, maybe it's worth adding a separate test that uses a different callback function (which doesn't use __sev()), just to rule out any "side effect" behaviour? 🤷

I don't claim to understand what this code is doing, but does the callback function now _always_ need to do a `__sev()`, or is that just an "implementation detail" of this particular test? If the `__sev()` isn't required, maybe it's worth adding a separate test that uses a different callback function (which doesn't use `__sev()`), just to rule out any "side effect" behaviour? :shrug:

lurch (Migrated from github.com) reviewed

2026-07-20 02:53:44 -05:00

test/pico_time_test/CMakeLists.txt

					
				@ -3,10 +3,22 @@ if (NOT PICO_TIME_NO_ALARM_SUPPORT)

				    set_target_properties(pico_time_test PROPERTIES PICO_TEST_TIMEOUT "20")

It looks like you've got a if (PICO_RP2040) inside a if (NOT PICO_RP2040), so presumably this code will never be reached?

It looks like you've got a `if (PICO_RP2040)` inside a `if (NOT PICO_RP2040)`, so presumably this code will never be reached?

lurch (Migrated from github.com) reviewed

2026-07-20 03:00:07 -05:00

test/short_sleep_test/CMakeLists.txt

					
				@ -0,0 +2,4 @@

				    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

It looks like you need to add this define to test/short_sleep_test/BUILD.bazel too? (Because the Bazel builds are currently failing CI)

It looks like you need to add this define to `test/short_sleep_test/BUILD.bazel` too? (Because the Bazel builds are currently failing CI)

lurch (Migrated from github.com) reviewed

2026-07-20 03:00:56 -05:00

test/pico_sync_test/pico_sync_test.c

					
				@ -28,42 +33,45 @@ int64_t notify_lock_with_flag(__unused alarm_id_t id, void *user_data) {

				    return 0;

				}

Thanks Graham 😃

Thanks Graham :smiley:

kilograham (Migrated from github.com) reviewed

2026-07-21 17:21:25 -05:00

test/pico_time_test/CMakeLists.txt

					
				@ -3,10 +3,22 @@ if (NOT PICO_TIME_NO_ALARM_SUPPORT)

				    set_target_properties(pico_time_test PROPERTIES PICO_TEST_TIMEOUT "20")

ahha

kilograham commented

2026-07-28 11:16:28 -05:00

(Migrated from github.com)

Summary of changes:

sleep_until() adds a timer callback, to notify itself to wake up. This was using a spin_lock/spin_unlock_with_wait, with the callback doing a spin_lock/spin_unlock_with_notify. This was, if I recall, done (vs just WFE and SEV) because these primitives are what are overridden by FreeRTOS, and the additional overhead of the spinlock is minimal. The problem on RP2350 with software spinlocks, is that the spin_lock/spin_unlock_with_wait wakes up immediately. The fix here is to use WFE/SEV unless the macros are overridden
Violation of "alarm always wakes up on or before active alarm" was broken if an alarm was added, and deleted again before the IRQ handler woke up. In this case the alarm was never added to the timer. This was fixed by re-ordering code in alarm_pool_irq_handler and using a function callback marker constant instead of changing the timeout, so that the timeout value was not lost
The timeout function markers (repeating_timer_marker and deleted_timer_marker were changed to be small immediate even values, to reduce code size, and so attempting to call them will trap)
The recent addition of a special check in best_effort_wfe_or_timeout for last_added == timeout_timestamp meant we always did a WFE in that case, which is wrong if best_effotrt_wfe_or_timeout is called repeatedly for the same timestamp that has now passed. A check is added for the WFE for this case

Summary of changes: 1. `sleep_until()` adds a timer callback, to notify itself to wake up. This was using a `spin_lock/spin_unlock_with_wait`, with the callback doing a `spin_lock/spin_unlock_with_notify`. This was, if I recall, done (vs just WFE and SEV) because these primitives are what are overridden by FreeRTOS, and the additional overhead of the spinlock is minimal. The problem on RP2350 with software spinlocks, is that the spin_lock/spin_unlock_with_wait wakes up immediately. The fix here is to use WFE/SEV unless the macros are overridden 2. Violation of "alarm always wakes up on or before active alarm" was broken if an alarm was added, and deleted again before the IRQ handler woke up. In this case the alarm was never added to the timer. This was fixed by re-ordering code in `alarm_pool_irq_handler` and using a function callback marker constant instead of changing the timeout, so that the timeout value was not lost 3. The timeout function markers (`repeating_timer_marker` and `deleted_timer_marker` were changed to be small immediate even values, to reduce code size, and so attempting to call them will trap) 4. The recent addition of a special check in `best_effort_wfe_or_timeout` for last_added == timeout_timestamp meant we always did a WFE in that case, which is wrong if best_effotrt_wfe_or_timeout is called repeatedly for the same timestamp that has now passed. A check is added for the WFE for this case

lurch (Migrated from github.com) reviewed

2026-07-29 04:50:45 -05:00

src/common/pico_time/time.c

					
				@ -13,6 +13,27 @@

				const absolute_time_t ABSOLUTE_TIME_INITIALIZED_VAR(nil_time, 0);

I'm, guessing that this was supposed to be

#if LOCK_INTERNAL_SPIN_UNLOCK_WITH_WAIT_OVERRIDDEN | LOCK_INTERNAL_SPIN_UNLOCK_WITH_NOTIFY_OVERRIDDEN | LOCK_INTERNAL_SPIN_UNLOCK_WITH_BEST_EFFORT_WAIT_OR_TIMEOUT_OVERRIDDEN

?

I'm, guessing that this was supposed to be ```suggestion #if LOCK_INTERNAL_SPIN_UNLOCK_WITH_WAIT_OVERRIDDEN | LOCK_INTERNAL_SPIN_UNLOCK_WITH_NOTIFY_OVERRIDDEN | LOCK_INTERNAL_SPIN_UNLOCK_WITH_BEST_EFFORT_WAIT_OR_TIMEOUT_OVERRIDDEN ``` ?

kilograham commented

2026-08-08 18:30:09 -05:00

(Migrated from github.com)

Additional changes:
5. Fixed a claude detected race conditoin
6. Added PICO_PLATFORM_NAME so tests that want to print something meaningful don't have to #Ifdef
7. Added newer even more improved claude co-written test which is also designed to test (the almost certainly broken) FreeRTOS - interop.. tests currently pass on rp2040,rp2350-arm-s,rrp2350-riscv with hardware/software spinlocks on RP2350

Additional changes: 5. Fixed a claude detected race conditoin 6. Added PICO_PLATFORM_NAME so tests that want to print something meaningful don't have to #Ifdef 7. Added newer even more improved claude co-written test which is also designed to test (the almost certainly broken) FreeRTOS - interop.. tests currently pass on `rp2040`,`rp2350-arm-s`,`rrp2350-riscv` with hardware/software spinlocks on RP2350

Wren6991 commented

2026-08-11 17:26:41 -05:00

(Migrated from github.com)

Reviewed the deletion race fixes -- look correct to me. Not digested the other changes.

Wren6991 (Migrated from github.com) reviewed

2026-08-11 17:33:48 -05:00

src/common/pico_time/time.c

					
				@ -13,6 +13,27 @@

				const absolute_time_t ABSOLUTE_TIME_INITIALIZED_VAR(nil_time, 0);

				const absolute_time_t ABSOLUTE_TIME_INITIALIZED_VAR(at_the_end_of_time, INT64_MAX);

				// Whether sleep_until() waits on a lock_core rather than on a bare __wfe().

If you're going to generate code, please at least write the comments yourself. This is obvious slop and I'd rather not read it every time I look in this file.

src/common/pico_time/time.c

Single-copy atomicity of aligned loads and stores is a RISC-V guarantee, not a Hazard3 one.

Wren6991 commented

2026-08-11 17:35:25 -05:00

(Migrated from github.com)

I don't think the use of LLMs here is a good direction. This code is already incredibly demanding to understand, which is why I left it unreviewed for so long. I think it's made worse by these changes.

kilograham commented

2026-08-11 18:44:06 -05:00

(Migrated from github.com)

yeah i plan to redact a lot of the comments which is why i haven't merged this yet

kilograham commented

2026-08-11 20:06:06 -05:00

(Migrated from github.com)

i disagree on whether LLM is helpful here - it has been; the upshot though is actually that a fix I'd made in June fixed A and introduced B. A separate fix i myself made as part of this PR fixed C (and unknown to me was actually a fix for A). Fixing B in this PR actually broke D. so the actual correct fix was just my change that fixed C. The tests added by this PR have been invaluable. The net code changes themselves are small, but I will actually split them out into separate commits because half of them are not even related to fixing the particular known bugs and can be applied individually to fix E & F etc

kilograham commented

2026-08-12 19:49:24 -05:00

(Migrated from github.com)