rp2/modmachine: Fix lightsleep returning early. #16412
Conversation
fixes #16181 Commit 74fb42a added code using hardware timer 2. Prior to that only lightsleep used hardware timers specifically 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>
|
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.
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.
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.
@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...
There was a problem hiding this comment.
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.
@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 in 74fb42a 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.
@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 for this pull request. This commit masks the race condition by changing the condition in the while() loop. Now the while loop continues as long as timer 3 is armed and there are 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't occur before __wfi() is called. And it is small enough someone calling lightsleep() won't notice 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 called for 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 lightsleep re-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. |
fixes #16181
Commit 74fb42a 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 commit 74fb42a and has also made changes to modmachine.c lightsleep.
Testing
Tested on PICO W using thonny with code from the issue. Works fine.