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

Alexandre Abadie notifications at github.com
Sun Mar 24 15:34:39 CET 2019


aabadie requested changes on this pull request.

Found minor things (see below) and tested this PR on nucleo-l073rz/l476rg. It works.

It would be great if you could add a simple python test script.

> +
+    /* Wait for register to be updated */
+    volatile int timeout = 10000;
+    while(IWDG->SR && timeout){
+        timeout--;
+    }
+
+    /* Refresh wdg counter*/
+    wdg_reset();
+
+    return time_set;
+}
+
+#ifdef __cplusplus
+}
+#endif

missing newline

> +static uint8_t _find_prescaler(uint32_t rst_time){
+    /* Divide by the range to get power of 2 of the prescaler*/
+    uint16_t r = ( rst_time / IWDG_STEP_US );
+    uint8_t pre = 0;
+    DEBUG("[wdg]: range value %d\n", (int) r);
+
+    /* Calculate prescaler */
+    while(r > 0) {
+        r = r >> 1;
+        pre++;
+    }
+    DEBUG("[wdg]: prescaler value %d\n", pre);
+    return pre;
+}
+
+static uint16_t _find_reload_value(uint8_t pre, uint32_t rst_time){

The opening curly brace should on the next line

> +#define IWDG_STEP_US              ((4U)*(LSI_CLOCK_US)*(MAX_RELOAD))
+#define IWDG_MAX_US               ((4U)*(LSI_CLOCK_US)*(MAX_RELOAD) * \
+                                  (1 << MAX_PRESCALER))
+
+#define IWDG_REMANENT             ( RESET_TIME_US / IWDG_STEP_US )
+
+#define IWDG_KR_KEY_RELOAD        ((uint16_t)0xAAAA)
+#define IWDG_KR_KEY_ENABLE        ((uint16_t)0xCCCC)
+
+#define IWDG_UNLOCK               ((uint16_t)0x5555)
+#define IWDG_LOCK                 ((uint16_t)0x0000)
+
+/**
+ * @brief   Macro for calculating WDT timings in us from enum values
+ */
+#define WDG_TIMING(pre, rel) \

Instead of a macro, you can use a `static inline` function.

> + *
+ * 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.
+ */
+
+/**
+ * @ingroup     cpu_stm32_common
+ * @ingroup     drivers_periph_wdg
+ *
+ * @brief
+ *
+ * @{
+ *
+ * @file        wdg.c
+ * @brief       Independent Watchdog timer for STM3L

s/STM3**L**/STM32/ ?

> +    IWDG->PR = prescaler;
+    _iwdg_lock();
+}
+
+static void _set_reload(uint16_t reload)
+{
+    assert(reload <= IWDG_RLR_RL);
+
+    _iwdg_unlock();
+    IWDG->RLR = reload;
+    _iwdg_lock();
+}
+
+static uint8_t _find_prescaler(uint32_t rst_time){
+    /* Divide by the range to get power of 2 of the prescaler*/
+    uint16_t r = ( rst_time / IWDG_STEP_US );

The parenthesis are not needed.

> +
+    _iwdg_unlock();
+    IWDG->PR = prescaler;
+    _iwdg_lock();
+}
+
+static void _set_reload(uint16_t reload)
+{
+    assert(reload <= IWDG_RLR_RL);
+
+    _iwdg_unlock();
+    IWDG->RLR = reload;
+    _iwdg_lock();
+}
+
+static uint8_t _find_prescaler(uint32_t rst_time){

I think this file require some uncrustify.

> @@ -0,0 +1,7 @@
+Expected result
+===============
+If everything is working OK, device should reboot every 2 s,

"**the** device should reboot every 2s"

> @@ -0,0 +1,44 @@
+/*
+ * Copyright (C) 2019 INRIA

s/INRIA/Inria/

> +#include "periph/wdg.h"
+#include "timex.h"
+
+#ifndef WDG_RESET_TIME
+#define WDG_RESET_TIME      (2U*US_PER_SEC)
+#endif
+
+int main(void)
+{
+    printf("RIOT edg test application\n");
+    printf("Application should reset every ~ %d seconds\n", WDG_RESET_TIME);
+
+    wdg_init(WDG_RESET_TIME);
+    wdg_enable();
+
+    while(1){};

space missing before `{}`

> + *
+ *              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 "periph/wdg.h"
+#include "timex.h"
+
+#ifndef WDG_RESET_TIME
+#define WDG_RESET_TIME      (2U*US_PER_SEC)

I would keep 2U here, but move the multiplication with `US_PER_SEC` when calling `wdg_init`. Otherwise, you get this message when the board is booting:
`Application should reset every ~ 2000000 seconds` instead of `Application should reset every ~2 seconds`

> + *
+ * @}
+ */
+
+#include <stdio.h>
+
+#include "periph/wdg.h"
+#include "timex.h"
+
+#ifndef WDG_RESET_TIME
+#define WDG_RESET_TIME      (2U*US_PER_SEC)
+#endif
+
+int main(void)
+{
+    printf("RIOT edg test application\n");

s/edg/wdg/

-- 
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-218084255
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190324/4dd04b14/attachment.html>


More information about the notifications mailing list