Watchdog: Set wdt behaviour correctly

[Riksu9000]

The first removed line shifts a mask, but the mask itself is shifted, so the shift is applied twice. The shift is zero, so effectively this sets the first bit to zero.

The second line creates a mask with halt run parameter at sleep position, which is clearly wrong. It sets the first bit to one, making the first line redundant.

The third line shifts a mask again, like the first line. This sets the seventh bit to one, which is undefined and probably (hopefully) does nothing.

The fourth line does a logical or with HALT_Pause, but HALT_Pause is zero, so it does nothing.

Effectively none of the lines are correct.

Use NRF hal to explicitly set the behaviour to what it is when the first bit is set.

[github-actions[bot]]

Build size and comparison to main:	Section	Size	Difference
text	406440B	-48B
data	940B	0B
bss	53560B	0B

[JF002]

Let's try to understand what's happening there. Here's the code in question:

NRF_WDT->CONFIG &= ~(WDT_CONFIG_SLEEP_Msk << WDT_CONFIG_SLEEP_Pos);
NRF_WDT->CONFIG |= (WDT_CONFIG_HALT_Run << WDT_CONFIG_SLEEP_Pos);

NRF_WDT->CONFIG &= ~(WDT_CONFIG_HALT_Msk << WDT_CONFIG_HALT_Pos);
NRF_WDT->CONFIG |= (WDT_CONFIG_HALT_Pause << WDT_CONFIG_HALT_Pos);

And the definition of the variable from NRFSDK/modules/nrfx/mdk/nrf52_bitfields.h

/* Register: WDT_CONFIG */
/* Description: Configuration register */

/* Bit 3 : Configure the watchdog to either be paused, or kept running, while the CPU is halted by the debugger */
#define WDT_CONFIG_HALT_Pos (3UL) /*!< Position of HALT field. */
#define WDT_CONFIG_HALT_Msk (0x1UL << WDT_CONFIG_HALT_Pos) /*!< Bit mask of HALT field. */
#define WDT_CONFIG_HALT_Pause (0UL) /*!< Pause watchdog while the CPU is halted by the debugger */
#define WDT_CONFIG_HALT_Run (1UL) /*!< Keep the watchdog running while the CPU is halted by the debugger */

/* Bit 0 : Configure the watchdog to either be paused, or kept running, while the CPU is sleeping */
#define WDT_CONFIG_SLEEP_Pos (0UL) /*!< Position of SLEEP field. */
#define WDT_CONFIG_SLEEP_Msk (0x1UL << WDT_CONFIG_SLEEP_Pos) /*!< Bit mask of SLEEP field. */
#define WDT_CONFIG_SLEEP_Pause (0UL) /*!< Pause watchdog while the CPU is sleeping */
#define WDT_CONFIG_SLEEP_Run (1UL) /*!< Keep the watchdog running while the CPU is sleeping */

I was a bit confused at first, but I think you're right : the code does not exactly what we expect it to do:

WDT_CONFIG_SLEEP_Msk << WDT_CONFIG_SLEEP_Pos

equals

(1 << WDT_CONFIG_SLEEP_Pos) << WDT_CONFIG_SLEEP_Pos
(1 << 0) << 0

And

WDT_CONFIG_HALT_Msk << WDT_CONFIG_HALT_Pos

equals

(1 << WDT_CONFIG_HALT_Pos) << WDT_CONFIG_HALT_Pos
(1 << 3) << 3

The shift is indeed applied twice. In the end, the value written in NRF_WDT->CONFIG is

NRF_WDT->CONFIG = 1 // Default value

NRF_WDT->CONFIG = 1 & (0b11111110) // = 0 - Clear bit 0
NRF_WDT->CONFIG = 0 | 1 // = 1 - Set bit 0 : Keep the watchdog running while the CPU is sleeping

NRF_WDT->CONFIG = 1 & (0b10111111) // = 1 - Clear bit 6 (we probably intended to clear bit 2
NRF_WDT->CONFIG = 1 | ((0<<3)<<3) // = 1

The end result is the correct one : we want to keep the watchdog running while the CPU sleep but not when the CPU is halted by the debugger (NRF52832 reference manual v1.1 p412):

However, the way to get that result is mostly bad.

---

Now, I'm afraid I don't agree with the proposed changes :

• they include the NRF driver from the NRF SDK, which is something I try to avoid as much as possible (See below).
• they mix custom implementation with the NRF driver which, depending on how the NRF driver is implemented, could cause unwanted behaviors.

To fix this issue, I would rather fix the current implementation of the driver instead of replacing it with another driver. I would also take the opportunity to improve the implementation of documentation of the driver to make improve the readability of the code. I'm willing to work on this :)

---

Why do I want to avoid using drivers from the NRF SDK?

This is a personal choice I made early in the development of InfiniTime. NRF drivers are nice, they are documented and implement all the functionalities of the chip.

However, they contain a lot of code we don't use (because we don't need all the functionalities of the chip) which makes the understanding of the code and debugging of the application a bit more tedious. They also heavily rely on concatenated #MACRO and global variables which do not work well with the OO design of InfiniTime.

It would have probably been possible to use those drivers anyway, but I eventually decided that it was easier to implement those driver from scratch, using the reference manual of the NRF52832. This made the fine tuning of the implementation for our use-case (memory usage, low power mode,...) a lot easier.

As I said, this is a personal choice. Using the NRF drivers would have probably been possible, but I preferred the other option. And I currently see no reason to re-implement all our drivers using the NRF drivers right now.

[Riksu9000]

Closing in favor of #1710