Skip to content

chore(usb): cdc_queue now use uint32_t length #2550

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

warmonkey
Copy link
Contributor

@warmonkey warmonkey commented Nov 4, 2024

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

…E_BUFFER_PACKET_NUMBER>512 or CDC_RECEIVE_QUEUE_BUFFER_PACKET_NUMBER>512.
which allows CDC_TRANSMIT_QUEUE_BUFFER_PACKET_NUMBER>512 or
CDC_RECEIVE_QUEUE_BUFFER_PACKET_NUMBER>512.

Signed-off-by: warmonkey <luoshumymail@gmail.com>
@warmonkey warmonkey closed this Nov 5, 2024
@fpistm fpistm reopened this Nov 5, 2024
@fpistm fpistm changed the title cdc_queue.cpp change to uint32_t length chore(usb): cdc_queue now use uint32_t length Nov 5, 2024
@fpistm fpistm added the enhancement New feature or request label Nov 5, 2024
@fpistm fpistm added this to the 2.9.0 milestone Nov 5, 2024
@fpistm fpistm removed this from the 2.9.0 milestone Nov 22, 2024
@fpistm
Copy link
Member

fpistm commented Feb 18, 2025

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).

void USB_WritePMA(USB_TypeDef const *USBx, uint8_t *pbUsrBuf, uint16_t wPMABufAddr, uint16_t wNBytes)

So I will not merge this PR, sorry.

@fpistm fpistm closed this Feb 18, 2025
@fpistm fpistm added the invalid This doesn't seem right label Feb 18, 2025
@warmonkey
Copy link
Contributor Author

warmonkey commented May 20, 2025

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).

void USB_WritePMA(USB_TypeDef const *USBx, uint8_t *pbUsrBuf, uint16_t wPMABufAddr, uint16_t wNBytes)

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.

@fpistm fpistm reopened this May 20, 2025
@github-project-automation github-project-automation bot moved this from Done to In progress in STM32 core based on ST HAL May 20, 2025
@fpistm fpistm removed the invalid This doesn't seem right label May 20, 2025
@warmonkey
Copy link
Contributor Author

warmonkey commented May 20, 2025

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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

2 participants