Add isochronous transfer support to fsdev by configuring double-buffering for isochronous endpoints. #372
Conversation
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
…ring for isochronous endpoints.
WalkthroughThis pull request adds ISOCHRONOUS endpoint support with double-buffering to the FSDEV USB device controller. Changes include removing ISO assertion checks, implementing dual-buffer PMA setup during endpoint initialization, updating endpoint write operations to handle ISO buffers, and extending the IRQ handler to manage ISO transfers with proper data toggle handling. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant FSDEV as FSDEV USB<br/>(Endpoint)
participant DMA as PMA<br/>(Dual Buffers)
participant ISR as IRQHandler
App->>FSDEV: Open ISO Endpoint
FSDEV->>DMA: Setup DBUF0 & DBUF1<br/>addresses
FSDEV->>FSDEV: Initialize DBUF counts<br/>& DTOG bits
Note over App,ISR: OUT (CTR_RX) ISO Transfer
App->>FSDEV: usbd_ep_start_read()
FSDEV->>DMA: RX_VALID (awaiting data)
DMA->>ISR: Data received in DBUF0
ISR->>DMA: Read from active buffer<br/>(DBUF0 or DBUF1 per DTOG RX)
ISR->>FSDEV: Free user buffer<br/>(toggle DTOG)
ISR->>App: Transfer complete event
Note over App,ISR: IN (CTR_TX) ISO Transfer
App->>FSDEV: usbd_ep_start_write()
FSDEV->>DMA: Write to active DBUF<br/>(per TX DTOG)
FSDEV->>DMA: TX status = VALID
DMA->>ISR: Transmission complete (CTR_TX)
ISR->>FSDEV: Select next DBUF counter<br/>(DBUF0_CNT or DBUF1_CNT)
ISR->>ISR: Advance transfer state
alt Transfer complete
ISR->>App: Completion event
else More data
ISR->>DMA: Write next buffer<br/>& set TX_VALID
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Comment on lines
+184
to
+200
| PCD_CLEAR_EP_KIND(USB, ep_idx); | ||
| /* Set buffer address for double buffered mode */ | ||
| PCD_SET_EP_DBUF_ADDR(USB, ep_idx, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, | ||
| g_fsdev_udc.out_ep[ep_idx].ep_pma_addr + g_fsdev_udc.out_ep[ep_idx].ep_mps); | ||
|
|
||
| /* Clear the data toggle bits for the endpoint IN/OUT */ | ||
| PCD_CLEAR_RX_DTOG(USB, ep_idx); | ||
| PCD_CLEAR_TX_DTOG(USB, ep_idx); | ||
|
|
||
| /* Set endpoint RX count */ | ||
| PCD_SET_EP_DBUF_CNT(USB, ep_idx, 0, ep->wMaxPacketSize); | ||
|
|
||
| /* Set endpoint RX to valid state */ | ||
| PCD_SET_EP_RX_STATUS(USB, ep_idx, USB_EP_RX_VALID); | ||
| PCD_SET_EP_TX_STATUS(USB, ep_idx, USB_EP_TX_DIS); | ||
| g_fsdev_udc.pma_offset += USB_GET_MAXPACKETSIZE(ep->wMaxPacketSize * 2); | ||
| } else { |
There was a problem hiding this comment.
Double buffer never enabled for ISO endpoints
PCD_CLEAR_EP_KIND disables the double-buffer flag. Without setting EP_KIND/PCD_SET_EP_DBUF, the hardware stays single-buffered, so all the subsequent DBUF address/count programming and DTOG handling never take effect. The new ISO paths will therefore malfunction. Please enable the DBUF bit (both OUT and IN branches) instead of clearing it.
- PCD_CLEAR_EP_KIND(USB, ep_idx);
+ PCD_SET_EP_DBUF(USB, ep_idx);
...
- PCD_CLEAR_EP_KIND(USB, ep_idx);
+ PCD_SET_EP_DBUF(USB, ep_idx);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PCD_CLEAR_EP_KIND(USB, ep_idx); | |
| /* Set buffer address for double buffered mode */ | |
| PCD_SET_EP_DBUF_ADDR(USB, ep_idx, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, | |
| g_fsdev_udc.out_ep[ep_idx].ep_pma_addr + g_fsdev_udc.out_ep[ep_idx].ep_mps); | |
| /* Clear the data toggle bits for the endpoint IN/OUT */ | |
| PCD_CLEAR_RX_DTOG(USB, ep_idx); | |
| PCD_CLEAR_TX_DTOG(USB, ep_idx); | |
| /* Set endpoint RX count */ | |
| PCD_SET_EP_DBUF_CNT(USB, ep_idx, 0, ep->wMaxPacketSize); | |
| /* Set endpoint RX to valid state */ | |
| PCD_SET_EP_RX_STATUS(USB, ep_idx, USB_EP_RX_VALID); | |
| PCD_SET_EP_TX_STATUS(USB, ep_idx, USB_EP_TX_DIS); | |
| g_fsdev_udc.pma_offset += USB_GET_MAXPACKETSIZE(ep->wMaxPacketSize * 2); | |
| } else { | |
| PCD_SET_EP_DBUF(USB, ep_idx); | |
| /* Set buffer address for double buffered mode */ | |
| PCD_SET_EP_DBUF_ADDR(USB, ep_idx, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, | |
| g_fsdev_udc.out_ep[ep_idx].ep_pma_addr + g_fsdev_udc.out_ep[ep_idx].ep_mps); | |
| /* Clear the data toggle bits for the endpoint IN/OUT */ | |
| PCD_CLEAR_RX_DTOG(USB, ep_idx); | |
| PCD_CLEAR_TX_DTOG(USB, ep_idx); | |
| /* Set endpoint RX count */ | |
| PCD_SET_EP_DBUF_CNT(USB, ep_idx, 0, ep->wMaxPacketSize); | |
| /* Set endpoint RX to valid state */ | |
| PCD_SET_EP_RX_STATUS(USB, ep_idx, USB_EP_RX_VALID); | |
| PCD_SET_EP_TX_STATUS(USB, ep_idx, USB_EP_TX_DIS); | |
| g_fsdev_udc.pma_offset += USB_GET_MAXPACKETSIZE(ep->wMaxPacketSize * 2); | |
| } else { |
🤖 Prompt for AI Agents
In port/fsdev/usb_dc_fsdev.c around lines 184-200, the code calls
PCD_CLEAR_EP_KIND which disables double-buffering so the subsequent DBUF
address/count programming and DTOG handling are ignored for ISO endpoints;
replace the clear with the DBUF enable call(s) (e.g., PCD_SET_EP_KIND /
PCD_SET_EP_DBUF or the platform-specific macro) for both the OUT and IN branches
so the hardware is configured double-buffered, then leave the DBUF address, DBUF
count and DTOG clears as-is to complete proper double-buffer setup.
Comment on lines
+488
to
505
| /* manage double buffer bulk out */ | ||
| /* free EP OUT Buffer */ | ||
| PCD_FREE_USER_BUFFER(USB, ep_idx, 0U); | ||
|
|
||
| if ((PCD_GET_ENDPOINT(USB, ep_idx) & USB_EP_DTOG_RX) != 0U) { | ||
| /* read from endpoint BUF0Addr buffer */ | ||
| read_count = (uint16_t)PCD_GET_EP_DBUF0_CNT(USB, ep_idx); | ||
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, (uint16_t)read_count); | ||
| } else { | ||
| /* read from endpoint BUF1Addr buffer */ | ||
| read_count = (uint16_t)PCD_GET_EP_DBUF1_CNT(USB, ep_idx); | ||
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr + g_fsdev_udc.out_ep[ep_idx].ep_mps, (uint16_t)read_count); | ||
| } | ||
| } else { | ||
| read_count = PCD_GET_EP_RX_CNT(USB, ep_idx); | ||
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, (uint16_t)read_count); | ||
| } | ||
| g_fsdev_udc.out_ep[ep_idx].xfer_buf += read_count; |
There was a problem hiding this comment.
Freeing ISO OUT buffer before copy corrupts data
Calling PCD_FREE_USER_BUFFER before copying toggles the SW_BUF bit, so the controller may reuse that PMA slot for the next ISO frame while we are still reading it. The PMA read must happen first, then free the user buffer. Please move the PCD_FREE_USER_BUFFER call to after the read completes.
- /* free EP OUT Buffer */
- PCD_FREE_USER_BUFFER(USB, ep_idx, 0U);
-
if ((PCD_GET_ENDPOINT(USB, ep_idx) & USB_EP_DTOG_RX) != 0U) {
/* read from endpoint BUF0Addr buffer */
read_count = (uint16_t)PCD_GET_EP_DBUF0_CNT(USB, ep_idx);
fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, (uint16_t)read_count);
} else {
/* read from endpoint BUF1Addr buffer */
read_count = (uint16_t)PCD_GET_EP_DBUF1_CNT(USB, ep_idx);
fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr + g_fsdev_udc.out_ep[ep_idx].ep_mps, (uint16_t)read_count);
}
+
+ /* free EP OUT Buffer */
+ PCD_FREE_USER_BUFFER(USB, ep_idx, 0U);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* manage double buffer bulk out */ | |
| /* free EP OUT Buffer */ | |
| PCD_FREE_USER_BUFFER(USB, ep_idx, 0U); | |
| if ((PCD_GET_ENDPOINT(USB, ep_idx) & USB_EP_DTOG_RX) != 0U) { | |
| /* read from endpoint BUF0Addr buffer */ | |
| read_count = (uint16_t)PCD_GET_EP_DBUF0_CNT(USB, ep_idx); | |
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, (uint16_t)read_count); | |
| } else { | |
| /* read from endpoint BUF1Addr buffer */ | |
| read_count = (uint16_t)PCD_GET_EP_DBUF1_CNT(USB, ep_idx); | |
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr + g_fsdev_udc.out_ep[ep_idx].ep_mps, (uint16_t)read_count); | |
| } | |
| } else { | |
| read_count = PCD_GET_EP_RX_CNT(USB, ep_idx); | |
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, (uint16_t)read_count); | |
| } | |
| g_fsdev_udc.out_ep[ep_idx].xfer_buf += read_count; | |
| /* manage double buffer bulk out */ | |
| if ((PCD_GET_ENDPOINT(USB, ep_idx) & USB_EP_DTOG_RX) != 0U) { | |
| /* read from endpoint BUF0Addr buffer */ | |
| read_count = (uint16_t)PCD_GET_EP_DBUF0_CNT(USB, ep_idx); | |
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, (uint16_t)read_count); | |
| } else { | |
| /* read from endpoint BUF1Addr buffer */ | |
| read_count = (uint16_t)PCD_GET_EP_DBUF1_CNT(USB, ep_idx); | |
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr + g_fsdev_udc.out_ep[ep_idx].ep_mps, (uint16_t)read_count); | |
| } | |
| /* free EP OUT Buffer */ | |
| PCD_FREE_USER_BUFFER(USB, ep_idx, 0U); | |
| } else { | |
| read_count = PCD_GET_EP_RX_CNT(USB, ep_idx); | |
| fsdev_read_pma(USB, g_fsdev_udc.out_ep[ep_idx].xfer_buf, g_fsdev_udc.out_ep[ep_idx].ep_pma_addr, (uint16_t)read_count); | |
| } | |
| g_fsdev_udc.out_ep[ep_idx].xfer_buf += read_count; |
🤖 Prompt for AI Agents
In port/fsdev/usb_dc_fsdev.c around lines 488 to 505, the PCD_FREE_USER_BUFFER
call is currently executed before reading the PMA which can toggle SW_BUF and
let the controller reuse the PMA while we're still copying; move the
PCD_FREE_USER_BUFFER(USB, ep_idx, 0U) invocation to immediately after the
fsdev_read_pma(...) completes for both the double-buffer branches and the
single-buffer branch so the PMA is read first and then the buffer is freed.
Collaborator
|
Thanks very much, i will test for this. |
sakumisu
force-pushed
the
master
branch
13 times, most recently
from
669e41e to
cf85c88
Compare
sakumisu
force-pushed
the
master
branch
2 times, most recently
from
4ff88ff to
a2352bf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 by CodeRabbit
New Features
Refactor