esp-hal icon indicating copy to clipboard operation
esp-hal copied to clipboard

Allow SYSTIMER alarms to be configured for blocking or async individually

Open MabezDev opened this issue 1 year ago • 2 comments
trafficstars

Title, and a bit more analysis here: https://github.com/esp-rs/esp-hal/issues/1318#issuecomment-2059823266

MabezDev avatar Apr 19 '24 13:04 MabezDev

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.

Dominaezzz avatar Jun 22 '24 12:06 Dominaezzz

I opted for a Config object that users can share with any appropriate locking mechanism of their choice.

Dominaezzz avatar Jul 01 '24 21:07 Dominaezzz

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.

ProfFan avatar Jul 28 '24 15:07 ProfFan

*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

ProfFan avatar Jul 28 '24 15:07 ProfFan

cc @JurajSadel we need test coverage for this

bjoernQ avatar Jul 28 '24 15:07 bjoernQ