make dcd_edpt_iso_alloc/activate as default API for ISO endpoint #3345
Conversation
…ivate() as default driver implementation. dcd_edpt_close() is deprecated and will be removed from all driver in the future.
There was a problem hiding this comment.
Pull Request Overview
This PR transitions TinyUSB to use dcd_edpt_iso_alloc()/dcd_edpt_iso_activate() as the default driver API for ISO endpoints, deprecating dcd_edpt_close(). The changes include:
- Inverting the conditional compilation logic in
dcd.hto make ISO APIs the default - Defining
TUP_DCD_EDPT_CLOSE_APIfor drivers that still implement the old API - Updating multiple DCD drivers to either stub out or implement the new ISO APIs
- Updating MCX board configuration files (regenerated from NXP Config Tools)
- Removing build restrictions for MCXA15 devices in video examples
Reviewed Changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/device/dcd.h | Inverts API conditional: ISO APIs now default, dcd_edpt_close() behind TUP_DCD_EDPT_CLOSE_API flag |
| src/common/tusb_mcu.h | Defines TUP_DCD_EDPT_CLOSE_API for drivers keeping old API; defines TUP_DCD_EDPT_ISO_ALLOC for new API |
| src/portable//dcd_.c (multiple) | Adds stub ISO API implementations or actual implementations depending on driver capabilities |
| hw/bsp/mcx/* | Regenerated NXP board configuration files with updated paths and settings |
| hw/bsp/family_support.cmake | Adds -w compile option to suppress warnings for board targets |
| examples/device/video_*/skip.txt | Removes MCXA15 restriction for video examples |
| docs/porting.rst | Adds deprecation warning and documentation for ISO APIs |
| ENDPTCTRL_TYPE_POS = 2, // Endpoint type is 2-bit field | ||
| }; | ||
| #define ENDPTCTRL_TYPE(_type) ((_type) << ENDPTCTRL_TYPE_POS) | ||
| #define ENDPTCTRL_RESET_MASK (ENDPTCTRL_TYPE(TUSB_XFER_BULK) | (ENDPTCTRL_TYPE(TUSB_XFER_BULK) << 16u)) |
There was a problem hiding this comment.
[nitpick] The macro definition for ENDPTCTRL_RESET_MASK should be placed with other related masks in the enum block below (lines 71-76) rather than separately. This would improve code organization and make it easier to find related constants.
hw/bsp/family_support.cmake
Outdated
| family_add_board(${BOARD_TARGET}) | ||
| set_target_properties(${BOARD_TARGET} PROPERTIES | ||
| ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib | ||
| COMPILE_OPTIONS -w |
There was a problem hiding this comment.
Adding -w to suppress all warnings for board targets is concerning. This silences all compiler warnings which could hide real issues. If specific warnings need to be suppressed for auto-generated code, use targeted warning suppressions (e.g., -Wno-specific-warning) instead of disabling all warnings.
| COMPILE_OPTIONS -w |
|
|
||
| _dcd_data.qhd[epnum][dir].qtd_overlay.halted = 1; | ||
| // _dcd_data.qhd[epnum][dir].qtd_overlay.halted = 1; | ||
| // dcd_dcache_clean_invalidate(&_dcd_data, sizeof(dcd_data_t)); |
There was a problem hiding this comment.
These commented-out lines in dcd_edpt_iso_activate() should either be removed if unnecessary or uncommented with an explanation if they're needed. Leaving ambiguous commented code reduces maintainability.
| // dcd_dcache_clean_invalidate(&_dcd_data, sizeof(dcd_data_t)); | |
| #if CFG_TUD_MEM_DCACHE_ENABLE | |
| // Ensure data cache coherency for DMA: clean and invalidate dcd_data_t before hardware access | |
| dcd_dcache_clean_invalidate(&_dcd_data, sizeof(dcd_data_t)); | |
| #endif |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #if defined(TUP_USBIP_IP3511) || defined(TUP_USBIP_RUSB2) | ||
| #define TUP_DCD_EDPT_CLOSE_API | ||
| #endif | ||
|
|
||
| // USBIP implement dcd_edpt_close() and does not support ISO alloc & activate API | ||
| #ifndef TUP_DCD_EDPT_CLOSE_API | ||
| #define TUP_DCD_EDPT_ISO_ALLOC | ||
| #endif |
There was a problem hiding this comment.
Making TUP_DCD_EDPT_ISO_ALLOC the default in src/common/tusb_mcu.h (lines 692‑699) now routes every isochronous endpoint through dcd_edpt_iso_alloc()/dcd_edpt_iso_activate(), but several drivers added in the same commit still leave those functions as return false stubs – for example src/portable/microchip/samd/dcd_samd.c 248‑258, src/portable/microchip/samx7x/dcd_samx7x.c 562‑569, src/portable/microchip/pic32mz/dcd_pic32mz.c 441‑451, src/portable/bridgetek/ft9xx/dcd_ft9xx.c 809‑818, src/portable/raspberrypi/pio_usb/dcd_pio_usb.c 116‑133, and src/portable/wch/dcd_ch32_usbfs.c 286‑295. These MCUs previously configured ISO endpoints via dcd_edpt_open, so audio/video classes now TU_ASSERT when opening the endpoints because the driver reports failure. Each affected driver either needs a real ISO allocator/activator or must opt back into TUP_DCD_EDPT_CLOSE_API so that ISO endpoints remain functional.
Useful? React with 👍 / 👎.
hathach
force-pushed
the
update-iso-alloc
branch
from
cbd4aef to
423e510
Compare
…ivate() as default driver implementation. dcd_edpt_close() is deprecated and will be removed from all driver in the future.
hathach
force-pushed
the
update-iso-alloc
branch
from
423e510 to
be9d197
Compare
…ceeded with espressif build
hathach
force-pushed
the
update-iso-alloc
branch
from
2efc5f4 to
1173502
Compare
|
make TUP_DCD_EDPT_ISO_ALLOC i.e dcd_edpt_iso_alloc()/dcd_edpt_iso_activate() as default driver implementation. dcd_edpt_close() is deprecated and will be removed from all driver in the future.