py/scheduler: warning about C callbacks scheduling new tasks. #17248
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #17248 +/- ##
==========================================
- Coverage 98.54% 98.54% -0.01%
==========================================
Files 169 169
Lines 21890 21929 +39
==========================================
+ Hits 21572 21610 +38
- Misses 318 319 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Code size report: |
Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
…sks. Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
|
@dpgeorge I've updated this to add a warning comment to future developers and updated the unit test to show the behaviour. I don't believe there was previously any unit test coverage of the C scheduler so this is beneficial for that reason alone. |
af4a8e9 to
149927a
Compare
|
FWIW, I bumped into this problem a while back (maybe while working on USB) and came up with a patch to allow C scheduler callbacks to "chain" a call to another callback while still supporting normal execution. Then I think I turned out not to need the patch, as I don't seem to have ever commited it. From memory it was pretty simple, something like this (untested): diff --git i/py/scheduler.c w/py/scheduler.c
index 2170b9577e..969202f5ce 100644
--- i/py/scheduler.c
+++ w/py/scheduler.c
@@ -88,6 +88,7 @@ static inline void mp_sched_run_pending(void) {
#if MICROPY_SCHEDULER_STATIC_NODES
// Run all pending C callbacks.
+ mp_sched_node_t *original_tail = MP_STATE_VM(sched_tail);
while (MP_STATE_VM(sched_head) != NULL) {
mp_sched_node_t *node = MP_STATE_VM(sched_head);
MP_STATE_VM(sched_head) = node->next;
@@ -99,6 +100,9 @@ static inline void mp_sched_run_pending(void) {
MICROPY_END_ATOMIC_SECTION(atomic_state);
callback(node);
atomic_state = MICROPY_BEGIN_ATOMIC_SECTION();
+ if (node == original_tail) {
+ break; // Don't execute any callbacks scheduled during this run
+ }
}
#endif
If there isn't somewhere we rely on the current behaviour then maybe this is an alternate way forward? |
Summary
If a
mp_sched_schedule_nodebased C scheduler function re-schedules itself then themp_sched_run_pendingfunction can get stuck in an infinite loop re-running the function where ctrl-c cannot break out of it anymore.This change adds a unit test to display the behavior (in a self-limiting fashion) along with a minimal change to auto-break out ofmp_sched_run_pendingif the node is re-scheduled. It will still be run on next run ofmp_sched_run_pendingbut other aspects of the application / micropython will be able to run as well in this case.This MR adds a coverage unit test showing the behavior and adds a comment in the scheduler warning developers of this behavior. It should only apply to people working on C code within the micropython codebase.
See #17246
Testing
Tested on a STM32WB55 with manually broken rfcore, before this it was stuck in an infinite loop trying to init the radio.
Trade-offs and Alternatives
If there are two C scheduled functions that continually re-schedule each other this fix won't catch that, but I tried to keep the minimal fix as simple as possible as I believe the goal is to keep
mp_sched_run_pendingas fast as possible as its run in the interpreter core?