STM32 F4 – VCP confusion

Sysprogs forums Forums VisualGDB STM32 F4 – VCP confusion

Viewing 5 posts - 1 through 5 (of 5 total)
  • Author
    Posts
  • #9992
    parsec67
    Participant

    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.

    #9996
    support
    Keymaster

    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.

    • This reply was modified 7 years, 2 months ago by support. Reason: fixed incorrect packet splitting in code example
    #10017
    parsec67
    Participant

    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.

    • This reply was modified 7 years, 2 months ago by parsec67. Reason: Spelling correction
    #10019
    parsec67
    Participant

    Okay then, after further investigation your VCP_write() must still have a bug. I wrote an alternate function with code stolen from inspired 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…? 😉

    #10194
    support
    Keymaster

    Hi,

    Thanks, we have managed to reproduce and fix this. We have updated the code example in the post above and will ship the correct code in the next STM32 BSP.

Viewing 5 posts - 1 through 5 (of 5 total)
  • You must be logged in to reply to this topic.