fix(usb_device): don't redefine tud_umount_cb (esp_tinyusb owns it); add mount/unmount hooks - #779
Conversation
…d mount/unmount hooks #774 added a tud_umount_cb definition to usb_device.cpp, but esp_tinyusb already defines it (tinyusb.c) -> "multiple definition of tud_umount_cb", breaking every manager-off USB example link (bldc_haptics, can_bridge, coredump, mcp266, ota, usb_device). esp_tinyusb owns the TinyUSB device lifecycle callbacks and forwards them to a tinyusb_config_t::event_cb, so: - remove usb_device's tud_umount_cb definition; - register an event_cb (event_arg = this) and route ATTACHED/DETACHED to handle_usb_mount()/handle_usb_unmount(); - handle_usb_unmount() clears the vendor + CDC TX FIFOs (the behavior #774 intended) then invokes an app callback; - expose set_mount_callback()/set_unmount_callback() so applications register their mount/unmount handlers via UsbDevice instead of defining tud_*_cb (which would also collide with esp_tinyusb). Also (requested): add CFG_TUD_CDC / CFG_TUD_VENDOR guards around the remaining CDC/vendor method bodies (is_cdc_connected, is_vendor_connected, cdc_write_available, cdc_write_clear, handle_cdc_rx), returning a safe default when the interface is disabled -- matching write_vendor / vendor_write_* which were already guarded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅Static analysis result - no issues found! ✅ |
There was a problem hiding this comment.
🟡 Changes recommended
The new event_cb path doesn’t call note_tinyusb_task(), which can make write_*() mis-detect TinyUSB-task context and block inside the USB task when invoked from mount/unmount callbacks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a link-time multiple-definition failure caused by defining tud_umount_cb inside usb_device.cpp even though esp_tinyusb already provides the TinyUSB lifecycle callbacks, and adds a supported mechanism for applications to hook mount/unmount events through UsbDevice.
Changes:
- Remove the local
tud_umount_cbimplementation and route lifecycle events viatinyusb_config_t::event_cb. - Add
set_mount_callback()/set_unmount_callback()APIs and internal mount/unmount handlers (including TX FIFO clearing on unmount). - Add compile-time guards for CDC/vendor helper methods so they return safe defaults when the interface is compiled out.
File summaries
| File | Description |
|---|---|
| components/usb_device/src/usb_device.cpp | Removes duplicate TinyUSB callback, registers event_cb, adds mount/unmount handling and additional CFG guards. |
| components/usb_device/include/usb_device.hpp | Exposes mount/unmount callback API and declares internal mount/unmount handler methods. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…lback espp_usb_device_event_cb runs in the TinyUSB device task; call note_tinyusb_task() first (like the other tud_*_cb callbacks) so a mount/unmount callback that writes via write_cdc()/write_vendor() takes the non-blocking fail-fast TX path rather than vTaskDelay()-ing inside the TinyUSB task and deadlocking USB servicing. Addresses the review note on #779. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The lifecycle trampoline bypasses teardown routing and TinyUSB-task detection, and its public documentation inaccurately includes suspension.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
components/usb_device/include/usb_device.hpp:304
- The unmount callback is not invoked for suspension: this implementation dispatches it only for
TINYUSB_EVENT_DETACHED, while esp_tinyusb exposes suspension as a distinct optionalTINYUSB_EVENT_SUSPENDED. Remove “suspended” from this API contract (or add separate suspend handling rather than treating a temporary suspend as unmount).
/// @brief Register a callback invoked when the device is unmounted (detached /
/// re-enumerated / suspended). The component clears the vendor + CDC TX
/// FIFOs before invoking it. Runs in the TinyUSB device-task context;
/// nullptr detaches. Register here instead of defining tud_umount_cb
/// (esp_tinyusb already defines it).
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
…he lifecycle API Follow-ups on #779: - espp_usb_device_event_cb now loads the teardown-guarded s_device singleton instead of using event_arg, so a destructor that has atomically detached the instance yields nullptr here (matching the other tud_*_cb trampolines); dropped the now-unused tusb_cfg.event_arg. - set_unmount_callback docs no longer claim "suspended" (only ATTACHED/DETACHED are routed to mount/unmount). - documented the new set_mount_callback/set_unmount_callback + vendor/cdc write_available/write_clear helpers and the automatic unmount TX-FIFO clear in the component README (Key methods, Notes) and the usb_cdc RST (Notes). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| if (event->id == TINYUSB_EVENT_ATTACHED) | ||
| dev->handle_usb_mount(); | ||
| else if (event->id == TINYUSB_EVENT_DETACHED) | ||
| dev->handle_usb_unmount(); |
| // Route esp_tinyusb's device lifecycle events (mount / unmount) to us so we | ||
| // can clear the TX FIFOs on unmount and invoke any app-registered callbacks. | ||
| // The callback loads the teardown-guarded s_device singleton itself, so no | ||
| // event_arg is needed. | ||
| tusb_cfg.event_cb = espp_usb_device_event_cb; |
…ampoline Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
|
@copilot fix the build failures you introduced |
…nding it Co-authored-by: finger563 <213467+finger563@users.noreply.github.com>
Fixed in a5... (see latest commit): the bare |
There was a problem hiding this comment.
🟡 Changes recommended
Mount delivery has an initialization race, and lifecycle callback ownership is documented inaccurately.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
components/usb_device/src/usb_device.cpp:854
esp_tinyusbstarts the TinyUSB task before copyingtinyusb_config_t::event_cbinto its global context. With a host already connected, the task can processtud_mount_cb()during that window, so this callback is still null and the application's first mount notification is lost; on a reinstall, a stale callback can instead dispatch before CDC setup and beforeinitialized_is set. Please track/reconcile the mounted state and deliver the initial mount exactly once only after function initialization is complete.
tusb_cfg.event_cb = espp_usb_device_event_cb;
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
| - The TinyUSB device lifecycle callbacks (`tud_mount_cb` / `tud_umount_cb` / | ||
| `tud_suspend_cb` / `tud_resume_cb`) are owned by `esp_tinyusb`. Register mount | ||
| / unmount handlers via `set_mount_callback()` / `set_unmount_callback()` | ||
| rather than defining those callbacks yourself. The mount / unmount handlers | ||
| also run in the TinyUSB device task. |
| // NOTE: the TinyUSB device lifecycle callbacks (tud_mount_cb / tud_umount_cb / | ||
| // tud_suspend_cb / tud_resume_cb) are defined by esp_tinyusb itself, which | ||
| // forwards them to the tinyusb_config_t::event_cb we register in initialize(). |
| - The TinyUSB device lifecycle callbacks (``tud_mount_cb`` / ``tud_umount_cb`` / | ||
| ``tud_suspend_cb`` / ``tud_resume_cb``) are owned by ``esp_tinyusb``. Register | ||
| mount / unmount handlers via ``set_mount_callback()`` / ``set_unmount_callback()`` | ||
| rather than defining those callbacks yourself (which would be a duplicate | ||
| symbol). On unmount the component clears the vendor + CDC TX FIFOs — so a |
The break
#774 added a
tud_umount_cbdefinition tousb_device.cpp, but esp_tinyusb already defines it (external/esp-usb/device/esp_tinyusb/tinyusb.c:87):That fails the link on every manager-off USB example (bldc_haptics, can_bridge, coredump, mcp266, ota, usb_device) — visible on #776's CI.
The fix
esp_tinyusb owns the TinyUSB device lifecycle callbacks (
tud_mount_cb/tud_umount_cb/…) and forwards them to atinyusb_config_t::event_cb. So:tud_umount_cbdefinition.event_cb(event_arg = this) ininitialize()and routeTINYUSB_EVENT_ATTACHED/DETACHEDtohandle_usb_mount()/handle_usb_unmount().handle_usb_unmount()clears the vendor + CDC TX FIFOs (the behavior feat(usb_device): TX FIFO space queries + clear; clear stale TX on unmount #774 intended) and then invokes an app callback.set_mount_callback()/set_unmount_callback()so applications register their handlers throughUsbDeviceinstead of definingtud_*_cb(which would also collide with esp_tinyusb).Also (requested)
CFG guards around the remaining CDC/vendor method bodies —
is_cdc_connected,is_vendor_connected,cdc_write_available,cdc_write_clear,handle_cdc_rx— returning a safe default when the interface is compiled out, matchingwrite_vendor/vendor_write_*which were already guarded.No example changes are needed — the collision was purely usb_device ↔ esp_tinyusb.
🤖 Generated with Claude Code