shared/usb: correctly report the SD LUN as removable media#10967
shared/usb: correctly report the SD LUN as removable media#10967mikeysklar wants to merge 2 commits intoadafruit:mainfrom
Conversation
Respond with "unsupported" for the SD LUN (removable media) and OK for internal flash / SAVES LUNs (non-removable). Responding OK for a removable LUN tells macOS the medium is always present, and macOS then skips TEST_UNIT_READY polling. If the SD isn't ready at the single enumeration probe, macOS never re-checks and LUN 1 fails to publish an IOMedia node. Hathach documented this behavior back in adafruit#6555; the SD case wasn't differentiated at the time. Fixes adafruit#10965.
tannewt
left a comment
There was a problem hiding this comment.
I think we want them all removable.
Per @tannewt review on adafruit#10967: tinyusb advertises is_removable=1 for every LUN in its default INQUIRY response (msc_device.c line 781), so the per-LUN branch was inconsistent with what the host already sees. Collapse to a single unconditional ILLEGAL_REQUEST reply — host keeps polling TUR on all LUNs, so eject/re-mount works uniformly across CIRCUITPY, SAVES, and SD.
|
You're right, thanks — simplifying to apply the unsupported reply to all LUNs. |
|
Quick cross-board, cross-OS regression test of this PR combined with #10963 on
Notes:
|
|
It would be good to test on Windows as well, and also on boards that are not RP2xxx, such as a PyPortal, and a Metro ESP32-S3 (onboard SD socket), etc. I'm assuming you don't have a Windows box or you would have done so. I could test some of these on Windows. |
|
I don't think that reporting |
I agree with this. |
|
Thanks both. On Comparing the two versions for the 3-LUN case (Fruit Jam: CIRCUITPY + SAVES + SD): The per-LUN version keeps identical user-visible behavior (eject buttons from RMB=1 are unchanged, SD re-insert still auto-remounts) but drops 2/3 of the poll traffic on 3-LUN boards. @tannewt — OK to revert to per-LUN given the polling-cost point? On Windows / non-RP2xxx boards: I don't have a Windows host, but I do have a PyPortal and a Metro ESP32-S3 here — happy to run macOS + Linux checks on both. @dhalbert — gladly hand off Windows coverage to you. Let's agree on which version of the PR we're testing against (current / per-LUN revert) before I spin up the non-RP2xxx builds, so we're all benching the same stack. |
|
I don't think the polling cost issue has been proven out. The linked issue says it occurs even when a host isn't connected. When I've looked at traces from Linux it only queried the drive state once a second. That'd shouldn't be too much overhead. Although the flash can't be physically removed it is advantageous to "eject" it. The host flushes before doing this and then sends a signal to the device to eject. We have to remember this state so it doesn't immediately pop up again. Now, we can do |
|
Ran bablokb's chaos benchmark on a Metro RP2040 to check the polling overhead question. 5 passes each, SD auto-mounted by firmware, USB host connected (macOS):
~6% difference, ~13ms absolute. The 3–4x slowdown in #10733 appears to be a separate CP9→CP10 regression unrelated to the removable flag — current code shows no significant overhead either way. Agreed on keeping all LUNs removable for the eject/ |
|
Tested 4 boards across 4 architectures with firmware built from No user code — SD mounted via
macOS: both CIRCUITPY and SD appear as separate volumes, eject/reinsert works cleanly. Linux: both mount read-only as expected (USB MSC path). No regressions observed on any board. |
Did you average from POR or did you run 5 iterations in one go? In the latter case, averaging 5 passes is not necessarily relevant. Many use-cases for microcontrollers use a "startup-work-deepsleep" scenario. So the results for the first pass are more interesting. You can see this in the tests that Dan and I ran in #10733. Anyway, 6% difference is relevant because this adds up with all the other things that slow down CircuitPython. So I would really like to see this "feature" compile-time configurable. |
|
@bablokb — you were right on both counts. Methodology: the 5 passes were in-loop from a single soft-reset, not power-cycled. That captured steady state only. Re-ran with physical power cycles (unplug/replug → full USB re-enumeration) on the same Metro RP2040:
First-pass overhead: ~2.86× (not 6%). The slow phase lasts ~20s after USB re-enumeration then drops sharply. For a startup-work-deepsleep program that finishes in under 20s, every run is ~3× slower — exactly the scenario you described. The 6% figure was real but measured the wrong thing: it's the steady-state delta once the host has settled its polling. First-boot cost is what matters for your use case. Compile-time config: the data now supports this more strongly. The SD LUN has to keep replying ILLEGAL_REQUEST for the macOS fix to work, but CIRCUITPY/SAVES don't need it for the fix itself. A flag like @tannewt — given the ~20s first-boot overhead on the all-LUN path, does a compile-time opt-out make sense, or does the eject benefit on CIRCUITPY/SAVES outweigh this for typical use? |
What was wrong
The SD card exposed as a USB MSC LUN doesn't mount on macOS.
ioregshows the SCSI nub appears but never progresses to publishing anIOMedia. Linux handles the same firmware fine.Why: CircuitPython was telling the host "this LUN's medium is always present" for every LUN, including the SD card. On macOS that means "skip
TEST_UNIT_READYpolling" — so if the SD wasn't ready at the one-shot enumeration probe, macOS never re-checked and gave up on the LUN. Linux ignored the hint and kept polling, which is why it worked there.@hathach documented this exact macOS behavior back in #6555; the per-LUN distinction wasn't applied at the time.
What changed
Report the SD LUN as removable media, report internal flash and SAVES as non-removable. macOS now keeps polling TUR on the SD LUN and correctly picks up card insertion and removal.
~10 lines in
supervisor/shared/usb/usb_msc_flash.c— per-LUN branch on the existingPREVENT_ALLOW_MEDIUM_REMOVALhandler. No RAM or flash cost.Tested on
/dev/sdc+/dev/sddcorrect sizes, no regressionWindows not tested.
Related
Credit to @dhalbert for pointing at #6555 and @hathach for the original analysis.