Features/driver/iwdg #21

PratyushMakkar · 2023-07-10T22:54:12Z

Description

The pull request is for the independent watchdog driver. The driver can be initialized with either a prescaler/reload value or a IDWG watchog typedef, the driver will then copy the values from the watchdog instance The driver allows changing the prescaler/reload values while the program is running and to refresh the watchdog timer.

Why was this done (if applicable)?
The driver is part of Milestone 1

Testing

Since the driver is not accessible in debug mode, to test it, I set an indicator LED to turn on and off at the start of the program so indicate that the CPU restarted. One test was to see if prescaler values can be modified while the program ran. To do that, the IOC file was configured to set the prescaler as 16 and reload value as 4095, in the program, the prescaler was set to 8 and reload as 2095. It was then manually checked that the CPU restarted more often (almost instantly) when the prescaler value was lower.

The second test was to refresh the watchdog a fixed amount of times with a HAL_Delay(); and then not to refresh the watchdog. The expected result is that the CPU will restart after 5 seconds + The Independent watchdog timer delay.

What unit tests were used to validate the code?

HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
HAL_Delay(500);
HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
IWDG_t watchdogDriver{&hiwdg};
 if (!watchdogDriver.SetWatchdogParameters(IWDG_PRESCALER_16, 4095)) {
	  HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
	  HAL_Delay(2000);
	  HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
  }

  while (1) {
	 HAL_Delay(1000);
  }

Second Test

HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
  HAL_Delay(500);
  HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
  IWDG_t watchdogDriver{&hiwdg};
           for (uint8_t i = 0; i<100; i++) {
                 if (i<10) {
                      if (watchdogDriver.RefreshWatchdog()) {
                    	  HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
                    	  HAL_Delay(250);
                    	  HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
                    	  HAL_Delay(250);
                    	  HAL_GPIO_TogglePin(GPIOA, GPIO_PIN_5);
                      }
                  }
                  HAL_Delay(500);
           }

Documentation

Milestone number and name:
Milestone 1, Name: RSSI/Watchdog
Link to Asana task:
https://app.asana.com/0/1203458353737758/1204645025850461
Link to Confluence documentation:
TODO: Unsure how to use confluence.

Reminders

Add reviewers to the PR
Mention the PR in the appropriate discord channel

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

SnackkOverflowError · 2023-07-11T04:51:36Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+ * @brief Initalize the driver using custom IWDG_Prescaler and realod values.
+ *
+ */
+IWDG_t::IWDG_t(uint8_t iwdg_prescaler, uint16_t iwdg_reload) {


Any reason the Init function and this constructor cant be the same? Like, just do the Init function's logic in this constructor rather than have two separate functions?

I have modified the driver so that setting prescaler and reload is the same as re-initializing the watchdog.

SnackkOverflowError · 2023-07-11T04:53:26Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+ * to the value within the reload register.
+ *
+ */
+HAL_StatusTypeDef IWDG_t::RefreshWatchdog(IWDG_HandleTypeDef *watchdog) {


return a bool rather than HAL_StatusTypeDef here. perhaps return false for an error or something like that? Or make a status bool stored in the driver that can be checked by a manager. @TheSpaceDragon @Chrisc110 @Ayounggie Thoughts?

Yeah, I agree with changing the return type to bool. It would be nice to have something like didWatchdogRefresh confirmation after the function returns.

Should I use try catch to supress errors and return false as well in the newest commit?

TheSpaceDragon · 2023-07-12T00:04:29Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+ * to the value within the reload register.
+ *
+ */
+HAL_StatusTypeDef IWDG_t::RefreshWatchdog(IWDG_HandleTypeDef *watchdog) {


Yeah, I agree with changing the return type to bool. It would be nice to have something like didWatchdogRefresh confirmation after the function returns.

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

Aidan-B

Overall, it seems like there is a little bit of redundancy in the code that should be pretty easy to simplify.

Aidan-B · 2023-07-12T04:56:53Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

+#include <stdint.h>
+#include "main.h"
+
+class IWDG_t {


This seems like an odd class name

Would IWDGDriver be more suitable?

I like the name IndependentWatchdog, like you have named the file. IndependentWDG or IWDG might also be suitable.

Aidan-B · 2023-07-12T04:58:00Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

I think the formatting isn't quite right. Could you run clang-format? There's some extra whitespace and the alignment doesn't match what we use elsewhere

is clang-format a bash command? I was unable to run it on my system. I am editing on Vscode and I am using tabs.

I'm pretty sure if you run Ctrl + Shift + I in vscode it will format the code for you, using our clang-format rules.

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

Aidan-B

I would suggest thinking about how you intend for users to interact with the watchdog. In my opinion, the primary actions would be to:
a) setup the watchdog. This can be done entirely from the constructor
b) pet the watchdog. This may need to be done from different threads

Think about then how your driver's API can meet the needs of the user.

Aidan-B · 2023-07-14T04:54:49Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

-	return refresh;
+
+bool IWDG_t::RefreshWatchdog() {
+	if (!this->watchdog) {


I think the following would be more readable:

Suggested change

if (!this->watchdog) {

if (this->watchdog == nullptr) {

Aidan-B · 2023-07-14T04:56:35Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+void IWDG_t::SetIWDGWatchdog(IWDG_HandleTypeDef* watchdog) {
+	this->watchdog = watchdog;
 }
+


Why would we ever need to do this in the lifetime of the watchdog?

also, this will likely cause a memory leak, the old watchdog doesn't get deleted and it isn't very clear what owns that object.

Aidan-B · 2023-07-14T04:58:19Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+IWDG_HandleTypeDef* IWDG_t::GetIWDGWatchdog(IWDG_HandleTypeDef* watchdog) {
+	return this->watchdog;
+}


I don't think anything would ever need to get this? I don't really see the utility here.

Additionally, there is the issue of what owns the pointer, and the implications of passing it around (or even deleting it)

I agree, setting and getting the watchdog handle arent really important for this driver imo.

Also, for this function, wouldn't we be passing a watchdog pointer and then returning the same watchdog?

This function has been removed

Aidan-B · 2023-07-14T04:59:33Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

 		uint8_t iwdg_prescaler;
 		uint16_t iwdg_reload;


I don't think we need to store these values, since they exist in the IWDG_HandleTypeDef. We are just duplicating the storage of data by having them in this scenario.

This has been addressed. The values are not stored anymore.

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

Aidan-B · 2023-07-14T05:06:50Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

-		static HAL_StatusTypeDef RefreshWatchdog(IWDG_HandleTypeDef* watchdog);
+		void SetIWDGWatchdog(IWDG_HandleTypeDef* watchdog);
+		IWDG_HandleTypeDef* GetIWDGWatchdog(IWDG_HandleTypeDef* watchdog);
+		bool SetWatchdogParameters(uint8_t prescaler, uint16_t reload);


I think this should be something like UpdateWatchdogParameters, since it updates them. Currently it implies that it sets values that haven't been otherwise set.

Okay, after further review, I think I understand why you created this function - to allow the user to set the parameters of the watchdog (before starting the watchdog).
I would argue (and have been basing my reviews off this assumption) that this should configuration should be done by the user before they pass the IWDG_HandleTypeDef to the driver, with the Init function being called from the constructor. I believe it to be the general approach that the team has been taking with other drivers thus far. Curious what you and @SnackkOverflowError think about this approach.

Hmmm theres a lot of ways to handle this. On one hand, the watchdog parameters can be set in our IOC (which will generate code to configure them in the watchdog handle I believe). Then the handle is passed into this driver and the parameters can be retrieved from the handle. In which case I agree that this function should be named UpdateWatchDogParameters .
What exactly is the usecase for resetting the watchdog parameters after the driver has been created though? Is there a scenario where we want to change the frequency that we pet it at? Seems like we should agree on the watchdog parameters once and then never touch them again? If the usecase is being able to configure them in model config, IMO these params should be parameters in the constructor and set there once, then never reset during the programs lifetime.

I agree, I feel like updating watchdog parameters after initialization could cause some unforeseen issues later on. Though, it doesn't hurt to have the function when/if need arises for that.

This function has been removed as requested.

Aidan-B · 2023-07-14T05:15:47Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+	this->iwdg_prescaler = prescaler;
+	this->iwdg_reload = reload;
+
+	if (!IS_IWDG_PRESCALER(prescaler) || !IS_IWDG_RELOAD(reload)) return false;


Where are these macros defined? I didn't see them in my skim of the HAL docs

I believe the HAL docs do not mention it even though it is available. I saw that the f4 did not mention it either even though it is available in the stm32f4xx_iwdg.h file. However the function is removed in the newest commit so that should resolve any potential issues.

Aidan-B · 2023-07-14T05:15:49Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+	this->iwdg_reload = reload;
+
+	if (!IS_IWDG_PRESCALER(prescaler) || !IS_IWDG_RELOAD(reload)) return false;
+	watchdog->Init.Reload = reload;


You should probably check that the reload value is less than the max value for the reload (0x0FFF iirc)

Aidan-B · 2023-07-14T05:18:48Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

+#include <stdint.h>
+#include "main.h"
+
+class IWDG_t {


I like the name IndependentWatchdog, like you have named the file. IndependentWDG or IWDG might also be suitable.

Aidan-B · 2023-07-14T05:19:35Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

 /**
- * @brief Initalize the driver using custom IWDG_Prescaler and realod values.
+ * @brief Reset the reload and counter values inside a watchdog
+ * using the member variables of IWDG_t
 *


Suggested change

*

* @returns true on success, false on failure

SnackkOverflowError · 2023-07-14T06:57:24Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

+
+	public:
+		IWDG_t(IWDG_HandleTypeDef* watchdog);
+		void SetIWDGWatchdog(IWDG_HandleTypeDef* watchdog);


Will this function ever need to be used? Seems like you can just use the constructor? I don't really see a reason that the watchdog handle will need to be reset.

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

SnackkOverflowError · 2023-07-14T07:00:18Z

Drivers/IWDG_driver/Src/IndependentWatchdog.cpp

+IWDG_HandleTypeDef* IWDG_t::GetIWDGWatchdog(IWDG_HandleTypeDef* watchdog) {
+	return this->watchdog;
+}


I agree, setting and getting the watchdog handle arent really important for this driver imo.

SnackkOverflowError · 2023-07-14T07:06:23Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

-		static HAL_StatusTypeDef RefreshWatchdog(IWDG_HandleTypeDef* watchdog);
+		void SetIWDGWatchdog(IWDG_HandleTypeDef* watchdog);
+		IWDG_HandleTypeDef* GetIWDGWatchdog(IWDG_HandleTypeDef* watchdog);
+		bool SetWatchdogParameters(uint8_t prescaler, uint16_t reload);


Hmmm theres a lot of ways to handle this. On one hand, the watchdog parameters can be set in our IOC (which will generate code to configure them in the watchdog handle I believe). Then the handle is passed into this driver and the parameters can be retrieved from the handle. In which case I agree that this function should be named UpdateWatchDogParameters .
What exactly is the usecase for resetting the watchdog parameters after the driver has been created though? Is there a scenario where we want to change the frequency that we pet it at? Seems like we should agree on the watchdog parameters once and then never touch them again? If the usecase is being able to configure them in model config, IMO these params should be parameters in the constructor and set there once, then never reset during the programs lifetime.

SnackkOverflowError · 2023-07-14T07:06:57Z

Drivers/IWDG_driver/Inc/IndependentWatchdog.h

@@ -0,0 +1,22 @@
+#ifndef SRC_DRIVERS_INDEPENDENTWATCHDOG_H_
+#define SRC_DRIVERS_INDEPENDENTWATCHDOG_H_
+


Please fix the filenames according to our style guide: https://uwarg-docs.atlassian.net/wiki/spaces/ZP/pages/2268692486/Style+Standards

Aidan-B · 2023-07-14T16:31:39Z

I just realized this class doesn't inherit from a base watchdog interface class, which it probably should do (so we can more easily mock it for unit tests)

SnackkOverflowError

Looks good except for some code in your constructor

SnackkOverflowError · 2023-07-18T19:39:17Z

Drivers/IWDG_driver/Src/independent_watchdog.cpp

+	this->watchdog = watchdog;
+	IWDG_InitTypeDef initDef = watchdog->Init;
+	this->iwdg_prescaler = initDef.Prescaler;
+	this->iwdg_reload = initDef.Reload;


iwdg_prescaler and iwdf_reload dont exist anymore (and arent used) so probably remove lines 11,12 and 13 I think

This reverts commit ccfa777.

PratyushMakkar added 3 commits July 5, 2023 18:14

Added independent watchdog driver

6cbb940

Finished testing IWDG

c97c7d4

Small chnage to uint16_t

0ffcc15

SnackkOverflowError requested changes Jul 11, 2023

View reviewed changes

TheSpaceDragon reviewed Jul 12, 2023

View reviewed changes

Aidan-B requested changes Jul 12, 2023

View reviewed changes

Addressed comments

d034ba7

Aidan-B requested changes Jul 14, 2023

View reviewed changes

SnackkOverflowError requested changes Jul 14, 2023

View reviewed changes

Addressed comments

3ac89d4

SnackkOverflowError requested changes Jul 18, 2023

View reviewed changes

PratyushMakkar added 6 commits July 18, 2023 17:30

Removed prescaler values from constructor

1ecbb49

Added files to cmake

ccfa777

Revert "Added files to cmake"

5fd3e30

This reverts commit ccfa777.

Modified CMAKE to add IWDG driver

a1e51bd

Configured IOC

c305c2c

Added base class header-file

d95fb73

		@@ -0,0 +1,22 @@
		#ifndef SRC_DRIVERS_INDEPENDENTWATCHDOG_H_
		#define SRC_DRIVERS_INDEPENDENTWATCHDOG_H_

Features/driver/iwdg #21

Are you sure you want to change the base?

Features/driver/iwdg #21

Conversation

PratyushMakkar commented Jul 10, 2023 • edited

Description

Testing

Documentation

Reminders

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aidan-B left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aidan-B left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aidan-B commented Jul 14, 2023

SnackkOverflowError left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PratyushMakkar commented Jul 10, 2023 •

edited

SnackkOverflowError left a comment •

edited