-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
Various ports: add support for Timer hard= option #17580
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17580 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 171 171
Lines 22296 22296
=======================================
Hits 21937 21937
Misses 359 359 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code size report:
|
5e3cf0b
to
2f2b833
Compare
Rebased onto HEAD of master. |
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.
Great work broadening the timer features and port consistency here!
ports/esp32/machine_timer.c
Outdated
if (self->ishard) { | ||
// When executing code within a handler we must lock the scheduler to | ||
// prevent any scheduled callbacks from running, and lock the GC to | ||
// prevent any memory allocations. | ||
mp_sched_lock(); | ||
gc_lock(); | ||
nlr_buf_t nlr; | ||
if (nlr_push(&nlr) == 0) { | ||
mp_call_function_1(self->callback, MP_OBJ_FROM_PTR(self)); | ||
nlr_pop(); | ||
} else { | ||
// Uncaught exception; disable the callback so it doesn't run again. | ||
self->repeat = 0; | ||
mp_printf(MICROPY_ERROR_PRINTER, "uncaught exception in timer callback\n"); | ||
mp_obj_print_exception(MICROPY_ERROR_PRINTER, MP_OBJ_FROM_PTR(nlr.ret_val)); | ||
} | ||
gc_unlock(); | ||
mp_sched_unlock(); | ||
} else { | ||
mp_sched_schedule(self->callback, MP_OBJ_FROM_PTR(self)); | ||
mp_hal_wake_main_task_from_isr(); | ||
} |
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.
Perhaps this common snippet could be consolidated into the extmod machine timer C file for reuse by all the ports.
Though arguably it could be used by any hardware peripheral interrupts so perhaps it should be somewhere even more common?
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 originally hoped to consolidate all these stanzas into a single place, ideally merging into the similar IRQ object too, and I have provided a shared implementation of this in the patch to extmod/machine_timer.c
.
However, many ports roll their own and don't use extmod/machine_timer.c
at all, and there are a lot of incidental differences between these ports. For example, in the esp32 patch you quote above, we have to mp_hal_wake_main_task_from_isr()
after mp_sched_schedule()
, unlike any other port. Some ports (renesas-ra, stm32) have the notion of channels which change the shape of the error messages. Different ports have different structures for the timer object with different ways to disable callbacks on uncaught exceptions.
A lot of these incidental differences could be smoothed out, e.g. using callback = NULL
as the disable technique everywhere with appropriate modifications to the callers. But I confess that without testing on real hardware, I chickened out and kept the code in existing ports as similar as I could in this first step, planning a clean-up-and-unify patch as a follow-up!
I expect it would be possible to pull out something sufficiently generalised like
if (hard) {
mp_sched_lock();
gc_lock();
nlr_buf_t nlr;
if (nlr_push(&nlr) == 0) {
mp_call_function_1(callback, MP_OBJ_FROM_PTR(argument));
nlr_pop();
} else {
/* caller has to disable and print exception */
return nlr.ret_val;
}
gc_unlock();
mp_sched_unlock();
} else {
mp_sched_schedule(callback, MP_OBJ_FROM_PTR(argument));
}
return NULL;
and then use it as
ret_val = dispatch_callback(self->callback, self);
if (ret_val) {
self->repeat = 0;
mp_printf(MICROPY_ERROR_PRINTER, "uncaught exception in timer callback\n");
mp_obj_print_exception(MICROPY_ERROR_PRINTER, MP_OBJ_FROM_PTR(ret_val));
}
if (!self->hard) {
/* special case that isn't handled by dispatch_callback for esp32 */
mp_hal_wake_main_task_from_isr()
}
or something like that. Perhaps pass the "timer callback" string from the error message as an argument so the exception handling can all live in dispatch_callback too. If that covered the IRQ case as well as the Timer case for each port, it might give us a slight code shrinkage too, albeit slightly slower at interrupt time.
(I think I'm assuming that callbacks can't raise a VM abort in the above, or at least ignoring the exception if they do, by assuming nlr.ret_val
is non-NULL in an exception return. Possibly we're doing that already by calling mp_obj_print_exception with them?)
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.
If we're being really brave about eliminating repetition here: where ports roll their own soft-timers instead of using the standard implementation (esp32, rp2, etc)... should they? Or, once hard=True and hard=False are properly supported on the shared extmod/machine_timer.c
implementation, should everything 'custom' be replaced by that? Probably a bit controversial for this PR though! :)
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.
Yeah it's always hard to know how far to take a change like this!
FWIW most ports already have a stub mp_hal_wake_main_task_from_isr
for exactly this kind of consolidation.
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.
Okay, you've emboldened me! I've pushed an additional patch as a suggestion for how we might consolidate this, factoring out mp_irq_dispatch() as a generalised version of mp_irq_handler() and then using it throughout the timer implementations. Hopefully this should give a small size reduction on quite a few ports. What do you think?
I see esp8266 doesn't link against mpirq.c at all, so I've not tried to consolidate that one for now. (But mpirq.c is quite minimal apart from mp_irq_handler(), so maybe the penalty for linking it would be negligible...)
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 consolidation looks good to me!
ports/esp32/machine_timer.c
Outdated
@@ -238,6 +246,7 @@ static mp_obj_t machine_timer_init_helper(machine_timer_obj_t *self, mp_uint_t n | |||
self->repeat = args[ARG_mode].u_int; | |||
self->handler = machine_timer_isr_handler; | |||
self->callback = args[ARG_callback].u_obj; | |||
self->ishard = args[ARG_hard].u_bool; |
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.
It might be just me but I read ishard
as "I shard", personally I'd go for is_hard
or just hard
. Minor detail though...
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 not a great fan of 'isfoo' naming either, but the ishard in the timer structs is copying the existing ishard bool in the irq structs. I've tried to be as consistent with the existing code as possible, especially any conventions in the 'core' parts of micropython.
mp_sched_schedule(entry->py_callback, MP_OBJ_FROM_PTR(entry)); | ||
if (mp_irq_dispatch(entry->py_callback, MP_OBJ_FROM_PTR(entry), | ||
entry->flags & SOFT_TIMER_FLAG_HARD_CALLBACK)) { | ||
// Uncaught exception; disable the callback so it doesn't run again. |
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.
Looks like the callback disable is missing now?
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.
It's the line below that comment where I set entry->mode to SOFT_TIMER_MODE_ONE_SHOT.
@@ -30,6 +30,7 @@ | |||
|
|||
#define SOFT_TIMER_FLAG_PY_CALLBACK (1) | |||
#define SOFT_TIMER_FLAG_GC_ALLOCATED (2) | |||
#define SOFT_TIMER_FLAG_HARD_CALLBACK (4) |
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.
Was 4 used here for a reason? Is 3 used elsewhere?
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.
These are bit masks that get ORed together, so powers of two.
Yes, I like it too to be honest. The size reduction was better than I expected. Thanks for encouraging me not to be too timid! |
[Rebased to HEAD of master.] |
Thanks for the contribution! Always good to clean up things and consolidate across ports. Unfortunately esp32 cannot support hardware interrupts, because the hardware interrupt does not run on the main thread. The interrupt handler needs to acquire the GIL which is non-trivial. Testing the So, I think for esp32 just raise an exception if For esp8266, the I didn't check other boards yet. |
I tested PYBV10 (stm32 board). This port doesn't support non-soft Also, |
tests/extmod/machine_timer.py
Outdated
print("SKIP") | ||
raise SystemExit | ||
|
||
if sys.platform in ("nrf"): |
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.
This won't work, it should be sys.plaftorm in ("nrf",)
.
Ah, so my check for 'pyboard' in the hard timer test is not the right thing at all. Are there two versions of the pyboard, one of which supports hard timers and one of which does not? (pybv10 and pybv11 maybe?) Or is the situation more complicated than that?
Oops, that's embarrassing! I'll fix. While I'm doing this, what do you think about the final 'consolidation' patch? It reduces code size a bit and cleans things up, but I wasn't sure what you'd make of splitting mp_irq_dispatch() like that. If you're happy with it, I could tap into mp_irq_dispatch() at the start of the series and use it straight away in the patches that extend each port. That would reduce diff noise and make things a bit clearer, while still leaving things in exactly the same state at the end of the series? I didn't do that when I added it in case you wanted the rest of the changes but didn't like the last one. |
Thanks, I'll do that. Getting the GIL will definitely be hard as it might already be held by the code that got interrupted!
Sounds like a good plan! I'll get the various issues with this one straightened out (including using timer.init() in this test) and then open that and link it. |
They all support This is starting to dig up the main issue that the Ideally we'd address #2971 first, and then work on implementing it. But --for the sake of making progress -- this PR is good. It just might end up being a bit imperfect due to the incomplete nature of the
Yes, that's quite nice. Always good to factor code, eliminate duplication, and reduce code size! Eventually we might want to add |
Separate out a routine to call an arbitrary function with arbitrary argument either directly as a hard-IRQ handler or scheduled as a soft-IRQ handler, adjusting mp_irq_handler() to wrap this. This can then be used to implement other hard/soft callbacks, such as for machine.Timer. Signed-off-by: Chris Webb <chris@arachsys.com>
Add a flag SOFT_TIMER_HARD_CALLBACK to request that a soft timer's python callback is run directly from the IRQ handler with the scheduler and heap locked, instead of being scheduled via mp_sched_schedule(). Signed-off-by: Chris Webb <chris@arachsys.com>
machine.Timer() has inconsistent behaviour between ports: some run callbacks in hard IRQ context whereas others schedule them like soft IRQs. As on the rp2 port, add support to the generic software timer for a hard= argument to explicitly choose between these, setting the default to False to match the existing behaviour. This enables hard timer callbacks for the alif, mimxrt and samd ports. Signed-off-by: Chris Webb <chris@arachsys.com>
Now that mp_irq_dispatch() is available to dispatch arbitary hard/soft callbacks, take advantage of this for rp2 machine.Timer. This should slightly reduce binary size. Signed-off-by: Chris Webb <chris@arachsys.com>
machine.Timer() has inconsistent behaviour between ports: some run callbacks in hard IRQ context whereas others schedule them like soft IRQs. As on the rp2 port, add support to the renesas-ra port for a hard= argument to explicitly choose between these, setting the default to True to match the existing behaviour. Signed-off-by: Chris Webb <chris@arachsys.com>
machine.Timer() has inconsistent behaviour between ports: some run callbacks in hard IRQ context whereas others schedule them like soft IRQs. As on the rp2 port, add support to the stm32 port for a hard= argument to explicitly choose between these, setting the default to True to match the existing behaviour. Signed-off-by: Chris Webb <chris@arachsys.com>
machine.Timer() has inconsistent behaviour between ports: some run callbacks in hard IRQ context whereas others schedule them like soft IRQs. As on the rp2 port, add support to the zephyr port for a hard= argument to explicitly choose between these, setting the default to False to match the existing behaviour. Signed-off-by: Chris Webb <chris@arachsys.com>
machine.Timer() has inconsistent behaviour between ports: some run callbacks in hard IRQ context whereas others schedule them like soft IRQs. As on the rp2 port, add support to the esp8266 port for a hard= argument to explicitly choose between these, setting the default to False to match the existing behaviour. Open-code this as we don't link against mpirq.c so can't use mp_irq_dispatch(). Signed-off-by: Chris Webb <chris@arachsys.com>
I've restructured the commits, fixed the various incompatibilities you found, and have updated the patch series. Sorry I wasn't able to test these properly before sending the original series. My hardware collection clearly needs to grow! |
machine.Timer() has inconsistent behaviour between ports: some run callbacks in hard IRQ context whereas others schedule them like soft IRQs. Most ports now support a hard= argument to the machine.Timer constructor or initialiser to explicitly choose between these behaviours. However, esp32 does not support hardware interrupts because they are not delivered to the main thread, so the interrupt handler would need to acquire the GIL. Raise a ValueError if hard=True is requested for esp32 machine.Timer. Signed-off-by: Chris Webb <chris@arachsys.com>
Now all ports with machine.Timer except nrf support both hard and soft callbacks, generalise tests/ports/rp2_machine_timer.py into tests/extmod/machine_timer.py. There is an existing machine_soft_timer.py which varies period= and covers the nrf port but skips esp32/esp8266 because they don't support software timers. In our new test, we try varying freq= instead of period=, and cover esp32/esp8266 (with a fixed choice of hardware timer) but skip nrf because it doesn't support hard= or freq=. Add a check that the heap is locked (so allocation fails) in hard callbacks and it is unlocked (so allocation succeeds) in soft callbacks, to ensure we're getting the right kind of callback, not falling back to the default. Signed-off-by: Chris Webb <chris@arachsys.com>
On platforms where hardware timers are available, test these in each combination of hard/soft and one-shot/periodic in the same way as for software timers. Where a platform supports both software (id = -1) and hardware (id >= 0) timers, the behaviour of both is now checked. For now, esp8266 is the only platform that supports hardware timers and both hard and soft callbacks. Signed-off-by: Chris Webb <chris@arachsys.com>
Update the main machine.Timer specification, and any references to hard/soft interrupts in port-specific documentation. There is a separate copy of the machine.Timer documentation for the pyboard, so update that too to keep everything consistent. Signed-off-by: Chris Webb <chris@arachsys.com>
Not a problem. There are many hardware combinations that need testing, and that's why we are trying to get Octoprobe up and running, for automated hardware testing. I've started an Octoprobe run for this PR, to see how it goes. In maybe 5 hours (yes it takes that long!) it'll appear at the top of the list here: https://reports.octoprobe.org/ (under the "275" on the left there will eventually be a "success" link that'll take you to the report). |
Summary
7816b1f, 8f85eda and 8162468 (PR #17363) added a
hard=
option to the rp2Timer
constructor and initialiser to allow the callback to be switched between hard-IRQ and soft-IRQ mode.The behaviour of other ports varies: either hard-IRQ only or soft-IRQ only. @dpgeorge suggested that it would be good to make this consistently configurable across all ports, preserving the current default but allowing both behaviours everywhere.
This patch series is an attempt to do that, adding support to every port that has a
machine.Timer
except nrf (see below), ensuring there is cross-port test coverage of both variants, and documenting as appropriate.Notes and issues
nrf is the only port with a
machine.Timer
that isn't covered by this patch series. This port doesn't havemp_sched_schedule()
enabled so I don't think I can supporthard=False
there? I've left it out altogether, assuming that extra code size isn't worth it just to add an ignoredhard=True
option for consistency. (I also note that nrf doesn't support thefreq=
option to theTimer
constructor/init, justperiod=
, so it's already a cut-back timer implementation compared to other ports.)I've added hard IRQ support to
shared/runtime/softtimer
, but have gated it to avoid bloat on platforms which have their own timer infrastructure and don't need hard IRQ support from the software timer. alif, mimxrt and samd are the three ports which turn this on.Testing
Although I have compile tested every patch and every port via the Micropython test infrastructure, I don't have hardware to test ports other than rp2. The generalised test I have written in
tests/extmod/machine_timer.py
would exercise old and new codepaths on every modified platform if run there.I would greatly appreciate help in verifying that this series does indeed work correctly on every port it claims to cover! Obviously I will happily fix any issues that do show up.
For the purposes of writing tests:
machine.Timer
except nrf supports bothhard=True
andhard=False
machine.Timer
except nrf supportsfreq=N
(N
in Hz)machine.Timer
except renesas-ra supportsperiod=N
(N
in ms)id = -1
)To try to cover the various cases a bit more thoroughly, my new test covers all ports with a
machine.Timer
including esp32/esp8266 but excluding nrf, exercising hard and soft timers with varyingfreq=
and explicitly checking that the heap is locked in hard callbacks and unlocked in soft callbacks. (The preexistingtests/extmod/machine_soft_timer.py
already covers all the functionality available on nrf, and tests period= instead of freq=.)Additional consolidation
As suggested by @andrewleech, I've added an additional (somewhat bolder!) patch to show what consolidating the hard/soft callback from mp_irq_handler() with the various timer implementations would look like. The test suite suggests this gives small binary size improvements on ports like stm32 and rp2 as well as having a pleasingly negative diffstat.