Re: hal watchdog

[Mathew Calmer]

I also worry (slightly) that the default time between sanity waking up and the 
watchdog firing may not be enough. It is configurable so that is all good, but 
maybe the default should be a bit longer as sanity is checked in the idle task 
and if a system has lots of tasks the idle task may not run for a bit (although 
200 msecs is a while).

Feels to me that if your idle task is taking longer than 200 msecs to circle, 
you probably _want_ your watchdog kicking in.

200 ms is really long.


> On Sep 22, 2016, at 10:58 PM, Sterling Hughes 
> <sterl...@apache.org><mailto:sterl...@apache.org%3E> wrote:
>
> Hey —
>
> A follow up on this, I’ve committed initial support for the Nordic platforms 
> for the watchdog, along with modifying this API a bit.
>
> I made the watchdog expiry a millisecond value (in hal_watchdog_init()), as 
> pretty much every watchdog I’ve seen executes in millisecond resolution. I 
> removed the hal_watchdog_stop() function, as many processors don’t offer the 
> ability to stop the watchdog once started.
>
> I also hooked the watchdog into the OS with two new configuration options:
>
> SANITY_INTERVAL:
> description: 'The interval at which the sanity checks should run, should be 
> at least 200ms prior to watchdog'
> value: 299500
> WATCHDOG_INTERVAL:
> description: 'The interval at which the watchdog should reset if not tickled, 
> in ms'
> value: 30
>
> These are default values, defined by the OS (libs/os/pkg.yml), that can be 
> overridden by the BSP.
>
> By default the OS initializes the watchdog with hal_watchdog_init() when OS 
> start is called. I have removed the sanity task (to save stack space, and 
> make it run by default), and instead, put all sanity related functions within 
> the idle task.
>
> The logic is here (os.c), for reference. os_sanity_run() calls into 
> os_sanity.c, and runs the sanity checks. Code has also been added to make 
> sure that we don’t sleep beyond the sanity interval, and trip up the watchdog 
> in our idle loop.
>
> sanity_itvl_ticks = (MYNEWT_VAL(SANITY_INTERVAL) * OS_TICKS_PER_SEC) / 1000;
> sanity_last = 0;
>
> hal_watchdog_tickle();
>
> while (1) {
> ++g_os_idle_ctr;
>
> now = os_time_get();
> if (OS_TIME_TICK_GT(now, sanity_last + sanity_itvl_ticks)) {
> os_sanity_run();
> /* Tickle the watchdog after successfully running sanity */
> hal_watchdog_tickle();
> sanity_last = now;
> }
>
> OS_ENTER_CRITICAL(sr);
> now = os_time_get();
> sticks = os_sched_wakeup_ticks(now);
> cticks = os_callout_wakeup_ticks(now);
> iticks = min(sticks, cticks);
> /* Wakeup in time to run sanity as well from the idle context,
> * as the idle task does not schedule itself.
> */
> iticks = min(iticks, ((sanity_last + sanity_itvl_ticks) - now));
>
> if (iticks < MIN_IDLE_TICKS) {
> iticks = 0;
> } else if (iticks > MAX_IDLE_TICKS) {
> iticks = MAX_IDLE_TICKS;
> } else {
> /* NOTHING */
> }
> /* Tell the architecture specific support to put the processor to sleep
> * for 'n' ticks.
> */
> os_tick_idle(iticks);
> OS_EXIT_CRITICAL(sr);
>
>
> For BSPs where hal_watchdog has not been implemented, calling these functions 
> has no effect, and I’ve added stubs to the stm32f4 and native BSPs.
>
> Sterling
>
>
> On 30 Aug 2016, at 15:01, marko kiiskila wrote:
>
>>
>>> On Aug 30, 2016, at 12:59 PM, Mathew Calmer 
>>> <mcal...@exploramednc7.com><mailto:mcal...@exploramednc7.com%3E> wrote:
>>>
>>> On August 30, 2016 at 12:28:50 PM, will sanfilippo 
>>> (wi...@runtime.io<mailto:wi...@runtime.io><mailto:wi...@runtime.io><mailto:wi...@runtime.io%3E>)
>>>  wrote:
>>> Sounds reasonable. As I am sure you know, doing it through the sanity task 
>>> sometimes is an issue getting the time right as you would then need to know 
>>> the worst-case timing of all the tasks that could be running… but any way 
>>> you cut it, you have to put some time limit on that… in past lives I have 
>>> seen some pretty complicated ways to deal with this but this seems 
>>> reasonable and if developers need something different they can implement it 
>>> with this hal.
>>>
>>>
>>>
>>> I would consider making the return value of init() be the time or some 
>>> reference to the time that was actually set. So, for example, if the user 
>>> asks for 1 ticks, and the system can only support 2000, it could return 
>>> 2000 from init, after trying it’s best to support the request.
>>>
>>> It could be done in powers of two or some other mechanism, but conceptually 
>>> using that return value to

Re: hal watchdog

[Mathew Calmer]

On August 30, 2016 at 12:28:50 PM, will sanfilippo 
(wi...@runtime.io) wrote:
Sounds reasonable. As I am sure you know, doing it through the sanity task 
sometimes is an issue getting the time right as you would then need to know the 
worst-case timing of all the tasks that could be running… but any way you cut 
it, you have to put some time limit on that… in past lives I have seen some 
pretty complicated ways to deal with this but this seems reasonable and if 
developers need something different they can implement it with this hal.



I would consider making the return value of init() be the time or some 
reference to the time that was actually set.  So, for example, if the user asks 
for 1 ticks, and the system can only support 2000, it could return 2000 
from init, after trying it’s best to support the request.

It could be done in powers of two or some other mechanism, but conceptually 
using that return value to explain what was actually set would be a nice 
interface.   If watchdog was not implemented on given hardware, default return 
could be negative (error) or 0, implying watchdog was not set (although 0 also 
implies success… so…).