h2zero / n-able-Arduino

An arduino core for ARM based BLE devices supported by the NimBLE stack.
GNU Lesser General Public License v2.1
34 stars 12 forks source link

Default CONFIG_WDT_TIMEOUT_SECONDS to zero (disabled) #39

Closed jhmaloney closed 4 months ago

jhmaloney commented 4 months ago

This change prevents beginners from being taken by surprise by WDT restarts if they are not resetting the watchdog timer and did not explicitly set CONFIG_WDT_TIMEOUT_SECONDS to zero to disable it. The bit me when I was getting started and, from the issues, it has also bitten others. (FYI, I'm using Platformio to build.)

jhmaloney commented 4 months ago

This PR also includes a fix to avoid compile errors when using the pulse() function on nRF52 processors. (Issue #33.) I have tested it on the micro:bit V2 and it works.

Apologies for combining two fixes into a single PR; I meant to create two separate PR's but Github combined them.

h2zero commented 4 months ago

Thank you for this, looks good and no issue with the extra commit, I'll rebase merge anyway. I'm still curious about the watchdog, what is it that you are doing that takes more than 5 seconds before the loop iterates?

jhmaloney commented 4 months ago

Many thanks!

My problem with the watchdog was that I didn't know about it so my code was not explicitly resetting it. That was causes a restart every five seconds. What I didn't know DID hurt me!

MicroBlocks is an interpreter, so it once it enters the interpreter loop it never returns control to the Arduino loop() function. However, it does not appear that returning control to the loop() function would have reset the watchdog; doing that appears to be the responsibility of the programmer. However, a beginner just getting started, especially one used to writing bare-metal Arduino programs, might not know to that they needed that (I didn't). That's especially true if they (ahem!) don't read all the documentation carefully before they plunge in.

It sounds as though this surprised at least one other programmer who was just getting started with n-able-arduino. So it seems more beginner-friendly to leave the WDT disabled by default. That way, a beginner can learn about it and enable the feature only if when they actually need it.

h2zero commented 4 months ago

Thanks, I just never expected this use case. The loop is just a task that expects the call to loop to return and that resets the watchdogs since your use case and also another users, doesn't seem to do this is the cause for the reset.

The reason for enabling it was because it was common in the other cores to have an endless loop with no feedback as to why the device seems to hang, which is frustrating to debug without hardware debugging.

jhmaloney commented 4 months ago

Ahh, that makes sense!

Just out of curiosity, I see that vApplicationIdleHook() resets the watchdog, but I haven't figured out how that gets called if the user's code doesn't call it. It is not called directly from loopTask() in main.cpp, as I would have expected. I'm probably just not looking in the right place...

h2zero commented 4 months ago

That's called when there are no other tasks ready to run, thus the name. The idea is that the application should yield to let other tasks run every now and then and the watchdog lets you know when you're hogging 🙂.

jhmaloney commented 4 months ago

Got it!