InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.64k stars 907 forks source link

Refactor, document and fix the Watchdog driver #1710

Closed JF002 closed 1 year ago

JF002 commented 1 year ago

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 https://github.com/InfiniTimeOrg/InfiniTime/pull/1705.

github-actions[bot] commented 1 year ago
Build size and comparison to main: Section Size Difference
text 406440B -48B
data 940B 0B
bss 53560B 0B
JF002 commented 1 year ago

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.