Conversation
![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.
Reviewed commit: 868ad07127
ℹ️ 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 (CFG_TUSB_MCU == OPT_MCU_RP2040) && !CFG_TUD_RPI_PIO_USB | ||
| #define CFG_TUD_EDPT_DEDICATED_HWFIFO 1 | ||
| #define CFG_TUD_EDPT_DEDICATED_HWFIFO 0 |
There was a problem hiding this comment.
dcd_edpt_xfer_fifo() when hwfifo support is off
If an RP2040 build still uses the driver-level dcd_edpt_xfer_fifo() entry point (it remains exported in src/portable/raspberrypi/rp2040/dcd_rp2040.c:508-513), setting CFG_TUD_EDPT_DEDICATED_HWFIFO to 0 makes prepare_ep_buffer()/sync_ep_buffer() skip the only ep->is_xfer_fifo handling. hw_endpoint_xfer_start() stores the FIFO pointer in the user_buf/user_fifo union (rp2040_usb.h:85-88), so the fallback unaligned_memcpy() now reads from or writes into the tu_fifo_t struct itself instead of the FIFO payload. That turns any FIFO transfer into immediate memory corruption rather than a cleanly disabled feature.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR disables the RP2040 device “dedicated hardware FIFO” transfer path by default and gates RP2040-specific hwfifo helpers/logic behind the corresponding compile-time option.
Changes:
- Set
CFG_TUD_EDPT_DEDICATED_HWFIFOto0for RP2040 (non PIO-USB device). - Compile
tu_hwfifo_write()/tu_hwfifo_read()only whenCFG_TUD_EDPT_DEDICATED_HWFIFOis enabled. - Wrap FIFO-based buffer copy paths in
rp2040_usb.cwith#if CFG_TUD_EDPT_DEDICATED_HWFIFO.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/tusb_option.h |
Disables RP2040 dedicated endpoint hwfifo by default (non PIO-USB device). |
src/portable/raspberrypi/rp2040/rp2040_usb.c |
Gates hwfifo helpers and FIFO transfer branches behind CFG_TUD_EDPT_DEDICATED_HWFIFO. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| #if CFG_TUD_EDPT_DEDICATED_HWFIFO | ||
| if (ep->is_xfer_fifo) { | ||
| // not in sram, may mess up timing with E15 workaround | ||
| tu_hwfifo_write_from_fifo(hw_buf, ep->user_fifo, buflen, NULL); | ||
| } else { | ||
| } else | ||
| #endif | ||
| { |
| #if CFG_TUD_EDPT_DEDICATED_HWFIFO | ||
| if (ep->is_xfer_fifo) { | ||
| // not in sram, may mess up timing with E15 workaround | ||
| tu_hwfifo_read_to_fifo(hw_buf, ep->user_fifo, xferred_bytes, NULL); | ||
| } else { | ||
| } else | ||
| #endif | ||
| { |
| @@ -62,6 +63,7 @@ void tu_hwfifo_read(const volatile void *hwfifo, uint8_t *dest, uint16_t len, co | |||
| (void)access_mode; | |||
| unaligned_memcpy(dest, (const uint8_t *)(uintptr_t)hwfifo, len); | |||
| } | |||
| #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 |
|---|---|---|---|---|---|---|
| raspberry_pi_pico/dfu_runtime | 13,584 → 13,456 (-128) | — | 2,920 → 2,840 (-80) | — | 22,572 → 22,364 (-208) | -0.9% |
| raspberry_pi_pico/audio_test | 16,760 → 16,496 (-264) | — | 2,936 → 2,856 (-80) | 3,404 → 3,508 (+104) | 26,224 → 25,984 (-240) | -0.9% |
| raspberry_pi_pico/hid_generic_inout | 14,440 → 14,312 (-128) | — | 2,936 → 2,856 (-80) | — | 23,484 → 23,276 (-208) | -0.9% |
| raspberry_pi_pico/dfu | 14,320 → 14,192 (-128) | — | 2,920 → 2,840 (-80) | — | 23,508 → 23,300 (-208) | -0.9% |
| raspberry_pi_pico/hid_multiple_interface | 15,064 → 14,936 (-128) | — | 3,000 → 2,920 (-80) | — | 24,216 → 24,008 (-208) | -0.9% |
| raspberry_pi_pico/hid_boot_interface | 15,120 → 14,992 (-128) | — | 3,000 → 2,920 (-80) | — | 24,228 → 24,020 (-208) | -0.9% |
| raspberry_pi_pico/hid_composite | 15,176 → 15,048 (-128) | — | 3,016 → 2,936 (-80) | — | 24,420 → 24,212 (-208) | -0.9% |
| raspberry_pi_pico/cdc_uac2 | 19,576 → 19,392 (-184) | — | 2,952 → 2,872 (-80) | 5,472 → 6,000 (+528) | 31,580 → 31,844 (+264) | +0.8% |
| raspberry_pi_pico/usbtmc | 17,416 → 17,280 (-136) | — | 3,300 → 3,220 (-80) | — | 27,664 → 27,448 (-216) | -0.8% |
| raspberry_pi_pico/midi_test | 15,656 → 15,416 (-240) | — | 2,920 → 2,840 (-80) | 3,052 → 3,180 (+128) | 24,792 → 24,600 (-192) | -0.8% |
hathach
force-pushed
the
rp2-disable-hwifo
branch
from
868ad07 to
9b17d55
Compare
No description provided.