Skip to content

Conversation

arachsys
Copy link
Contributor

@arachsys arachsys commented Jun 29, 2025

Summary

7816b1f, 8f85eda and 8162468 (PR #17363) added a hard= option to the rp2 Timer 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 have mp_sched_schedule() enabled so I don't think I can support hard=False there? I've left it out altogether, assuming that extra code size isn't worth it just to add an ignored hard=True option for consistency. (I also note that nrf doesn't support the freq= option to the Timer constructor/init, just period=, 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:

  • every port with machine.Timer except nrf supports both hard=True and hard=False
  • every port with machine.Timer except nrf supports freq=N (N in Hz)
  • every port with machine.Timer except renesas-ra supports period=N (N in ms)
  • esp32 and esp8266 don't support software timers (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 varying freq= and explicitly checking that the heap is locked in hard callbacks and unlocked in soft callbacks. (The preexisting tests/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.

Copy link

codecov bot commented Jun 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (053aade) to head (9db08b4).
⚠️ Report is 2 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

github-actions bot commented Jun 29, 2025

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:  -124 -0.031% PYBV10
     mimxrt:   +72 +0.019% TEENSY40
        rp2:   -80 -0.009% RPI_PICO_W
       samd:   +68 +0.025% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@arachsys arachsys changed the title Hardtimer Various ports: add support for hard-IRQ or soft-IRQ Timer Jun 29, 2025
@arachsys arachsys force-pushed the hardtimer branch 5 times, most recently from 5e3cf0b to 2f2b833 Compare June 29, 2025 20:43
@arachsys arachsys marked this pull request as ready for review June 30, 2025 12:52
@arachsys arachsys changed the title Various ports: add support for hard-IRQ or soft-IRQ Timer Various ports: add support for Timer hard= option Jun 30, 2025
@arachsys
Copy link
Contributor Author

Rebased onto HEAD of master.

Copy link
Contributor

@andrewleech andrewleech left a 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!

Comment on lines 160 to 174
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();
}
Copy link
Contributor

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?

Copy link
Contributor Author

@arachsys arachsys Jul 20, 2025

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?)

Copy link
Contributor Author

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! :)

Copy link
Contributor

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.

Copy link
Contributor Author

@arachsys arachsys Jul 22, 2025

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...)

Copy link
Contributor

@andrewleech andrewleech left a 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!

@@ -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;
Copy link
Contributor

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...

Copy link
Contributor Author

@arachsys arachsys Jul 22, 2025

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@arachsys
Copy link
Contributor Author

The consolidation looks good to me!

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!

@arachsys
Copy link
Contributor Author

arachsys commented Aug 9, 2025

[Rebased to HEAD of master.]

@dpgeorge dpgeorge added this to the release-1.27.0 milestone Aug 16, 2025
@dpgeorge
Copy link
Member

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 extmod/machine_hard_timer.py test on ESP32 it indeed crashes running the hard=True part of the test, and the REPL locks up.

So, I think for esp32 just raise an exception if hard=True is passed in.

For esp8266, the tests/extmod/machine_hard_timer.py doesn't currently work because esp8266 doesn't support passing in arguments to the constructor. Instead it needs to do timer=Timer(0) then timer.init(...). Might be worth a separate PR to make that work the same as other ports?

I didn't check other boards yet.

@dpgeorge
Copy link
Member

I tested PYBV10 (stm32 board). This port doesn't support non-soft machine.Timer, ie the id must be -1 on this port, so machine_hard_timer.py test fails.

Also, machine_timer.py fails because the modes and kinds dicts evaluate to a different order than what they are defined in (eg it does periodic before one-shot). Probably needs to use a tuple for those to ensure consistent order across all ports.

print("SKIP")
raise SystemExit

if sys.platform in ("nrf"):
Copy link
Member

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",).

@arachsys
Copy link
Contributor Author

arachsys commented Aug 25, 2025

I tested PYBV10 (stm32 board). This port doesn't support non-soft machine.Timer, ie the id must be -1 on this port, so machine_hard_timer.py test fails.

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?

Also, machine_timer.py fails because the modes and kinds dicts evaluate to a different order than what they are defined in (eg it does periodic before one-shot). Probably needs to use a tuple for those to ensure consistent order across all ports.

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.

@arachsys
Copy link
Contributor Author

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 extmod/machine_hard_timer.py test on ESP32 it indeed crashes running the hard=True part of the test, and the REPL locks up.

So, I think for esp32 just raise an exception if hard=True is passed in.

Thanks, I'll do that. Getting the GIL will definitely be hard as it might already be held by the code that got interrupted!

For esp8266, the tests/extmod/machine_hard_timer.py doesn't currently work because esp8266 doesn't support passing in arguments to the constructor. Instead it needs to do timer=Timer(0) then timer.init(...). Might be worth a separate PR to make that work the same as other ports?

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.

@dpgeorge
Copy link
Member

Are there two versions of the pyboard, one of which supports hard timers and one of which does not?

They all support pyb.Timer which is the stm32 specific way to do hardware timers. For machine.Timer, only soft timer is supported by stm32 at the moment.

This is starting to dig up the main issue that the machine.Timer class is not well specified nor very consistent across ports, see #2971 for that discussion.

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 Timer class on various ports.

While I'm doing this, what do you think about the final 'consolidation' patch?

Yes, that's quite nice. Always good to factor code, eliminate duplication, and reduce code size! Eventually we might want to add Timer.irq (like eg Pin.irq and UART.irq) and then it would use the standard mpirq.c interface/code and not need any refactoring like that. But -- again in the interest of making progress -- your refactoring is good and I think we should do it. So feel free to expose mp_irq_dispatch() in an earlier commit and then use it immediately when updating each port.

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>
@arachsys
Copy link
Contributor Author

So feel free to expose mp_irq_dispatch() in an earlier commit and then use it immediately when updating each port.

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>
@dpgeorge
Copy link
Member

Sorry I wasn't able to test these properly before sending the original series. My hardware collection clearly needs to grow!

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants