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

Alexandre Abadie notifications at github.com
Thu Sep 12 09:50:31 CEST 2019


aabadie commented on this pull request.

I made another round of review and found a few nits and typos. I'll test on the STM32 boards I have around (L0/L4).

> @@ -15,4 +15,9 @@ ifneq (,$(filter periph_flashpage periph_eeprom,$(USEMODULE)))
   SRC += flash_common.c
 endif
 
+ifneq (,$(filter periph_wdt,$(USEMODULE)))
+$(warning Attention! WDT is clocked by CLOCK_LSI, it needs manual measuring\

Please indent with 2 spaces inside the if block

> @@ -15,4 +15,9 @@ ifneq (,$(filter periph_flashpage periph_eeprom,$(USEMODULE)))
   SRC += flash_common.c
 endif
 
+ifneq (,$(filter periph_wdt,$(USEMODULE)))
+$(warning Attention! WDT is clocked by CLOCK_LSI, it needs manual measuring\
+    since values can deviate upto 50% from reference)

put space between _up_ and _to_ ?

> @@ -0,0 +1,11 @@
+# Periph Wdg Test
+
+## About
+
+This application allows testing the wdg peripheral. User can define the reset
+time threw the shell.

s/threw/through/ ?

> @@ -0,0 +1,11 @@
+# Periph Wdg Test
+
+## About
+
+This application allows testing the wdg peripheral. User can define the reset
+time threw the shell.
+
+## Expected Result
+
+If wdg was initiated with a valid reset period then once it is started user

s/initiated/initialized/ ?

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

This line is not needed anymore

> +    (void)argc;
+    (void)argv;
+    printf("[wdt]: start time: %" PRIu32 " us\n", xtimer_now_usec());
+    wdt_start();
+    while (1) {
+        printf("[wdt]: reset time: %" PRIu32 " us\n", xtimer_now_usec());
+    }
+    return 0;
+}
+
+#if WDT_HAS_STOP
+int stop_wdt(int argc, char **argv)
+{
+    (void)argc;
+    (void)argv;
+    puts("[wdt]: stoping wdt timer");

s/stoping/stopping/

> +    wdt_stop();
+    return 0;
+}
+#endif
+
+int kick_wdt(int argc, char **argv)
+{
+    (void)argc;
+    (void)argv;
+    puts("[wdt]: delaying wdt timer");
+    wdt_kick();
+    return 0;
+}
+
+static const shell_command_t shell_commands[] = {
+    { "range", "Return Wdg Timer range", get_range },

Here and line below Watchdog is called _Wdg_ but in other places you use _wdt_. Let's be consistent and use _wdt_ everywhere.

> +{
+    (void)argc;
+    (void)argv;
+    puts("[wdt]: starting wdt timer");
+    wdt_start();
+    return 0;
+}
+
+int start_loop_wdt(int argc, char **argv)
+{
+    (void)argc;
+    (void)argv;
+    printf("[wdt]: start time: %" PRIu32 " us\n", xtimer_now_usec());
+    wdt_start();
+    while (1) {
+        printf("[wdt]: reset time: %" PRIu32 " us\n", xtimer_now_usec());

Prefixing with _[wdt]_ in the test application is not helpful to determine from the message comes from: the same prefix is used when enable debug in the peripheral implementation. Maybe just remove the prefix in the test application ? (and save some bytes on ROM).

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

I think this include can be removed, since it's might be already included by `periph/wdt.h`.

> @@ -0,0 +1,19 @@
+BOARD ?= nucleo-l152re
+include ../Makefile.tests_common
+
+TEST_ON_CI_WHITELIST += all
+
+FEATURES_REQUIRED += periph_wdt
+
+USEMODULE += xtimer
+USEMODULE += shell
+
+# Comment this out to disable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:
+DEVELHELP ?= 1

I think this is not needed, same for `QUIET`.

> +from testrunner import run
+
+# We test only up to 10ms, with smaller times mcu doesn't have time to
+# print system time before resetting
+reset_times_ms = [1e2, 5e2, 1e3, 5e3]
+
+# We don't check for accuracy, only order of magnitude. Some MCU use an
+# an internal un-calibrated clock as reference which can deviate in
+# more than 50% from theoretical values (e.g STM32 board CLOCK_LSI)
+error_marge = 0.5
+
+def get_reset_time(child):
+    reset_time = 0
+    try:
+        while True:
+            child.expect(u"\[wdt\]: reset time: (\\d+) us", timeout=1)

`(\d+)` works the same as `(\\d+)` (only `\`).

> +# 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.
+
+import sys
+import pexpect
+from testrunner import run
+
+# We test only up to 10ms, with smaller times mcu doesn't have time to
+# print system time before resetting
+reset_times_ms = [1e2, 5e2, 1e3, 5e3]
+
+# We don't check for accuracy, only order of magnitude. Some MCU use an
+# an internal un-calibrated clock as reference which can deviate in
+# more than 50% from theoretical values (e.g STM32 board CLOCK_LSI)
+error_marge = 0.5

Maybe use `error_margin` for variable name ?

> +    wdt_lower_bound = int(child.match.group(1))
+    wdt_upper_bound = int(child.match.group(2))
+
+    for rst_time in reset_times_ms:
+        child.sendline("setup 0 {}".format(rst_time))
+        if rst_time < wdt_lower_bound or rst_time > wdt_upper_bound:
+            child.expect_exact("[wdt]: invalid time, see \"range\"", timeout=1)
+        else:
+            child.sendline("startloop")
+            child.expect(u"\[wdt\]: start time: (\\d+) us", timeout=1)
+            start_time_us = int(child.match.group(1))
+            reset_time_us = get_reset_time(child)
+            wdt_reset_time = (reset_time_us - start_time_us) / 1e3
+
+            if wdt_reset_time < rst_time*(1 - 0.5) or \
+               wdt_reset_time > rst_time*(1 + 0.5) :

no need for space before `:`

-- 
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-287236202
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190912/145b7eb5/attachment-0001.htm>


More information about the notifications mailing list