rp2,docs: Stop machine.idle() blocking indefinitely, update docs. #15345
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Code size report: |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
5edcfaa to
4bc14ee
Compare
3079deb to
42d0b28
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15345 +/- ##
=======================================
Coverage 98.43% 98.43%
=======================================
Files 161 161
Lines 21275 21275
=======================================
Hits 20942 20942
Misses 333 333 ☔ View full report in Codecov by Sentry. |
|
This PR is ready now but depends on #15398 for correct behaviour. |
42d0b28 to
657fd48
Compare
A lot of existing code (i.e. micropython-lib lps22h, lcd160cr sensor drivers, lora sync_modem driver, usb-device-hid) calls machine.idle() inside a tight loop that is polling some condition. This reduces the power usage compared to constantly looping, but can be faster than calling a sleep function. However on a tickless port there's not always an interrupt before the condition they are polling for, so it's difficult to restructure this code if machine.idle() doesn't have any upper limit on execution time. This commit specifies an upper limit of 1ms before machine.idle() resumes execution. This is already the case for all ports except rp2. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
Updates rp2 port to always resume from idle within 1ms max. When rp2 port went tickless the behaviour of machine.idle() changed as there is no longer a tick interrupt to wake it up every millisecond. On a quiet system it would now block indefinitely. No other port does this. See parent commit for justification of why this change is useful. Also adds a test case that fails without this change. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
657fd48 to
ba98533
Compare
|
@dpgeorge Rebased to pick up the WFE fix, and split into two commits. I ended up making the new test rp2-only for now as there are several ports it won't pass on. |
|
Thanks! I tested it and the test works well (on rp2, and also stm32 just for my interest). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Updates rp2 port to always resume from idle within 1ms max.
When rp2 port went tickless the behaviour of machine.idle() changed as there is no longer a tick interrupt to wake it up every millisecond. On a quiet system without interrupts it can now block indefinitely. No other port does this.
A lot of existing code (i.e. micropython-lib lps22h, lcd160cr sensor drivers, lora sync_modem driver, usb-device-hid) calls
machine.idle()inside a tight loop that is polling some condition. This reduces the power usage compared to constantly looping, but can be faster than calling a sleep function. However there's not always an interrupt before the condition they are polling for, so it's difficult to restructure this code ifmachine.idle()doesn't have any upper limit on execution time.Also changes the machine module documentation to explain the new behaviour and recommended usage. I believe all ports will pause for at most 1ms now, and suggest this is good behaviour to stick with.
This fix depends on #15398 for correct behaviour, and that PR should be merged before this.
Testing
Tested on rp2 PICO and PICO_W:
machine.idle()wakeup time. This test fails on master (30ms average wakeup time on PICO-W, never completes on PICO). The test also fails in this branch without the linked PR (average wakeup time of 0.02ms). Passes after this branch is rebased with the WFE fix.while True: machine.idle()and verified power consumption was approximately the same as tunningtime.sleep(10). This is also depends on the linked PR with the WFE fix.Trade-offs and Alternatives
machine.idle(sleep_ms=1)argument but it's hard to come up with a use case to justify the extra complexity. This isn't a sleep function so an interrupt can resume execution at any time, and looping with 1ms periods has basically the same power consumption as looping with 1000ms idle periods.This work was funded through GitHub Sponsors.