Repository navigation
Fix interrupt storm lockup in BLHeli passthrough and get crash dumps for interrupt starvation - #34597
Conversation
andyp1per
left a comment
andyp1per
left a comment
There was a problem hiding this comment.
This looks very nice! I would need to do some testing of the BLHeli part. My main comment is that this should be two PRs - one is the locking around the line setting and blheli fixes, the other is the enhancements to the crash detection machinery. The second part could have a much larger blast radius so I think needs more careful testing (I'm assuming you will want this on by default).
for 4.7 I have split it: For 4.8 I think separate commits in this PR makes sense |
Global Size Summary (Total Flash delta in bytes)Built from 442e981 rebased onto 0ed15ec
|
Previous review (2026-10-03)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head All three review passes (primary, cold, validation) are status complete at the pinned head 5012443. None of them raised a finding, and there were no earlier findings to carry over. The fresh snapshot shows the same head, the same title and the same two thread entries. The head has not moved, and this review claims nothing about any later code. I re-checked the work against the pinned tree. The SysTick lockup detector (https://github.com/ArduPilot/ardupilot/pull/34597/files#diff-1100ce5f00d9546fe251ecf118739a7d7377cbaef323e1fed75b3491a111954eR136 and https://github.com/ArduPilot/ardupilot/pull/34597/files#diff-1100ce5f00d9546fe251ecf118739a7d7377cbaef323e1fed75b3491a111954eR170) runs at priority 1. ChibiOS chcore.h sets CORTEX_FAST_PRIORITIES to 2 and puts the kernel BASEPRI at 3 or above, so priority 1 preempts chSysLock and any EXTI storm at kernel priority. Nothing in AP_HAL_ChibiOS or the ChibiOS port sets SCB->SHCSR USGFAULTENA, so the UDF from The retained logs from the earlier passes also back these points. Builds passed for Pixhawk6C, CubeOrange and CubeBlack copter, MatekF405 plane, three AP_Periph targets, iomcu iofirmware, a clean Pixhawk6C bootloader and SITL copter. The host harnesses for the serial abort and for CrashCatcher pass. check_branch_conventions passes: every commit touches exactly one allowed subsystem. No parameters are added and no allocations change. On the thread: andyp1per asked for the BLHeli and pin-locking fixes to be split from the crash-detection work, and said the BLHeli part needs testing. tridge replied that 4.7 is split into PR #34598, and that for 4.8 separate commits in this PR make sense. The pinned stack does have separate per-subsystem commits. The contents of #34598 were not fetched and are UNCONFIRMED. No run exercised anything on hardware, so these remain open test gaps. They do not block this review. The detector actually firing during EXTI starvation on H7 and on F4/F7 is untested; on F4/F7 a flash erase stall can also stop SysTick, which the commit message acknowledges as best effort. Real IWDG timing and crash-dump completion are untested. BLHeli passthrough to a real ESC is untested. Since the detector defaults on for every crash-dump board, the hardware testing andyp1per asked for is still warranted. The validation pass did not independently reproduce the primary's SITL crash-trigger run or its full board matrix; it rebuilt Pixhawk6C copter only. CI was still pending when the head was pinned. |
peterbarker
left a comment
peterbarker
left a comment
There was a problem hiding this comment.
Good for me past the minor
5012443 to
442e981
Compare
Previous review (2026-10-07)Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at 442e981. The head moved from 5012443, and the earlier ACCEPT does not carry over. Blocking: the lockup detector is paused only around the bootloader erase in Util::flash_bootloader() (https://github.com/ArduPilot/ardupilot/pull/34597/files#diff-a4a3d06086bfc0e82120db26f5962d8ffc4f623e8e3090a2604e29b78c2225b1R303). AP_CheckFirmware::write_bootloader() (libraries/AP_CheckFirmware/AP_CheckFirmware_secure_command.cpp:169) erases the same page 0 for secure-command key updates on signed builds, with no pause. stm32_flash_erasepage() holds the kernel lock, so nothing can pat during the erase. If the erase runs past about 1.8s, the detector traps with the bootloader part-erased, where master would ride on to the 2.048s IWDG. VERIFIED in source and in the signed Pixhawk6C ELF (no pause call in write_bootloader); real erase timing is UNCONFIRMED on hardware. Pausing inside stm32_flash_erasepage() or Flash::erasepage() would cover every caller. Smaller points:
Builds at this head passed for Pixhawk6C (including signed), CubeOrange, MatekF405, MatekH743, the Pixhawk6C bootloader and f103-GPS AP_Periph, and commit conventions pass. Hardware behaviour, SITL and CI (pending) were not covered. |
no functional change, this moves the 42/24/71 failure creation handling out of handle_preflight_reboot() into its own file
saves flash on boards with 1MB or less of flash. It can still be enabled with the FAILURE_INJECTION build option
42/24/71/104 creates a never ending EXTI software interrupt storm on an unused EXTI channel on ChibiOS boards. This starves the ChibiOS system timer and all threads, reproducing the lockup seen when a soft-serial EXTI line is fed a fast clock, and is used to test that such lockups produce a crash dump
CrashCatcher advances info.sp past the exception frame, so reading lr_thd from it gave a stack value, not the stacked LR
CrashCatcher reads the instruction at the faulting PC, so fault on a readable undefined instruction rather than jumping to an invalid address
the monitor thread can't trigger a crash dump if an interrupt storm starves the ChibiOS timer, as no thread will ever wake. This was seen with a soft-serial EXTI line receiving a 4MHz MCO clock during BLHeli passthrough, giving a watchdog with no crash dump. Use SysTick at priority 1 (above all peripheral interrupts and the kernel lock) to watch for watchdog pats, triggering a crash dump 250ms before the nominal watchdog timeout. Only enabled on boards with crash dump support and a TIM based system timer. The monitor thread crash dump trigger is not needed when this is enabled. The detector is paused while erasing a flash sector (bootloader update, secure key update, flash storage), as an erase can take close to the watchdog timeout. STM32_WDG_TIMEOUT_MS must be at least 1000ms. The watchdog uses the LSI clock, which on F4/F7 can be fast enough for the watchdog to fire before the detector, so the crash dump is best effort there
chEvtGetAndClearEvents() and chVTReset() take the kernel lock themselves, so calling them inside chSysLock() unlocked the critical section early. Use the I-class variants
palSetLineMode() does an unlocked read-modify-write of the GPIO mode registers, and palToggleLine() writes the whole output register. When two threads change pins on the same port these can corrupt each other. This was seen with BLHeli passthrough on Pixhawk6C: SPI1 restoring its SCK pin (PA5) raced with soft-serial switching PA8 to input, leaving PA8 in alternate function 0 (MCO1, a 4MHz clock) with its EXTI interrupt enabled. The resulting interrupt storm locked up the board. Add stm32_set_line_mode(), which locks the mode change, and stm32_toggle_line(), which toggles with a single BSRR write. Use them for all runtime pin changes in the main firmware (IOMCU firmware is not changed). Also lock palReadLineMode() and palLineSetPushPull()
board_init_debug() runs after other threads have started, so use stm32_set_line_mode() to avoid racing other pin mode changes on the same port
palToggleLine() writes the whole port output register, which can undo concurrent writes to other pins on the same port
442e981 to
d053c01
Compare
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head Reviewed at d053c01. The head moved from 442e981, so this covers the new head only. The earlier blocker is fixed. The lockup-detector pause now sits inside ChibiOS::Flash::erasepage() (https://github.com/ArduPilot/ardupilot/pull/34597/files#diff-09076c35be9b3e238ffef7bad7a430eb3459c8053876f0bf2a28e448c0fced6bR16), so the secure-command bootloader erase, the bootloader update and flash storage erases are all covered. VERIFIED by grep and in the signed Pixhawk6C ELF: 2 pause calls around the single erase. The STM32_WDG_TIMEOUT_MS >= 1000 check is now an unconditional Builds at this head passed for signed Pixhawk6C, MatekF405, CubeOrange, f103-GPS AP_Periph, iomcu and the Pixhawk6C bootloader, and commit conventions pass. Not covered: hardware behaviour (erase timing, EXTI starvation, BLHeli passthrough), SITL, individual CI jobs and #34598. |
I've done a lot of BLHeli tests myself, plus I've removed the rate limiter change on the serial code, so I don't think I need to wait for your re-review and testing now |
removed the large blheli path changes, now just adds the interrupt disable about pin changes which is the key thing and is definitely required
|
Do we want this included in 4.7.3-beta1? |
Summary
Fixes a lockup on Pixhawk6C (and many other F4/F7/H7 boards) during BLHeli/AM32 passthrough that produced a watchdog with no crash dump. Makes interrupt-starvation lockups produce a crash dump, and fixes the GPIO race that caused it.
Classification & Testing (check all that apply and add your own)
Tested on a Pixhawk6C with an AM32 ESC on motor 1 and an ST-Link attached:
WDG: T-3 ... FL0and no CrashDump.DAT.Tools/debug/gdb_crashdump.shshowsRCOutput::serial_bit_irqin the EXTI5_9 ISR, with the main thread inAP_BLHeli::BL_GetACK.957b45d4: 100/100, no lockup, reboot or crash dump.b3e25fb4: 100/100, uptime continuous.957b45d4,cae1c5fcand5012443b. The backtrace runs fromSysTick_Handlerinto the interrupted ISR and the main thread.cae1c5fc, where the detector pause was inUtil::flash_bootloader(). At the current head the pause has moved intoChibiOS::Flash::erasepage(). The update was ACKed and the board stayed up, but its status text wasn't captured.Software tests at the current head
d053c010, all passing:FAILURE_INJECTIONforced on, KakuteF7, SPRacingH7 (no crash dump), MatekH743 planecheck_branch_conventions.pypasses.Copter.ArmSwitchAfterReboot,Copter.ArmFeatures,Plane.WatchdogHome,Plane.ThrottleFailsafe.Description
Root cause:
palSetLineMode()does an unlocked read-modify-write of the GPIO mode registers.Crash dump commits:
GCS_MAVLink:GCS_CrashTrigger.cpp.FAILURE_INJECTIONbuild option for these commands.AP_HAL_ChibiOS:udfto trigger the lockup crash dump.STM32_WDG_TIMEOUT_MSmust now be at least 1000.Root cause commits:
AP_HAL_ChibiOS:stm32_set_line_mode()(locked) andstm32_toggle_line()(single BSRR write), and use them for all runtime pin changes in the main firmware.AP_BoardConfigandAP_Periphare converted to the new helpers.Known limitations:
MAVProxy:
isrstormcommand in ArduPilot/MAVProxy#1768.Note that this bug can impact systems other than bl-heli pass-thru. The bl-heli code is the most impacted, but it could lead to:
These could be triggered by GPIO operations on the same port as a SPI SCK pin by lua scripts for example
Test flown on a CubeOrange quadplane