[Panic] add user configurable reboot delay in seconds (IDFGH-8506)

[chipweinberger]

Issue: #9462

Adds a CONFIG_ESP_SYSTEM_PANIC_REBOOT_DELAY_SECONDS option to the panic handler.

Very simple change & tested on a ESP32-S3, and ESP32-S2.

Why?

• "print & reboot" is annoying. Boot loops log way too much, forever & ever. There's no time to look at the stack trace. I always need to unplug the device, and then scroll back and look for the point it crashed.
• "print & halt" is not good for production or beta testing. A device that does not reboot is not useable.

This middle approach is the best of both, especially for beta testing, and is sorely needed! =)

[Alvin1Zhang]

Thanks for your contribution.

[chipweinberger]

I have been testing this change for a ~week without issue. Very nice quality of life change =)

[0xjakob]

@chipweinberger We will discuss this PR tomorrow morning, then let you know about the result.

[chipweinberger]

Any word?

This feature is awesome! If I do say so myself 😜

[chipweinberger]

@0xjakob bump.

I've been using this myself for weeks now. Users will really appreciate the option.

[chipweinberger]

@igrr Just hoping for an update. positive or negative, just want to house-clean.

[0xjakob]

Hi @chipweinberger, sorry for the huge delay! Part of the reason is that we had some discussion going on internally about this. Let me explain:

One issue is that we're not sure if it's not a tooling problem. E.g., on the command line using idf.py monitor, you can always press Ctrl+] to stop the output (if not, let us know, it's probably a bug). Using tools to interrupt the output, or alternatively log to a log file, wouldn't require any additional configuration in IDF. What tool are you using to monitor output from devices?

Another minor risk is that something goes wrong inside the code where the WDT is disabled. It's a minor risk but might still be an issue for people who have devices in beta-phase. Maybe adding a brief explanation in the Kconfig option would be nice. Alternatively, changing the WDT to match the delay might be possible, but I'm not familiar with WDT.

Another option would be to wrap panic_restart(), but that would still be on the user side. In case you want to try, you can use the wrappers example.

So far, we haven't reached a final conclusion what is the best solution.

[0xjakob]

Code in general looks good to me. Just one very minor note about coding style.

[chipweinberger]

Thanks for the update!

What tool are you using to monitor output from devices?

VSCode. AFAIK, there is no way to "pause" the VSCode console after a crash. I always need to pull the cable.

you can always press Ctrl+] to stop the output

Even still, by the time I've noticed the crash my device will have logged 1000 lines immediately on boot, so it's quite a bit of annoying scrolling backwards...

Another minor risk is that something goes wrong inside the code where the WDT is disabled.

disable_all_wdts(); is already called in the panic handler code, so this should not add additional risk.

I don't see the reason not to add it, given that it is just an "option", with no obvious downside AFAICT. I've really enjoyed the past month of using it. But of course, it is your decision ultimately.

[chipweinberger]

Not sure why precommit just failed... was working before.

"fatal: could not read Username for 'https://gitlab.com/': No such device or address"

[chipweinberger]

Not sure how to fix precommit. All I did was fix a bracket.

fatal: could not read Username for 'https://gitlab.com/': No such device or address

[0xjakob]

Not sure how to fix precommit. All I did was fix a bracket.

fatal: could not read Username for 'https://gitlab.com/': No such device or address

Sorry for the inconvenience, this is an issue with a mirror of a third party project. The fix has been merged already on our internal gitlab server, but needs to run the pipeline on master to sync to github. With the next sync, it should be available here, please rebase then.

[chipweinberger]

updated.

Please decide to merge or deny! Don't want too many open PRs 😃 thanks @0xjakob !

[0xjakob]

sha=71a8410547c4718393cc4dee354bf7d99cb0b2fa

[0xjakob]

Hi @chipweinberger,

we finally discussed this PR internally and found a couple of unsolved questions that we would like to address before we merge it. Feel free to resolve them yourself, otherwise we would like to add another commit addressing these.

First of all, the concern is that disabling the watchdog timers for such a long period is quite risky. It would be better to adjust the timeout of the watchdog according to the reboot delay. We furthermore would like to test somehow that the configuration works, probably inside the test applications. Also, would you be OK to adjust the code so that there is no continuous reboot print anymore but just a one-time print, like "Waiting %d seconds before reboot"?

[chipweinberger]

That sounds OK to me.

It is great if you make the adjustments you like.

[0xjakob]

@chipweinberger We merged the feature. It's available on master now.

[chipweinberger]

I noticed! looks great. thanks for you work getting it in.