DUE: Fix random watchdog resets, improve watchdog handling, debug helper, tone fix...

[ejtagle]

This PR does several things:

1. Solves some bootloops caused by a watchdog timeout on the initialization of the SD card.
2. Moves HAL_Init and HAL_idletask to HAL_Due.cpp, where they belong. In that way, we can use HAL_Init to initialize the tone generator, the USB stack an all things related to the DUE hal, instead of polluting Marlin.cpp with conditional defines
3. Fixed the disabling of the Watchdog. SAM3X datasheet says the Watchdog starts ENABLED, so , if the user wants to disable it, Marlin must EXPLICITLY do so. We must overrride watchdogSetup() and explicitly disable the watchdog
4. Added WATCHDOG_RESET_MANUAL support for DUE: Instead of direct resetting the part, we can dump to the serial terminal the LOCATION (PC) where the watchdog kicked in. This HELPS A LOT in tracing timeouts and bootloops!!
5. Also made watchdog to be disabled while debugging using an external JTAG adapter.
6. Added handlers for Hardware and software faults that DUMP the location of the code that triggered them to the Serial console on the programming port. Again, this results in an invaluable aid while trying to diagnose crashes and bootloops
7. Watchdog is enabled at startup: To make sure no unintended resets happen during setup phase, the thermalmanager, if not inited, will reset the watchdog as needed to avoid unneccesary resets.
8. Made sure that timers are actually stopped before reprogramming them. This hopefully will fix the tone problems.
9. Finally, tried to improve on SAM3X reset when reflashed through the native port

[thinkyhead]

Looks good! but please watch those commit messages! I've cleaned them up for you this time and squashed commits. We have helpful guidelines for commit messages posted on the Contributing page:

https://github.com/MarlinFirmware/Marlin/blob/1.1.x/.github/contributing.md#git-commit-messages

Basically, keep the summary short. Put the long description after a blank line.

[ejtagle]

@thinkyhead :Sorry about the commit messages.. Yesterday i was too tired and english is not my native language, but i wanted to make this pull request available as soon as it was possible. It fixes bootloops for me mostly.
The complete fix would be to enable watchdog in watchdogSetup() instead of watchdog_init() because the main problem DUE has is unreliable initialization of the Watchdog at reset time. The sooner it is reprogrammed, the better, but i've been told there was a reason to delay the initialization related to UBL. I simply cannot understand that reason.
I have seen also some rare bootloops..When they happen, they are always related to initialization of the SD because it can take several seconds to reinit a SD from an unknown state, and that van trigger the watchdog. But,as i said, are very very rare.
Maybe you could explain why we cannot enable watcgdog at watchdogSetup(). If there is no real reason, i van move the initialization there (it is executed 1 second before setup() and that will solve mostly all our issues related to bootloops

[Bob-the-Kuhn]

♥️ Where have you been all my debugging life? ♥️

[Bob-the-Kuhn]

There should be a blank line on the end of all the files.

I'm getting the following compile errors on PlatformIO:

Marlin\src\HAL\HAL_DUE\usb\udi_msc.c: In function 'udi_msc_enable':
Marlin\src\HAL\HAL_DUE\usb\udi_msc.c:387:2: warning: implicit declaration of function 'UDI_MSC_ENABLE_EXT' [-Wimplicit-
function-declaration]
if (!UDI_MSC_ENABLE_EXT())
^
Marlin\src\HAL\HAL_DUE\usb\udi_msc.c: In function 'udi_msc_disable':
Marlin\src\HAL\HAL_DUE\usb\udi_msc.c:404:2: warning: implicit declaration of function 'UDI_MSC_DISABLE_EXT' [-Wimplicit
-function-declaration]
UDI_MSC_DISABLE_EXT();
^

Linking .pioenvs\DUE USB\firmware.elf
.pioenvs/DUE USB/src/src/HAL/HAL_DUE/usb/udi_msc.c.o: In function `udi_msc_disable':
C:\Users\bobku\Documents\GitHub\Marlin-Bob-2/Marlin\src\HAL\HAL_DUE\usb/udi_msc.c:404: undefined reference to `UDI_MSC_
DISABLE_EXT'
.pioenvs/DUE USB/src/src/HAL/HAL_DUE/usb/udi_msc.c.o: In function `udi_msc_enable':
C:\Users\bobku\Documents\GitHub\Marlin-Bob-2/Marlin\src\HAL\HAL_DUE\usb/udi_msc.c:387: undefined reference to `UDI_MSC_
ENABLE_EXT'

I tried the following change to lines 228-229 in conf_usb.h but there's no host support on the native USB port when the SD card is disabled.

FROM

#define  UDI_MSC_ENABLE_EXT()          usb_task_msc_enable()
#define  UDI_MSC_DISABLE_EXT()         usb_task_msc_disable()

TO

//! Interface callback definition
#if ENABLED(SDSUPPORT)
  #define  UDI_MSC_ENABLE_EXT()          usb_task_msc_enable()
  #define  UDI_MSC_DISABLE_EXT()         usb_task_msc_disable()
#else  
  #define  UDI_MSC_ENABLE_EXT()          false
  #define  UDI_MSC_DISABLE_EXT()         {}
#endif  

[ejtagle]

humm... udi_msc.c should be ifdefd out if no SD support is enabled. PlatformIO is playing tricks on you, probably caching previously compiled versions.

Look at line 60 onwards on udi_msc.c ... There is it the ifdef

[Bob-the-Kuhn]

udi_msc.c isn't in the files Changed list,.

[thinkyhead]

i've been told there was a reason to delay the initialization related to UBL. I simply cannot understand that reason.

There was a large block of serial output during EEPROM startup that caused a long delay. Although the serial output uses safe_delay which normally ensures a watchdog reset, all of this was occurring before the thermalManager ISR (which provides temperature ADC readings) was enabled. The watchdog is only reset if new temperature readings are available.

So, the solution is to initialize the thermalManager and start its ISR before enabling the watchdog.

[ejtagle]

hummm.. reiniting watchdog on an ISR doesnt feel good at all..

[ejtagle]

i am pretty sure a saw a watchdogReset() on the idle() task

[ejtagle]

also, there is a bug ob watchdogReset() .. gcc optimizes it out..:(

[ejtagle]

@Bob-the-Kuhn : the file udi_msc.c was changed by PR #10148. And at line 60 of that file there is a
#if ENABLED(SDSUPPORT) ..

The file is not compiled at all if SDSUPPORT is not defined. The error you are getting is a linker error, and it says the udi_msc.c.o: In function udi_msc_enable': undefined reference to UDI_MSC_ENABLE_EXT'

That means the object code contains a compiled function called udi_msc_enable. That function should be ifdefd out by the #if ENABLED(SDSUPPORT) .. So, i guess that PlatformIO is failing in dependency analysis and is reusing an older compiler module.

[thinkyhead]

hummm.. reiniting watchdog on an ISR doesnt feel good at all..

That's not where it happens. The ISR provides new temperature readings. The watchdog reset happens in the main thread — when new temperature readings become available.

[Bob-the-Kuhn]

udi_msc.c is in my copy of bugfix-2.0.x because of the way I update it. Apparently my process has a hole in it that doesn't account for deleted files.

My fork of Marlin has a default branch of bugfix-2.0.x. Whenever I start a new issue I use the git desktop app to copy the default into a new branch and then I use the following git commands to update it:

git remote -v
git remote add upstream https://github.com/MarlinFirmware/Marlin.git
git fetch upstream
git rebase upstream/bugfix-2.0.x -Xours

-Xours will make the common files the same but apparently doesn't touch any "extra" files.

[Bob-the-Kuhn]

I don't have tracking setup. Off to do some more reading.

[ejtagle]

@thinkyhead : In that case, it is perfectly fine. I think i'd prefer to initialize the watchdog as soon as possible in the boot process. If the problem is the Serial dump taking too long (4 seconds is 250000/10*4 = 100000 characters long !!!!), then the solution is to clear the watchdog at that specific point, in that specific routine.
The Arduino runtime performs several actions between the watchdogSetup() and the execution of setup(). On my machine, a Windows installation, the file is located at

c:\Users\ejtagle\appdata\Local\Arduino15\packages\arduino\hardware\sam\1.6.11\cores\arduino\main.cpp

Note that at the point of watchdogSetup(), no hardware or ports or anything is configured yet. So, it is not advisable to enable any ISR.

[thinkyhead]

Whenever I start a new issue I use the git desktop app to copy the default into a new branch and then I use the following git commands to update it…

If you have a BASH shell you can use the provided git scripts. The command to create a new branch as a copy of bugfix-2.0.x would be mfnew 2 my_feature_work, which does this:

git fetch upstream
git checkout upstream/bugfix-2.0.x -b my_feature_work

Then the command firstpush pushes it to your origin and sets tracking, as in…

git push --set-upstream origin my_feature_work

---

I use the following git commands to update it

If all you want to do is maintain a current copy of bugfix-2.0.x in your fork, then these are also reasonable ways to do it:

git fetch upstream
git branch -D bugfix-2.0.x
git checkout upstream/bugfix-2.0.x -b bugfix-2.0.x
git push -f --set-upstream origin bugfix-2.0.x

…or…

git checkout bugfix-2.0.x
git fetch upstream
git reset --hard upstream/bugfix-2.0.x
git push -f --set-upstream origin bugfix-2.0.x

Of course the --set-upstream to set tracking is only needed when the branch is first created.

I don't tend to use my origin copies of the bugfix-x.x.x branches, since the Marlin git scripts just rebase directly onto the upstream/bugfix-x.x.x refs. But once in a while I like to update my origin copies to keep them fresh.

[thinkyhead]

I think i'd prefer to initialize the watchdog as soon as possible in the boot process

It should really be initialized last. Certainly after thermalManager.init and not too long before entering the main loop, since the main loop is responsible for resetting it. The job of the watchdog in Marlin is to reset the board when sensor data gets so old as to be dangerously unreliable.

[ejtagle]

@thinkyhead : I do understand. The main problem is that we need all the things before thermalManager.init to happen, as changing the initialization order could break anything. But i think i have an idea :) ...
I´ll add a flag to thermalManager to be able to find out if it is already running or not. If it is not, then safe_delay will directly reset the watchdog, otherwise it will run as it is running now.
With that modification, it shouldn´t be a problem to enable the watchdog at the beginning ... 👍

[ejtagle]

And if you are bothered by an extra byte of RAM, i could contribute a bitvector implementation that compacts all those bool variable[count] to something less wasteful in terms of SRAM :)

[thinkyhead]

If it is not, then safe_delay will directly reset the watchdog

Is it not possible to simply disable the watchdog as the very first step? Then before entering the main loop re-enable it? Adding logic to safe_delay that lives for the full runtime just to deal with a bootup condition seems like a waste of electrons.

[ejtagle]

@thinkyhead Unfortunately not. On SAM3X8, the Watchdog config register is write-once. So, once you set an specific configuration you simply can´t change it anymore until the next hardware reset. And this watchdog starts preconfigured as enabled... According to the datasheet, it should start preconfigured with a 16 second timeout.
But, certainly, that is not the case. I can reproduce the bootloops with no problems. And the cause is the watchdog... And initialization is taking way less time than 16 seconds.
Yes, and i tend to agree with you. I was looking for an alternative to adding such variable to detect that condition, but the simplest thing is to add that variable.
There is a lot of improvement opportunities on bool types on the codebase, so, we can easily offset the cost of adding such variable.
I tend to group all bools and declare them inside an anonymous struct using bitfields. On embedded platforms, that allows to use bits for bools, instead of bytes for bools... ;) - Code is usually faster, and SRAM usage is also reduced

[ejtagle]

As said, implemented enabling and configuring of watchdog as the first thing. This, i can confirm, prevents all the bootloops on the SAM3X based boards.
Also tried my own fix for the tone generator. The timers should not be reprogrammed when running.

[Bob-the-Kuhn]

compare register value calculation is wrong

because I didn't do it correctly

[Bob-the-Kuhn]

I totally agree with your changes to tone.cpp with one exception:
FROM
HAL_timer_start(TONE_TIMER_NUM, 2 * frequency);
TO
HAL_timer_start(TONE_TIMER_NUM, VARIANT_MCK / 2 / 2 / frequency); // 84MHz / 2 prescaler / 2 interrupts per cycle /Hz

As the frequency increases the compare value needs to decrease.

I've verified on a logic anaklyzer that this formula gives the correct frequency and duration.

Now if I could figure out (again) how to do a proper review ...

[Bob-the-Kuhn]

You are following my lead - silly person.

In HAL_timers_Due.cpp the following is useless because the counter (CV register) is read only.

+  // Start counter in 0 value 
+  tc->TC_CHANNEL[channel].TC_CV = 0; 

The TC_Start(tc, channel); statement starts the clock to counter and it sets counter to zero via the SWTRG bit in the TC_CCR register.

---

My previous comment is totally wrong.

HAL_timer_set_compare and HAL_timer_start need different methods of calculating the frequency arguments.

---

Please delete the routines HAL_timer_set_count and HAL_timer_restrain_count in the file HAL_timers_Due.h. They don't work and they aren't needed.

[Bob-the-Kuhn]

I've verified duration & frequency are correct via a logic analyzer.

[ejtagle]

Sorry, @Bob-the-Kuhn ... Yesterday i was too tired, and fell asleep... (was 2AM ...I am at GMT-3).
I do agree with all your comments.
I will review the timer code against the datasheet 👍
It is not the calculation of the frequency the problem. I think the problem is to alter the period while the timer is running. That is why I added an TC_Stop() in the HAL_timers.cpp:HAL_timer_start

[ejtagle]

If you take a look at my Tone.cpp file, you will notice we agree 👍 ... I will remove the line
tc->TC_CHANNEL[channel].TC_CV = 0;

[ejtagle]

And all the unused functions from HAL_timers.h

[ejtagle]

BTW, hw reviews are always hard. Dont worry: The usual "I like it/I dislike it/do it other way, this does not make sense"... It´s fine with me 👍

[Bob-the-Kuhn]

TC_Configure stops the clock to the counter so TC_Stop() isn't need. As far as I can tell none of the changes in that file are required but none of them hurt either. Your choice.

[Bob-the-Kuhn]

.platformio\packages\framework-arduinosam\system\sam\libsam\source\tc.c is where all the low level TC routines are at.

[ejtagle]

probably TC_Stop is unneeded. But it is good practice to disable ints in NVIC before changing timer options, or you could end with an spare interrupt as soon as you enable the timer. Most of this is caused by the fact of the reprogramming of a running timer. I'd split HAL_timer_start in 2 HAL_timer_start and HAL_timer_stop.. but, this is not time critical code..
As you wish..:+1:

[ejtagle]

On a 2nd thought, i'd prefer to leave the TC_Stop there. It makes reading the code flow and figuring out how it works more obvious.. ;)

[thinkyhead]

All this added stuff for resetting the watchdog should only be added for the platforms where it matters. Recommend adding to Conditionals_post.h something like this:

#define EARLY_WATCHDOG (ENABLED(USE_WATCHDOG) && defined(ARDUINO_ARCH_SAM))

…and then wrap these additions in #if EARLY_WATCHDOG…#endif.

[ejtagle]

I could do it. I do get your point: "Avoid wasting an extra byte of SRAM on platforms that do not need it"...
Dont believe i am happy with the current solution: It is more like a hack, but HW is forcing us to use it...
I believe that on AVR watchdog can be reprogrammed as needed, so this hack is not needed there. On LPC i am not sure...

[ejtagle]

Somehow i also think disabling the watchdog at the setup phase is also some kind of hack: In my specific case, i am using an Arduino DUE on a modified Ramps1.4
SAM3X8E starts with all IO ports configured as inputs with pull ups. So, it is very important to reconfigure the ports as fast as possible, because otherwise, the pullups turn on the Hotend and bed heaters. Watchdog is an important safety measure at all times, not only at loop() time ;)

[thinkyhead]

Avoid wasting an extra byte of SRAM on platforms that do not need it

Well, it's also a minor matter of the general style that Marlin is coded in. The use of compiler directives to leave out unnecessary code not only keeps the binary slimmer, it also acts as a handy reference to what is really required. We can look at the code and see: ah, this is needed, but that is not needed. The reason why the #include lines at the top of the G-code handler files are wrapped in #if blocks is not just to make the compilation quicker, it also gives us a nice statement at the top of these files about what the real dependencies are. And this gives us some insight into how we might re-organize things as we refactor in the future.

Watchdog is an important safety measure at all times…

Certainly. And if we discover that terrible things can happen in the earliest stages of boot-up on certain MCUs, and we can use the watchdog to prevent those disasters, then we should. But I haven't seen any situations where –for example– all the heaters get stuck on during boot-up, and I'd be very surprised if that starts to occur in spite of Marlin's thorough pin initializations during setup. I'd be doubly-surprised if the watchdog turns out to be the solution to that kind of issue.

[ejtagle]

I agree. Watchdog is never a solution. It is a safeguard against bugs in sw or hw where is it preferable to reset a regain control rather than losing control of the process

[ejtagle]

Hummm... I accidentally replaced the default configuration with my own . I'll revert the configuration.h / configuration_adv.h files to the default ones

[thinkyhead]

Rebased, squashed, combined commit messages. Will merge shortly!

[marcio-ao]

I wanted to check whether the issues in this thread could explain an issue I am seeing. I've written some code that is using SPI to transfer blocks from the SD card to the SPI display controller. I am doing this to play an animated logo at startup. I'm transferring 512 byte blocks and this takes about 2ms. Despite of this, the video freezes randomly and the board resets. This is despite calling either thermalManager.manage_heater(); or watchdog_reset() after each block. I am perplexed, because the failure is intermittent. Sometimes it plays the video just fine, other times it reboots the board.

[ejtagle]

2ms is no problem, but, if transferring from SD, i am pretty confident there could be up to 1 second of delay while reading some SD card sector (SD specification says nothing about "seek" time, and while most of the time reading the next sector takes nearly no time, sometimes the SD controller could take to get the next sector such time.
SAM3x has a problem (or maybe is a board problem) that when DUE starts, the watchdog starts enabled, but the reset period is unknown...

[marcio-ao]

@ejtagle: The odd thing is that the reboots seems to happen when the code is writing the blocks to my display controller, not when reading from the SD card.

By any chance do Marlin interrupts use SPI, and if so, is there a possibility of a deadlock if I were to be accessing SPI at the same time?

[ejtagle]

No, SPI is not using interrupts. At some point i did consider to implement that, but the u8g lib would not allow to use interrupts (or DMA!) for the SPI, unless patching the (u8g) library...

[thinkyhead]

sometimes the SD controller could take to get the next sector such time.

I think it depends on how fast the tiny disk inside the SD card is spinning.

[ejtagle]

@thinkyhead : Not exactly that, but there is an explanation called "wear leveling"... While you read or while you write, there could be extra things going on on the SD controller that is embedded in the SD card. A lot of time ago, i started having problems on SD reads (on an embedded platform i was working on at that time) and after countless hours of debugging, I found out the problem was a timeout while reading "sectors" from the SD card. Increasing the timeout from 10mS to 1S solved the issue.
Inside the SD, there is a controller that emulates a disk by using nand flash.
NAND flash is known for having failed cells (and that makes it possible to manufacture it very cheaply). But, to be able to reliably store information in such a defective cell array requires to use error correcting codes. And error correcting codes are computationally expensive to compute (both for reads when you need to correct read errors) and for writes.
Add to it that the nand flash "write sector" size is normally 2048 bytes, that minimum erase sector size usually is 128kbytes, and that you must perform wear leveling or you risk ruining the cells, and you will understand why the controller could delay a sector write or read for a very long time.
There are expensive cards that use a faster controller, and has less latency. But you never know....

[thinkyhead]

Indeed! That's the TL;DR for my tongue-in-cheek metaphor. I learned more than I ever wanted to know about wear leveling while I was adapting Creality3D's SD-card-based POWER_LOSS_RECOVERY.