ESP32: Configurable WDT #3929

andcss93 · 2018-07-06T10:39:08Z

Hi,
this is my first PR so i hope i made it in the right way :).
I began to study a few days ago what is under micropython, i still have several doubts so i hope i have implemented everything in the right way.
This fix allow to configure WDT with their parameters (id, timeout and panic) so now is possible to initialize WDT as follows:

machine.WDT(1, 10, True)

if id 1 is specified, the default WDT will be updated with the new configurations.
I have also ignored some Visual Studio Code files.

nickovs

I'm not sure that using the id to determine if the other configuration options are used is the right move. The id parameter is there because on some platforms you have to specify which timer to use as the watchdog timer but on the ESP32 there's a dedicated hardware block for the watchdog so this is unnecessary. Being able to set the timeout and panic status are certainly useful though; I think that a better option might be to set these up as keyword arguments rather than positional.

nickovs · 2018-07-06T17:23:13Z

ports/esp32/machine_wdt.c

    if (n_args > 0) {
        id = mp_obj_get_int(args[0]);
+        timeout = mp_obj_get_int(args[1]);
    }


I don't see any place where you read the value of args[2]. It looks like this will always panic on timeout if you use id==1.

nickovs · 2018-07-06T17:26:59Z

ports/esp32/machine_wdt.c

+
+        case 1: {
+            mp_int_t rs_code = esp_task_wdt_init(timeout, panic);
+            if(rs_code != ESP_OK) mp_raise_OSError(rs_code);


According to the coding conventions even blocks of code with a single command should use braces and a separate line. Also a space between if and (rs_code would be good.

andcss93 · 2018-07-07T17:52:32Z

I removed the id parameter and added support for the keyword arguments but the coverage decreased. How do i fix it?

nickovs · 2018-07-07T18:06:19Z

The coveralls coverage metrics are unreliable; it doesn't even run checks on the code in the ESP32 port so I wouldn't worry about it.

nickovs · 2018-07-07T18:09:08Z

ports/esp32/machine_wdt.c

+    mp_map_init_fixed_table(&kw_args, n_kw, args + n_args);
+    mp_arg_parse_all(n_args, args, &kw_args, MP_ARRAY_SIZE(allowed_args), allowed_args, mp_args);
+
+    if (mp_args[ARG_timeout].u_int == 0) {


I think it might be easier and less code if you simply put sensible defaults for the keyword arguments and then unconditionally called esp_task_wdt_init() with whatever values you get from the keywords.

I took as an example the file machine_uart.c to understand how to take the keyword arguments. What specifically does the mp_map_init_fixed_table() method do?

I'd tried to pin my comment on line 63; I'm not sure why it came out above.

What I was meaning is that if you set the default value for timeout to something other than 0 (as discussed below) then you can skip the whole of the True side of the if (mp_args[ARG_timeout].u_int == 0) statement and go strait to the code that is currently in the else side.

nickovs · 2018-07-07T18:10:42Z

ports/esp32/machine_wdt.c

-    }
+    static const mp_arg_t allowed_args[] = {
+        { MP_QSTR_timeout, MP_ARG_INT, {.u_int = 0} },
+        { MP_QSTR_panic, MP_ARG_BOOL, {.u_bool = true} }


Is the current default behaviour to panic or to reset? As far as I can tell it currently resets so you should probably keep that behaviour. Doing a panic is less useful when running MicroPython.

Hi @nickovs,
thanks for the help!
Reset is the current default behaviour so i keep it as you suggest. I have also changed default timeout from 0 to 10s to avoid an immediately reset on WDT constructor call if no timeout arg is passed.

nickovs · 2018-07-08T13:42:23Z

Thanks @dreasGh, that looks good to me, although I'm not a maintainer (just an interested party who's been using the watchdog timer on the ESP32 recently) so my say has limited weight.

I guess my only remaining concern is if for production purposes it is really necessary to have the call to ESP_LOGI() at the end, since this takes up valuable bytes in the resulting image. I think you should leave it to The Powers that Be to decide if they want to remove that line before they merge.

Anyway, thanks for submitting this. I will use it myself if/when it makes it into the master branch.

andcss93 · 2018-07-08T14:43:18Z

@nickovs thank you very much for your help.
I will put ESP_LOGI() at the end but just for information, why ESP_LOGI() takes up valuable bytes in the resulting image if not placed at the end?
I hope maintainers merge this pull request asap :)

nickovs · 2018-07-08T14:59:47Z

@dreasGh Sorry if I was not clear. My suggestion was that the logging line should be removed altogether since it is probably not needed in production code. It takes up space for the instructions and the strings wherever you put it.

dpgeorge · 2018-07-09T05:39:47Z

ports/esp32/machine_wdt.c

@@ -42,20 +43,30 @@ typedef struct _machine_wdt_obj_t {
 STATIC machine_wdt_obj_t wdt_default = {{&machine_wdt_type}};

 STATIC mp_obj_t machine_wdt_make_new(const mp_obj_type_t *type_in, size_t n_args, size_t n_kw, const mp_obj_t *args) {
-    mp_arg_check_num(n_args, n_kw, 0, 1, false);
+    mp_arg_check_num(n_args, n_kw, 0, 2, true);


This is not needed if using mp_arg_parse_all

dpgeorge · 2018-07-09T05:40:54Z

ports/esp32/machine_wdt.c

-        id = mp_obj_get_int(args[0]);
-    }
+    static const mp_arg_t allowed_args[] = {
+        { MP_QSTR_timeout, MP_ARG_INT, {.u_int = 10} },


You'll need to add a line for the id. Check how ports/stm32/wdt.c does it.

Also, the default timeout should be 5 seconds to retain the original behaviour.

What is the role of the id? The IDF documentation states that the function esp_task_wdt_init() only has timeout and panic parameters.
Thanks for your patience.

It's so that it matches with other ports. In some cases there may be multiple WDT's and the id will select which one. It is an optional parameter so you can still do something like machine.WDT(timeout=5000).

dpgeorge · 2018-07-09T05:46:34Z

Thanks for the contribution. It's definitely important to be able to set the timeout of the WDT. But note that the timeout should be given in milliseconds, following the stm32 and cc3200 ports.

As for the panic argument: IMO this isn't really generalisable to other ports. A WDT should always reset the system if it times out otherwise there's not much point in having it! What is the panic option used for?

As for the additions to .gitignore: usually such changes would be made in your own global git configuration, and then you can easily support any IDE or whatever you like. So I'd prefer not to merge those changes.

…) and timeout managed in millis (5000 as default)

andcss93 · 2018-07-23T20:48:46Z

I added the id parameter, removed the panic (true as default value) to keep the default behavior and i managed the timeout in milliseconds (with 5000 as default value).
I hope I have done everything right.

tahouse · 2018-08-23T00:28:25Z

Hi all, is there a plan to merge this to master soon? Is this waiting for appropriate approval or am I missing something in the history?

andcss93 · 2018-09-25T07:59:54Z

Hi, news?

hmaerki · 2018-12-01T20:32:46Z

Hi
I would be pleased if you could pull this feature!

stanleypa · 2019-04-18T11:26:20Z

I'm using the WDT part and it works very well - would be great if that was promoted to master

junhuanchen · 2019-04-21T09:16:06Z

The simple fixes are as follows. @dreasGh

junhuanchen@0a7cfbd

dpgeorge · 2019-04-30T06:57:00Z

Sorry for losing track of this. I've merged it (the WDT improvements) in fbd4e61, with some changes to the code layout.

Note: the OTA addition and other changes here were not merged.

ulab: update to 1.6.0

andcss93 added 2 commits July 6, 2018 12:06

esp32: WDT id, timeout and panic flag now they are configurable

6585d65

visual studio code files ingored

866222c

andcss93 changed the title ~~Configurable WDT~~ ESP32: Configurable WDT Jul 6, 2018

nickovs reviewed Jul 6, 2018

View reviewed changes

esp32: WDT constructor now accept keyword arguments

64881b9

nickovs reviewed Jul 7, 2018

View reviewed changes

esp_task_wdt_init() now called unconditionally

3a072e2

esp32: ESP_LOGI() removed from WDT make_new

45dacac

dpgeorge reviewed Jul 9, 2018

View reviewed changes

esp32: WDT id param added (for ports), panic removed (true as default…

a5a5965

…) and timeout managed in millis (5000 as default)

andcss93 added 4 commits January 18, 2019 12:52

esp OTA update support added

817b1d4

connection retry skipped

b375e94

frozen boot.py and main.py executed instead of regular files

62c4f80

internalfs now occupies all the remaining space

f998a2f

dpgeorge closed this Apr 30, 2019

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 21, 2021

Merge pull request micropython#3929 from jepler/update-ulab3

f6f1ef7

ulab: update to 1.6.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESP32: Configurable WDT #3929

ESP32: Configurable WDT #3929

andcss93 commented Jul 6, 2018 •

edited

Loading

nickovs left a comment

nickovs Jul 6, 2018

nickovs Jul 6, 2018

andcss93 commented Jul 7, 2018

nickovs commented Jul 7, 2018

nickovs Jul 7, 2018

andcss93 Jul 7, 2018

nickovs Jul 8, 2018

nickovs Jul 7, 2018

andcss93 Jul 7, 2018 •

edited

Loading

nickovs commented Jul 8, 2018

andcss93 commented Jul 8, 2018

nickovs commented Jul 8, 2018

dpgeorge Jul 9, 2018

dpgeorge Jul 9, 2018

andcss93 Jul 10, 2018

dpgeorge Jul 10, 2018

dpgeorge commented Jul 9, 2018

andcss93 commented Jul 23, 2018

tahouse commented Aug 23, 2018

andcss93 commented Sep 25, 2018

hmaerki commented Dec 1, 2018

stanleypa commented Apr 18, 2019

junhuanchen commented Apr 21, 2019 •

edited

Loading

dpgeorge commented Apr 30, 2019

ESP32: Configurable WDT #3929

ESP32: Configurable WDT #3929

Conversation

andcss93 commented Jul 6, 2018 • edited Loading

nickovs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andcss93 commented Jul 7, 2018

nickovs commented Jul 7, 2018

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

andcss93 Jul 7, 2018 • edited Loading

Choose a reason for hiding this comment

nickovs commented Jul 8, 2018

andcss93 commented Jul 8, 2018

nickovs commented Jul 8, 2018

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

dpgeorge commented Jul 9, 2018

andcss93 commented Jul 23, 2018

tahouse commented Aug 23, 2018

andcss93 commented Sep 25, 2018

hmaerki commented Dec 1, 2018

stanleypa commented Apr 18, 2019

junhuanchen commented Apr 21, 2019 • edited Loading

dpgeorge commented Apr 30, 2019

andcss93 commented Jul 6, 2018 •

edited

Loading

andcss93 Jul 7, 2018 •

edited

Loading

junhuanchen commented Apr 21, 2019 •

edited

Loading