Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.2k
Alarm pool sleep changes#16454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Alarm pool sleep changes#16454
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedDec 19, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Code size report:
|
codecovbot commentedDec 19, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## master #16454 +/- ##======================================= Coverage 98.54% 98.54% ======================================= Files 169 169 Lines 21896 21896 ======================================= Hits 21578 21578 Misses 318 318 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Related PR#16431 |
Uh oh!
There was an error while loading.Please reload this page.
Quick update for anyone wondering where all this has got to: This is the preferred fix for the rp2 lightsleep issues, but we can't merge it untilraspberrypi/pico-sdk#2127 makes it into a pico-sdk release. |
ssotheremail commentedJan 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for the update projectgus. Any thoughts at all on when? I notice: |
We've updated to pico-sdk v2.1.1 in#16783, which includes the fix we're waiting on for the alarm pool. @peterharperuk are you able to come back to this PR, or is it better if one of us picks it up from here? |
Sorry, yes. I'll try and take a look this week. |
@peterharperuk All good, we only just updated anyhow. Please let us know how you get on. |
ssotheremail commentedMar 17, 2025
Thanks for all the work on this. Sorry if I am being dim but I am slightly confused because earlier on (above) it says "waiting for pico sdk 2127" but thats still milestoned for 2.2.0. Obviously it would be great if lightsleep is fixed before that. In case its useful as a baseline I reran my test program with 1.25.0 preview 389 and got 20 seconds for pico (correct), 15 seconds for pico2 (lightsleep after sleep returns early), 5 seconds picow and pico2w (lightsleep always returns early). |
Peter will know more than I do about this, but even though the milestone is set to 2.2.0 in their tracker the fix commit looks like it was included in the 2.1.1 tag:raspberrypi/pico-sdk@969f589 (The list of tags is on the line under the commit title in the GitHub UI.) |
bd5dbf0
to05b02b8
Compare05b02b8
tob9cfba5
Compare
projectgus left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks@peterharperuk for submitting this. The changes look good to me, and I think we should merge this.
I've tested the USB power consumption of both a PICO W and a PICO 2W with and without this patch, and it does fix the lightsleep and idle issues with RP2350 from what I can see.
Board | Version | Idle at REPL | Busy | time.sleep() | machine.lightsleep in a loop |
---|---|---|---|---|---|
RPI_PICO_W | master | 18mA | 18mA | 18mA | 3mA |
this PR | 18mA | 20mA | 18mA | 3mA | |
RPI_PICO2_W | master | 16mA | 16mA | 16mA | 3mA (wakeup issue) |
this PR | 16mA | 16mA | 13mA | 3mA |
However, I think there's probably still some follow-up improvements we can make:
- The issue with lwIP wakeups limiting lightsleep time remains,details here. I think the fix for this will be to disable lwIP's timers when there are no active network interfaces. Will pick this up on the relevant issuelightsleep on Pico W regression for 1.24.0 #16181.
- The consumption when idle at a REPL vs running Python code in a loop vs
time.sleep()
still seem a bit close to me, surely idle should be a little lower. However I haven't dug into what's happening there.
I also cherry-picked the unit test I added for lightsleep on CPU1 and pushed ithere. It passes. I think could either add it to this PR, or I'll submit it as a follow-up.
ssotheremail commentedApr 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Please forgive me if I am wasting your time because of my lack of knowledge of github processes but I would be grateful if someone could answer a few questions: On my copy of micropython I have done a "git pull" today, compiled micropython (I struggled slightly with picotool but fixed that) and I get a 1.25.0 preview 442 for the 4 types of pico. On the download page the newest version for everything I have looked at is for example:https://micropython.org/resources/firmware/RPI_PICO2-20250327-v1.25.0-preview.428.g50da085d9.uf2 which is dated 27/03/25. I notice that#16454 is milestoned for 1.26.0 (undated) and that 1.25.0 has progressed very rapidly since I last looked (although described as overdue by 3 months) but it doesn't seem to have 16454 included as far as I can tell. For info: On the pico_w and pico2_w Q: is this the behaviour I should expect with the 442 build? Q: do any of the lightsleep PRs currently being considered fix the "lightsleep preceded by time.sleep" issue? I have just had a thought when writing this and will test it tomorrow. Say I want: would the lightsleep(5000) work properly if I did: If there is a known short delay it could be subtracted form 5000 to give the correct overall delay Good news, the extra lightsleep trick works! I put an extra lightsleep(5000) in and it came back from from that very quickly and the second lightsleep(5000) worked. I can recalibrate the timing and get on with the projects I have now that I have a workaround for both bugs. I feel a bit silly I didnt think of that before! |
ssotheremail commentedApr 7, 2025
I notice: https://micropython.org/resources/firmware/RPI_PICO2-20250407-v1.25.0-preview.447.g9e9be83fd.uf2 has now appeared, I will test again tomorrow.. |
This nightly build is available, but the nightly builds only contain changes which have been merged to the "master" branch. While a PR is in the Open state, this hasn't happened yet so these changes are not included in that build. If you want to test these changes then you have to check out this exact PR's code and compile with it. There are multiple ways to do this, including GitHub Desktop (I believe). The way I do it is to install the GitHub CLI tool (separate to git) and use thegh pr checkout command. |
ssotheremail commentedApr 8, 2025
Thanks for your time, clarification, and the info on the gh command, projectgus. I will try that later. That leaves me with 2 remaining questions from earlier:
|
ssotheremail commentedApr 10, 2025
Projectgus, |
Timers firing in LwIP? |
ssotheremail commentedApr 10, 2025
Probably the Firing in LwIP thing, if I have understood properly. if there is a PR at a state that's worth testing I am willing to give it a go. |
No PR for this yet, follow issue#16181 for that one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks@peterharperuk for the work on this PR,@projectgus for testing, and others for comments and discussion.
I confirm that all the known pico-sdk bugs for alarm pool have been fixed and merged to pico-sdk 2.1.1, which we currently use here in MicroPython.
I also extensively tested this PR on RPI_PICO, RPI_PICO2 in RISCV mode and RPI_PICO2_W. All the tests pass, there are no regressions.
Power consumption at the REPL, doingtime.sleep()
andmachine.lightsleep()
also look good, on RP2350time.sleep()
now reduces power consumption by about 3mA (and as noted above I see that there's probably room for improvement on RP2350 to make the idle REPL use less power as well).
Would be good to add this test to this PR. Angus will look into that. |
Pushed! |
@projectgus I tested your new test on a RPI_PICO2_W, and it fails. The third test fails:
Can you reproduce that failure? Maybe we can just merge this PR without your test, and investigate your test failure as a follow up? |
projectgus commentedMay 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I can't, I just ran the test 15 times in a loop on RPI_PICO2_W and it passes every time. That's with the commit in this branch (a0880ec), I haven't rebased it against master. Any chance your board might have a boot.py or something on it that's causing some subtle change? |
The plot thickens:
|
ssotheremail commentedMay 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
My initial comment below was flawed by a code typo. I have removed the affected error report. FWIW I have been using the "safe_sleep" code below to work around the "lightsleep returns early" problem on Pico W and 2w, when I test with: Iin cases its news, the "Lightsleep returns early" still happens on Pico2 W (haven't tried Pico W). Safe_sleep(5000)
|
madozu commentedMay 2, 2025
@ssotheremail: Replace the line |
ssotheremail commentedMay 2, 2025
idiot me, I was in a rush and did a global edit in another file replacing lightsleep with safe_sleep, then cut and pasted the def safe_sleep here.... Oh dear. I will retest tomorrow. |
projectgus commentedMay 8, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
592ede7
tofd2f3cf
Compareprojectgus commentedMay 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
EDIT: Important context: when looking at the test failure I realised it was only the After discussing with@dpgeorge I was going to check if the hanging test case is a regression from master. Turns out it is, the test doesn't get correct timing on master but it doesn't hang indefinitely. I've pushed another commit here to enforce mutual exclusion for calling into This allowed cleaning up the test so it now passes on |
Stop using soft timer for `mp_wfe_or_timeout`. Now uses the alarm poolagain as issues with this code have been fixed. This resolves the "sev"issue that stops the RP2350 going idle.Also, change the lightsleep code to use the hardware timer library andalarm 1, as alarm 2 is used by and soft timers and alarm 3 is used by thealarm pool.Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
Add some debug code that can be enabled to determine why lightsleep isreturning early.Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
This reverts commitb42bb91.Signed-off-by: Peter Harper <peter.harper@raspberrypi.com>
Not currently passing.This work was funded through GitHub Sponsors.Signed-off-by: Angus Gratton <angus@redyak.com.au>
There's no specified behaviour for what should happen if both CPUs call`lightsleep()` together, but the latest changes could cause a permanenthang due to a race in the timer cleanup code. Add a flag to prevent hangsif two threads accidentally lightsleep, at least.This allows the new lightsleep test to pass on RPI_PICO and RPI_PICO2, andeven have much tighter time deltas. However, the test still fails onwireless boards where the lwIP tick wakes them up too frequently.This work was funded through GitHub Sponsors.Signed-off-by: Angus Gratton <angus@redyak.com.au>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The updated test and mutual exclusion formachine.lightsleep()
look good!
Tested on RPI_PICO, RPI_PICO2 and RPI_PICO2 in RISCV mode. Everything seems to work.
fd2f3cf
to69993da
Compare69993da
intomicropython:masterUh oh!
There was an error while loading.Please reload this page.
rp2350 generates a sev when using spin locks, which can prevent the device sleeping, seeraspberrypi/pico-sdk#1812 for more background.
The pico-sdk alarm pool handles this.
However Micropython moved away from using the alarm pool due to issues like this...
Fixed in sdk 2.0.0:raspberrypi/pico-sdk#1552
Fixed in sdk 2.1.0:raspberrypi/pico-sdk#1953
Fixed in develop:raspberrypi/pico-sdk#2127
This change puts back the use of the alarm pool in mp_wfe_or_timeout to hopefully fix sleep issues.