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

Separate TIMG into timer0, (timer1), wdt #104

Merged
merged 4 commits into from
Jul 20, 2022

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Jul 15, 2022

  • introduce TimerGroup which is split into timer0, (timer1) and wdt
  • adds support for WDT configuration and feeding
  • gives us more timers on Xtensa based chips
  • add example for using the watchdog for all chips
  • extend the timer_interrupt example for Xtensa based chips to use 4 timers

There is some level of duplicated code but the only alternative would be another macro. Since working with macros isn't really convenient, I accepted the duplicated code here. If we had more timers a macro might be worth it but I think in this case the duplication is acceptable

THIS IS A BREAKING CHANGE!

Better to do this sooner than later

This looks like a huge PR but it's not as bad as it looks like since every example had to be changed.

One note: The Xtensa based chips seem to have three timers in each timer group (called LACT on ESP32) but the TRM and also apparently ESP-IDF only ever talk about two. If we ever decide to add that mysterious third timer the change shouldn't affect most user code. So going with two timers now

@bjoernQ bjoernQ requested a review from jessebraham July 15, 2022 14:05
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good overall, thanks for doing this! Just left a few little comments, and I'd also prefer not to include the VS Code config files as mentioned in the other PR.

esp-hal-common/src/timer.rs Outdated Show resolved Hide resolved
esp-hal-common/src/timer.rs Outdated Show resolved Hide resolved
esp-hal-common/src/timer.rs Outdated Show resolved Hide resolved
@bjoernQ
Copy link
Contributor Author

bjoernQ commented Jul 20, 2022

@jessebraham tackled the comments

for settings.json: I added it to .gitignore and removed it from esp-hal-common but I left the templates there ... I think it's not a bad solution: this way when checking out users can just copy the template to settings.json which matches their target and we won't get all those annoying changes in settings.json

I kept the chip-specific settings since they usually don't change

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes!

@jessebraham jessebraham merged commit 147d8de into esp-rs:main Jul 20, 2022
jrmoulton pushed a commit to jrmoulton/esp-hal that referenced this pull request Jul 30, 2022
* Separate TIMG into timer0, (timer1), wdt
* Apply suggestions from code review
* Remove left-over code
* Ignore settings.json
@bjoernQ bjoernQ deleted the timer-refactoring branch October 27, 2022 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants