uavcan: prevent system crash on stm32h7 when restarting uavcan node

[oystub]

Solved Problem

When restarting uavcan on STM32, for example by running uavcan stop uavcan start, the system crashes.

The issue was introduced with #20746, and affects 1.14 stable release and current main.

The issue occurs because the can driver is re-initialized when the node is restarted. The previous author noted in a comment that it shouldn't be re-initialized, but accidentally put the init call in the node initialization section of UavcanNode::Run. This made it run once per node restart instead of once per system boot.

Solution

Move driver initialization to UavcanNode::start, where the CanInitHelper is initialized, and where there is already a check if the can driver has been previously initialized.

Now we can also avoid fetching the bitrate parameter twice.

Changelog Entry

For release notes:

Bugfix: Prevent system crash on stm32 when restarting uavcan node

Test coverage

• Tested on a CubeOrange flight controller

Context

Crash dump from 1.14 when stopping and starting uavcan:
fault_2000_01_01_00_02_28.log

[PetervdPerk-NXP]

Oh sorry to hear this my PR caused this bug

But actually there's a good reason to init the CAN driver UavcanNode::Run this is because UavcanNode::start runs in a different thread and you want to initialize the CAN device in the same thread you're reading it from. If this PR goes in it will probably break the FMUK66, UCANS32K1, V6X-RT targets.

Wouldn't it better to actually de-initialize the CAN driver in, avoiding the crash?

PX4-Autopilot/src/drivers/uavcan/uavcan_main.cpp

Line 993 in 9f0f3b9

if (_task_should_exit.load()) {

Because technically after uavcan start uavcan stop the STM32 can driver is still running.

[oystub]

@PetervdPerk-NXP Thanks for the quick response!

Yeah, sorry, I was a bit quick and didn't look close enough at the entire commit history and PR to see that I just moved the initialization back to where it was, and that there was a good reason for the change in the first place 😅

According to the comment in the source code, there is no way to deinit the driver on STM32. We could maybe add a static flag that skips re-initialization on stm32?

[oystub]

Or I guess the best would be to actually include a deinit method in the CanInitHelper and perform proper deinitialization.

[oystub]

@PetervdPerk-NXP
I investigated a bit further why the code actually crashed by running init twice, and it is because the init bit is not properly set here, so it times out:

PX4-Autopilot/src/drivers/uavcan/uavcan_drivers/stm32h7/driver/src/uc_stm32h7_can.cpp

Line 691 in 926e787

return -ErrCCCrINITNotSet;

Edit: It seems like just letting initOnce run every time and not just once, the driver handles being re-initialized just fine...

[oystub]

It is because initOnce resets the FDCan driver. Just always performing a reset with RCC_APB1RSTR_CAN1RST also solves the issue on CubeOrange (stm32h7). The same probably applies to regular stm32 driver. Unsure about any side effects running this multiple times could cause. This is beyond my area of expertise 😅
Manually stopping and starting uavcan is kind of an edge case anyway, might not really be that important to do anything here.

[PetervdPerk-NXP]

@davids5 any idea about this?