Implement watchdog interface

[teamplayer3]

I tried implementing a TWDT interface. Added an example in the watchdog::task module on how I thought of using this implementation. To access the watchdog functionality, I implemented a Task struct in module task.

I don't know if esp-idf-hal is the correct crate to implement it.

My thoughts on the implementation:

• there is one global TWDT which can be accessed by taking it.
• when I take the watchdog, it is initialized with default params (set in sdkconfig)
• to reconfigure them at runtime, reconfigure() can be called
• in a thread Task::current() accesses the current task
• on this task struct, the watchdog can be set/deleted/reset

While implementing, I noticed that the interface of TWDT changed a little from v4.4 to master. So in the future, some adjustments must be done to upgrade.

[ivmarkov]

The overall comment whether this API is safe (and can be made safe in the first place) is the biggest question for me.

[ivmarkov]

Hmmm, if the API is not safe (as I suspect), maybe a safer API would be something where

• The Task structure is completely absent. Instead, you just have a Watchdog thing, that you can always new or take or even clone but cannot Send (how that works under the hood is not so interesting for now).
• The moment you instantiate a Watchdog instance, it is automatically attached to the current task
• Dropping the Watchdog instance (implicitly or explicitly) would detach the current task from it
• Since the Watchdog instance is effectively task-local (it is not Send) the API as a result should be safe, because you WILL drop it (explicitly or implicitly) before the current task is over
• Trying to create two Watchdog instances in the same task/thread should fail, or should use a single watchdog instance (so to say) for that task under the hood

The only question is if you have the use case where you need to "feed" the watchdog for "another" task which is not your current task. But even with the existing approach, I don't think this is possible, or if it is possible, this is so by accident.

[ivmarkov]

Of course we still have core::mem::forget which also makes my API unsafe, but at least it is not "easily" unsafe?

[teamplayer3]

The Task structure is completely absent. Instead, you just have a Watchdog thing, that you can always new or take or even clone but cannot Send (how that works under the hood is not so interesting for now).

I thought of a single watchdog instance to configure it. Else, where does the configuration happen to make it clear the global watchdog is configured?

Since the Watchdog instance is effectively task-local (it is not Send) the API as a result should be safe, because you WILL drop it (explicitly or implicitly) before the current task is over

That's a good point. I tested it without removing the task from the watchdog, and it crashed.

I thought on this a second time, could there be a use case where you don't want to unsubscribe from the watchdog when the thread terminates and want the watchdog behavior to be triggered? But I don't think so.

The only question is if you have the use case where you need to "feed" the watchdog for "another" task which is not your current task. But even with the existing approach, I don't think this is possible, or if it is possible, this is so by accident.

It isn't possible to feed the watchdog of a specific task. esp-idf impl

core::mem::forget

I think using this you can break most of the apis when it depends on dropping at the end.

Concept

// send, sync, clone
struct WatchdogManager;

let twdt = peripherals.TWDT;
let mut wd_manager = watchdog::task::WatchdogManager::new(twdt); // init configure
let mut wd_manager = watchdog::task::WatchdogManager::new_configure(twdt, Config {});
wd_manager.reconfigure(Config {});

{
    let watchdog: impl Watchdog = wd_manager.subscribe_current();

    watchdog.feed();

    drop(watchdog);
}

[ivmarkov]

The Task structure is completely absent. Instead, you just have a Watchdog thing, that you can always new or take or even clone but cannot Send (how that works under the hood is not so interesting for now).

I thought of a single watchdog instance to configure it. Else, where does the configuration happen to make it clear the global watchdog is configured?

That's OK and can still be modeled, as per your concept below.

Since the Watchdog instance is effectively task-local (it is not Send) the API as a result should be safe, because you WILL drop it (explicitly or implicitly) before the current task is over

That's a good point. I tested it without removing the task from the watchdog, and it crashed.

I thought on this a second time, could there be a use case where you don't want to unsubscribe from the watchdog when the thread terminates and want the watchdog behavior to be triggered? But I don't think so.

But when the thread terminates and you are still subscribed, your program will crash (just as you confirmed above), because TaskHandle_t is in fact a pointer, which - after the task it models had quit - is invalid and should not be used.

The only question is if you have the use case where you need to "feed" the watchdog for "another" task which is not your current task. But even with the existing approach, I don't think this is possible, or if it is possible, this is so by accident.

It isn't possible to feed the watchdog of a specific task. esp-idf impl

Indeed. However, it is possible to add a task to the watchdog from another thread (esp_task_wdt_add). Another question how useful that is:

esp_err_t esp_task_wdt_add(TaskHandle_t task_handle)

I think using this you can break most of the apis when it depends on dropping at the end.

Sure, but you shouldn't crash though. Regardless, this is the best we can do anyway with the existing ESP IDF C APIs which do not support Arc and weak refs, obviously.

Concept

// send, sync, clone
struct WatchdogManager;

For symmetry, WatchdogManager should perhaps be named TaskWatchdogDriver or just WatchdogDriver or TWDTDriver or WDTDriver

let twdt = peripherals.TWDT;

Obviously, TWDT should be lower case and either twdt or wdt or watchdog. The peripheral interface should be Twdt or Wdt or Watchdog, and finally, the (only) peripheral impl should be TWDT, WDT or WATCHDOG.

let mut wd_manager = watchdog::task::WatchdogManager::new(twdt); // init configure

Yes. Taking twdt by move, or by &mut ref. Check Perihperal and PerihperalRef.

let mut wd_manager = watchdog::task::WatchdogManager::new_configure(twdt, Config {});

Actually, new should always take a Config instance (by ref), just like all the other drivers do. We aim at symmetry here. The Config can have a const new() and implement Default.

wd_manager.reconfigure(Config {});

Optional but possible, IMO.

{
let watchdog: impl Watchdog = wd_manager.subscribe_current();

watchdog should be a struct, we don't need to implement an "interface" (trait). Once a trait appears in e-hal, we'll implement it on top of the struct.

The struct can be named as a variation of the TaskWatchdogDriver from above, with or without a "Driver" suffix. Perhaps WatchdogSubscription or WatchdogActivation or something. Note that it needs to be lifetimed by 'a, where 'a comes from TaskWatchdogDriver { pub fn subscribe_current<'a>(&'a self) -> WatchdogSubscription<'a>. Alternatively, you can do it under the hood with refcounting based on AtomicUsize and make sure, that you call esp_task_wdt_deinit only when all WatchdogSubscription instances are dropped, and the TaskWatchdogTimer as well. To avoid dependencies on Arc (and thus on the alloc module), and because your TWDT peripheral (and by extension - its *Driver) is a singleton, you can just use a static AtomicUsize throughout the code.

watchdog.feed();

drop(watchdog);

}

But overall, yes - the concept should work.

[teamplayer3]

Now I implemented my concept following the naming conventions.

[teamplayer3]

let mut another_driver = driver.to_owned();
let mut watchdog = driver.watch_current_task()?;
let mut watchdog_2 = another_driver.watch_current_task()?;

Currently, this will fail with ESP_ERR_INVALID_ARG what means current task is already subscribed.

Solutions:

• Option 1: Return Result<Option<WatchdogSubscription<'_>>, EspError> from watch_current_task() -> None if current task is already subscribed
• Option 2: ignore error in watch_current_task() resulting from task already subscribed and in Drop of WatchdogSubscription resulting from current task is not subscribed at second drop

[ivmarkov]

let mut another_driver = driver.to_owned();
let mut watchdog = driver.watch_current_task()?;
let mut watchdog_2 = another_driver.watch_current_task()?;

Currently, this will fail with ESP_ERR_INVALID_ARG what means current task is already subscribed.

Solutions:

• Option 1: Return Result<Option<WatchdogSubscription<'_>>, EspError> from watch_current_task() -> None if current task is already subscribed
• Option 2: ignore error in watch_current_task() resulting from task already subscribed and in Drop of WatchdogSubscription resulting from current task is not subscribed at second drop

Option 2 is not possible to implement if you think about it for a while.

You should do Option 1, but we already discussed once that Result<Option<...>> is pointles. Please simply return `Result<WatchdogSubmission<'_>, EspError> as you do now. It is just that we have to be sure, that only one subscription to a task succeeds.

[ivmarkov]

Final set of primarily stylistic changes prior to merge.

[ivmarkov]

@teamplayer3 Still failing for the esp32c3.

[teamplayer3]

@ivmarkov found the mistake, please rerun.

[teamplayer3]

@ivmarkov What should I do with get_idle_task()? It is only used by watchdog, but a nice to have function as well?

• move it into watchdog
• make it public
• add a #![cfg(<same as watchdog>)]

[ivmarkov]

@ivmarkov What should I do with get_idle_task()? It is only used by watchdog, but a nice to have function as well?

• make it public

^^^ Make it public.

[teamplayer3]

Hopefully the last time!

[teamplayer3]

How can I fix the current error in xtensa-esp32s3-espidf, release/v4.4?

[ivmarkov]

No idea what is going on with the CI. It seems to be a CI problem, yet it is failing only with this PR. Let me merge and see if the main CI would build.