esp32/machine_timer.*: Adding support for Timer(-1) soft/virtual timers.

[LucienMP]

Summary

Soft timers using Timer(-1) has been missing, but appeared to work due to type checking not being right on the ESP32 platforms.
The timers are limited to system hardware timers that are, at most, limited to 4. However there are times when more timers would be very useful so I added soft timers.

See the discussion here: https://github.com/orgs/micropython/discussions/18056

before 1.26 AFAICS the Timer( id, ... ) would take the id and wouldn't check what format the id was assuming it was an integer from 0..MAX in the type expected as GROUP:INDEX. So when specifying -1 this would erroneously select a hardware timer, but the user of "machine.Timer" would have expected, and assumed almost limitless amount of, soft/virtual timers. ESP32 is limited to a max of 4 timers, but depends on the hardware. There would be collision between timers if you allocated more.

Starting in 1.26 there is a check for the value being 0..MAX, and so -1 is now correctly rejected.

I add this functionality for soft timers, the -1 flag, to the ESP32 library.

Starting in 1.27 it seems the Timer(nn,...,hard=...) flag is also now supported.

Testing

I tested on Spotpear ESP32 board for which I am running LVGL + MicroPython 1.26 and wrote this extension for.
See: https://github.com/Spotpear-Scratch/board_firmware on the v1.26 branch

I tested manually thru console with the following set of conditions:

1. creating timer
2. creating timer, then initializing it
3. creating timer, initialize to period, then de-initializing it
4. creating timer, initialize to period, then checking its value/printing
5. creating timer, initialize to period, with repeating
6. creating timer, initialize to frequency
7. creating timers with invalid timing
8. creating timer, try to access value

Trade-offs and Alternatives

This is an expansion to existing machine.Timer so code size will increase by only a small amount (few functions, and a few additional if statements plus memory structure change for timer), it expands the number of timers available from the less than 4 hardware timers, adding an additional soft/virtual set up to as many as the user has memory from albeit at a lower resolution.

[LucienMP]

Updated main comment to answer other questions, and improve readability of reasons for this enhancement

[dpgeorge]

Did you try to run the existing tests tests/extmod/machine_timer.py and tests/extmod/machine_soft_timer.py? It would be great if one or both of those could pass with this PR.

[LucienMP]

Did you try to run the existing tests tests/extmod/machine_timer.py and tests/extmod/machine_soft_timer.py? It would be great if one or both of those could pass with this PR.

Q#1: Did I run the tests... ?

The tests werent there in 1.26, but I see them in 1.27 and i have run them with some caveats

1. I had to remove the in 'esp32' otherwise it gets skipped.
2. the machine_timer runs the soft timers ok
3. the machine_timer runs the hard timers and fails because i dont support hard=True mode
4. machine_soft_timer didnt run out of box because of id=-1 being missing, i fudged it and it worked
5. machine_hard_timer also works for all but HARD=True

Q#2: The machine_soft_timer wont pass as is, but can run the timer code if -1 is passed as id

They sort of pass but there is a little bit of confusion from my part. Is doing the following legal;

t = machine.Timer(freq=1)

The documentation to the way I read it as machine.Timer(id, ... ) where the ... are optional BUT id is a must and must be argument 0. It maybe -1 if soft timers are supported, otherwise a positive integer but it must always be there. Zephyr does it this way, so i took that as a sign.

Documentation here: https://docs.micropython.org/en/latest/library/machine.Timer.html

This will pass just ok

t = machine.Timer(-1,freq=1)
t.deinit()

Q#3: Documentation says hard=True, but I default to hard=False since i dont support hard=True types, is it ok?

Maybe default hard=False is a better default as most python coders aren't trying to do extremely precise timing and the greater flexibility afforded would be better for the average lower skill programmer? (over hard=True where it maybe confusing why allocation, or other issues are happening to them?)

Virtual/Soft Timers

In FreeRTOS soft timers from the OS dont run in the ISR afaik but rather in some task of their own so the callback would never be able to run directly, however there is the concept of scheduled, and called directly. ( I am using xTimerCreate ).

Hardware Timers

I assume all I would have to do for the hardware timers being hard is to call/not-call mp_sched_schedule, and directly call the callback based on hard=...

Below is the current code, failry simple switch between calling sched or not.

static void machine_timer_isr(void *self_in) {
    machine_timer_obj_t *self = self_in;

    uint32_t intr_status = timer_ll_get_intr_status(self->hal_context.dev);

    if (intr_status & TIMER_LL_EVENT_ALARM(self->index)) {
        timer_ll_clear_intr_status(self->hal_context.dev, TIMER_LL_EVENT_ALARM(self->index));
        if (self->repeat) {
            timer_ll_enable_alarm(self->hal_context.dev, self->index, true);
        }
        self->handler(self);
    }
}

static void machine_timer_isr_handler(machine_timer_obj_t *self) {
    mp_sched_schedule(self->callback, self);  // <--- warp this and do/dont call sched.
    mp_hal_wake_main_task_from_isr();
}

If thats the case then I can put that into this PR pretty easy.

Summary

In summary overall apart from my putting hard=False as the default if unspecified, not supporting hard virtual timers and the quandry about allowing machine.Timer( freq=1 ) then it all would pass.

[projectgus]

Thanks for submitting this, @LucienMP! It's been an important missing feature on esp32 port for a long time.

I see you chose to use FreeRTOS timers for this. Did you consider the ESP Timer feature of ESP-IDF as an alternative? ESP Timer has some advantages (for example: 1us resolution instead of 10ms, timer callbacks run at a much higher priority in the system).

Q#2: The machine_soft_timer wont pass as is, but can run the timer code if -1 is passed as id

You're right about the docs, but this does work on a number of ports so it would be good to support it here (and to be able to pass the test!)

You can follow an approach like this one, from the generic extmod/ implementation:
https://github.com/micropython/micropython/blob/master/extmod/machine_timer.c#L106

Q#3: Documentation says hard=True, but I default to hard=False since i dont support hard=True types, is it ok?

Yes, this is OK. We can't easily run Python code in hard interrupts on esp32 port because we have the "global interpreter lock" when Python code is running.

[projectgus]

It's a little confusing that this code is in this function, but the same two lines of code (and the equivalent malloc) for hardware timers are in machine_timer_create().

Suggest moving all of this into machine_timer_create() if you can, so it's easier to see what is similar (and different) between soft vs hard timers.

[LucienMP]

I was trying to leave the code as much as original, but yes this could all move into create timer and yes it would make more sense. Will do that.

[LucienMP]

@projectgus ok updated with the changes you suggested;

1. self->is virtual to make it clear if the timer is virtual or not; actually "repeat" could become a bool too and isnt
2. swapped from FreeRTOS timers to ESP timers, much nicer thanks - better res
3. swapped the code for creation around, there is still some duplication in it but i left it that way for easier readability - it could be reorganized for a slightly smaller binary output
4. Simplified the _print having more reuse
5. Fixed the deinit logic, i think the deinit shouldnt deallocate and leave it to GC to do
6. Added support for the Timer(freq=...) style, where id=-1 is default assumed for virtual timers

[LucienMP]

Q#2: The machine_soft_timer wont pass as is, but can run the timer code if -1 is passed as id

You're right about the docs, but this does work on a number of ports so it would be good to support it here (and to be able to pass the test!)

You can follow an approach like this one, from the generic extmod/ implementation: https://github.com/micropython/micropython/blob/master/extmod/machine_timer.c#L106

The code example confused me until I realized n_args and n_kw_args were two different things.

Can we change the the documentation to be very clear that "classmachine.Timer(id, /, ...)" is specifying id=-1 and soft timer is defaulted to if missing. Actually in other places in the documentation "[ ]" are used so it might be better to document this as "classmachine.Timer([id=-1], /, ...)"

https://docs.micropython.org/en/latest/library/machine.Timer.html#machine.Timer

[projectgus]

Thanks @LucienMP, I have one suggestion but this approach looks good to me!

Can we change the the documentation to be very clear that "classmachine.Timer(id, /, ...)" is specifying id=-1 and soft timer is defaulted to if missing. Actually in other places in the documentation "[ ]" are used so it might be better to document this as "classmachine.Timer([id=-1], /, ...)"

Yes, we should. I'll submit a PR for this.

[projectgus]

Rather than repeating this block, could you do:

bool new_timer = false;

[...]

if (new_timer) {
   // Add the timer to the linked-list of timers
   [...]
}

[...]

return self

[projectgus]

(Edited my suggestion as I realised it had a bug!)

[LucienMP]

Does the above still stand, I will review and resubmit

[projectgus]

Yes, if you're able to refactor this way then it should be easier to follow. Thanks!

[DvdGiessen]

We don't have a check for None in for hardware timers (see machine_timer_isr_handler). Instead the scheduled function call will raise because it's not actually callable, printing TypeError: 'NoneType' object isn't callable.

Perhaps for consistency we should do that here as well, to make it easier for users to notice a bug where they passed None.

[DvdGiessen]

Note for hardware timers there's an additional layer of indirection: The ISR called when the hardware timer triggers is machine_timer_isr, which in turn calls the function stored in self->handler (defaults to machine_timer_isr_handler) which actually performs the mp_sched_schedule call. Reason for this indirection is that it allows using the timer from other C code by replacing handler. For example the UART RXIDLE implementation sets timer->handler = uart_timer_callback.

Optionally if we use the same pattern here we could make virtual timers compatible with such C code.

[projectgus]

Good point @DvdGiessen, I missed this! It would be nice to have RXIDLE not consume a hardware timer as well. (Not in this PR, necessarily.)

[DvdGiessen]

Given that this module now depends on finalisers, should we check for MICROPY_ENABLE_FINALISER? Some other modules have a explicit compile-time check at the top of the file, something like:

#if !MICROPY_ENABLE_FINALISER
#error "machine.Timer requires MICROPY_ENABLE_FINALISER."
#endif

[DvdGiessen]

We could make the code a tiny bit smaller by eliminating the edge case:

    // remove our timer from the linked list
    for (machine_timer_obj_t **t = &MP_STATE_PORT(machine_timer_obj_head); *t != NULL; t = &(*t)->next) {
        if (*t == self) {
            *t = self->next;

            // Remove memory allocation
            if (self->vtimer != NULL) {
                esp_timer_stop(self->vtimer);
                esp_timer_delete(self->vtimer);
            }
            m_del_obj(machine_timer_obj_t, self);
            break;
        }
    }