Refactor USB control transfer handling into usbd.c#3627
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors TinyUSB device control transfer handling by moving EP0 control transfer state and logic into src/device/usbd.c, deprecating the legacy usbd_control.c translation unit, and updating build/test wiring accordingly.
Changes:
- Moved EP0 control-transfer state into
_usbd_devand inlined formerusbd_control.clogic intousbd.c. - Dropped
usbd_control.cfrom in-tree build source lists; left a deprecated stub file for downstream builds. - Updated unit-test/fuzz/make/cmake build definitions to stop compiling
usbd_control.c.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit-test/CMakeLists.txt | Stops linking usbd_control.c into Ceedling-backed unit test executables. |
| test/fuzz/rules.mk | Removes usbd_control.c from fuzz build sources. |
| src/tinyusb.mk | Removes usbd_control.c from the TinyUSB makefile source list. |
| src/device/usbd_control.c | Replaced with a deprecated stub TU that warns downstream builders. |
| src/device/usbd.c | Adds control-transfer state to _usbd_dev and hosts the EP0 control transfer implementation. |
| src/CMakeLists.txt | Removes usbd_control.c from TinyUSB CMake source enumeration. |
| hw/bsp/rp2040/family.cmake | Removes usbd_control.c from rp2040 BSP target sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+857
to
+865
| (void) request; | ||
| return status_stage_xact(rhport, status_stage_ep(&_usbd_dev.ctrl_xfer.request)); | ||
| } | ||
|
|
||
| // Transmit data to/from the control endpoint. If wLength is zero, a status packet is sent instead. | ||
| bool tud_control_xfer(uint8_t rhport, const tusb_control_request_t* request, void* buffer, uint16_t len) { | ||
| // _usbd_dev.ctrl_xfer.request and reset fields are pre-initialized at process_setup_received entry | ||
| (void) request; | ||
| usbd_control_xfer_t* const ctrl_xfer = &_usbd_dev.ctrl_xfer; |
There was a problem hiding this comment.
tud_control_xfer() ignores its request argument and instead uses _usbd_dev.ctrl_xfer.request. This breaks call sites that intentionally pass a modified request (e.g. process_get_descriptor() creates mod_request with a shorter wLength to avoid sending a ZLP on the very first GET_DEVICE_DESCRIPTOR when EP0 size < 18). Consider copying *request into _usbd_dev.ctrl_xfer.request (or at least using request->wLength for data_len/status-stage direction) so these existing semantics keep working.
Suggested change
| (void) request; | |
| return status_stage_xact(rhport, status_stage_ep(&_usbd_dev.ctrl_xfer.request)); | |
| } | |
| // Transmit data to/from the control endpoint. If wLength is zero, a status packet is sent instead. | |
| bool tud_control_xfer(uint8_t rhport, const tusb_control_request_t* request, void* buffer, uint16_t len) { | |
| // _usbd_dev.ctrl_xfer.request and reset fields are pre-initialized at process_setup_received entry | |
| (void) request; | |
| usbd_control_xfer_t* const ctrl_xfer = &_usbd_dev.ctrl_xfer; | |
| usbd_control_xfer_t* const ctrl_xfer = &_usbd_dev.ctrl_xfer; | |
| TU_ASSERT(request); | |
| ctrl_xfer->request = *request; | |
| return status_stage_xact(rhport, status_stage_ep(&ctrl_xfer->request)); | |
| } | |
| // Transmit data to/from the control endpoint. If wLength is zero, a status packet is sent instead. | |
| bool tud_control_xfer(uint8_t rhport, const tusb_control_request_t* request, void* buffer, uint16_t len) { | |
| // _usbd_dev.ctrl_xfer.request and reset fields are pre-initialized at process_setup_received entry | |
| usbd_control_xfer_t* const ctrl_xfer = &_usbd_dev.ctrl_xfer; | |
| TU_ASSERT(request); | |
| ctrl_xfer->request = *request; |
| @@ -459,17 +473,6 @@ static char const *const _usbd_event_str[DCD_EVENT_COUNT] = { | |||
| "Func Call" | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
usbd_driver_print_control_complete_name() was removed from this file, but it’s still declared in src/device/usbd_pvt.h behind #if CFG_TUSB_DEBUG >= CFG_TUD_LOG_LEVEL. If any in-tree or out-of-tree code links against that symbol when debug logging is enabled, this will become an undefined reference. Either keep a (possibly static-less) implementation in usbd.c under the same #if, or remove/adjust the declaration in the header.
Suggested change
| void usbd_driver_print_control_complete_name(void) { | |
| // Keep the debug symbol declared in usbd_pvt.h available for any | |
| // in-tree or out-of-tree code that links against it when debug logging | |
| // is enabled. | |
| } |
Comment on lines
+30
to
+31
| // keep resolving. Drop usbd_control.c from your build to silence the warning. | ||
| #warning "src/device/usbd_control.c is deprecated and now empty; remove it from your build (its content lives in src/device/usbd.c)." |
There was a problem hiding this comment.
This file is now a stub with an unconditional #warning. #warning is non-standard and, combined with common -Werror settings (or compilers that don’t support it), can break downstream/vendor builds that still compile this translation unit. Consider guarding the warning by compiler support and/or a config macro (or replacing it with a portable deprecation mechanism) so including the legacy path doesn’t hard-fail builds.
Suggested change
| // keep resolving. Drop usbd_control.c from your build to silence the warning. | |
| #warning "src/device/usbd_control.c is deprecated and now empty; remove it from your build (its content lives in src/device/usbd.c)." | |
| // keep resolving. Drop usbd_control.c from your build to silence the message. | |
| #if !defined(TINYUSB_SILENCE_DEPRECATED_USBD_CONTROL_WARNING) | |
| #if defined(_MSC_VER) || defined(__clang__) || defined(__GNUC__) | |
| #pragma message("src/device/usbd_control.c is deprecated and now empty; remove it from your build (its content lives in src/device/usbd.c).") | |
| #endif | |
| #endif |
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in size
No changes
|
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| raspberrypi_cm4/cdc_msc | 57,372 → 57,116 (-256) | 4,584 → 744 (-3,840) | — | — | 61,956 → 57,860 (-4,096) | -6.6% |
| fomu/dfu_runtime | 10,580 → 10,012 (-568) | — | — | — | 10,607 → 10,039 (-568) | -5.4% |
| fomu/hid_generic_inout | 12,232 → 11,636 (-596) | — | — | — | 12,259 → 11,663 (-596) | -4.9% |
| fomu/hid_multiple_interface | 13,412 → 12,816 (-596) | — | — | — | 13,439 → 12,843 (-596) | -4.4% |
| fomu/hid_boot_interface | 13,468 → 12,872 (-596) | — | — | — | 13,495 → 12,899 (-596) | -4.4% |
| fomu/midi_test | 13,868 → 13,260 (-608) | — | — | — | 13,895 → 13,287 (-608) | -4.4% |
| fomu/hid_composite | 13,740 → 13,144 (-596) | — | — | — | 13,767 → 13,171 (-596) | -4.3% |
| lpcxpresso1347/audio_test_multi_rate | 9,956 → 9,656 (-300) | — | — | 3,332 → 3,076 (-256) | 13,764 → 13,208 (-556) | -4.0% |
| fomu/cdc_dual_ports | 15,260 → 14,652 (-608) | — | — | — | 15,287 → 14,679 (-608) | -4.0% |
| fomu/printer_to_cdc | 15,628 → 15,020 (-608) | — | — | — | 15,651 → 15,043 (-608) | -3.9% |
…`_usbd_dev` structure and remove `usbd_control_reset`
hathach
force-pushed
the
merge-usbd-control
branch
from
b301f79 to
eb66712
Compare
…all endpoint state handling methods and accesses
Comment on lines
+1007
to
+1031
| switch (p_request->wValue) { //-V2520 | ||
| case TUSB_REQ_FEATURE_REMOTE_WAKEUP: | ||
| TU_LOG_USBD(" Enable Remote Wakeup\r\n"); | ||
| // Host may enable remote wake up before suspending especially HID device | ||
| _usbd_dev.remote_wakeup_en = 1; | ||
| tud_control_status(rhport, p_request); | ||
| return true; | ||
|
|
||
| #if CFG_TUD_TEST_MODE | ||
| case TUSB_REQ_FEATURE_TEST_MODE: { | ||
| // Only handle the test mode if supported and valid | ||
| TU_VERIFY(0 == tu_u16_low(p_request->wIndex)); | ||
|
|
||
| uint8_t const selector = tu_u16_high(p_request->wIndex); | ||
| TU_VERIFY(TUSB_FEATURE_TEST_J <= selector && selector <= TUSB_FEATURE_TEST_FORCE_ENABLE); | ||
|
|
||
| _usbd_dev.ctrl_xfer.complete_cb = process_test_mode_cb; | ||
| tud_control_status(rhport, p_request); | ||
| return true; | ||
| } | ||
| #endif | ||
|
|
||
| // Stall unsupported feature selector | ||
| default: return false; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of Changes
_usbd_devstructure for improved organization and maintainability.usbd_control_resetfunction.usbd_control.c, merging its functionality intousbd.cto reduce redundancy and streamline codebase structure.