Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.2k
rp2/modmachine: Fix lightsleep returning early.#16412
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fixes#16181Commit74fb42a added code using hardware timer 2.Prior to that only lightsleep used hardware timersspecifically timer 3.lightsleep now loops calling __wfi() until timer 3 has fired.Without the loop lightsleep would return very early.Tested on PICO W with code from the issue.Works fine.Signed-off-by: Carl Pottle <cpottle9@outlook.com>
github-actionsbot commentedDec 14, 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: |
| // A timer other than #3 could fire. | ||
| // Repeatedly go into low-power mode until timer 3 is no longer armed. | ||
| while (timer_hw->armed & (1u << alarm_num)) { | ||
| __wfi(); |
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.
I'm pretty sure there's a race condition in here: between the check of the timer and the wfi the interrupt may go off, in which case the wfi will sit there waiting "forever" because the interrupt already fired.
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.
Agreed.
Needs to be fixed, but it won't wait forever. Timer 2 is going off about every 100 ms which will cause the __wfi() to return.
@dpgeorge, it is not obvious to me how to resolve the race condition.
I still a relative neophyte on ARM and micropython internals.
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.
@peterharperuk tried to fix this before but there's not really any way to do it.
A proper fix needs the pico-sdk update (which fixes issues with the alarm pool and wfe), but the latest version of that has a new alarm pool bug...
peterharperukDec 16, 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.
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.
We've fixed the latest issue and have a tentative plan to do a small sdk update in the new year.
Also, this change would prevent wakeup for USB interrupts?
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.
@peterharperuk, it does not prevent wakeup for USB interrupts it just delays it. Potentially a lot.
Without my change the single __wfi() wakes up every 60 to 100 milliseconds due to the change in74fb42a which added code using hardware timer 2.
USB interrupts will be masked for that time.
With the while loop I added USB interrupts will be masked for the entire delay time.
But they will get serviced eventually.
I did notice while testing that if I did a lightsleep(10000) and hit the thonny stop button the prompt would not come back until the sleep expired.
Longer delays like lightsleep(30000) and thonny would timeout if I hit the stop button.
But once the lightsleep completed (after 30 seconds) hitting the stop button again would bring it back.
I suggest to you that someone calling lightsleep() for a long time (EG: several minutes) needs to be aware thonny will timeout.
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.
@projectgus has a pull request open for the same issue.
Likely I will close this pull request without a merge and his fix will go in.
It is end of day were I live. I will get back to this tomorrow.
fixes#16181@dpgeorge noted a race condition in my initial commit forthis pull request.This commit masks the race condition by changing the conditionin the while() loop.Now the while loop continues as long as timer 3 is armed and thereare at least 50 microseconds left to run.50 microseconds is a magic number I chose.It could be smaller.It is more than long enough to ensure timer 3 interrupt won'toccur before __wfi() is called.And it is small enough someone calling lightsleep() won'tnotice it returning early.I tested using a modified version of test/ports/rp2/rp2_lightsleep.py.In my modified version of the test lightsleep was calledfor 1, 2, 4, 8, 16, ..., 4096, 8192 milliseconds.Using time.ticks_us() I measured the time to execute lightsleep().Pretty consistently it was 800-900 microseconds longer than requested.I suspect this is one or more interrupts being serviced when lightsleepre-enable interrupts before returning.I did my testing on a PICO W connected to a PI 4 running thonny.Signed-off-by: Carl Pottle <cpottle9@outlook.com>
I tested@projectgus changes in#16431. |
Uh oh!
There was an error while loading.Please reload this page.
fixes#16181
Commit74fb42a added code using hardware timer 2.
Prior to that only lightsleep used hardware timers specifically timer 3.
It turned off almost all hardware blocks except hardware timers and waits for an interrupt.
It did not check if the interrupt is from hardware timers or if it is from timer 3.
With this change, lightsleep now loops calling __wfi() until timer 3 has fired. Without the loop lightsleep would return very early.
Ideally this pull request should be reviewed by user@projectgus who made the changes in commit74fb42a and has also made changes to modmachine.c lightsleep.
Testing
Tested on PICO W using thonny with code from the issue. Works fine.