Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
MabezDev opened this issue Apr 19, 2024 · 5 comments · Fixed by #1871
Closed

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

MabezDev opened this issue Apr 19, 2024 · 5 comments · Fixed by #1871
Labels
peripheral:timer Timer peripheral status:needs-attention This should be prioritized

Comments

@MabezDev
Copy link
Member

Title, and a bit more analysis here: #1318 (comment)

@Dominaezzz
Copy link
Collaborator

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 😢, 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
Copy link
Collaborator

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

@jessebraham jessebraham added the peripheral:timer Timer peripheral label Jul 8, 2024
@MabezDev MabezDev added the status:needs-attention This should be prioritized label Jul 26, 2024
@ProfFan
Copy link
Contributor

ProfFan commented Jul 28, 2024

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
Copy link
Contributor

ProfFan commented Jul 28, 2024

*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
Copy link
Contributor

bjoernQ commented Jul 28, 2024

cc @JurajSadel we need test coverage for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
peripheral:timer Timer peripheral status:needs-attention This should be prioritized
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants