- This topic has 4 replies, 2 voices, and was last updated 7 years, 11 months ago by
.
-
Posts
-
Hi,
I’m curious whether the function below, part of the VisualGDB USB example, is supplied by ST or Sysprogs? It is located in usbd_cdc_if.c and for me it hangs on the second while (pCDC->TxState) {} if two character strings are sent directly one after another:
int VCP_write(const void *pBuffer, int size) { if (size > CDC_DATA_HS_OUT_PACKET_SIZE) { int offset; for (offset = 0; offset < size; offset++) { int todo = MIN(CDC_DATA_HS_OUT_PACKET_SIZE, size - offset); int done = VCP_write(((char *)pBuffer) + offset, todo); if (done != todo) return offset + done; } return size; } USBD_CDC_HandleTypeDef *pCDC = (USBD_CDC_HandleTypeDef *)USBD_Device.pClassData; while (pCDC->TxState) {} //Wait for previous transfer USBD_CDC_SetTxBuffer(&USBD_Device, (uint8_t *)pBuffer, size); if (USBD_CDC_TransmitPacket(&USBD_Device) != USBD_OK) return 0; while (pCDC->TxState) {} //Wait until transfer is done return size; }I made a modified function of the above which works better, however, in my first version listed below, if the number of sent bytes exceed CDC_DATA_HS_OUT_PACKET_SIZE (=512 bytes) and the recursive call is invoked then garbage data will be injected after the first 512 bytes and before subsequent bytes are sent. Below, millis() just returns system ticks and USBTX_BUSYWAIT is defined as 2.
int usb_cdc_write(const void *pBuffer, int size) { if (size > CDC_DATA_HS_OUT_PACKET_SIZE) { int offset; for (offset = 0; offset < size; offset++) { int todo = MIN(CDC_DATA_HS_OUT_PACKET_SIZE, size - offset); int done = usb_cdc_write(((char *)pBuffer) + offset, todo); if (done != todo) return offset + done; } return size; } if (USBD_Device.pClassData == NULL) return USBD_FAIL; USBD_CDC_HandleTypeDef *pCDC = (USBD_CDC_HandleTypeDef *)USBD_Device.pClassData; uint32_t wait_us = millis() + USBTX_BUSYWAIT; while (pCDC->TxState && wait_us > millis()) ; if (pCDC->TxState != 0) return USBD_BUSY; USBD_CDC_SetTxBuffer(&USBD_Device, (uint8_t *)pBuffer, size); int result = USBD_CDC_TransmitPacket(&USBD_Device); wait_us = millis() + USBTX_BUSYWAIT; while (pCDC->TxState && wait_us > millis()) ; if (pCDC->TxState != 0) return USBD_BUSY; return size; }My final modded function version which is able to send more than 512 bytes (my custom packet is exactly 1076 bytes, not tested larger sizes than this) without injecting garbage data is below. Basically I simply removed the buffer size check and recursive call.
int usb_cdc_write(const void *pBuffer, int size) { if (USBD_Device.pClassData == NULL) return USBD_FAIL; USBD_CDC_HandleTypeDef *pCDC = (USBD_CDC_HandleTypeDef *)USBD_Device.pClassData; uint32_t wait_us = millis() + USBTX_BUSYWAIT; while (pCDC->TxState && wait_us > millis()) ; if (pCDC->TxState != 0) return USBD_BUSY; USBD_CDC_SetTxBuffer(&USBD_Device, (uint8_t *)pBuffer, size); int result = USBD_CDC_TransmitPacket(&USBD_Device); wait_us = millis() + USBTX_BUSYWAIT; while (pCDC->TxState && wait_us > millis()) ; if (pCDC->TxState != 0) return USBD_BUSY; return size; }So I now have everything working as I want but trying to understand what is going on, e.g. how come that in my latest version exceeding the USB HS buffer size has no impact and actually improves quality of data? Also, shouldn’t there be a check for USB FS max packet size in the original VCP_Write() to begin with, which is 64 bytes? After all I’m not using HS here but rather FS.
The above applies to ST’s Disco F407 and mikroElektronikas Mini M4 for STM32 (F415RG), testing on both boards produces the same outcome. All clocks/prescalers are set correctly. VCP driver on the PC is ST’s own version 1.4.
Hi,
The VCP_write() function is provided by us and it’s based on the original sample from ST and it indeed contains a bug with HS instead of FS. The correct version should look like this:
#ifdef USE_USB_HS enum { kMaxOutPacketSize = CDC_DATA_HS_OUT_PACKET_SIZE }; #else enum { kMaxOutPacketSize = CDC_DATA_FS_OUT_PACKET_SIZE }; #endif int VCP_write(const void *pBuffer, int size) { if (size > kMaxOutPacketSize) { int offset; int done = 0; for (offset = 0; offset < size; offset += done) { int todo = MIN(kMaxOutPacketSize, size - offset); done = VCP_write(((char *)pBuffer) + offset, todo); if (done != todo) return offset + done; } return size; } USBD_CDC_HandleTypeDef *pCDC = (USBD_CDC_HandleTypeDef *)USBD_Device.pClassData; while (pCDC->TxState) {} //Wait for previous transfer USBD_CDC_SetTxBuffer(&USBD_Device, (uint8_t *)pBuffer, size); if (USBD_CDC_TransmitPacket(&USBD_Device) != USBD_OK) return 0; while (pCDC->TxState) {} //Wait until transfer is done return size; }We have updated this internally and will include the fix in the next release of the BSP. Regarding packets larger than CDC_DATA_FS_OUT_PACKET_SIZE, they do sometimes work, but may not be fully supported by the hardware and hence may cause strange effects, so we would not recommend using them.
Thanks, this does not work for me. With your bug fixed version I am still seeing noise on the data and with the corrected, reduced FS buffer limit it is now worse than before. Here is how the signal data looks — and should look — with my no-size-check version:
And here is the equivalent output via VCP_write():
To test further I disabled all irrelevant peripherals in my program to avoid interference and printed out this long string once every 5 seconds in a loop:
“ABCDEFGHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210:::::\r\n”
In a terminal window (termite or SmartTTY) a single VCP_write() call outputs the below. Note that the string is not transmitted to the end and also clipped from start, one character at the time:
ABCDEFGHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyzBCDEFGHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_CDEFGHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9DEFGHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_98EFGHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_987FGHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876GHIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_98765HIJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_987654IJKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543JKLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_98765432KLMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_987654321LMNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210MNOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210:NOPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::OPQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210:::PQRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::QRSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210:::::RSTUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: STUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: TUVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: UVWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: VWXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: WXYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: XYZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: YZ_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: Z_0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: _0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 0123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 123456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 23456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 3456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 456789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 56789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 6789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 789_abcdefghijklmnopqrstuvwxyz_9876543210::::: 89_abcdefghijklmnopqrstuvwxyz_9876543210::::: 9_abcdefghijklmnopqrstuvwxyz_9876543210::::: _abcdefghijklmnopqrstuvwxyz_9876543210::::: abcdefghijklmnopqrstuvwxyz_9876543210::::: bcdefghijklmnopqrstuvwxyz_9876543210::::: cdefghijklmnopqrstuvwxyz_9876543210::::: defghijklmnopqrstuvwxyz_9876543210::::: efghijklmnopqrstuvwxyz_9876543210::::: fghijklmnopqrstuvwxyz_9876543210::::: ghijklmnopqrstuvwxyz_9876543210::::: hijklmnopqrstuvwxyz_9876543210::::: ijklmnopqrstuvwxyz_9876543210::::: jklmnopqrstuvwxyz_9876543210::::: klmnopqrstuvwxyz_9876543210::::: lmnopqrstuvwxyz_9876543210::::: mnopqrstuvwxyz_9876543210::::: nopqrstuvwxyz_9876543210::::: opqrstuvwxyz_9876543210::::: pqrstuvwxyz_9876543210::::: qrstuvwxyz_9876543210::::: rstuvwxyz_9876543210::::: stuvwxyz_9876543210::::: tuvwxyz_9876543210::::: uvwxyz_9876543210::::: vwxyz_9876543210::::: wxyz_9876543210::::: xyz_9876543210::::: yz_9876543210::::: z_9876543210::::: _9876543210::::: 9876543210::::: 876543210::::: 76543210::::: 6543210::::: 543210::::: 43210::::: 3210::::: 210::::: 10::::: 0::::: ::::: :::: ::: :: :
I’ll keep investigating this during the remainder of the day to see if I can find the issue.
Okay then, after further investigation your VCP_write() must still have a bug. I wrote an alternate function with code
stolen frominspired by Atmel SAM3X USB core lib and it works now. Here is my signal data looking good and all:
And here is the function which should handle transmitting >64 bytes over USB FS correctly:
int VCP_AltWrite(const void *pBuffer, int len) { uint32_t n; int r = len; uint8_t* data = (uint8_t*)pBuffer; USBD_CDC_HandleTypeDef *pCDC = (USBD_CDC_HandleTypeDef *)USBD_Device.pClassData; while (pCDC->TxState) {} //Wait for previous transfer while (len) { n = kMaxOutPacketSize; if (n > len) n = len; len -= n; USBD_CDC_SetTxBuffer(&USBD_Device, data, n); if (USBD_CDC_TransmitPacket(&USBD_Device) != USBD_OK) return 0; while (pCDC->TxState) {} //Wait until transfer is done data += n; } return r; }Note also the long string mentioned in my previous post prints out correctly using this function. Now, where do I pick up my paycheck…? 😉
- You must be logged in to reply to this topic.