Skip to content

Refactor, document and fix the Watchdog driver#1710

Merged
JF002 merged 3 commits intomainfrom
refactor-watchdog
Apr 30, 2023
Merged

Refactor, document and fix the Watchdog driver#1710
JF002 merged 3 commits intomainfrom
refactor-watchdog

Conversation

@JF002
Copy link
Copy Markdown
Collaborator

@JF002 JF002 commented Mar 26, 2023

Refactor and document the Watchdog driver to make it more readable.

Fix the configuration of the behaviours configuration that was not properly implemented (but it didn't cause any side effect since the correct value was eventually set in NRF_WDT->CONFIG).

Fix the wrong interpretation of the reset reasons caused by implicit conversions of int to bool.

Watchdog is now probably one of the most documented class of the project. What do you think of this format? It's quite helpful in IDEs that support Doxygen like CLion:
image

This PR is an alternative solution to #1705.

Fix the configuration of the behaviours configuration that was not properly implemented (but it didn't cause any side effect since the correct value was eventually set in NRF_WDT->CONFIG).

Fix the wrong interpretation of the reset reasons caused by implicit conversions of int to bool.
@github-actions
Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 406440B -48B
data 940B 0B
bss 53560B 0B

Copy link
Copy Markdown
Member

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

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

I personally prefer this solution to #1705.

Comment thread src/drivers/Watchdog.cpp Outdated
Comment thread src/drivers/Watchdog.h Outdated
Comment thread src/drivers/Watchdog.h Outdated
Comment thread src/drivers/Watchdog.cpp Outdated
Copy link
Copy Markdown
Contributor

@Riksu9000 Riksu9000 left a comment

Choose a reason for hiding this comment

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

I like this. There are a lot of comments, but in low level code some are necessary.

There are a few potential improvements I can see, which nonetheless shouldn't be blocking. The timeout could be set using std::chrono durations, and the reset reason enum class could contain the values that the reset reason register itself may contain, so it can also be used in GetResetReason() and the value in the register doesn't need to be converted to another format.

Comment thread src/drivers/Watchdog.h Outdated
Rename enum ResetReasons, SleepBehaviours and HaltBehaviours to ResetReason, SleepBehaviour and HaltBehaviour. Rename the function ResetReason() to GetResetReason() to avoid name conflict.

Use named constants (WDT_CONFIG_HALT_Pos & WDT_CONFIG_SLEEP_Pos) from the NRF MDK instead of the numerical ones.

Fix typo in the comments.
@JF002
Copy link
Copy Markdown
Collaborator Author

JF002 commented Apr 1, 2023

I like this. There are a lot of comments, but in low level code some are necessary.

Thanks! Some comments might be a bit overkill, but they explain how the datasheet was translated into code :)

There are a few potential improvements I can see, which nonetheless shouldn't be blocking. The timeout could be set using std::chrono durations,

I also had this idea, but that would make the implementation of SetTimeout() much more complex if we want to handle all the possible values.

and the reset reason enum class could contain the values that the reset reason register itself may contain, so it can also be used in GetResetReason() and the value in the register doesn't need to be converted to another format.

We cannot simply cast the value of the register to the value of the enum because the register might have multiple bits that are set (especially if the register was not cleared between 2 resets).
We can use a bitfield instead of an enum for ResetReason, but in this case, the application will need to check for all the bits instead of a single value here. That's why I decided to 'convert' the value of the register to a single value - the 1st reset reason specified in the register.

@JF002 JF002 merged commit 5f19f68 into main Apr 30, 2023
@Riksu9000 Riksu9000 deleted the refactor-watchdog branch May 9, 2023 13:15
@FintasticMan FintasticMan added this to the 1.13.0 milestone Jun 22, 2023
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