esp-hal
esp-hal copied to clipboard
Allow SYSTIMER alarms to be configured for blocking or async individually
Title, and a bit more analysis here: https://github.com/esp-rs/esp-hal/issues/1318#issuecomment-2059823266
I started looking into this as I needed more timers and have written an implementation. The issue is I've hit an awkward hardware limitation that the current code doesn't handle correctly and wanted to get some thoughts about how it should be handled.
The problem is around two registers, SYSTIMER.CONF and SYSTIMER.INT_ENA.
Ideally as a hal user (and developer I guess) I'd like to have three independent Alarm objects, each of which I could use in different parts of my application or send to another core. This is what the current implementation is doing.
Unfortunately those two registers I mentioned above are shared between the Alarm objects :cry:, and the hal uses modify(|_, w| .... ) on them which is a non-atomic read-modify-write operation. So if two cores (or one core + interrupt) try to perform this operation, it's possible that only of the modifications actually persists. This isn't unsound but it leads to incorrect behavior.
So for Alarm to be correctly Send, the calls to modify(|_, w| .... ) need to be protected or users should be prevented from calling any methods that modify those registers.
I opted for a Config object that users can share with any appropriate locking mechanism of their choice.
I have a reproducer for the incorrect behavior here: https://github.com/ProfFan/esp-hal-bughunt
You can change the GPIO to be whichever your board have. And change SYSTIMER to TIMG0 or other timers.
LED should stop blinking in 10-60s, and resume after indefinite amount of time without resetting.
*It's quite hard to trigger because I only have 1 fast timer, if you change the Timer::afters to 1-3ms it triggers immediately
cc @JurajSadel we need test coverage for this