Fix ch58x fs ip send bug and add ch585 usbhs dc #290
Conversation
WalkthroughThe changes introduce a USB device controller implementation for the CH585 microcontroller, including modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -533,6 +533,7 @@ USBD_IRQHandler(void) | |||
| memcpy(usb_dc_cfg.ep_in[epid].ep_ram_addr, usb_dc_cfg.ep_in[epid].xfer_buf, usb_dc_cfg.ep_in[epid].mps); | |||
| } else { | |||
| memcpy(usb_dc_cfg.ep_in[epid].ep_ram_addr, usb_dc_cfg.ep_in[epid].xfer_buf, usb_dc_cfg.ep_in[epid].xfer_len); | |||
| EPn_SET_TX_LEN(epid, usb_dc_cfg.ep_in[epid].xfer_len); | |||
There was a problem hiding this comment.
LGTM! Critical bug fix for USB IN transfers.
The added line correctly sets the transfer length before enabling the endpoint for transmission. This fixes a critical bug where the endpoint would use an undefined/previous length for partial transfers, potentially causing data corruption or incomplete transfers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
port/ch32/ch585_usbhs_dc.c (2)
123-126: Implement or remove unimplemented function.
Currently,usbd_set_remote_wakeupalways returns-1. If remote wakeup support is not required by your application or the hardware, consider removing the function altogether. Otherwise, implement the required logic to enable/disable remote wakeup.
261-273: Check alignment constraints for DMA buffers.
The code checks(uint32_t)data & 0x03to reject unaligned buffers. Ensure that higher-level code employingusbd_ep_start_writealways provides 4-byte aligned buffers. Alternatively, you might add fallback logic to handle unaligned buffers if hardware or performance constraints allow.port/ch32/ch585_usbhs_reg.h (2)
15-16: Correct typographical errors in comments.
Minor spelling issues can reduce clarity. Consider fixing them for better readability:
- Line 15: “suspeng” → “suspend”
- Line 16: “interupt” → “interrupt”
- Line 23: “expceted” → “expected”
- Line 61: “requirment” → “requirement”
Example fix (doc lines only):
-#define USBHS_UD_PHY_SUSPENDM 0x08 // RW, suspeng USB PHY +#define USBHS_UD_PHY_SUSPENDM 0x08 // RW, suspend USB PHY -#define USBHS_UD_CLR_ALL 0x04 // RW, clear all interupt flag +#define USBHS_UD_CLR_ALL 0x04 // RW, clear all interrupt flag -#define USBHS_UD_SPEED_TYPE 0x03 // RW, speed mode excpeted by the equipment +#define USBHS_UD_SPEED_TYPE 0x03 // RW, speed mode expected by the equipment -#define USBHS_UDMS_SUSP_REQ 0x10 // RO, requirment of suspending USB +#define USBHS_UDMS_SUSP_REQ 0x10 // RO, requirement of suspending USBAlso applies to: 23-23, 61-61
354-450: Maintain consistent naming and style for registers.
You have used a robust naming convention (R8_USB2_,R16_USB2_,R32_USB2_). Ensure that future additions remain consistent to prevent confusion. Also, ensure macros describing register fields match the naming style for clarity and easy maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
port/ch32/ch585_usbhs_dc.c(1 hunks)port/ch32/ch585_usbhs_reg.h(1 hunks)
🔇 Additional comments (3)
port/ch32/ch585_usbhs_dc.c (2)
510-511: ConfirmEPn_SET_RX_VALID(0)usage.
Line 511 leaves theEPn_SET_RX_VALID(0)call commented out. Confirm that explicitly enabling the endpoint 0 RX side is not required post-reset. If necessary, uncomment or callEPn_SET_RX_VALID(0)to ensure the control endpoint can receive new setup packets immediately after reset.
80-89: Validate register writes for control and mode.
CH585_USBHS_DEV->CONTROLis assigned twice (lines 82 and 89). Confirm that overwriting the earlier bits (likeUSBHS_UD_RST_LINK) is intentional and that the final register value is correct for initialization without missing any needed bits from the first assignment.port/ch32/ch585_usbhs_reg.h (1)
46-54: Clarify interrupt flag clearing logic.
The comments for the interrupt flag register macros often use phrases like "clear ... interrupt flag," but some flags appear to be read-only or cleared by writing 1. Confirm that each enumerated bit is correctly documented and that writing 1 indeed clears the flag, as is common practice with hardware-based interrupt registers.
| int usbd_ep_is_stalled(uint8_t busid, const uint8_t ep, uint8_t *stalled) | ||
| { | ||
| if (USB_EP_DIR_IS_OUT(ep)) { | ||
| } else { | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Complete the stall status check.
The usbd_ep_is_stalled function does not populate the stalled parameter, leaving it perpetually empty. You may need to read the corresponding register or a cached endpoint stall state to return the correct stall status.
Example fix:
int usbd_ep_is_stalled(uint8_t busid, const uint8_t ep, uint8_t *stalled)
{
+ uint8_t ep_idx = USB_EP_GET_IDX(ep);
+ if (USB_EP_DIR_IS_OUT(ep)) {
+ *stalled = ((USB_GET_RX_CTRL(ep_idx) & USBHS_UEP_R_RES_MASK) == USBHS_UEP_R_RES_STALL);
+ } else {
+ *stalled = ((USB_GET_TX_CTRL(ep_idx) & USBHS_UEP_T_RES_MASK) == USBHS_UEP_T_RES_STALL);
+ }
return 0;
}📝 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.
| int usbd_ep_is_stalled(uint8_t busid, const uint8_t ep, uint8_t *stalled) | |
| { | |
| if (USB_EP_DIR_IS_OUT(ep)) { | |
| } else { | |
| } | |
| return 0; | |
| } | |
| int usbd_ep_is_stalled(uint8_t busid, const uint8_t ep, uint8_t *stalled) | |
| { | |
| uint8_t ep_idx = USB_EP_GET_IDX(ep); | |
| if (USB_EP_DIR_IS_OUT(ep)) { | |
| *stalled = ((USB_GET_RX_CTRL(ep_idx) & USBHS_UEP_R_RES_MASK) == USBHS_UEP_R_RES_STALL); | |
| } else { | |
| *stalled = ((USB_GET_TX_CTRL(ep_idx) & USBHS_UEP_T_RES_MASK) == USBHS_UEP_T_RES_STALL); | |
| } | |
| return 0; | |
| } |
HaiMianBBao
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Documentation