ports/esp32/machine_timer.c: Solves #4078, ESP32 timer reinitilization error

[dybber]

Solves the problem where multiple calls to machine.Timer.init() causes an OSError 261 on ESP32 boards.

The fix is explained here: espressif/arduino-esp32#1869 (comment)

Solves issue #4078.

[dpgeorge]

Thanks for the patch, but I'm not sure it's the right way to solve that issue of error 261. Active timers are put on a linked list (on the MicroPython side) and if you create a timer twice then it'll be placed twice on this list. I think it would be better to check for an existing timer on the list when creating one, and reuse it if found.

[dybber]

Oh yes, didn't actually think of that! But now when checking the code, I don't believe that's the case.

The init function calls machine_timer_disable which removes the existing timer from the list, before changing parameters and reenabling it.

https://github.com/micropython/micropython/blob/master/ports/esp32/machine_timer.c#L179

[dpgeorge]

The init function calls machine_timer_disable which removes the existing timer from the list, before changing parameters and reenabling it.

But that doesn't stop there being multiple instances of Timer all pointing to the same underlying hardware resource, because these multiple Timer instances are distinct pointers to the MicroPython heap.

[dybber]

Okay, I see. I can implement a reference counting solution. Just as a sanity check before I start, do you think the following would be a suitable solution:

• Timer's are added to the list already in the Timer constructor, not by the .init() method, if added twice reference counter is increased and the existing timer returned.
• Timer's are stopped started by .init() / .deinit() method, but does not alter the list
• Timer's are removed from the list in __del__ (or -1 ref count)

Btw. I still believe the flags in the patch are necessary though, to solve the problem that occurs in programs such as this, which also raises OSError 261:

timer0 = Timer(0)
timer0.init(period=1000, mode=Timer.PERIODIC, callback=my_callback)
timer0.deinit()
timer0.init(period=1000, mode=Timer.PERIODIC, callback=my_callback)

[dpgeorge]

I can implement a reference counting solution.

I don't think it needs that. There are only 4 timers in total so just keeping them in the link list forever after creation is fine (until soft reset, via machine_timer_deinit_all()). Adding them to the list in the constructor sounds good.

I still believe the flags in the patch are necessary though, to solve the problem that occurs in programs such as this, which also raises OSError 261

That particular code runs fine for me, no error.

[dybber]

Okay, check the latest commit.

You are right, the flags are not necessary. The example raising OSError 261 above only appeared when re-executing the code after a soft-reboot, however, it works now without the flags.

[dpgeorge]

Thank you! Merged in de76f73