Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers/wdt: add periph_wdt_auto_start for early watchdog #18257

Merged
merged 2 commits into from
Feb 14, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Jun 24, 2022

Contribution description

Add an option to enable the watchdog early to detect hangs during initialization.

Testing procedure

Add USEMODULE += auto_init_wdt_thread or USEMODULE += auto_init_wdt_event to your application.

This will automatically spawn a watchdog thread that runs with the lowest priority.

Issues/PRs references

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Jun 24, 2022
@benpicco benpicco requested a review from maribu June 24, 2022 12:14
@github-actions github-actions bot added Area: build system Area: Build system Area: sys Area: System labels Jun 24, 2022
Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like this. But I would like a word if warning in the doc that this is not exactly best practice on how to implement a watchdog. Typically, one would want to check in the application has made some "progress" since the last time the watchdog was fed before feeding it again.

But I doubt that this can be done in a generalized way without knowing the application and this would still be better than no watchdog at all. So this could be a quick tool to improve robustness of a firmware a bit with no effort spent.

sys/auto_init/wdt/wdt.c Outdated Show resolved Hide resolved
/ 2;
while (1) {
ztimer_sleep(ZTIMER_MSEC, sleep_ms);
wdt_kick();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could have some trivial cheap plausibility check here. E.g. if all threads are waiting for a mutex hold by another thread, the application is in a deadlock but this thread would still happily feed the watchdog. But I don't really know how to tell a thread waiting on an IRQ to unlock a mutex from a thread waiting on another thread apart.

Add an option to enable the watchdog early to detect hangs during
initialisation.
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 17, 2023
@riot-ci
Copy link

riot-ci commented Jan 17, 2023

Murdock results

✔️ PASSED

80fe4e5 sys/auto_init: add auto_init_wdt_{event, thread} modules

Success Failures Total Runtime
6865 0 6865 12m:16s

Artifacts

@maribu
Copy link
Member

maribu commented Feb 13, 2023

lgtm. To me, auto_init_wdt_event and auto_init_wdt_thread are mutually exclusive. Maybe the build system should catch a config where both are enabled?

@benpicco
Copy link
Contributor Author

Good point, added.

@benpicco
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Feb 13, 2023
18257: drivers/wdt: add periph_wdt_auto_start for early watchdog r=benpicco a=benpicco



19272: gcoap: Do not send responses from multicast addresses r=chrysn a=chrysn

### Contribution description

Since #18026, CoAP requests to multicast addresses (eg. `ff02::1`) came back from that exact address, which Linux rightfully just drops.

The fix uses the existing multicast check from #17978 (thanks `@benpicco` for making me write this as dedicated function, I just had to generalize it removing one struct layer), and foregoes setting the source address when responding to multicasts.

### Testing procedure

* Run the gcoap example
* Send a CoAP request to a multicast address RIOT listens to, eg. `./aiocoap-client coap:https://'[ff02::1%tapbr0]'/.well-known/core --non`

Before, this got no response (while you see it arrive on wireshark). After, you get a correct response with two lines of note:

```
WARNING:coap:Sending request to multicast via unicast request method
Response arrived from different address; base URI is coap:https://[fe80::3c63:beff:fe85:ca96%tapbr0]/.well-known/core
```

(The former is aiocoap telling us that we're not using the nonexistent multicast API so it's really more of an anycast, the latter is useful factual information).

Co-authored-by: Benjamin Valentin <[email protected]>
Co-authored-by: chrysn <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 13, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@bors
Copy link
Contributor

bors bot commented Feb 14, 2023

Build succeeded:

@bors bors bot merged commit 1c55118 into RIOT-OS:master Feb 14, 2023
@benpicco benpicco deleted the periph_wdt_auto_start branch February 14, 2023 00:29
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: drivers Area: Device drivers Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants