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

Disable all watchdog timers at startup by default #763

Merged
merged 5 commits into from
Aug 30, 2023

Conversation

jessebraham
Copy link
Member

  • Use the __post_init hook to disable all watchdog timers at startup
  • Refactor the watchdog timer drivers to be less reliant on the embedded-hal traits, add enable functions
  • Update the watchdog-related examples
    • All examples will need updating, and I have largely completed this already (oof), however I'd like to take some time to review all the changes and make sure I haven't done anything silly, so I will save that for another PR I think)

All modified examples have been tested, and I tried a couple others as well to ensure the watchdogs were indeed disabled.

@bugadani
Copy link
Contributor

bugadani commented Aug 29, 2023

It's definitely a good change, less boilerplate is always appreciated!

Weirdly enough, though, I never had to disable TIMG0/1 WDTs. Do you happen to know why? Edit: maybe it's a bootloader thing...?

@jessebraham
Copy link
Member Author

I'm not 100% sure to be honest, I've noticed the same thing though. Just included for completeness here. I think you may be right and there may be some configuration for that somewhere, but again not sure.

@bjoernQ
Copy link
Contributor

bjoernQ commented Aug 30, 2023

It's definitely a good change, less boilerplate is always appreciated!

Weirdly enough, though, I never had to disable TIMG0/1 WDTs. Do you happen to know why? Edit: maybe it's a bootloader thing...?

If these WDTs are enabled or not depends on whether it's an direct-boot or a esp-idf-bootloader boot (and I think for the esp-idf bootloader it might be even configurable - not sure). So to be on the safe side we just disable all the WDTs - won't hurt if they are already disabled

@bjoernQ
Copy link
Contributor

bjoernQ commented Aug 30, 2023

Looks good but it seems it breaks the rtc_watchdog example on ESP32 at least 🤔

@jessebraham jessebraham marked this pull request as draft August 30, 2023 14:15
@jessebraham jessebraham force-pushed the feature/disable-watchdogs branch from 670aa52 to 324b326 Compare August 30, 2023 14:54
@jessebraham
Copy link
Member Author

jessebraham commented Aug 30, 2023

Looks good but it seems it breaks the rtc_watchdog example on ESP32 at least 🤔

Ahh yeah my bad, misunderstood the example so I thought it was working 😀 I had accidentally introduced a bug, should be fixed now.

(I did test both examples for all devices this time, and they seem to be working as far as I can tell. I opened #766 but this was already an issue in main prior to this PR)

@jessebraham jessebraham marked this pull request as ready for review August 30, 2023 14:54
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks!

@jessebraham jessebraham merged commit 4dd9fbd into esp-rs:main Aug 30, 2023
16 checks passed
@jessebraham jessebraham deleted the feature/disable-watchdogs branch August 30, 2023 16:04
Comment on lines -22 to -23
- Add multicore-aware embassy executor for Xtensa MCUs (#723).
- Add interrupt-executor for Xtensa MCUs (#723).
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are those CHANGELOG entries removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

they were duplicates caused by incorrect rebasing

Copy link
Member Author

@jessebraham jessebraham Aug 30, 2023

Choose a reason for hiding this comment

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

Duplicate entry, to avoid needing to rebase PRs all the time we modify the merge settings for that file in .gitattributes, it's not perfect so sometimes things need cleaning up.

I just happened to do that while I was modifying this file anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I didn't see they were present 2 lines below 😅

playfulFence pushed a commit to playfulFence/esp-hal that referenced this pull request Sep 26, 2023
* Rework watchdog timer drivers to allow enabling/disabling and feeding without traits

* Disable all watchdogs prior to `main` using the `__post_init` hook

* Update all watchdog-related examples

* Update CHANGELOG

* Address review comment
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.

4 participants