[Due] Watchdog timer improvement

[bob cousins]

Hi all,

Currently the watchdog timer on Due is disabled in variant.cpp init(). On the SAM3X8E, once the watchdog is disabled it cannot be re-enabled (and vice versa). Therefore users can't use the watchdog function without modifying variant.cpp.

User gogol has a proposal to fix this by introducing a weak function which defaults to disabling the watchdog, keeping existing behaviour, but can be overridden by user code. http://forum.arduino.cc/index.php?topic=233175.0

This seems like a good proposal to me, so I would like to invite comments and see about how to get something included into future Arduino releases.

Bob

[David Cuartielles]

​this sounds good to me ...

are you ready to make a pull request for this to be tested out with the current toolchain?

/d

---

De: bob cousins <bobcou...@googlemail.com>
Enviado: jueves, 17 de julio de 2014 20:01
Para: devel...@arduino.cc
Asunto: [Developers] [Due] Watchdog timer improvement

 

--
You received this message because you are subscribed to the Google Groups "Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.

[Cristian Maglie]

Hi Bob,

In data giovedì 17 luglio 2014 20:01:16, bob cousins ha scritto:
> User gogol has a proposal to fix this by introducing a weak function which
> defaults to disabling the watchdog, keeping existing behaviour, but can be
> overridden by user code. http://forum.arduino.cc/index.php?topic=233175.0

that was exactly how I imagine to make the watchdog available some time ago.
In particular I'd like to have the weak function defined here:

https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/cores/arduino/hooks.c

with a proper comment and together with the other hooks.

If you or someone else would like to make a pull request I'll be happy to
review and merge it. Otherwise I'll try to find a bit of time and make it in
the next days.

C

[bob cousins]

Hi,

I am working on a patch now.

Putting the wdt_initialize() functions in hooks.c would match the other hooks, but it means that the user must supply a C function to override it. I think it would be better for the user experience if it had a C++ template?

So I am not sure where this function and other functions wdt_enable() , wdt_disable(), wdt_reset() should go. As the implementation is specific to SAM series, perhaps in a new file sam_wdt.cpp.

I hope to get a pull request together later, and I can amend as necessary. For the moment I am putting the functions into variant.cpp.

Regards

[bob cousins]

I have created a pull request https://github.com/arduino/Arduino/pull/2210. As this is my first pull request to the Arduino project, I am quite happy to receive advice and guidance :)

I have created some test cases, which all pass successfully, I am not sure where to post them so attached here.

[Cristian Maglie]

In data domenica 27 luglio 2014 14:20:40, bob cousins ha scritto:
> I have created a pull request https://github.com/arduino/Arduino/pull/2210.
> As this is my first pull request to the Arduino project, I am quite happy
> to receive advice and guidance :)

Hey Bob,
great work, looks very nice!

I guess we should move everything out of variants and put directly into the
core in a specific file watchdog.c and call wdt_initialize just before init()
here:
https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/cores/arduino/main.cpp#L43

I would like also to rename the method as:

wdt_initialize -> watchdogSetup
wdt_enable -> watchdogEnable
wdt_disable -> watchdogDisable
wdt_reset -> watchdogReset

to better match Arduino API style.

And finally I'll get rid of the constants WDTO_1S, WDTO_2S, etc. and implement
a function that accept an uint32_t timeout in millis and do the calculation of
the constant to use in the watchdog register, is that possible?

C

[bob cousins]

Ok, I have done that, I will amend the pull request later.

The only thing I would offer is that there are some options for the SAM watchdog, e.g. whether to reset the CPU or generate an interrupt (AVR has similar options), which we could present to the user.

However, because precise options are dependent on CPU, I think it would be best to keep it simple. If necessary, the user can still call WDT_Enable() directly in watchdogSetup() to enable any specific options that are required.

I mention this, because it might be awkward to extend the API later, but I think the current proposal meets most users needs?

[David Cuartielles]

​How would you enable specific options, with flags in the enable call?

/d

---

De: bob cousins <bobcou...@googlemail.com>
Enviado: jueves, 31 de julio de 2014 23:51
Para: devel...@arduino.cc
Asunto: Re: [Developers] [Due] Watchdog timer improvement

 

[Cristian Maglie]

Hi Bob,

> The only thing I would offer is that there are some options for the SAM
> watchdog, e.g. whether to reset the CPU or generate an interrupt (AVR has
> similar options), which we could present to the user.

That's interesting but, as you said, it's very CPU-specific and needs some
preliminary discussion here (beware: this kind of API-discussion may easily
"explode" and take weeks to complete).

> I mention this, because it might be awkward to extend the API later, but I
> think the current proposal meets most users needs?

Issues comes when an API is changed in an incompatible way, while adding
functionality is generally not a problem. IMHO In this case we already cover
the vast majority of use cases and we don't need to add other kind of support.

C

[bob cousins]

On Friday, August 1, 2014 9:19:41 AM UTC+1, david.cuartielles wrote:

​How would you enable specific options, with flags in the enable call?

Yes, something like watchdogEnable (uint32_t timeout, uint32_t options);

with # defines something like

#define WD_OPT_CPU_RESET  0x01
#define WD_OPT_INTERRUPT    0x02

Normally, I would say extended functions could be easily added later, but with the SAM watchdog the user only gets the chance to set it once. So a call like watchdogEnableInterrupt() would not work, if the watchdog has already been enabled.

We could add a new function later called watchdogEnableExt (...) to extend functionality without breaking existing code, it's a bit untidy having multiple functions.

I am happy to go with the consensus, my main goal is just to get the basic watchdog function enabled.