AP_HAL_ChibiOS: add reboot to USB mass storage mode

[tridge]

Summary

Adds a reboot-to-USB-mass-storage mode that exports the flight controller's microSD card as a USB block device. This provides much faster access to logs and other files than MAVLink file transfer. It should be particularly good for uploading and checking terrain data.

The mode runs as an early application service, before the normal scheduler and filesystem users start. It remains in mass-storage mode until power is removed, preventing the flight firmware and USB host from accessing the filesystem concurrently. What the user sees is their normal USB interfaces to the flight controller disappear and an ArduPilot mass storage device appears and should auto-mount. To get back to the normal flight controller mode you need to power cycle.

Depends on ArduPilot/ChibiOS#110 and ArduPilot/mavlink#517

Classification & Testing (check all that apply and add your own)

• Checked by a human programmer
• Non-functional change
• No-binary change
• Infrastructure change (e.g. unit tests, helper scripts)
• Automated test(s) verify changes (e.g. unit test, autotest)
• Tested manually, description below (e.g. SITL)
• Tested on hardware
• Logs attached
• Logs available on request

Hardware testing performed:

• CubeOrange running Plane: built and uploaded over USB; verified the MAVLink reboot request, USB block-device enumeration, mounting and file transfers.
• CubeBlack running Copter: built with mass storage enabled and uploaded over USB; verified reboot into mass-storage mode and microSD access.
• MatekF405-STD running Copter with SPI microSD: built with --enable-MASS_STORAGE, uploaded and tested from a disarmed Copter. The 15.9 GB card remained stable as a USB disk. A 32 MiB transfer measured approximately 692 kB/s write and 869 kB/s read, with a successful byte-for-byte comparison. A final 8 MiB write test measured 726 kB/s and also passed comparison.
• Follow-up MatekF405-STD testing at 95b16c24 exercised the custom-build define AP_REBOOT_MASS_STORAGE_ENABLED 1 path. The firmware built, uploaded and verified byte-for-byte; reboot action 5 entered mass-storage mode and enumerated the 14.8 GiB FAT card. An existing 2 MiB file read at 851 kB/s. After a clean unmount and full USB power cycle, the one-shot request was consumed and the board did not re-enter mass-storage mode; an explicit bootloader handoff restored a normal Copter heartbeat on 95b16c24.

Description

Builds on:

• common: added reboot to mass storage option mavlink#517
• Improve the speed of mass storage over USB implementation ChibiOS#110
• common: added reboot to mass storage option mavlink/mavlink#2572

The reboot request uses REBOOT_SHUTDOWN_ACTION_REBOOT_TO_MASS_STORAGE (param1=5) with MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN. It is rejected while the vehicle is armed, including when the normal forced-reboot magic value is supplied.

The request is stored in the existing persistent watchdog data using a spare bit alongside the armed flag. After reboot, the flag is consumed once and startup enters the early USB mass-storage service instead of the normal flight application. The service initializes the microSD block device directly and continually services the watchdog.

The ChibiOS changes add multi-block SCSI transfers and double-buffer pipelining so SD-card I/O can overlap USB transfers. Both read and write paths track outstanding operations and drain them on error.

The feature requires an STM32F4, F7 or H7 board with USB and microSD support. It is enabled by default on normal STM32H7 builds whose HAL_PROGRAM_SIZE_LIMIT_KB is at least 2048. It can be selected explicitly for other compatible builds with:

./waf configure --board <board> --enable-MASS_STORAGE

It can be removed from a build with --disable-MASS_STORAGE.

To test with MAVProxy, first disarm the vehicle and run:

long PREFLIGHT_REBOOT_SHUTDOWN 5

The USB connection will re-enumerate as a mass-storage disk containing the microSD filesystem. Safely unmount/eject the disk on the host and fully power-cycle the flight controller to return to normal operation.

We will need MissionPlanner and qgroundcontrol updates for this feature as well

• MissionPlanner PR: FlightData: add mass storage reboot action MissionPlanner#3756
• MAVProxy added "reboot massstorage" command MAVProxy#1732

AI assistance

Codex was used to inspect review feedback, implement and test follow-up fixes, and obtain an independent Claude review. The human author remains responsible for the contribution.

[rmackay9]

Does this have any effect on our secure-bootloader feature? I wonder if we need to add anything to our security page, perhaps a note on how to disable this.

[tridge]

Does this have any effect on our secure-bootloader feature? I wonder if we need to add anything to our security page, perhaps a note on how to disable this.

I don't think it impacts that, it only gives access to the microSD, and only over USB, so you need physical access and this doesn't give you a way to change the firmware

[timtuxworth]

This provides much faster access to logs and other files than MAVLink file transfer.

how much faster?

[tridge]

how much faster?

it depends how fast you're getting now. This method should give around 800kbyte/s read, and often mavlink log download is significantly slower than that.
It also is much nicer using a normal file manager interface to get files on/off, access scripts, terrain etc

[IamPete1]

Should this be accepted over all MAVLink links? Maybe it should only be accepted from a USB endpoint.

It would be nice if ejecting the mass storage could be made to reboot back to "normal" mode.

[tridge]

It would be nice if ejecting the mass storage could be made to reboot back to "normal" mode.

I did consider that, but one of the uses of the mass storage support is doing things like a filesystem check, format, partition etc of the microsd. To do those things you need to unmount the drive. For users that have aircraft where the microsd is extremely difficult to remove (I have some aircraft like that) this will be a big help if the microsd becomes corrupted. I know we can format the microsd, but it is nice to be able to do a chkdisk or re-format as exfat for example.

[tridge]

Should this be accepted over all MAVLink links? Maybe it should only be accepted from a USB endpoint.

we could, but it would limit some use cases, for example you may want to do this when its plugged into a companion computer on the aircraft, and the command could be coming in over a different link. I know you could arrange for the command to come via the companion computer, but it seems like a needless complication for the user.
What risk are we trying to prevent? Within the same command we already have a code for "reboot and stay in the bootloader" and "reboot to DFU". Neither of those are only accepted on USB and I haven't heard of it causing issues.

[tridge]

Deprecated - see below for the updated review.

Previous review (2026-08-17)

Automated review note - AI-generated (Claude), independently cross-checked by a second model and re-verified against the diff. Please sanity-check before acting.

Reviewed at head 749e3575bb.

Verdict: REQUEST CHANGES

• BUG - libraries/AP_HAL_ChibiOS/sdcard.cpp:165 - SD connection attempts go from three to nine on every boot. Confirmed against the file at the PR head: sdcard_init_raw() keeps its own const uint8_t tries = 3; for (i=0; i<tries; i++) loop around sdcStart/sdcConnect (both the SDC and the MMC_SPI branch), and the new sdcard_init() wraps the whole call in for (uint8_t i = 0; i < 3; i++). 3 × 3 = 9 connection attempts where master did 3. The retry was moved out so that f_mount could be retried, which is the right intent, but the inner loop needs to drop to one attempt (or the outer one removed) or every boot with a missing or marginal card pays triple — amplified again by AP_BoardConfig's adaptive slowdown retries on top.

• ISSUE - libraries/AP_HAL_ChibiOS/USB_MSD.cpp:182 - Mass-storage mode silently ignores BRD_SD_SLOWDOWN. The header declares bool sdcard_init_raw(uint8_t slowdown = 0); and usb_msd_run() calls it as sdcard_init_raw(), taking the default 0, while the normal mount path passes AP_BoardConfig::get_sdcard_slowdown(). A board that needs a slowdown to talk to its card at all will therefore fail or corrupt transfers in MSD mode specifically. I appreciate the parameter may genuinely not be readable that early in boot — if so the value wants carrying in persistent data alongside boot_to_mass_storage, rather than defaulting silently.

• NOTE - libraries/AP_HAL_ChibiOS/USB_MSD.cpp:186 - usb_msd_run() ends in an unconditional while (true) that only pats the watchdog, so the only exit from mass-storage mode is a power cycle or the watchdog. That is a defensible design for this feature, but it is worth saying so in the commit message and the docs, because a user who triggers it without a host attached has no in-band way back.

• NOTE - libraries/AP_HAL_ChibiOS/USB_MSD.cpp:184 - The allocation pair is handled correctly — calloc plus malloc_type(..., MEM_FILESYSTEM) with both freed on either failing — and the #error guards for unsupported families and missing HAL_USE_USB_MSD/HAL_USE_SDC are a good touch.

Full report, including what was checked and found clean: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html

[IamPete1]

Within the same command we already have a code for "reboot and stay in the bootloader" and "reboot to DFU". Neither of those are only accepted on USB and I haven't heard of it causing issues.

The bootloader maybe accessible over more than just USB, the main flight code has no way of knowing which ports could be used for flashing.

But in this case it can be 100% sure only the USB port can be used for USB mass storage.

As you say maybe there is no benefit to adding the protection, the worst I can think is that you have to power cycle to get back to "normal" mode if your entered mass storage by mistake. I guess we might be able to think of some complicated security bypass, but it would only be on USB.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-08-17)

Automated review note - AI-generated (Claude), independently cross-checked by a second model and re-verified against the diff. Please sanity-check before acting.

Reviewed at head 756d1cbfc8.

Verdict: COMMENT

• RESOLVED - libraries/AP_HAL_ChibiOS/sdcard.cpp:165 - The 3→9 SD retry multiplication is fixed. sdcard_init_raw() now takes the count as a parameter (uint8_t sd_slowdown, uint8_t tries), the internal const uint8_t tries = 3 is gone from both the SDC and MMC_SPI branches, and sdcard_init() passes tries=1 inside its own for (i < 3) — 3 × 1 = 3 attempts, matching master exactly. The header default argument has also been removed so callers must be explicit, and a tree-wide search finds only two callers (sdcard_init_raw(sd_slowdown, 1) and the MSD path's (0, 3)), so nothing reintroduces the multiplication. Closed.

• ISSUE - libraries/AP_HAL_ChibiOS/USB_MSD.cpp:176 - BRD_SD_SLOWDOWN is still not honoured in mass-storage mode — now deliberately and with a comment, rather than by a silent default. The header default is gone, but the call site chose sdcard_init_raw(0, 3) explicitly, on the reasoning that MSD is a bulk transfer path wanting maximum speed. The problem with that reasoning: BRD_SD_SLOWDOWN is not purely a speed/robustness dial — AP_BoardConfig auto-raises it (up to 8) and saves it when the card will not mount, so boards that genuinely cannot talk to their card at slowdown 0 exist in the field. On such a board the MSD init fails, usb_msd_run() returns, and the FC boots normally with no indication the requested mode failed. The fix is cheap: usb_msd_set_boot_request() runs at runtime from the MAVLink handler where the parameter is readable, so the value can ride in PersistentData next to boot_to_mass_storage — there are spare bits in that byte.

• WITHDRAWN - not a defect - modules/mavlink - I raised the unmerged submodule bumps as a blocker. That was wrong and I withdraw it. Depending on unmerged submodule changes is normal ArduPilot practice provided the submodule PRs are linked in the description — and they are: the description says “Depends on Improve the speed of mass storage over USB implementation ChibiOS#110 and common: added reboot to mass storage option mavlink#517”. So the process requirement is already met and there is nothing to fix. Recorded rather than deleted so nobody re-raises it.

• NOTE - modules/mavlink - One thing from that worth keeping as a question rather than a finding: the mavlink bump adds REBOOT_SHUTDOWN_ACTION_REBOOT_TO_MASS_STORAGE = 5 to the upstream-owned common.xml, and upstream mavlink/mavlink currently defines only 0–4 for that enum, so 5 is the next free value. Worth confirming upstream is happy to allocate it, since a value that upstream later assigns to something else is awkward to undo once it is in released firmware. Purely a heads-up on the linked mavlink PR, not something to change here.

• ISSUE - libraries/GCS_MAVLink/GCS_Common.cpp:3686 - No user feedback on the mass-storage path, which matters because of the finding above. The DFU sibling sends “Entering DFU mode”; this action sends nothing, and if usb_msd_run() bails on SD init or its allocations the board just boots normally. On a board that needs a slowdown, the user sends the command, the vehicle reboots, and nothing whatsoever appears — no MSD device, no message. Also worth noting the whole path is compiled out on SITL, so the new MAVLink branch including the armed refusal is never exercised by autotest.

• NOTE - libraries/AP_HAL_ChibiOS/hwdef/scripts/chibios_hwdef.py:998 - USB_USE_WAIT TRUE is emitted whenever mass storage is enabled, and the feature defaults on for every STM32H7 board with ≥2 MB — so this changes the ChibiOS USB driver configuration for the ordinary serial-USB path across a large fleet, not just for MSD. No regression demonstrated, but it deserves a sentence of justification in the commit message. Separately, an explicit --enable-MASS_STORAGE on a board that cannot support it only prints a progress line and builds a non-functional feature; self.error() would be better for the explicitly-requested case, leaving the quiet skip for auto-defaulting. And usb_msd_run() still ends in an unconditional while (true), so the only exit is a power cycle — defensible, but worth documenting.

• WITHDRAWN - not a defect - The second-opinion pass flagged the lower-case --enable-mass-storage alias as failing to set the -D macro. Not a defect in this PR and I am not raising it against you: the generic build-option loop in boards.py and the alias registration in wscript are both untouched by this diff and identical on master, so any weakness there is pre-existing and affects every option equally. Worth noting the opposite, in fact — this PR's own new code checks both enable_MASS_STORAGE and enable_mass_storage, so it handles the lower-case form more carefully than the loop around it.

Full report, including what was checked and found clean: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html

[tridge]

As you say maybe there is no benefit to adding the protection, the worst I can think is that you have to power cycle to get back to "normal" mode if your entered mass storage by mistake.

I really think it is unnecessary, and I think it would need a new HAL virtual on the UART driver which will cost us flash for something that is a non-issue.
I can also think of cases where being able to do this on non-USB would be useful. For example, a lua scripted web server may want to have a button for "Reboot to mass storage". That would go via the ability of lua to send command int messages but would not be on USB yet I could see this as being genuinely useful.
More generally I am opposed to adding restrictions on things "just in case". It adds flash and doesn't actually make the system really safer. If someone can send this command then they have full system access and trying to do one-at-a-time "prevent X because I can't think of a use for it" just adds firmware bloat for no actual advantage.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-08-22)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_08_22_1100/devcall_pr_reviews.html

Re-reviewed at head e64b232314 (previous review was at 756d1cbfc8); my earlier comment above is superseded.

There is zero ArduPilot source change in this delta. The only patch-text difference is one index line in Tools/scripts/build_options.py, caused by master renaming ADSB_UAVIONIX → ADSB_MAVLINK underneath the rebase — upstream, not this PR. The substantive effect is that modules/ChibiOS moved from the pre-merge PR-branch commit onto the merged master commit, and git diff between the two trees is empty. So there is no new ChibiOS code to review; the hygiene step is right and done correctly. ArduPilot/ChibiOS#110 merged 2026-08-21. CI is still running (1 passed, 82 pending) so this verdict does not rest on a green board build.

New

The same treatment is still owed to modules/mavlink. The PR points it at d2cb89f50d4, which lives only on the pr-reboot-mass-storage branch — but ArduPilot/mavlink#517 merged on 2026-08-18 as 072c798533, the current master tip, with an identical tree. Pure pointer bump, no content change, and it stops the submodule referencing a commit that is not on master and would become unfetchable if that branch is deleted. Exactly the step just applied to ChibiOS, three days later on the calendar. (To be clear: pointing at an unmerged submodule commit while both are in review is normal practice and was never the objection — only that this one has since merged.)

The MSD worker stack is now fully consumed by the measured read path. USB_MSD_THREAD_WA_SIZE defaults to 256 and ArduPilot overrides it nowhere. Compiled for cortex-m7 -Os with the CubeOrange include set and -fstack-usage, the read path sums to exactly 256 bytes: usb_msd_worker 40 + scsiExecCmd 40 + data_read_write10 88 (was 56 — this PR adds +32) + sdcRead 24 + sdc_lld_read 24 + sdc_lld_read_aligned 40. PORT_WA_SIZE(256) is 692 bytes, of which 128 is the ISR reserve and 308 the context-save reserve, leaving precisely 256 for the thread's own frames — and the total lands there before the leaf calls under sdc_lld_read_aligned. Pre-PR the read path was 224, so this change consumes the remaining margin. CH_DBG_ENABLE_STACK_CHECK will not catch it: it checks only at context switch against the working-area base, so an overrun quietly eats the ISR reserve — intermittent corruption rather than a clean halt. The 32 MiB and 8 MiB transfers passing says it survives today, not that there is margin. Fix is free in flash and costs only heap that exists solely in MSD mode: define USB_MSD_THREAD_WA_SIZE to 768 or 1024. UNCONFIRMED: static worst case only, no on-hardware high-water measurement.

Minor, performance only: block_buffer comes from malloc_type(8192, MEM_FILESYSTEM) and the allocator guarantees only 8-byte alignment. On H7 mem_is_dma_safe() requires address and size to be 32-byte aligned, so every MSD block transfer is routed through the SDC bounce buffer and memcpy'd. 32-byte-aligning the allocation would let it go direct. Almost certainly not the bottleneck at ~800 kB/s, which is near the USB full-speed ceiling given the 64-byte bulk endpoints — recorded only so it is not mistaken for something already handled.

Resolved

The SD retry count (3→9 via sdcard_init_raw(slowdown, tries) with the inner tries = 3 removed from both branches), and the upstream enum-allocation question — mavlink/mavlink#2572 merged 2026-08-19 and ArduPilot/mavlink#517 merged 2026-08-18, so value 5 is upstream-allocated. The "no way out but a power cycle" note is partly addressed: the PR description now says it plainly and there is a code comment, though the commit message is still empty.

Still open

• BRD_SD_SLOWDOWN is still not honoured in MSD mode (libraries/AP_HAL_ChibiOS/USB_MSD.cpp:179, sdcard_init_raw(0, 3)). The comment above it makes the choice explicit — "dedicated to bulk transfers, so deliberately use no SD slowdown for maximum speed" — so this is a deliberate trade, and the independent pass reporting it as a plain bug is arguing against a documented decision. The counter-argument that survives: AP_BoardConfig auto-raises and saves the slowdown when a card will not mount, so boards that need it exist in the field and did not choose the setting. On those the MSD request silently does nothing — which compounds with the next item.
• No user feedback on the mass-storage path. Nothing over MAVLink (the DFU sibling sends "Entering DFU mode"), and if usb_msd_run() returns early the board just boots normally. The path is compiled out on SITL, so autotest never exercises even the armed refusal.
• An explicit --enable-MASS_STORAGE on an incompatible board only emits a self.progress() rather than failing the configure.
• The USB_USE_WAIT TRUE commit still has an empty body — but I withdraw the reservation about its substance, having traced it: it adds a thread reference per endpoint state, NULL-guarded resume loops, and a BASEPRI raise/lower per endpoint-completion interrupt. I checked the one hazard that looked real — _usb_isr_invoke_out_cb dereferences out_state->thread with no NULL check while ArduPilot's interrupt endpoints have out_state == NULL — and it is unreachable, because usb_lld_init_endpoint clears DAINTMSK_OEPM for those endpoints so the handler never runs. No data-path change; worth one sentence in the commit message, not a risk.

Checked and clean

Buffer sizing is right — USB_MSD_IO_SIZE 4096, allocation 8192, and msdStart is passed the per-buffer size, matching max_blocks = blkbuf_size / bs; no overlap, no overrun; bs > blkbuf_size is handled; blk_cnt is uint16_t so the transfer length cannot overflow. The armed guard is correctly ordered — the rejection happens before the magic_force_reboot_value bypass, so that magic cannot force it. An FC/host write collision is impossible by construction: usb_msd_run() runs before usb_initialise() and before main_loop(), so no FATFS mount, no logger and no scheduler ever start, and the function never returns. Feature gating is ordered right in both directions (the hwdef gate runs after process_hwdefs(); the board header is included before the #ifndef default, so the auto-default actually fires). The persistent-data bitfield keeps the existing offsets with compile-time asserts, and a stale flag cannot cause a boot loop.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-08-23)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_08_24_0643/devcall_pr_reviews.html

Re-reviewed at head 4add2a2674 (previous review was at e64b232314); my earlier comment above is superseded.

CI is red, and one of the two failures is exactly the item I raised in the last two rounds — it is now a hard gate rather than advice. Verdict: REQUEST CHANGES, on two counts, both cheap. The PR's own delta since e64b232314 is small: 52 changed patch lines across an ADSB change, a warning suppression, and a boards.py blob-index shift from master drift.

Blocking 1 — modules/mavlink still points at the pre-merge branch commit, and check-merge-commits fails on it

The job's own output is unambiguous:

✗ 3a7d0583e17c references modules/mavlink@d2cb89f50d43: not reachable from master in ArduPilot/mavlink (diverged)
✓ 5bf38e8a68f7 references modules/ChibiOS@9aebaf4a4044: reachable from master in ArduPilot/ChibiOS

Every other check in that job passes — no merge commits, clean subsystem tags, isolated submodule commits — so this one line is the whole failure.

The fix is a pointer bump with provably zero content change. 072c798533 is the merged form of the same work, and its tree hash is 7cacb2ca7ef2629559c8591099cc7db9546c1529 — identical to d2cb89f50d's. Bumping to it clears the check and changes not one byte of XML. Don't bump to mavlink master tip (71d92585) instead: that pulls in two unrelated commits, and ArduPilot master's own pin is 13f2f7351a, three behind 072c798533.

The ChibiOS half is resolved — CI confirms 9aebaf4a40 is reachable from ChibiOS master, which is what last round asked for.

Blocking 2 — the MSD worker stack is over budget, not merely full (upgraded from last round)

Last round I measured the read path at exactly 256 bytes — usb_msd_worker 40 + scsiExecCmd 40 + data_read_write10 88 + sdcRead 24 + sdc_lld_read 24 + sdc_lld_read_aligned 40 — and said the total landed there before the leaf calls under sdc_lld_read_aligned. Those leaf calls are not empty.

sdc_lld_read_aligned calls _sdc_wait_for_transfer_state() at modules/ChibiOS/os/hal/ports/STM32/LLD/SDMMCv2/hal_sdc_lld.c:738, inside its own frame, and my same -fstack-usage data gives _sdc_wait_for_transfer_state_internal a 24-byte frame. So the worst case is ≥ 280 bytes against a 256-byte budget — that much follows from my own measurements with no further assumption.

It gets worse on the wait path. SDC_NICE_WAITING is TRUE (libraries/AP_HAL_ChibiOS/hwdef/common/halconf.h:395), so when the card reports DATA/RCV/PRG the loop calls osalThreadSleepMilliseconds(1); the independent pass rebuilt CubeOrange at this exact head (GCC 10.2.1, -Os) and measured that tail at a further ~64 bytes (chThdSleep 8 + chSchGoSleepTimeoutS 40 + chSchGoSleepS 16) — ~344 bytes on an ordinary READ_10 where the card is briefly busy. Both reviewers reached this independently, from different call chains.

CH_DBG_ENABLE_STACK_CHECK will not catch it: it only checks at context switch against the working-area base, so the overrun quietly eats the ISR reserve. Fix is unchanged and free in flash — define USB_MSD_THREAD_WA_SIZE to 768 or 1024.

New — an unrelated ADSB change with an empty commit message, applied inconsistently

cb6e2b027e ("AP_ADSB: validate Sagetech latitude formatting") adds a snprintf(...) != 10 early return to libraries/AP_ADSB/AP_ADSB_Sagetech.cpp:497 and libraries/AP_ADSB/AP_ADSB_Sagetech_MXS.cpp:624. Two observations, and I'd rather ask than guess:

• It cannot fire for valid input. Latitude is bounded by ±90 so (unsigned)lat_deg is at most 2 digits, minutes are < 60 so 2 digits, and the fraction is ≤ 99999 so 5 — exactly 10 characters, always. The guard only trips on garbage, where returning is the right answer. So it reads as warning-suppression or hardening rather than a bug fix, and the empty commit message leaves no way to tell which.
• If it is warning-suppression, the line immediately above has the same shape and was left alone. The longitude call at AP_ADSB_Sagetech.cpp:494 is snprintf(&pkt.payload[0], 12, "%03u%02u.%05u", ...) — same fixed-width packing, same exact-fit destination (11 chars + NUL into 12), no return check.

Neither is a defect in the ADSB code as it stands; the independent pass checked the arithmetic and agrees the new guards are correct. The problem is a transponder-driver change riding along in a USB-mass-storage PR with no stated reason. Either say why in the commit message or split it out.

New — the warning suppression is broader than the problem

libraries/AP_HAL_ChibiOS/hwdef/common/chibios_board.mk:230 adds -Wno-unterminated-string-initialization to CWARN for the whole C build whenever USE_USB_MSD=yes. The actual trigger is one deliberate fixed-width initializer in ChibiOS's hal_usb_msd.c (the "00000000" serial-number field filling a uint8_t[8]), which ArduPilot reaches because USB_MSD.cpp:203-205 passes nullptr for all four inquiry-response overrides and inherits the driver defaults. Suppressing it across every C file on MSD-enabled boards means a genuine unterminated-string bug elsewhere stops being reported on exactly those boards. Narrower options: a #pragma GCC diagnostic around that initializer, or express the field as an eight-element character initializer. The flag itself is harmless on older toolchains — GCC ignores an unrecognised -Wno- unless another diagnostic fires, and -Werror already forbids that — so this is maintainability, not build risk.

The second CI failure is not yours

build-test fails at ardupilot_dds_tests::test_dds_udp_joy_msg_recv with AssertionError: Could not climb. That's a ROS2/DDS SITL test; mass storage is compiled out on SITL and nothing here touches the copter climb path. The independent pass agrees. Needs a re-run rather than a fix — but it does need to go green, since it's currently one of two red checks.

Still open from previous rounds, none blocking

BRD_SD_SLOWDOWN is still forced to zero in libraries/AP_HAL_ChibiOS/USB_MSD.cpp:179. This remains a documented deliberate trade and I'm not re-arguing it; the counter-argument that survives is that AP_BoardConfig auto-raises and saves the slowdown for boards that can't mount at full speed, so those boards exist in the field and didn't choose the setting. The independent pass adds a concrete mechanism worth a thought: capture the saved slowdown into persistent data when handling the reboot request, so the MSD path can honour it without a filesystem.

Also still open, and I'm recording rather than pressing it: no user-facing feedback on the MSD path (the DFU sibling sends "Entering DFU mode"). Your 2026-08-21 reply to @peterbarker argues against adding restrictions and HAL surface "just in case" because of flash cost, and that argument is sound — a single GCS_SEND_TEXT is a much smaller ask than the UART-type check that thread was about, but it's still flash, and it's your call.

And: block_buffer comes from malloc_type with only 8-byte alignment, so every transfer bounces through the SDC bounce buffer on H7; and an explicit --enable-MASS_STORAGE on an incompatible board only emits a self.progress() rather than failing the configure.

Checked and unchanged: buffer sizing is still right (USB_MSD_IO_SIZE 4096, malloc_type(USB_MSD_IO_SIZE * 2), msdStart given the per-buffer size); the armed guard is still correctly ordered ahead of the magic_force_reboot_value bypass; usb_msd_run() still runs before usb_initialise() and never returns, so an FC/host write collision remains impossible by construction. The USB_USE_WAIT reservation I withdrew last round stays withdrawn.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-08-24)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html

Re-reviewed at head 36c7ec01ba (previous comment was at 4add2a2674); my earlier comment above is superseded. Reviewed for the DevCallTopic call.

Both blockers from the previous round are fixed, and I confirmed each myself rather than taking it on trust. Verdict is still REQUEST CHANGES, but on a new and much narrower item.

Resolved

• git ls-tree 36c7ec01ba modules/mavlink gives 072c7985335c… — exactly the merged commit prescribed, with the tree hash unchanged. Running check-merge-commits' own criterion (the GitHub compare API, which passes on behind/identical) against both submodule refs: ArduPilot/mavlink@072c798533 → behind, ArduPilot/ChibiOS@9aebaf4a40 → identical. Both pass.
• chibios_hwdef.py:1002 now emits #define USB_MSD_THREAD_WA_SIZE 1024, about 3× the ≥280-byte measured read path. Cost is +2×768 = 1536 bytes of heap that exists only in MSD mode.
• The unrelated AP_ADSB commit has been split out of the branch entirely — 7 commits now, no ADSB files in the PR.
• The -Wno-unterminated-string-initialization suppression is narrowed to a single object rule for hal_usb_msd.o, which is exactly right: the target-specific CWARN does reach the compile, $(OBJDIR)/hal_usb_msd.o is the real target name under USE_THUMB, and the usual target-specific-variable footgun doesn't bite because the only generated prerequisite is produced by a waf task, not a make recipe.
• --enable-MASS_STORAGE on an incompatible board is now self.error() (chibios_hwdef.py:3145) with the self.progress() retained for the default case — exactly the split asked for, and it can't break CI.

BUG — MSD is default-enabled on MatekH7A3-Wing, where the reboot request provably cannot survive the reboot

Every link verified in source at this head:

1. usb_msd_boot_requested() reads the flag via stm32_watchdog_load().
2. stm32_watchdog_load() is get_rtc_backup(1, data, nwords) (hwdef/common/watchdog.c:150).
3. With AP_FASTBOOT_ENABLED == 0, get_rtc_backup() and set_rtc_backup() are empty no-ops (hwdef/common/stm32_util.c:275-292).
4. hwdef/scripts/STM32H7A3xx.py:59 forces 'AP_FASTBOOT_ENABLED' : '0' for the whole H7A3 family.
5. The new default-enable gate (chibios_hwdef.py:3134-3145) tests mcu_series.startswith('STM32H7') — which H7A3 matches — plus program size, SD and USB. It does not test fastboot.

So the request is written into a discarded backup register and read back as zero: the board boots normally and nothing is said. usb_msd_boot_requested() zero-initialises its local PersistentData, so the no-op load leaves it false rather than garbage — silent, not erratic.

Scope is exactly one board. STM32H7A3xx.py is the only H7 MCU script that forces fastboot off, and of the two H7A3 boards MatekH7A3 is excluded by its DATAFLASH littlefs:w25nxx line (so have_sdcard is false) while MatekH7A3-Wing passes every clause — MCU STM32H7xx STM32H7A3xx, FLASH_SIZE_KB 2048, SPIDEV sdcard, OTG1. Fix is to add the fastboot intdefine to the gate. This compounds with the still-open "no user feedback" item: the user gets no MSD device and no message.

NOTE — the enablement predicate is implemented twice and the two disagree on ~19 boards

AP_HAL/board/chibios.h:33-43 gates only on STM32H7 && HAL_PROGRAM_SIZE_LIMIT_KB >= 2048, while chibios_hwdef.py:3143 additionally requires SD and USB. On an H7 board that logs to dataflash the two diverge: AP_REBOOT_MASS_STORAGE_ENABLED is 1 while HAL_USB_MSD_BOOT_ENABLED is 0. Consequences are mild — the MAVLink branch compiles in and always answers MAV_RESULT_UNSUPPORTED, plus a vtable slot and an extract_features.py mismatch. The ODR hazard this pattern usually carries is not present (AP_HAL/Util.h:6 pulls in AP_HAL_Boards.h transitively, so the macro is identical in every TU). Cheapest fix is to have the hwdef emit the define it already computed.

NOTE — the F405 hardware test predates the stack bump

USB_MSD_THREAD_WA_SIZE sizes both waMSDWorker and waMSDIOWorker, so 256→1024 adds 1536 bytes to the calloc at USB_MSD.cpp:183, on top of the 8 KiB malloc_type. On MatekF405-STD, listed as tested, that's a meaningful slice of early-boot heap. The failure path is graceful in code (msdp == nullptr → free → return) but silent to the user. Worth one re-run on the F405 before merge — I have no F4 heap figure at that point in boot.

Also, for the record so nobody re-raises it: the PR removes the old HAL_USE_USB_MSD hwdef escape hatch, and no hwdef.dat in the tree uses it (all of hwdef/*/hwdef*.dat grepped).

Still open, unchanged, none blocking

BRD_SD_SLOWDOWN forced to 0 at USB_MSD.cpp:179 — documented deliberate trade, not re-arguing it, though one concrete option is to capture the saved slowdown into persistent data at reboot-request time so the MSD path could honour it without a filesystem. No user-facing GCS message on the MSD path. block_buffer's 8-byte alignment forcing every H7 transfer through the SDC bounce buffer.

One finding from the independent pass I'm not carrying: that the lowercase --enable-mass-storage alias fails to set the define. This PR's own gate at boards.py:1143-1148 explicitly checks both spellings; any weakness is in the generic build-option loop, which is untouched here and identical on master.

CI at this head: 40 passing, 58 pending, 0 failing — neither previously-red check has reported yet.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-08-24)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html#pr34087

Re-reviewed at head 7cbca7c105; my earlier comment above (at 36c7ec01ba) is superseded. The MatekH7A3-Wing bug from the previous round is fixed exactly as prescribed — verified by tracing the ordering and running the gate over all 450 hwdefs — and the two-predicate note is resolved too. But an independent cold pass found that the feature cannot be toggled through the standard build_options.py/extra-hwdef mechanism the PR itself registers, and I reproduced it by generating hwdef.h. Verdict: REQUEST CHANGES on that one item.

Previous findings:

• RESOLVED — fastboot gate: libraries/AP_HAL_ChibiOS/hwdef/scripts/chibios_hwdef.py:3134-3136 reads AP_FASTBOOT_ENABLED from intdefines with the MCU script's DEFINES as fallback (the right source — MCU-script defines are never merged into intdefines) and ANDs it into both default_mass_storage and usb_mass_storage_enabled. Over all 450 hwdef.dat: 139 default-enabled (previous 140 minus MatekH7A3-Wing), explicit --enable-MASS_STORAGE on either H7A3 board now errors with the new text.
• RESOLVED — predicate implemented twice: :998 now emits AP_REBOOT_MASS_STORAGE_ENABLED from the computed value, and chibios.h:3 includes hwdef.h first so the fallback never fires; the two macros agree on every board.
• STILL OPEN (non-blocking) — F405 hardware test predates the 256→1024 stack bump (+1536 B early-boot calloc); one re-run on MatekF405-STD still worth it.
• Unchanged and not re-argued: BRD_SD_SLOWDOWN forced to 0, no GCS text on the MSD path.
• WITHDRAWN — my earlier note that block_buffer's 8-byte alignment forces every H7 transfer through the SDC bounce buffer was wrong: MEM_FILESYSTEM → malloc_axi_sram() → malloc_flags(size, MEM_REGION_FLAG_AXI_BUS) gives 32-byte alignment on H7 and rounds the size (malloc.c:60,125,139,373-376). Codex caught it; verified.

New:

• BUG — Tools/scripts/build_options.py:423 registers MASS_STORAGE with define AP_REBOOT_MASS_STORAGE_ENABLED (default 0). The custom build server and test_build_options.py toggle features by writing define AP_REBOOT_MASS_STORAGE_ENABLED 1 (or 0) into the extra hwdef, not via --enable-MASS_STORAGE, and the gate at chibios_hwdef.py:3140 reads only self.mass_storage_option. Reproduced with the head's chibios_hwdef.py: MatekF405-STD + extra define … 1 yields a hwdef.h with #define AP_REBOOT_MASS_STORAGE_ENABLED 0 (line 46, from :998) and #define AP_REBOOT_MASS_STORAGE_ENABLED 1 (line 161, the extra define), no HAL_USB_MSD_BOOT_ENABLED; CubeOrange + define … 0 gives 1 at line 46 and 0 at line 92. A macro redefined with a different value is a warning; with the standard 10.2.1 toolchain -Werror is off (boards.py:954-961 whitelists only GCC 11.3/11.4/12.1), so the last define wins: an F4/F7 custom build advertises the reboot action while always answering MAV_RESULT_UNSUPPORTED, and an H7 build with the feature deselected is off at runtime but still carries the MSD objects. With a whitelisted compiler or --Werror it simply fails to build. Fix: treat self.intdefines.get('AP_REBOOT_MASS_STORAGE_ENABLED') as the request when mass_storage_option < 0 (erroring, as for --enable, if it's 1 on an unsupported board); the generic define writer then re-emits the same value, which is a permitted identical redefinition.
• ISSUE — the gate also can't see ./waf configure --define AP_FASTBOOT_ENABLED=0 (waf --define only reaches compiler flags, boards.py:269-270), so an eligible H7 board built that way gets HAL_USB_MSD_BOOT_ENABLED 1 with the persistence compiled as no-ops — the H7A3 failure again, via a developer override. Low impact (a hwdef define line is the supported way and works), but a two-line guard in chibios.h (#if !AP_FASTBOOT_ENABLED → undef/0 HAL_USB_MSD_BOOT_ENABLED) would make the C side self-consistent however fastboot is turned off. (Codex; verified.)
• NOTE — libraries/AP_HAL/board/chibios.h:36-42 is now dead code restating the old, weaker predicate; dropping it (keeping the bootloader/periph #undef) leaves one source of truth.

Not carried from the cold pass: a claim about lib_scsi.c CSW handling after a media failure — that is upstream ChibiOS SCSI-binding code in the submodule, not this PR's diff. CI at this head: 1 pass / 82 pending; at the previous head all 99 checks including check-merge-commits were green, and this delta is Python-only.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-08-24)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html#pr34087

Re-reviewed at head 95b16c2424; my earlier comment above is superseded. The custom-build define bug is fixed exactly as prescribed, and the fix was verified by executing the head's chibios_hwdef.py over ten board/extra-hwdef combinations — plus the PR body now carries a MatekF405-STD hardware test at this exact head, which retires the stack-bump question too. Verdict: APPROVE — no blockers.

• RESOLVED (was the blocker) — the gate now reads AP_REBOOT_MASS_STORAGE_ENABLED from intdefines when no --enable/--disable was given (chibios_hwdef.py:3142-3146) and the define writer skips the user's line (:1068-1069) so the computed value is the single emission. Executed: MatekF405-STD + define … 1 → enabled (one #define, USB_MSD_THREAD_WA_SIZE 1024); CubeOrange + define … 0 → single 0, progress line suppressed; explicit 1 on MatekH7A3-Wing/dataflash boards → hard configure error with the right message; plain configs unchanged. The redefinition hazard is gone in every case. Custom-build-server interaction checked including AP_Periph (its undef X + define X pattern is handled; ticking MASS_STORAGE for a periph fails configure loudly — same contract as --enable).
• RESOLVED — the F405 hardware retest: the new test at 95b16c24 runs the bigger thread WAs and the +1536 B early-boot calloc on real hardware (14.8 GiB card, 851 kB/s, clean one-shot consumption).
• Still open, non-blocking — --define AP_FASTBOOT_ENABLED=0 remains invisible to the gate (a #if defined(AP_FASTBOOT_ENABLED) && !AP_FASTBOOT_ENABLED guard in chibios.h would close every override path at once — the bare form would fire where the macro is undefined), and the dead chibios.h:33-42 fallback still restates the old predicate — dropping it leaves one source of truth.
• NOTE — because the custom build server writes an explicit define for every feature and the build_options.py default is 0, custom H7 builds with untouched checkboxes get MSD off while official builds default it on. Forced by design; stated so nobody is surprised.

CI: 38 pass / 60 pending at this head; python-cleanliness green; the delta is Python-only.

[tridge]

Deprecated — see below for the updated review after the build-gating rework.

Previous review (at head 0381590)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Full report: https://uav.tridgell.net/DevCallReviews/DevCallTopic/devcall_pr_reviews.html#pr34087

Re-reviewed at head 0381590f74; my earlier comment above is superseded. The change since last round is a pure refactor. Verdict: APPROVE — no blockers.

The 46-line delta extracts the USB mass-storage gate from run() into a new setup_usb_mass_storage() method, called from the same point in run(). The gate block (flash/program-size, the fastboot_enabled read, default_mass_storage, the AP_REBOOT_MASS_STORAGE_ENABLED intdefine fallback, and the error/progress messages) moves unchanged, so every outcome I verified by executing the generator last round still holds. History is re-split into per-subsystem commits (Tools / waf / GCS_MAVLink / AP_HAL_ChibiOS).

Still open, non-blocking and unchanged by the refactor: --define AP_FASTBOOT_ENABLED=0 remains invisible to the gate, and the dead chibios.h fallback still restates the old predicate. CI 1 pass / 82 pending.

[tridge]

make it only prog size limit, not MCU

[peterbarker]

This stuff is supposed to go into defaults_periph.h and defaults_bootloader.h

[peterbarker]

Suggested change

	#if defined(STM32H7) && HAL_PROGRAM_SIZE_LIMIT_KB >= 2048
	#if HAL_PROGRAM_SIZE_LIMIT_KB >= 2048

[peterbarker]

H7 again

[peterbarker]

User None as a flag value rather than -1

[peterbarker]

Let's see if we can get rid of this.

[tridge]

@/tmp/claude-1000/-home-tridge-project-Maintainer/f692e5eb-5446-4757-99e0-f4c13fdfa9f0/scratchpad/tr4-1787741949/dep_34087.md

[peterbarker]

Good to go past the one patch removal

[tridge]

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_08_26_2116/devcall_pr_reviews.html

Re-reviewed at head 9a03d444fa (my earlier comment above is superseded); previously reviewed at 0dfa8566a6. Cross-checked by a second independent AI pass (Codex).

Verdict on the previous round: 0 of 3 prior findings resolved (all narrow, non-blocking) — but the structural blocker is now fixed. The push is a rebase that drops the modules/mavlink bump so the PR points at master's pin (which already carries REBOOT_SHUTDOWN_ACTION_REBOOT_TO_MASS_STORAGE=5), permanently closing the check-merge-commits failure. peterbarker has since approved.

Verdict: COMMENT — no blockers. The three carried-over edge cases remain (re-raised, non-blocking):

• USB_MSD.cpp:186 — on the alloc-failure path, sdcard_init_raw(0,3) has set sdcard_running=true but the guard returns without sdcard_stop(), so the boot mount short-circuits and the card reports mounted while unmounted for that boot. One-line fix: sdcard_stop() before the return.
• Scheduler.cpp:298 — StopLogging()/unmount() are under #if HAL_LOGGING_ENABLED, but the MSD feature doesn't require logging, so a --disable-Logging build can reboot to mass storage without unmounting. Gate the unmount on filesystem availability, not logging.
• reboot-to-storage doesn't sync open non-logger files (f_mount(nullptr,...) doesn't flush live FILs) — an in-flight MAVFTP upload can lose cached metadata. Quiesce/sync, or refuse the action, while writable handles are open.
• (unchanged) --define AP_FASTBOOT_ENABLED=0 is invisible to the hwdef gate; a hwdef define line works.

New code (gating compile-out, armed-refusal ordering, the dropped submodule bump) reviewed — no new findings. Codex additionally flagged two lib_scsi.c SCSI-stage issues, but those are inside the ChibiOS submodule (its own PR), not this diff.