Combined sleep fixes #3127

2026-08-12T19:48:51-05:00

kilograham commented

2026-08-12 19:48:51 -05:00

(Migrated from github.com)

Against usual tradition, this is worth landing as individual commits, however keeping here for review.

@Wren6991 please pile any (we expect comment only) changes in a new PR on top

Against usual tradition, this is worth landing as individual commits, however keeping here for review. @Wren6991 please pile any (we expect comment only) changes in a new PR on top

lurch (Migrated from github.com) reviewed

2026-08-13 03:30:17 -05:00

src/common/pico_base_headers/include/pico/config.h

					
				@ -18,6 +18,11 @@

				#include "pico/config_autogen.h"

				// PICO_CONFIG: PICO_PLATFORM_STRING, Name of platform as a string; defaulted to "unknown" if the build does not set it, group=pico_base

The code in tools/extract_configs.py will assume that any PICO_CONFIG: without an explicit type=, is of type int. I guess I ought to add support for type=string ?

Hmm, looks like PICO_CONFIG: PICO_CONFIG_RTOS_ADAPTER_HEADER below should also be type=string 🙂

The code in `tools/extract_configs.py` will assume that any `PICO_CONFIG:` without an explicit `type=`, is of type `int`. I guess I ought to add support for `type=string` ? Hmm, looks like `PICO_CONFIG: PICO_CONFIG_RTOS_ADAPTER_HEADER` below should also be `type=string` :slightly_smiling_face:

lurch (Migrated from github.com) reviewed

2026-08-13 03:32:36 -05:00

test/pico_float_test/double_benchmark.c

Is it worth adding a PICO_ARCH_STRING, or is that not needed? (because it can be inferred from PICO_PLATFORM_STRING?)

Is it worth adding a `PICO_ARCH_STRING`, or is that not needed? (because it can be inferred from `PICO_PLATFORM_STRING`?)

Wren6991 (Migrated from github.com) reviewed

2026-08-13 09:39:57 -05:00

test/pico_float_test/double_benchmark.c

No, we don't really want people decoding strings at runtime to infer booleans.

Wren6991 (Migrated from github.com) reviewed

2026-08-13 09:41:01 -05:00

test/pico_float_test/double_benchmark.c

PICO_PLATFORM_STRING looks to me to be a human-readable convenience and we shouldn't make it (ugh) load-bearing

`PICO_PLATFORM_STRING` looks to me to be a human-readable convenience and we shouldn't make it (ugh) load-bearing

lurch (Migrated from github.com) reviewed

2026-08-13 10:02:25 -05:00

test/pico_float_test/double_benchmark.c

Yeah, I think I kind of meant "inferred by the human reading the logfile output", rather than "inferred by code at runtime". Maybe.
In case it clarifies things, I was referring specifically to this part of the diff:

-#if PICO_RP2040
-    printf("platform          rp2040\n");
-#elif PICO_RP2350
-    printf("platform          rp2350\n");
-#endif
-#if PICO_RISCV
-    printf("arch              risc-v\n");
-#else
-    printf("arch              arm\n");
-#endif
+    printf("platform          " PICO_PLATFORM_STRING "\n");

I was wondering if somebody might want the ability to do e.g,

-    printf("arch              " PICO_ARCH_STRING "\n");

Yeah, I _think_ I kind of meant "inferred by the human reading the logfile output", rather than "inferred by code at runtime". Maybe. In case it clarifies things, I was referring specifically to this part of the diff: ```patch -#if PICO_RP2040 - printf("platform rp2040\n"); -#elif PICO_RP2350 - printf("platform rp2350\n"); -#endif -#if PICO_RISCV - printf("arch risc-v\n"); -#else - printf("arch arm\n"); -#endif + printf("platform " PICO_PLATFORM_STRING "\n"); ``` I was wondering if somebody might want the ability to do e.g, ```c - printf("arch " PICO_ARCH_STRING "\n"); ```

kilograham (Migrated from github.com) reviewed

2026-08-13 13:50:02 -05:00

test/pico_float_test/double_benchmark.c

yeah tbc - this is PICO_PLATFORM as a string... note i thought there were already some string PICO_CONFIGS?

Wren6991 (Migrated from github.com) reviewed

2026-08-17 04:15:12 -05:00

src/common/pico_base_headers/include/pico/config.h

					
				@ -18,6 +18,11 @@

				#include "pico/config_autogen.h"

				// PICO_CONFIG: PICO_PLATFORM_STRING, Name of platform as a string; defaulted to "unknown" if the build does not set it, group=pico_base

Hmm, looks like PICO_CONFIG: PICO_CONFIG_RTOS_ADAPTER_HEADER below should also be type=string 🙂

That one looks to me like it's a bare macro that gets expanded and stringified by the preprocessor, not a C string. So:

-DPICO_CONFIG_RTOS_ADAPTER_HEADER=foo.h

yields:

#include "foo.h"

This isn't the same thing as a C string literal. Maybe we shouldn't infer int for configs with unspecified types.

> Hmm, looks like PICO_CONFIG: PICO_CONFIG_RTOS_ADAPTER_HEADER below should also be type=string 🙂 That one looks to me like it's a bare macro that gets expanded and stringified by the preprocessor, not a C string. So: ``` -DPICO_CONFIG_RTOS_ADAPTER_HEADER=foo.h ``` yields: ```c #include "foo.h" ``` This isn't the same thing as a C string literal. Maybe we shouldn't infer `int` for configs with unspecified types.

lurch (Migrated from github.com) reviewed

2026-08-17 04:50:13 -05:00

src/common/pico_base_headers/include/pico/config.h

					
				@ -18,6 +18,11 @@

				#include "pico/config_autogen.h"

				// PICO_CONFIG: PICO_PLATFORM_STRING, Name of platform as a string; defaulted to "unknown" if the build does not set it, group=pico_base

Maybe we shouldn't infer int for configs with unspecified types.

I'd be more than happy to modify my config-extraction scripts to insist that a type= is always present, but note that git grep "PICO_CONFIG:" | grep -v "type=" | wc -l reports that there are currently 88 such instances.

This isn't the same thing as a C string literal.

Okay, so could you suggest a suitable type= to be used in this instance?

> Maybe we shouldn't infer `int` for configs with unspecified types. I'd be more than happy to modify my config-extraction scripts to insist that a `type=` is **always** present, but note that `git grep "PICO_CONFIG:" | grep -v "type=" | wc -l` reports that there are currently 88 such instances. > This isn't the same thing as a C string literal. Okay, so could you suggest a suitable `type=` to be used in this instance?

Wren6991 (Migrated from github.com) reviewed

2026-08-17 05:16:50 -05:00

src/common/pico_sync/include/pico/lock_core.h

"cannot block": is this true? IRQs are enabled inside of the spin_unlock. An interrupt could be immediately taken, and consume the event before the WFE executes. Should this instead be:

spin_unlock_unsafe((lock)->spin_lock);
__wfe(); /* consume self-generated event without interruption */
restore_interrupts_from_disabled(save);

It's probably safe on Armv8-M because it sets the event register on return from any non-fault exception, but on Hazard3 it's only set by __h3_unblock() (including from the same core due to external loopback).

"cannot block": is this true? IRQs are enabled inside of the `spin_unlock`. An interrupt could be immediately taken, and consume the event before the WFE executes. Should this instead be: ``` spin_unlock_unsafe((lock)->spin_lock); __wfe(); /* consume self-generated event without interruption */ restore_interrupts_from_disabled(save); ``` It's probably safe on Armv8-M because it sets the event register on return from any non-fault exception, but on Hazard3 it's only set by `__h3_unblock()` (including from the same core due to external loopback).

Wren6991 (Migrated from github.com) reviewed

2026-08-17 05:22:16 -05:00

src/common/pico_base_headers/include/pico/config.h

					
				@ -18,6 +18,11 @@

				#include "pico/config_autogen.h"

				// PICO_CONFIG: PICO_PLATFORM_STRING, Name of platform as a string; defaulted to "unknown" if the build does not set it, group=pico_base

My point is there is not always a suitable type, and plastering int on things with unspecified types doesn't help.

My point is there is not always a suitable type, and plastering `int` on things with unspecified types doesn't help.

Wren6991 (Migrated from github.com) reviewed

2026-08-17 05:38:08 -05:00

Wren6991 (Migrated from github.com) reviewed

2026-08-17 05:55:37 -05:00

Wren6991 (Migrated from github.com) reviewed

2026-08-17 06:45:15 -05:00

src/common/pico_time/time.c

					
				@ -13,6 +13,15 @@

				const absolute_time_t ABSOLUTE_TIME_INITIALIZED_VAR(nil_time, 0);

The callback's __sev() already sets a per-core, sticky event latch, which reaches a waiter that has not yet reached its __wfe() and cannot be consumed by anyone else

Already pointed this out in another comment, but Claude was wrong here: it can be consumed by an IRQ on RISC-V. (I'm going to strip out all the slop comments, but pointing out the issue before I do.)

> The callback's __sev() already sets a per-core, sticky event latch, which reaches a waiter that has not yet reached its __wfe() and cannot be consumed by anyone else Already pointed this out in another comment, but Claude was wrong here: it can be consumed by an IRQ on RISC-V. (I'm going to strip out all the slop comments, but pointing out the issue before I do.)

Wren6991 (Migrated from github.com) reviewed

2026-08-17 07:07:44 -05:00

src/common/pico_base_headers/include/pico/config.h

					
				@ -18,6 +18,11 @@

				#include "pico/config_autogen.h"

				// PICO_CONFIG: PICO_PLATFORM_STRING, Name of platform as a string; defaulted to "unknown" if the build does not set it, group=pico_base

filename or path would work here but it's not really general.

`filename` or `path` would work here but it's not really general.

Wren6991 (Migrated from github.com) reviewed

2026-08-17 08:08:58 -05:00

src/common/pico_sync/include/pico/lock_core.h

Fixed by 971351f, which just swaps the __wfe() with the preceding csrs mstatus/msr PRIMASK.

To hit this in practice you'd have to have a h3.block inside an IRQ handler (or in a switched-to context), which would be odd, but anyway I'm pretty sure it was a bug.

Fixed by [971351f](https://github.com/raspberrypi/pico-sdk/pull/3127/commits/971351f198838ad0f89ca6e5402d240015bf3c6d), which just swaps the `__wfe()` with the preceding `csrs mstatus`/`msr PRIMASK`. To hit this in practice you'd have to have a `h3.block` inside an IRQ handler (or in a switched-to context), which would be odd, but anyway I'm pretty sure it was a bug.

Wren6991 (Migrated from github.com) reviewed

2026-08-18 09:49:07 -05:00

src/common/pico_time/time.c

					
				@ -269,4 +329,4 @@

				                }

				                index = next;

				            }

				        }

Potentially a regression here: The negative target forced us to continue looping, but that is no longer the case. If multiple alarms are deleted, and there are other entries with target times earlier than the deleted alarms' target times, then the non-deleted alarms can get stranded.

Potential fix is to force the loop to continue while there are deleted entries at the head:

diff --git a/src/common/pico_time/time.c b/src/common/pico_time/time.c
index 37037e45..14bd26ce 100644
--- a/src/common/pico_time/time.c
+++ b/src/common/pico_time/time.c
@@ -333,7 +333,8 @@ static void alarm_pool_irq_handler(void) {
             }
         }
         // 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);
+    } while ((earliest_target - (int64_t)ta_time_us_64(timer)) <= 0 ||
+        (pool->ordered_head >= 0 && pool->entries[pool->ordered_head].callback == deleted_timer_marker));
     // We always want the timer IRQ to wake a WFE so that best_effort_wfe_or_timeout() will wake up. It will wake
     // a WFE on its own core by nature of having taken an IRQ, but we do an explicit SEV so it wakes the other core
     __sev();

Potentially a regression here: The negative target forced us to continue looping, but that is no longer the case. If multiple alarms are deleted, and there are other entries with target times earlier than the deleted alarms' target times, then the non-deleted alarms can get stranded. Potential fix is to force the loop to continue while there are deleted entries at the head: ```patch diff --git a/src/common/pico_time/time.c b/src/common/pico_time/time.c index 37037e45..14bd26ce 100644 --- a/src/common/pico_time/time.c +++ b/src/common/pico_time/time.c @@ -333,7 +333,8 @@ static void alarm_pool_irq_handler(void) { } } // 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); + } while ((earliest_target - (int64_t)ta_time_us_64(timer)) <= 0 || + (pool->ordered_head >= 0 && pool->entries[pool->ordered_head].callback == deleted_timer_marker)); // We always want the timer IRQ to wake a WFE so that best_effort_wfe_or_timeout() will wake up. It will wake // a WFE on its own core by nature of having taken an IRQ, but we do an explicit SEV so it wakes the other core __sev(); ```

Wren6991 (Migrated from github.com) reviewed

2026-08-18 20:02:13 -05:00

src/common/pico_time/time.c

					
				@ -269,4 +329,4 @@

				                }

				                index = next;

				            }

				        }

Resolved by f2fc82b, which pushes the deleted entries straight to the free list instead of doing the weird shuffle-to-front.

Resolved by f2fc82b, which pushes the deleted entries straight to the free list instead of doing the weird shuffle-to-front.

Wren6991 (Migrated from github.com) reviewed

2026-08-19 04:54:57 -05:00

test/sync_interop_test/interop_platform.c

					
				@ -0,0 +35,4 @@

				 * distinguish the two. A wait that really blocks takes 1-5 iterations; a busy-poll runs to

				 * thousands.

				 *

				 * NOTE: keep these in step with mutex.c, as pico_sync_test.c already has to.

This test uses a copy/pasted version of the mutex primitive. It doesn't cover the version in the SDK that people actually use. The comment here in the test will be missed by anyone who changes the SDK implementation. The implementation has a comment pointing to pico_sync_test, a different test which also now has a copy/pasted mutex.

Maybe this instrumentation should be added to the SDK implementation (either guarded by or injected through macros) rather than hoping things will stay in sync.

This test uses a copy/pasted version of the `mutex` primitive. It doesn't cover the version in the SDK that people actually use. The comment here in the test will be missed by anyone who changes the SDK implementation. The implementation has a comment pointing to `pico_sync_test`, a different test which *also* now has a copy/pasted `mutex`. Maybe this instrumentation should be added to the SDK implementation (either guarded by or injected through macros) rather than hoping things will stay in sync.

Wren6991 (Migrated from github.com) reviewed

2026-08-19 05:04:40 -05:00

test/hardware_sync_spin_lock_test/hardware_sync_spin_lock_test.c

					
				@ -190,6 +190,100 @@ static const test_t tests[] = {

					},

I think this should be deleted. It's "testing" a fact about the hardware that we simply know to be true. The fact Claude wanted to confirm it while debugging does not make it a meaningful test we should carry forward in the SDK. Also this part:

different instructions -- ldaexb/strexb, which takes a reservation, against amoor.w.aq, which does not

...is simply false, and we shouldn't leave something misleading like that lying around.

I think this should be deleted. It's "testing" a fact about the hardware that we simply know to be true. The fact Claude wanted to confirm it while debugging does not make it a meaningful test we should carry forward in the SDK. Also this part: > different instructions -- ldaexb/strexb, which takes a reservation, against amoor.w.aq, which does not ...is simply false, and we shouldn't leave something misleading like that lying around.

Wren6991 (Migrated from github.com) reviewed

2026-08-19 05:10:55 -05:00

test/pico_time_test/pico_time_test.c

					
				@ -264,2 +265,3 @@

				    picotest_error_code |= issue_2148_test();

				    issue_2186_test();

				    picotest_error_code |= issue_2186_test();

"the scan" here refers to the two-pass deletion code that was removed in the same batch of commits, so I don't think this is useful context to anyone reading this file in the future.

Wren6991 (Migrated from github.com) reviewed

2026-08-19 07:51:11 -05:00

Wren6991 (Migrated from github.com) reviewed

2026-08-19 09:35:00 -05:00

test/hardware_sync_spin_lock_test/hardware_sync_spin_lock_test.c

					
				@ -190,6 +190,100 @@ static const test_t tests[] = {

					},

Probably just revert or rebase out the commit that added this: 27aa1bd96

Claude's commit message for that commit is also false. AMOs do take reservations.

Probably just revert or rebase out the commit that added this: 27aa1bd96 Claude's commit message for that commit is also false. AMOs *do* take reservations.

Wren6991 (Migrated from github.com) reviewed

2026-08-19 09:57:06 -05:00

Wren6991 (Migrated from github.com) left a comment

I think the functional changes as of f2fc82b are all correct, and fix some severe bugs.

The copy/pasted mutex implementations in the new sync tests should ideally be linked to the actual SDK implementations, and I think it's worth cleaning up the things I flagged in the tests that will confuse us when we read them later.

Other than that LGTM.

I think the functional changes as of f2fc82b are all correct, and fix some severe bugs. The copy/pasted mutex implementations in the new sync tests should ideally be linked to the actual SDK implementations, and I think it's worth cleaning up the things I flagged in the tests that will confuse us when we read them later. Other than that LGTM.

kilograham (Migrated from github.com) reviewed

2026-08-19 16:13:46 -05:00

test/hardware_sync_spin_lock_test/hardware_sync_spin_lock_test.c

					
				@ -190,6 +190,100 @@ static const test_t tests[] = {

					},

i think i'm gonna keep it because it is verifying a fact about the SDK define being set correctly, but i will fix the other issues

Sign in to join this conversation.

No reviewers

No labels

compiler version issues

needs reporter detail

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-raspberrypi/pico-sdk!3127

				`@ -13,6 +13,15 @@`
				`const absolute_time_t ABSOLUTE_TIME_INITIALIZED_VAR(nil_time, 0);`

Rows
Columns