fix(tx16s): keep all code executed during USB sessions in flash bank 1 (PA12 errata)#7557
Open
AdsumSOK wants to merge 10 commits into
Open
fix(tx16s): keep all code executed during USB sessions in flash bank 1 (PA12 errata)#7557AdsumSOK wants to merge 10 commits into
AdsumSOK wants to merge 10 commits into
Conversation
On some TX16S MK1 units, instruction fetches from the upper flash bank return corrupted data under heavy IRQ/DMA load. USB joystick or mass-storage traffic then hard-faults inside the OTG/DMA interrupt handlers (UNDEFINSTR/NOCP on provably valid instructions) and the radio reboots into DFU. This is the long-standing "radio switches to DFU when plugged into USB" issue (EdgeTX#5899, EdgeTX#7356, EdgeTX#6047, EdgeTX#6414): unit- dependent (marginal flash), version-dependent (the linker moves the hot handlers across the bank boundary as the firmware grows), and the bootloader is immune (its code sits at the start of bank 1). Fix: link the USB device stack, PCD/LL drivers, MSC storage glue and SDIO diskio into a .fastcode section placed in RAM (copied at startup, ~14KB), so the hot interrupt paths never fetch from flash. Validated on an affected TX16S: continuous mass-storage stress-reads crashed after ~1 min with the code in flash, and ran clean with the code in RAM (same flash config otherwise). Build with -DNO_LTO=ON (or drop -flto from the USB object libraries): linker file-pattern matching cannot see through LTO partitions.
Linker-script file-pattern matching (used by the .fastcode RAM section) cannot see through LTO partitions; without this the hot interrupt code silently stays in flash.
On some STM32F42x units (flash rev < 3), USB D+ (PA12) switching noise corrupts reads from flash bank 2 (ST errata ES0166 §2.4.6, EdgeTX#5899). Restrict the FLASH region to bank 1 (overflow = link error) and relegate UI code/rodata (.uicode: colorlcd, lvgl, libopenui) and assets (.flash) to bank 2 via a per-target pre_text_sections.ld, claimed before the .text catch-all. Shared C++ comdats (std::) are pinned to bank 1 (.text.shared) with their own startup copy (no-op on XIP targets), so link order can never strand a template instantiation used by hot code in bank 2. Symbols called from non-UI tasks but living in UI objects (lv_tick_inc from SysTick, R9M inline helpers from the pulses path, popup mailbox from telemetry/log timers, SET_SCREEN from the mixer) are pinned individually (.text.pinned). bench/check_bank1.sh verifies after every link: no executable section in bank 2 except .uicode, USB handlers in RAM, and no direct call path from USB-active contexts (ISRs, mixer, audio, timers, perMain head) into bank 2. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All UI code lives in flash bank 2 which must not be read during USB traffic (PA12 errata). Draw a static USB screen before usbStart() for every mode (mass storage included: LVGL used to keep running during MSC), then return early from perMain() until unplug. The blocking popup dialogs are also guarded so audio/telemetry callers cannot start a nested UI loop while frozen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
usbJoystickUpdate() used to rewrite the report buffer and re-arm the IN endpoint at mixer rate even when the previous transfer was still pending, corrupting the DMA buffer mid-transmit and hammering the endpoint (analysis by ulph0, EdgeTX#5899). Skip the update until the endpoint is idle; covers both the legacy and USBJ_EX paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Validation builds (-DDIAG_BANK2_FENCE=ON) arm an MPU no-access region over flash bank 2 for the whole USB session: any stray access — including through function pointers invisible to the static scan — traps deterministically instead of corrupting reads probabilistically (technique proven by ulph0's experiments, EdgeTX#5899). The MemManage handler stores 0xB2FE0000|CFSR and the faulting address in RTC backup registers BKP4R/BKP5R (survive the reset) and reboots. Also adds bench/test_invariance.sh: two deliberately shifted builds (core/UI padding, swapped UI link order) that must both pass bench/check_bank1.sh, proving the layout guarantee is structural. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The stock HardFault handler returns into the fault without a debugger; record fence traps escalated to HardFault (magic 0xB2FA) like MemManage ones (0xB2FE) and reboot. On the next boot with storage up, append the trap to /bank2fence.log and clear the marker — the backup registers survive resets and power cycles (RTC cell). Scan: root every FreeRTOS timer callback (the timer-task dispatch is indirect, per10ms & co were unaudited) and reject roots living in bank 2 (UI callbacks caught by a broad pattern are frozen contexts, not hot ones). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
First on-radio fence trap (IACCVIOL @ cancelShutdownAnimation): pwrCheck(), polled by menusTask every 50ms even while the UI is frozen, fetched bank 2 code on every cycle of a USB session through its button-released branch — a strong candidate for the historical random crashes (EdgeTX#5899). Power long-press during USB now tears the session down first (usbSessionForceStop, same cleanup as an unplug — factored out of handleUsbConnection): with PA12 quiet, the bank 2 shutdown UI is safe again. The shutdown animation is skipped while frozen and cancelShutdownAnimation is pinned to bank 1. Deliberate trade-off: long-press during USB powers off without the confirmation dialog. Scan: root menusTask/pwrCheck and every FreeRTOS timer callback, add per-edge whitelist for runtime-guarded paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The same static screen serves all three USB modes; with only the mass-storage icon, a joystick session reads as the wrong mode being active (confused a tester into power-cycling the radio mid-session). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The emergency-mode screen is now drawn directly from menusTask at boot (before any USB session can exist): whitelist that edge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR adds STM32 linker sections and asset-bank mappings, relocates selected code at reset, freezes the COLORLCD UI during USB sessions, introduces optional bank-2 MPU diagnostics, improves HID readiness handling, and adds post-link bank-placement and invariance verification scripts. ChangesSTM32 linker layout and relocation
USB session and UI behavior
Bank-2 diagnostics
Bank-1 verification
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
radio/src/edgetx.cpp (1)
1511-1520: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove
bank2FenceDumpPostMortem()out of the!UNEXPECTED_SHUTDOWN()guard
UNEXPECTED_SHUTDOWN()is the fault-reboot path, so this diagnostic is skipped on the very boot it is meant to report. Keep the SD init guarded if needed, but call the post-mortem dump independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/edgetx.cpp` around lines 1511 - 1520, Move the bank2FenceDumpPostMortem() call outside the !UNEXPECTED_SHUTDOWN() block so it runs during fault-reboot boots as well. Keep sdMounted() and sdInit() within the existing guard, and preserve the DIAG_BANK2_FENCE conditional compilation around the diagnostic call.
🧹 Nitpick comments (5)
radio/src/targets/common/arm/stm32/bank2_fence.c (1)
112-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEnable PWR clock before reading from the Backup domain.
While read access to the RTC backup registers does not strictly require the Backup Domain write protection (
DBP) to be cleared, reading them before ensuring thePWRclock is enabled can lead to subtle issues or bus faults on certain STM32 families if the initialization sequence is ever changed. It is safer to enable thePWRclock at the very beginning of the function.🛠️ Proposed refactor
void bank2FenceDumpPostMortem(void) { + __HAL_RCC_PWR_CLK_ENABLE(); uint32_t r4 = RTC->BKP4R; uint32_t hi = r4 & 0xFFFF0000UL; if (hi != 0xB2FE0000UL && hi != 0xB2FA0000UL) return; FIL f; if (f_open(&f, "/bank2fence.log", FA_WRITE | FA_OPEN_APPEND) == FR_OK) { f_printf(&f, "%s cfsr=%04X addr=%08lX\n", (hi == 0xB2FE0000UL) ? "MEMMANAGE" : "HARDFAULT", (unsigned)(r4 & 0xFFFF), (unsigned long)RTC->BKP5R); f_close(&f); } - __HAL_RCC_PWR_CLK_ENABLE(); SET_BIT(PWR->CR, PWR_CR_DBP); RTC->BKP4R = 0; RTC->BKP5R = 0; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/common/arm/stm32/bank2_fence.c` around lines 112 - 130, Move __HAL_RCC_PWR_CLK_ENABLE() to the beginning of bank2FenceDumpPostMortem, before reading RTC->BKP4R. Keep the existing fault filtering, logging, and backup-register clearing behavior unchanged.radio/src/edgetx.cpp (1)
39-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
usbSessionForceStop()declaration to a shared header.It's declared manually here as a raw prototype but defined in
main.cpp. Sincehal/usb_driver.halready exports the relatedusbPlugged()/getSelectedUsbMode()primitives (used bypopups.cpp), declaring this there too would avoid signature drift risk between the two TUs.♻️ Suggested move
-#if defined(STM32) && !defined(SIMU) -void usbSessionForceStop(); -#else -inline void usbSessionForceStop() {} -#endif +#include "hal/usb_driver.h" // declares usbSessionForceStop()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/edgetx.cpp` around lines 39 - 48, Move the usbSessionForceStop() declaration from edgetx.cpp into the shared hal/usb_driver.h header alongside usbPlugged() and getSelectedUsbMode(), preserving the existing STM32/non-SIMU availability behavior and removing the local raw prototype.radio/src/gui/colorlcd/libui/popups.cpp (1)
44-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe USB-session "UI frozen" predicate is duplicated raw across three files instead of sharing one helper. All three sites encode the same safety-critical PA12/bank-2 condition (
usbPlugged() && getSelectedUsbMode() != USB_UNSELECTED_MODE, or its negation), risking silent drift if one site is updated without the others.
radio/src/gui/colorlcd/libui/popups.cpp#L44-L66: promote_usb_ui_frozen()out ofstaticfile scope (e.g. expose it as an inline function viahal/usb_driver.h) so it becomes the single source of truth.radio/src/main.cpp#L592-L601: replace the rawusbPlugged() && getSelectedUsbMode() != USB_UNSELECTED_MODEcheck with the shared helper.radio/src/edgetx.cpp#L1822-L1950: replace the repeated negated checks (usbConfirmedinit, the shutdown-animation draw guard, and the release-branchcancelShutdownAnimation()guard) with calls to the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/gui/colorlcd/libui/popups.cpp` around lines 44 - 66, Promote _usb_ui_frozen() from popups.cpp file scope into the shared USB header, preserving its platform-specific USB-session predicate and inline behavior. In radio/src/gui/colorlcd/libui/popups.cpp lines 44-66, use the shared declaration without redefining it; in radio/src/main.cpp lines 592-601, replace the raw USB condition with _usb_ui_frozen(); and in radio/src/edgetx.cpp lines 1822-1950, replace the repeated negated checks for usbConfirmed initialization, shutdown-animation drawing, and cancelShutdownAnimation() with the shared helper.radio/src/boards/generic_stm32/linker/firmware.ld (1)
41-59: 🚀 Performance & Scalability | 🔵 TrivialConfirm RAM budget on non-TX16S targets.
.fastcode(RAM-resident copy ofll_usb/hal_pcd/usb_driver/usbd_*/diskio_sdiocode) is defined here in the sharedfirmware.ld, so it applies to every STM32 target that includes this script — not only the TX16S/F429 unit affected by the PA12/bank-2 erratum. On RAM-constrained targets (e.g. F20x with 128K total RAM), this permanently reserves extra RAM for code that didn't need relocation there. Worth confirming link/map-file RAM headroom on the smaller targets isn't eroded by this addition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/boards/generic_stm32/linker/firmware.ld` around lines 41 - 59, Verify the .fastcode placement and size against linker map files for every non-TX16S target using firmware.ld, especially RAM-constrained F20x devices. Confirm REGION_BSS retains sufficient headroom after relocating the listed USB and diskio_sdio sections; if not, limit this section to the affected target or adjust the placement without impacting required TX16S behavior.tools/test_invariance.sh (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuote the command substitution to prevent word splitting.
It is recommended to double-quote the output of the command substitutions to prevent unintended word splitting and address the static analysis warnings.
♻️ Proposed refactor
- docker run --rm -u $(id -u):$(id -g) -v "$(pwd)":/src -w /src \ + docker run --rm -u "$(id -u):$(id -g)" -v "$(pwd)":/src -w /src \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/test_invariance.sh` at line 33, Update the docker run command in test_invariance.sh to quote the id command substitutions for both user and group values, preventing word splitting while preserving the existing volume and working-directory behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@radio/src/edgetx.cpp`:
- Around line 1511-1520: Move the bank2FenceDumpPostMortem() call outside the
!UNEXPECTED_SHUTDOWN() block so it runs during fault-reboot boots as well. Keep
sdMounted() and sdInit() within the existing guard, and preserve the
DIAG_BANK2_FENCE conditional compilation around the diagnostic call.
---
Nitpick comments:
In `@radio/src/boards/generic_stm32/linker/firmware.ld`:
- Around line 41-59: Verify the .fastcode placement and size against linker map
files for every non-TX16S target using firmware.ld, especially RAM-constrained
F20x devices. Confirm REGION_BSS retains sufficient headroom after relocating
the listed USB and diskio_sdio sections; if not, limit this section to the
affected target or adjust the placement without impacting required TX16S
behavior.
In `@radio/src/edgetx.cpp`:
- Around line 39-48: Move the usbSessionForceStop() declaration from edgetx.cpp
into the shared hal/usb_driver.h header alongside usbPlugged() and
getSelectedUsbMode(), preserving the existing STM32/non-SIMU availability
behavior and removing the local raw prototype.
In `@radio/src/gui/colorlcd/libui/popups.cpp`:
- Around line 44-66: Promote _usb_ui_frozen() from popups.cpp file scope into
the shared USB header, preserving its platform-specific USB-session predicate
and inline behavior. In radio/src/gui/colorlcd/libui/popups.cpp lines 44-66, use
the shared declaration without redefining it; in radio/src/main.cpp lines
592-601, replace the raw USB condition with _usb_ui_frozen(); and in
radio/src/edgetx.cpp lines 1822-1950, replace the repeated negated checks for
usbConfirmed initialization, shutdown-animation drawing, and
cancelShutdownAnimation() with the shared helper.
In `@radio/src/targets/common/arm/stm32/bank2_fence.c`:
- Around line 112-130: Move __HAL_RCC_PWR_CLK_ENABLE() to the beginning of
bank2FenceDumpPostMortem, before reading RTC->BKP4R. Keep the existing fault
filtering, logging, and backup-register clearing behavior unchanged.
In `@tools/test_invariance.sh`:
- Line 33: Update the docker run command in test_invariance.sh to quote the id
command substitutions for both user and group values, preventing word splitting
while preserving the existing volume and working-directory behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96c52f6c-e7a0-4174-82e9-d67ce956438b
📒 Files selected for processing (28)
radio/src/boards/generic_stm32/linker/definitions.ldradio/src/boards/generic_stm32/linker/firmware.ldradio/src/boards/generic_stm32/linker/stm32f20x/layout.ldradio/src/boards/generic_stm32/linker/stm32f20x/pre_text_sections.ldradio/src/boards/generic_stm32/linker/stm32f40x/layout.ldradio/src/boards/generic_stm32/linker/stm32f40x/pre_text_sections.ldradio/src/boards/generic_stm32/linker/stm32f413/layout.ldradio/src/boards/generic_stm32/linker/stm32f413/pre_text_sections.ldradio/src/boards/generic_stm32/linker/stm32f429_sdram/layout.ldradio/src/boards/generic_stm32/linker/stm32f429_sdram/pre_text_sections.ldradio/src/boards/generic_stm32/linker/stm32h750_sdram/layout.ldradio/src/boards/generic_stm32/linker/stm32h750_sdram/pre_text_sections.ldradio/src/boards/generic_stm32/linker/stm32h7rs_sdram/layout.ldradio/src/boards/generic_stm32/linker/stm32h7rs_sdram/pre_text_sections.ldradio/src/bootloader/CMakeLists.txtradio/src/edgetx.cppradio/src/gui/colorlcd/libui/popups.cppradio/src/main.cppradio/src/targets/common/arm/stm32/bank2_fence.cradio/src/targets/common/arm/stm32/cortex_m_isr.cradio/src/targets/common/arm/stm32/f4/CMakeLists.txtradio/src/targets/common/arm/stm32/system_init.cradio/src/targets/common/arm/stm32/usb_driver.cppradio/src/targets/common/arm/stm32/usbd_hid_joystick.cradio/src/targets/horus/CMakeLists.txtradio/src/thirdparty/STM32_USB_Device_Library/Class/HID/Inc/usbd_hid.htools/check_bank1.shtools/test_invariance.sh
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5899, related to #7356. Supersedes #7554 (its two commits are included here), as discussed in the plan comment.
Problem
On early TX16S MK1 units (STM32F429, flash rev < 3), toggling PA12 (USB D+) during an active USB session couples noise into the flash read path when bank 2 (
0x08100000+) is accessed simultaneously (ST errata ES0166 §2.4.6, "no fix planned"). Corrupted instruction fetches then fault or hang in random ISRs — the long-standing "radio crashes/reboots to DFU when USB is plugged" issue. Diagnosed independently with persistent fault capture (3 captured crashes, all faulting on fetches above0x08100000) and by @ulph0's MPU-fence experiments on #5899; both analyses converge.Because any bank 2 read during USB is a potential crash vector (thread mode included), moving only the hot interrupt path to RAM (#7554) improved things (55 s → 24 min under MSC stress) but could not be a guarantee: every rebuild redistributes code across the bank boundary.
Fix: deterministic layout, enforced by the linker
FLASHregion is restricted to bank 1 (1024K), so any overflow is a link error, not a lottery. UI code and its rodata (gui/colorlcd,lvgl,libopenui— ~613K) and cold assets (.flash, 165K) are placed in bank 2 through a new per-targetpre_text_sections.ld(.uicodeoutput section, claimed before the.textcatch-all). Non-F429 targets get an empty file: zero layout change for them. Bank 1 sits at ~804K/1024K, bank 2 at ~778K/1024K..text.sharedclaims thestd::comdats into bank 1 (with a dedicated startup copy, no-op on XIP targets), and.text.pinnedpins individual symbols called from non-UI contexts, found by the call scan:lv_tick_inc(called from SysTick every 1 ms), the R9M inline helpers (pulses, every mixer cycle), the deferred-popup mailbox (telemetry/logs timers),setRequestedMainView(SET_SCREEN special function),cancelShutdownAnimation(see below).usbStart(), thenperMain()returns early until unplug — for all three modes (mass storage used to keep LVGL running). Blocking popups are guarded, and a power long-press during USB now tears the session down first (usbSessionForceStop(), same cleanup as an unplug) so the bank 2 shutdown UI only runs once PA12 is quiet — trade-off: no confirmation dialog in that case..fastcode). Plus a HID race fix (analysis by @ulph0): the joystick report buffer is no longer rewritten while the previous transfer is still in flight.Verification
tools/check_bank1.shruns after every build: no executable section in bank 2 except.uicode, USB handlers in RAM, and no direct call path into bank 2 from 56 USB-active roots (ISRs, mixer, audio, FreeRTOS timer callbacks,pwrCheck, head ofperMain), with a per-edge whitelist for runtime-guarded paths.tools/test_invariance.shproves the guarantee is structural: two deliberately shifted builds (+8K core, +8K UI + swapped link order) must pass the same checks.-DDIAG_BANK2_FENCE=ON(validation builds) arms an MPU no-access region over bank 2 for the whole USB session — any stray access, including through function pointers invisible to the static scan, traps deterministically; the fault handler stores the faulting address in RTC backup registers and the next boot appends it to/bank2fence.logon the SD card.The fence proved itself immediately: the first on-radio test trapped
pwrCheck()→cancelShutdownAnimation()— polled from menusTask every 50 ms during any USB session, a strong candidate for the historical randomness. After fixing it, on an affected early MK1:Notes / open questions
_firmware_lengthcovers the full span for UF2/fw_desc consumers.EARLY_MK1_FLASH_ERRATA) instead of applying to all F429 targets? The bank 1 placement costs nothing on healthy units; the UI freeze during USB is the behavioral part worth discussing.tools/check_bank1.shcould be wired into CI for the TX16S target to make the layout guarantee permanent.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Diagnostics