[riot-notifications] [RIOT-OS/RIOT] cpu/stm32_common: add watchdog for stm32 (#11252)

Alexandre Abadie notifications at github.com
Mon Mar 25 13:29:13 CET 2019


aabadie requested changes on this pull request.

I have other comments, see below.

> Regarding the python test. I had done one previously but is hard to test proper timing only threw a uart connection. To verify that it is working properly you must not only verify a reset occurred but also verify the timing. When working with ms the delay caused by prints and the time for the cpu to restart greatly affect the time you get first prints.

I see the problem. The python test script can be useful for basic testing since there's no "complex" hardware setup required: it's easy to automatize the verification that the watchdog triggers a reboot. For timing, you could add a % tolerance parameter, we don't need ms precision at this level of testing IMHO.

> + *              and spins until a wdg reset is triggered
+ *
+ * @author      Francisco Molina <francois-xavier.molina at inria.fr>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "periph/wdg.h"
+#include "timex.h"
+#include "shell.h"
+
+int setup_wdg(int argc, char **argv){

You need to uncrustify your files, there are still missing spaces before opening curly braces or curly braces not on the right lines.

> + *              and spins until a wdg reset is triggered
+ *
+ * @author      Francisco Molina <francois-xavier.molina at inria.fr>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "periph/wdg.h"
+#include "timex.h"
+#include "shell.h"
+
+int setup_wdg(int argc, char **argv){

And maybe this function should be called `init_wdg`. It would be more consistent with the command name.

> + * @brief       Test for wdg Drivers
+ *
+ *              This test initializes the wdg counter to a preset value
+ *              and spins until a wdg reset is triggered
+ *
+ * @author      Francisco Molina <francois-xavier.molina at inria.fr>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "periph/wdg.h"
+#include "timex.h"

This include is not needed I think

> + * @ingroup tests
+ * @{
+ *
+ * @file
+ * @brief       Test for wdg Drivers
+ *
+ *              This test initializes the wdg counter to a preset value
+ *              and spins until a wdg reset is triggered
+ *
+ * @author      Francisco Molina <francois-xavier.molina at inria.fr>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+#include <string.h>

I don't think this include is required

> +    (void) argv;
+    printf("[wdg]: started wdg timer\n");
+    wdg_enable();
+
+    return 0;
+}
+
+static const shell_command_t shell_commands[] = {
+    { "init", "Setup Wdg Timer", setup_wdg },
+    { "start", "Start wdg timer", start_wdg },
+    { NULL, NULL, NULL }
+};
+
+int main(void)
+{
+    printf("RIOT wdg test application\n");

```suggestion
    puts("RIOT wdg test application");
```

> +
+int setup_wdg(int argc, char **argv){
+    if (argc < 2){
+        printf("usage: %s <time[us]>\n", argv[0]);
+        return 1;
+    }
+
+    uint32_t rst_time = wdg_init((uint32_t) atoi(argv[1]));
+    printf("[wdg]: configured with reset time %lu [us]\n", rst_time);
+    return 0;
+}
+
+int start_wdg(int argc, char **argv){
+    (void) argc;
+    (void) argv;
+    printf("[wdg]: started wdg timer\n");

```suggestion
    puts("[wdg]: started wdg timer");
```

> @@ -0,0 +1,19 @@
+include ../Makefile.tests_common
+BOARD ?= nucleo-l152re

This line should be put first, otherwise the default board won't be nucleo-l152re but samr21-xpro because it is defined first in `Makefile.tests_common`

> +void wdg_enable(void);
+
+/**
+ * @brief    Disable watchdog timer
+ */
+void wdg_disable(void);
+
+/**
+ * @brief    Reset the watchdog timer
+ */
+void wdg_reset(void);
+
+/**
+ * @brief    Sets the time before a wdg reset, best effort approximate value
+ *
+ * @param[out] time_set     actual time in us for wdg reset , time_set~time

You can remove the space before the comma

> +void wdg_enable(void);
+
+/**
+ * @brief    Disable watchdog timer
+ */
+void wdg_disable(void);
+
+/**
+ * @brief    Reset the watchdog timer
+ */
+void wdg_reset(void);
+
+/**
+ * @brief    Sets the time before a wdg reset, best effort approximate value
+ *
+ * @param[out] time_set     actual time in us for wdg reset , time_set~time

The param name doesn't corresponds to the parameter name in the function definition, doxygen will complain about that. And the param is input not output => use `@param[int] rst_time`

> + */
+
+/**
+ * @defgroup    drivers_periph_wdg
+ * @ingroup     drivers_periph
+ * @brief       Common watchdog interface
+ *
+ * @{
+ *
+ * @file		wdg.h
+ * @brief       watchdog peripheral interface
+ *
+ * @author      Francisco Molina <francois-xavier.molina at inria.fr>
+ */
+
+#ifndef PERIPH_WDG_H_

The trailing `_` must be removed

> + * @brief    Disable watchdog timer
+ */
+void wdg_disable(void);
+
+/**
+ * @brief    Reset the watchdog timer
+ */
+void wdg_reset(void);
+
+/**
+ * @brief    Sets the time before a wdg reset, best effort approximate value
+ *
+ * @param[out] time_set     actual time in us for wdg reset , time_set~time
+ *
+ * @return                  0 When out of bounds
+ * @return                  uint32_t with the setted reset time

`setted` is a word that doesn't exist
```suggestion
 * @return                  the reset time set
```

> @@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2019 Inria
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup    drivers_periph_wdg
+ * @ingroup     drivers_periph
+ * @brief       Common watchdog interface

Maybe extend a bit the documentation: this is what will be in the top-level periph_wdg API documentation of doxygen.

> +        return 0;
+    }
+
+    uint8_t pre = _find_prescaler(rst_time);
+    uint16_t rel = _find_reload_value(pre, rst_time);
+
+    /* Set watchdog prescaler and reload value */
+    _set_prescaler(pre);
+    _set_reload(rel);
+
+    /* Calculate the actual reset time in us */
+    uint32_t time_set = _wdg_time(pre, rel);
+    DEBUG("[wdg]: reset time %lu [us]\n", time_set);
+
+    /* Wait for register to be updated */
+    volatile int timeout = 10000;

Why 10000 ? Maybe use a define for this hardcoded value.

> +
+    uint8_t pre = _find_prescaler(rst_time);
+    uint16_t rel = _find_reload_value(pre, rst_time);
+
+    /* Set watchdog prescaler and reload value */
+    _set_prescaler(pre);
+    _set_reload(rel);
+
+    /* Calculate the actual reset time in us */
+    uint32_t time_set = _wdg_time(pre, rel);
+    DEBUG("[wdg]: reset time %lu [us]\n", time_set);
+
+    /* Wait for register to be updated */
+    volatile int timeout = 10000;
+    while(IWDG->SR && timeout){
+        timeout--;

So when this times out, the watchdog is not correctly initialised IIUC. Why not simply returning with a 0 value (which is invalid) ?
This makes me think that the API could be changed:
```c
int wdg_init(uint32_t rst_time)
```
And the function returns 0 (use an enum) when the initialization is successful. It returns -1 (or an errno code) in case of a problem.
I think it's useless to return the rst_time set: since it's also the input value, having a successful return (0) means rst_time is correctly set (e.g the value returned by `_wdg_time(pre, rel);` is a good approximation of the input parameter).

What do you think ?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11252#pullrequestreview-218281290
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190325/92888ba4/attachment-0001.html>


More information about the notifications mailing list