Conversation
… media as USB drives Implements the MscFunction extension point on espp::UsbDevice, built on esp_tinyusb's MSC storage backend (which provides the SCSI handling): - MscFunction exposes one or two media (LUNs): an initialized SD card (SDMMC or SDSPI host) and/or a `data, fat` flash partition, which the device mounts through wear levelling. - Ownership model: while the application owns a medium its FAT volume is mounted at MscMedium::base_path (fopen / std::filesystem); while the host owns it the path is unmounted and the PC sees the drive. With auto_handover (default) the host takes the media on mount and the app gets them back on eject / detach; set_msc_owner() hands them over explicitly. - msc_owner(), msc_capacity(), msc_lun_count(), format_msc_medium() and an MscEvent callback (hand-over started / done / failed, format required / failed) report and manage the state; set_msc_owner() reports failures esp_tinyusb's setter swallows. - One interface + bulk IN/OUT from the sequential allocator (endpoint budget unchanged); media are created before the TinyUSB driver starts so an already-connected host finds them, and torn down after it stops. - Validation: 1..2 media, at most one of each type (esp_tinyusb backends are singletons), distinct absolute base paths, SD media only on targets with an SDMMC host; CONFIG_TINYUSB_MSC_ENABLED is required. New msc_example exposes a 1 MiB flash FAT partition, writes a boot counter + README from the app, and lists the directory again after the host ejects the drive. README, docs page, Doxygen, CI matrix and the component manifest are updated; the "(future)" MSC notes are gone. Documented esp_tinyusb limits: formatting runs on FatFs drive 0 (only safe with no other FAT volume mounted), fixed SCSI inquiry strings, and LittleFS / SPIFFS cannot be exposed (hosts only read FAT). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds USB Mass Storage Class (MSC) support to espp::UsbDevice, allowing SD cards and/or FAT flash partitions to be exposed as USB drives with an explicit app/host ownership hand-over model.
Changes:
- Implemented
MscFunction(descriptor allocation + esp_tinyusb MSC storage backend integration) with ownership APIs and event callbacks. - Added a new
msc_exampleproject plus CI build entry. - Updated docs/README/Doxygen inputs to document and reference MSC support and the new example.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| doc/en/buses/usb_cdc.rst | Documents MSC feature, usage, and endpoint budget table updates. |
| doc/en/buses/msc_example.md | Adds doc page that includes the example README. |
| doc/Doxyfile | Adds the MSC example source to Doxygen example inputs. |
| components/usb_device/src/usb_device.cpp | Implements MSC driver install/storage creation, descriptor wiring, ownership/format APIs, and event bridging. |
| components/usb_device/include/usb_device.hpp | Adds public MSC types (MscFunction, MscMedium, events/owners) and new MSC APIs. |
| components/usb_device/CMakeLists.txt | Adds required/public deps for MSC types and private deps for flash MSC backend. |
| components/usb_device/idf_component.yml | Updates description/tags and registers msc_example. |
| components/usb_device/README.md | Adds MSC documentation and references new example. |
| components/usb_device/msc_example/* | New example project: config, partition table, README, and main implementation. |
| .github/workflows/build.yml | Adds CI build job for msc_example. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- static analysis: drop the constParameterPointer suppression CI's cppcheck reports as unmatched, and restructure the SD-card validation so no statement follows the return selected when the target has no SDMMC host (unreachableCode) - FormatFailed now logs a format-specific message (esp_tinyusb passes no error code; its own log has the FatFs result); OwnerChangeFailed names the side the hand-over was going to - comments at the event bridge (esp_tinyusb emits MOUNT_START before updating the owner and MOUNT_COMPLETE after, so event->mount_point is the previous / new owner) and at the base_path assignment (the field is a non-const char *, hence data()) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
There was a problem hiding this comment.
🟡 Changes recommended
Ownership failure detection and teardown can report success or release resources while the backend still retains them.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
components/usb_device/src/usb_device.cpp:982
- Combining MSC with X-Input still satisfies the existing
xinput_onlypredicate because it only excludes CDC, vendor, and HID. That makes an X-Input+MSC composite advertise the Microsoft VID/PID and 0xFF device class intended only for a standalone controller. Include!config_.mscin that predicate so composites retain the configured identity.
.github/workflows/build.yml:360 - The example matrix is required to stay alphabetized. Place
msc_examplebeforexinput_example.
doc/Doxyfile:206 EXAMPLE_PATHentries are required to remain alphabetized. Move the MSC example before the X-Input example.
components/usb_device/src/usb_device.cpp:2169
tinyusb_msc_delete_storage()can fail while deferred writes are pending. Clearing the handle anyway and then unmounting wear levelling leaves esp_tinyusb's live LUN referencing released resources; the ignored driver-uninstall failure also prevents a later instance from initializing. Only clear/release resources after successful deletion, and explicitly handle or retry this failure.
if (lun.storage) {
esp_err_t err = tinyusb_msc_delete_storage(lun.storage);
if (err != ESP_OK)
logger_.warn("MSC medium {}: deleting the storage failed: {}", i, esp_err_to_name(err));
lun.storage = nullptr;
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Balanced
…er against the VFS - esp_tinyusb frees a storage object still mapped as a LUN when the mount it performs during creation fails, returning no handle: the stale LUN could never be removed and SCSI requests would reach freed memory. Media are now always created host-owned (nothing can fail after the handle is ours) and handed to the application afterwards when initial_owner is App. A missing filesystem stays non-fatal; any other mount failure fails initialize() and tears the media down cleanly. - set_msc_owner() and the initial hand-over go through hand_over_msc(), which confirms the outcome with esp_vfs_fat_info() instead of relying on events: esp_tinyusb's setter records the requested owner whatever the mount / unmount did, and several failure paths raise no event. A failed mount (other than "no filesystem", which stays app-owned for formatting) quietly resets the medium to the host and reports io_error; a volume still mounted after a hand-over to the host is unregistered before success is reported, else io_error. - OwnerChangeFailed carries the side that still has the medium: the log now names the attempted destination, and the callback doc says so. - The creating-LUN event fallback is gone (handles are known before any hand-over now). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
There was a problem hiding this comment.
🟡 Changes recommended
MSC teardown, repeated unformatted handovers, dependency compatibility, and SD-card lifetime documentation need correction.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
components/usb_device/include/usb_device.hpp:532
- This API may run storage operations from an application task while
auto_handoverruns them in the TinyUSB task, but the component manifest still permits esp_tinyusb 2.0.0. Espressif's changelog says MSC storage-operation multitask protection was added only in 2.0.1, so projects locked to 2.0.0 can race these paths. Raise the esp_tinyusb dependency floor to>=2.0.1(or add equivalent serialization here).
.github/workflows/build.yml:360 - The CI matrix is kept alphabetized (the surrounding entries run through
touch,tla2528,tt21100,twai,usb_device, thenusb_host), but this placesmsc_exampleafterxinput_example. Putmsc_examplebeforexinput_exampleto preserve that ordering.
components/usb_device/include/usb_device.hpp:259 - This suggests obtaining
sd_cardfromesp_vfs_fat_sdspi_mount()and then unmounting it, but ESP-IDF's matchingesp_vfs_fat_sdcard_unmount()frees the helper-allocatedsdmmc_card_t. Passing that pointer here would therefore leave MSC with a dangling card. Document only caller-owned storage initialized withsdmmc_card_init()(as the example already does).
doc/Doxyfile:206 EXAMPLE_PATHis explicitly required to remain alphabetized (doc/Doxyfile:72-74), butmsc_exampleis inserted afterxinput_example. Move it between the base USB example andxinput_example.
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
…f-review fixes Review: - deinit_msc() only releases what sits behind a storage object that was actually deleted: a failed tinyusb_msc_delete_storage() keeps the handle and its wear-levelling mount (the LUN is still mapped), and the MSC driver is marked uninstalled only when uninstall succeeds. - Asking for the application again for an unformatted medium now returns no_such_device instead of being misread as a failed mount and reverted to the host: a per-LUN no_filesystem flag (set on FormatRequired, cleared on a successful mount or format) survives esp_tinyusb's same-owner no-op, which emits no event. A medium marked application-owned with nothing mounted for another reason is reset quietly and mounted again. Self-review: - Host writes are queued and run later on the TinyUSB task, and a storage object with writes queued cannot be deleted. The destructor stopped the task first, which lost the last queued writes and leaked the storage. It now drops the pull-up (tud_disconnect), deletes the media while the task still runs, retrying (bounded, 1 s) until queued writes have run, and only then uninstalls the driver. - format_msc_medium(): esp_tinyusb reports "no free FatFs drive" with the same ESP_ERR_NOT_FOUND as "filesystem exists", so that case was reported as file_exists. A mounted volume and a missing drive slot are now checked first (file_exists / device_or_resource_busy). - set_msc_owner() doc: a host mount / eject during the call races it with auto_handover on; msc_owner() doc: an unformatted medium reports App. - fatfs added to PRIV_REQUIRES (esp_vfs_fat_info, ff_diskio_get_drive). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
|
Self-review of the full MSC diff (fixes in 64e1850, alongside the two review threads above). Found and fixed:
Checked and left as is:
Known limits that remain, all in esp_tinyusb and documented:
Verified: |
There was a problem hiding this comment.
🟡 Changes recommended
MSC teardown can abort or strand backend state, and X-Input/MSC composites receive the wrong USB identity.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
components/usb_device/src/usb_device.cpp:997
- An MSC-enabled X-Input composite still satisfies the existing
xinput_onlypredicate below because that predicate does not checkconfig_.msc. The device will therefore advertise the Xbox VID/PID and 0xFF device class even though MSC is also present, contradicting the documented X-Input-only identity and preventing normal composite binding. Include!config_.mscinxinput_only(or reject this combination).
.github/workflows/build.yml:360 - The MSC matrix entry is after
xinput_example, so this section is no longer alphabetical as required by the repository instructions. Movemsc_examplebeforexinput_example.
doc/Doxyfile:206 - The new example is out of alphabetical order:
msc_examplemust precedexinput_example. The repository instructions requireDoxyfileentries to remain alphabetized when an existing component changes.
components/usb_device/src/usb_device.cpp:528
- If the one-second drain times out, the first
deinit_msc()intentionally leaves the storage mapped, but these lines still stop the TinyUSB task and retry deletion afterward. Deferred writes can no longer complete, so deletion remainsESP_ERR_INVALID_STATE; the MSC driver and backing storage survive whileimpl_(including the borrowedbase_path) is destroyed, and a later instance cannot install the global MSC driver. Make teardown report whether all LUNs were released and do not stop TinyUSB until release is guaranteed.
// Anything the drain above could not release (normally nothing).
deinit_msc();
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
…re deletion An unformatted medium (or one whose mount failed) is marked application-owned with no drive registered. esp_tinyusb's tinyusb_msc_delete_storage() does ESP_ERROR_CHECK(msc_storage_unmount()), whose medium unmount returns ESP_ERR_INVALID_STATE with no drive registered, aborting the device on teardown. deinit_msc() now resets such a medium to the host first (the setter records the owner even though its own unmount fails), so the delete skips the unmount. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
There was a problem hiding this comment.
🟡 Changes recommended
The allowed backend version and some teardown paths can lose deferred writes or leave dangling MSC resources.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
components/usb_device/src/usb_device.cpp:1480
- This failure path stops the TinyUSB task before deleting MSC storage, unlike the destructor's required ordering. A host can begin enumeration immediately after
tinyusb_driver_install(), so if CDC initialization then fails with an MSC write deferred,deinit_msc()cannot drain it after uninstall and leaves the MSC singleton/storage installed; subsequent initialization will remain busy. Disconnect and run the timed MSC drain while the TinyUSB task is still alive, then uninstall it.
.github/workflows/build.yml:360 - The CI matrix is required to remain alphabetical. Move
msc_examplebeforexinput_exampleso thecomponents/usb_device/*entries stay ordered.
doc/Doxyfile:206 - The Doxygen example list is required to remain alphabetical. Place
msc_examplebetweenexampleandxinput_example; the current insertion afterxinput_examplebreaks the maintained ordering.
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
…usb >= 2.0.1; composite identity - Teardown: MSC media are released by release_msc_before_uninstall() (disconnect, delete while the TinyUSB task still runs queued writes, bounded 1 s), and deinit_msc() now reports whether everything was released. If a storage object is still mapped, the driver is NOT uninstalled and impl_ is deliberately leaked (esp_tinyusb still points at its base_path strings and media) instead of stopping the task and freeing state it references. The initialize() CDC-init failure path uses the same ordering (a host may already have queued writes). - esp_tinyusb floor raised to >= 2.0.1: that release adds MSC storage-operation multitask protection and the delete_storage() guard against freeing a storage with queued writes (verified: esp-usb commit ec187f0 is in the v2.0.1 release). The vendored copy is 2.2.1. - An X-Input + MSC composite no longer takes the standalone Xbox 360 identity (xinput_only now also requires !msc). - MscMedium::sd_card doc: caller-owned card from sdmmc_card_init(), not the one esp_vfs_fat_sd*_mount() allocates (its unmount frees it). - msc_example ordered before xinput_example in the CI matrix and Doxyfile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU
|
Also addressed in afcbe06: the comments Copilot suppressed in its review summaries.
Verified: |
There was a problem hiding this comment.
🟡 Changes recommended
Manual ownership transitions and formatting can race pending host I/O, and dual-LUN ejection has incorrect per-medium semantics.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
components/usb_device/include/usb_device.hpp:295
- This auto-handover description is not true per medium when two LUNs are configured. esp_tinyusb's
tud_msc_start_stop_cbignores the ejected LUN and callsmsc_storage_mount_to_app(), which iterates every storage, so ejecting either host drive returns both media to the application and makes the other drive disappear too. Either use/pin a backend with per-LUN eject handling or explicitly expose/document auto-handover as an all-media operation.
- Files reviewed: 15/15 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (tinyusb_msc_set_storage_mount_point(lun.storage, to_app ? TINYUSB_MSC_STORAGE_MOUNT_APP | ||
| : TINYUSB_MSC_STORAGE_MOUNT_USB) != | ||
| ESP_OK) { |
| return false; | ||
| } | ||
| } | ||
| const esp_err_t err = tinyusb_msc_format_storage(l.storage); |
Implements the long-reserved
MscFunctiononespp::UsbDevice: expose an SD card and/or a FAT partition in flash as USB drives, while the firmware keeps reading and writing files on the same media through the VFS. Motivated by esp-box-emu, which exposes its uSD card over MSC with hand-rolled TinyUSB descriptors today.How it works
esp_tinyusb (≥ 2.0) already ships a complete MSC storage backend: SCSI callbacks, SD card and wear-levelled flash media, and the VFS hand-over between app and host. It also drives that hand-over from its own
tud_mount_cb/tud_umount_cb, which still fire underUsbDevice. So this PR does not add a block layer. It adds the MSC interface to the sequential descriptor allocator and wraps the storage backend in the component's API.Ownership model. Each medium belongs to one side at a time, so the firmware and a PC never write the same FAT volume at once:
MscOwner::Appbase_path(fopen,std::filesystem)MscOwner::Hostbase_pathunmountedWith
auto_handover(default) the host takes the media when it mounts the device, and the app gets them back on eject or detach. Turn it off to callset_msc_owner()yourself (e.g. esp-box-emu's "USB drive" toggle).API
set_msc_owner(lun, owner, ec), reporting failures that esp_tinyusb's own setter silently ignores (no_such_devicefor an unformatted medium,io_errorotherwise)msc_owner(lun),msc_capacity(lun),msc_lun_count(),format_msc_medium(lun, ec),set_msc_event_callback()CONFIG_TINYUSB_MSC_ENABLEDrequired.Example
New
components/usb_device/msc_example, which needs no SD card, so it runs on any ESP32-S3 board. It exposes a 1 MiB flash FAT partition. On boot the app writes a boot counter and a README to it. The host sees those files when it mounts the drive, and after the host ejects, the app lists the directory again, including anything the PC added. Added to the CI matrix, the docs toctree and Doxygen. The README and docs page gain an "Enabling mass storage (MSC)" section, and the "(future)" notes are gone.Limits worth knowing (all from esp_tinyusb's backend, documented in the header / README)
f_mkfs("")).format_if_unformatteddefaults off and is documented as safe only when no other FAT volume is mounted. This looks like an upstream bug worth reporting to esp-usb.TinyUSB/TEST MSC Storage. They are not weak symbols, so they cannot be overridden without replacing the backend.UsbDeviceunmounts an app-owned medium'sbase_path. An SD card stays initialized but must be mounted again if the app still needs it. For esp-box-emu's enable/disable-USB flow that means remounting/sdcardafter turning USB off. A small remount helper could be a follow-up.Verified
msc_examplebuilds for ESP32-S3 with and without the component manager (the CI command)example(CDC + vendor) andxinput_examplestill build (MSC-disabled path)🤖 Generated with Claude Code
https://claude.ai/code/session_01DFjjnq3XCTRAXENSxsCxJU