Arduino_Core_STM32 icon indicating copy to clipboard operation
Arduino_Core_STM32 copied to clipboard

chore(usb): cdc_queue now use uint32_t length

Open catbraincell opened this issue 1 year ago • 3 comments

raise cdc buffer size 32KB limit due to uint16_t length variables.

This PR fixes bug: cdc queue limited Now CDC_TRANSMIT_QUEUE_BUFFER_PACKET_NUMBER and/or CDC_RECEIVE_QUEUE_BUFFER_PACKET_NUMBER both can be greater than 512. which every buffer size is 64B, 64*512=32768

catbraincell avatar Nov 04 '24 17:11 catbraincell

Hi @warmonkey Sorry for the delay. :cry: I've dig into HAL to see why it was uint16_t and found the reason, even if changed to uint32_t it will be always truncated to uint16_t when accessing the PMA (R/W). https://github.com/stm32duino/Arduino_Core_STM32/blob/345138a96d51caf355c2350438b13b34fab7ad7c/system/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_ll_usb.c#L2817

So I will not merge this PR, sorry.

fpistm avatar Feb 18 '25 09:02 fpistm

Hi @warmonkey Sorry for the delay. 😢 I've dig into HAL to see why it was uint16_t and found the reason, even if changed to uint32_t it will be always truncated to uint16_t when accessing the PMA (R/W).

https://github.com/stm32duino/Arduino_Core_STM32/blob/345138a96d51caf355c2350438b13b34fab7ad7c/system/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_ll_usb.c#L2817

So I will not merge this PR, sorry.

On STM32F4 USB_WritePMA will not be called. The actual function to send data over USB is USBD_LL_Transmit() -> ... -> USB_EPStartXfer()

There two paths can trigger the USBD_LL_Transmit(): USBSerial::write() -> CDC_continue_transmit(), USBD_CDC_TransmitCplt() -> CDC_continue_transmit() -> USBD_CDC_TransmitPacket() -> USBD_LL_Transmit() -> HAL_PCD_EP_Transmit() -> USB_EPStartXfer()

In CDC_continue_transmit() file: usbd_cdc_if.c line 345, it's calling buffer = CDC_TransmitQueue_ReadBlock(&TransmitQueue, &size); which returns the size to send. In CDC_TransmitQueue_ReadBlock() file: cdc_queue.c line 88, the size value is limited to 65472 in my commit

as stated in stm32f4xx_ll_usb.c function USB_EPStartXfer line 782:

Program the transfer size and packet count as follows: 
xfersize = N * maxpacket + short_packet 
pktcnt = N + (short_packet_exist ? 1 : 0)
...
pktcnt = (uint16_t)((ep->xfer_len + ep->maxpacket - 1U) / ep->maxpacket);
USBx_INEP(epnum)->DIEPTSIZ |= (USB_OTG_DIEPTSIZ_PKTCNT & (pktcnt << 19));
...
USBx_INEP(epnum)->DIEPTSIZ |= (USB_OTG_DIEPTSIZ_XFRSIZ & ep->xfer_len);

from the code above we know that the size to send from CDC_TransmitQueue_ReadBlock() previously, is assigned to ep->xfer_len (at stm32f4xx_hal_pcd.c line 1940 for stm32f4), then write into OTG_FS_DIEPTSIZx or OTG_HS_DIEPTSIZx register as:

PKTCNT = (int)(xfer_len/MAX_PACKET_SIZE)     //64 for FS, 512 for HS
XFRSIZ = xfer_len

from RM0008 RM0090 RM0432 we know the PKTCNT has 10bits, max value 0x3ff (1023), XFRSIZ has 19bits, max value 0x7ffff (524287). 1023 * 512 = 523776 < 524287

but, it's not easy to know the USB is running in HS or FS mode.

In conclusion, we should limit the maximum size returned by CDC_TransmitQueue_ReadBlock() to OTG_MAX_PKTCNTUSB_FS_MAX_PACKET_SIZE which is 102364 (for now).

I will read pdev->ep_in[CDCInEpAdd & 0xFU].maxpacket instead in the next commit.


The CDC queue is a software buffer, it's not relevant to the PMA area. The PMA is located in 0x4000 6000(1) - 0x4000 63FF Shared USB/CAN SRAM 512 bytes (RM0008 Page 52) it has only 512 bytes shared with multiple endpoints. User should copy data from/to PMA manually.

Before I submit this PR I checked multiple times, compiled and deployed on actual products. I ran the test for at least 24hrs.

catbraincell avatar May 20 '25 10:05 catbraincell

Im working on an improvement. Will use maxpacket in USBD_EndpointTypeDef instead of use a fixed value 64. Just finished but i need more real world test.

catbraincell avatar May 20 '25 10:05 catbraincell