search

esp-rs / esp-hal

no_std Hardware Abstraction Layers for ESP32 microcontrollers
https://docs.esp-rs.org/esp-hal/
Apache License 2.0
671 stars 190 forks source link

Allow SYSTIMER alarms to be configured for blocking or async individually #1477

Closed MabezDev closed 1 week ago

MabezDev commented 4 months ago

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

Dominaezzz commented 2 months ago

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 commented 1 month ago

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

ProfFan commented 4 weeks ago

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 commented 4 weeks ago

*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

bjoernQ commented 4 weeks ago

cc @JurajSadel we need test coverage for this