CubeMX gen. project, CDC_Transmit_FS not working reliable

[nichtgedacht]

Posted on August 26, 2016 at 23:46

In a CubeMX (lastest version now) generated project CDC_Transmit_FS resp.

USBD_CDC_TransmitPacket do not work reliable. On every few request there are no data readable on host from the device (/dev/ttyACM0) despite the function returns a complete write. Target is a STM32F103CBT6 The trigger and reading on the host is at the command line It shall be replaced by a config tool later: Terminal line is set raw (stty -F /dev/ttyACM0 raw) and works at 115200 8N1. Commandline:

printf 'push_settings\0' > /dev/ttyACM0; cat flashpage.0x0801Fbin > /dev/ttyACM0

and the other direction which is malfunctioning some times:

printf 'pull_settings\0' > /dev/ttyACM0; ./read-settings.pl > flashpage.0x0801F400-down.bin

read-settings.pl is the perl script below. It just don't return if malfunctioning happens.

#!/usr/bin/perl
open $device, ''< /dev/ttyACM0'' or die;
my $buffer;
my $bufsize=1024;
my $total=0;
my $read=0;
while ( $total < 1024 and $read = sysread($device , $buffer , $bufsize, $total ) )
{
$total += $read;
}
close $device;
syswrite ( STDOUT, $buffer);

The debugging implementation on the MCU is as the follows. usbd_cdc_if.c:

...
#define APP_TX_DATA_SIZE 32
#define APP_RX_DATA_SIZE 64
...
static int8_t CDC_Init_FS(void)
{
/* USER CODE BEGIN 3 */
// Set Application Buffers
USBD_CDC_SetTxBuffer(&hUsbDeviceFS, UserTxBufferFS, 0);
USBD_CDC_SetRxBuffer(&hUsbDeviceFS, UserRxBufferFS);
return (USBD_OK);
/* USER CODE END 3 */
}
...
// This is functioning very well and fills reliable my 1k
// received_data buffer in 16 steps. Rearming in main.
static int8_t CDC_Receive_FS(uint8_t* Buf, uint32_t *Len)
{
/* USER CODE BEGIN 6 */
if ( cdc_received_tot + *Len > 1024 )
{
cdc_received_tot = 0;
}
// store bytes from UserRxBufferFS at next received_data index
memcpy(&received_data[cdc_received_tot], Buf, *Len);
cdc_received_tot += *Len;
cdc_received = 1;
USBD_CDC_SetRxBuffer(&hUsbDeviceFS, &Buf[0]);
return (USBD_OK);
/* USER CODE END 6 */
}
...
uint16_t CDC_Transmit_FS(uint8_t* Buf, uint16_t Len)
{
uint8_t result = USBD_OK;
/* USER CODE BEGIN 7 */
uint16_t bytes_written = 0;
uint16_t bytes_to_write;
USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef*) hUsbDeviceFS.pClassData;
if (hcdc->TxState != 0)
{
return USBD_BUSY;
}
// Check if USB interface is connected
if ( hUsbDeviceFS.dev_state != USBD_STATE_CONFIGURED )
{
return = USBD_FAIL;
}
else
{
// Busy wait if USB is busy or exit on success or disconnection happens
while (bytes_written < 
Len
)
{
HAL_Delay(10);
bytes_to_write = (Len - bytes_written) > APP_TX_DATA_SIZE ? APP_TX_DATA_SIZE : ( Len - bytes_written );
memcpy(UserTxBufferFS, &Buf[bytes_written], bytes_to_write );
USBD_CDC_SetTxBuffer(&hUsbDeviceFS, UserTxBufferFS, bytes_to_write);
//Check if USB disconnected while retrying
if ( hUsbDeviceFS.dev_state != USBD_STATE_CONFIGURED )
{
return USBD_FAIL;
}
// Try send
result = USBD_CDC_TransmitPacket(&hUsbDeviceFS);
if (result == USBD_OK)
{
bytes_written += bytes_to_write;
}
else if (result != USBD_BUSY) // other error
{
return result;
}
}
}
/* USER CODE END 7 */
return bytes_written;
}

main.c:

while (1)
{
...
// 200 Hz, flag from timer callback which don't have to be paced in this part/mode of the program
if (PeriodElapsed == 1)
{
...
if ( cdc_received == 1)
{
if (rcv_settings == 1)
{
if (cdc_received_tot < 1024)
{
cdc_received = 0;
USBD_CDC_ReceivePacket(&hUsbDeviceFS);
}
else
{
sprintf(buf, ''%d'', cdc_received_tot);
BSP_LCD_SetTextColor(LCD_COLOR_BLACK);
BSP_LCD_SetBackColor(LCD_COLOR_BLACK);
BSP_LCD_FillRect(0, 1 * 12, BSP_LCD_GetXSize(), 12);
BSP_LCD_SetTextColor(LCD_COLOR_YELLOW);
BSP_LCD_DisplayStringAtLine(1, (uint8_t *) buf);
cdc_received = 0;
cdc_received_tot = 0;
USBD_CDC_ReceivePacket(&hUsbDeviceFS);
rcv_settings = 0;
if (erase_flash_page() != HAL_OK)
{
Error_Handler();
}
else
{
if (write_flash_vars((uint32_t*) received_data, 256, 0) != HAL_OK)
{
Error_Handler();
}
}
}
}
else if (strcmp((const char *) received_data, (const char *) ''bootloader'') == 0)
{
//setting flag in flash
//so reborn as bootloader we can know that we should not start this live immediately again
// write string ''DFU'' (4 bytes incl. trailing \0) to last 32 bit wide space of flash page
write_flash_vars((uint32_t*) ''DFU'', 1, 1020);
// is this really needed? I think it is not
HAL_NVIC_DisableIRQ(USB_LP_CAN1_RX0_IRQn);
HAL_NVIC_DisableIRQ(USART1_IRQn);
HAL_NVIC_DisableIRQ(TIM2_IRQn);
// set stackpointer pointing to bootloader startup and reset system
__set_MSP(*(__IO uint32_t*) BOOTLOADER_ADDRESS);
HAL_NVIC_SystemReset();
}
else if (strcmp((const char *) received_data, (const char *) ''push_settings'') == 0)
{
cdc_received_tot = 0;
cdc_received = 0;
rcv_settings = 1;
USBD_CDC_ReceivePacket(&hUsbDeviceFS);
}
else if (strcmp((const char *) received_data, (const char *) ''pull_settings'') == 0)
{
cdc_received_tot = 0;
cdc_received = 0;
snd_settings = 1;
USBD_CDC_ReceivePacket(&hUsbDeviceFS);
read_flash_vars((uint32_t *) flash_buf, 256, 0);
BSP_LCD_SetTextColor(LCD_COLOR_BLACK);
BSP_LCD_SetBackColor(LCD_COLOR_BLACK);
BSP_LCD_FillRect(0, 0 * 12, BSP_LCD_GetXSize(), 12);
BSP_LCD_SetTextColor(LCD_COLOR_YELLOW);
BSP_LCD_DisplayStringAtLine(0, (uint8_t *) ''XXXX'');
}
}
if (snd_settings == 1 )
{
usb_res = CDC_Transmit_FS((uint8_t*) flash_buf, 1024);
sprintf(buf, ''%d'', usb_res);
BSP_LCD_SetTextColor(LCD_COLOR_BLACK);
BSP_LCD_SetBackColor(LCD_COLOR_BLACK);
BSP_LCD_FillRect(0, 0 * 12, BSP_LCD_GetXSize(), 12);
BSP_LCD_SetTextColor(LCD_COLOR_YELLOW);
BSP_LCD_DisplayStringAtLine(0, (uint8_t *) buf);
if ( usb_res != USBD_BUSY)
{
snd_settings = 0;
}
}
}
...
}
}
...

On the display one can read always the completed 1024 bytes even if there is nothing to read from /dev/ttyACM0. Of course variables like cdc_received_tot and so on are declared volatile. How to debug this thing? Bug in CDC Lib? Kind regards

[Tesla DeLorean]

Posted on August 27, 2016 at 02:58

The Erase and Write of FLASH while running in FLASH can cause significant stalling of the CPU and the loss of real-time operation and responsiveness. Peripheral that need immediate servicing may fail or otherwise timeout or over/under-flow.

Debugging should be done using telemetry output via SWV or USART channels.

[nichtgedacht]

Posted on August 27, 2016 at 11:53

The malfunction occurs with

''pull_settings

''. Flash gets only read in this case. I found out that increasing the delay to 20ms within the write loop in CDC_Transmit_FS solved the problem. By my opinion USBD_CDC_TransmitPacket must not return USBD_OK before the data is safely drained.

[christoph2399]

Posted on August 30, 2016 at 10:32

main problem could be the concept of the software or understanding how usb is working.

an usb-device can not initiate a transmission. the usb-host must address the device and get the data. its like an master / slave concept.therefor the CDC_Transmit_FS() just sets the pointer to the prepared buffers. when this is done you'll get the USBD_OK but physically the data may not be transmitted yet.the data will be transmitted with the usb-core which is based on interrupts.so all the buffers set to the CDC-driver are accessed out of an usb-interrupt. so be aware you have to mutex / block the access of the TX/RX buffer while the cdc-driver is not done yet.  USBD_CDC_SetTxBuffer (&hUsbDeviceFS, UserTxBufferFS, 0);  USBD_CDC_SetRxBuffer (&hUsbDeviceFS, UserRxBufferFS);

[nichtgedacht]

Posted on August 30, 2016 at 18:58

I'm talking about USBD_CDC_TransmitPacket() not CDC_Transmit_FS().

As you can see in my code the host is reading the CDC data or at least tries to read it. But it stucks sometimes because there is no data to read at all. Obviously this happens because USBD_CDC_TransmitPacket() is called to fast again after its previous call since a sufficient delay between the calls and before initializing the next send buffer solved the problem. But this is merely a dirty workarround. On device side USBD_CDC_TransmitPacket() returns USBD_OK not USBD_BUSY or USBD_FAIL If this is the expected behavior. Meaning the data are not send then and the buffer is still in use by the usb-core how can one decide whether it is ok to initialize the next send buffer and then call USBD_CDC_TransmitPacket() again? There is no example from ST of calling USBD_CDC_TransmitPacket() except within a HAL_TIM_PeriodElapsedCallback(). If this timer is slow enough the error can also never happen.

[christoph2399]

Posted on August 31, 2016 at 08:44

your right, you've to check if u can access the buffer again.

what i did is following, i placed a function within my mainloop which is handling the buffers. so i moved the buffer setting out of the interrupt into the mainloop, which is totaly fine with cdc in my application. there is no driver or hal-function for that so i accessed the variable

TxState

void

CDC_task (
void
)

{

cdc_data_t *p = &cdc_data; 
// pointer to my modul data



if
(p->active) 
// modul is running

{

// put received bytes into fifo

if
(p->rxbytes) 
// set in CDC_Receive_FS() to indicate new bytes

{

// copy bytes from rx buffer into rx fifo

uint32_t 
free
= fifo_GetFreeSpace (&p->rxfifo); 



if
(
free
>= p->rxbytes)

{

fifo_insert (&p->rxfifo, UserRxBufferFS, p->rxbytes);

p->rxbytes = 0; 
// reset receive flag

USBD_CDC_SetRxBuffer (&hUsbDeviceFS, UserRxBufferFS);

USBD_CDC_ReceivePacket (&hUsbDeviceFS); 
// start receiving

}

}



// get bytes out of fifo and transmit

USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef *) hUsbDeviceFS.pClassData;

if
(hcdc->TxState == 0) 
// dirty way to access transmission status of cdc

{

uint32_t bytes = fifo_retrieve (&p->txfifo, UserTxBufferFS, 
sizeof
(UserTxBufferFS)); // copy from tx fifo into send buffer



if
(bytes) // data are in sendbuffer

{

USBD_CDC_SetTxBuffer (&hUsbDeviceFS, UserTxBufferFS, bytes); 
// set transmit buffer

USBD_CDC_TransmitPacket (&hUsbDeviceFS); 
// start transmitting

}

}

}

PS: hopyfully you can read my code :) should be easy to understand. concept ist application is writing into a tx-fifo (transmit) and reading a rx-fifo (receiving), the driver in this case usb-cdc is reading the tx-fifo and copys it into the send buffer of usb-cdc and writing the rx-fifo with data of the receiving buffer of the ucb-cdc

[nichtgedacht]

Posted on August 31, 2016 at 15:46

Within USBD_CDC_TransmitPacket() there is already a check for TxState

The only location where this flag is set is also found there The problem is that this check doesn't deliver the expected.

uint8_t USBD_CDC_TransmitPacket(USBD_HandleTypeDef *pdev)
{
USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef*) pdev->pClassData;
if(pdev->pClassData != NULL)
{
if(hcdc->TxState == 0)
{
/* Tx Transfer in progress */
hcdc->TxState = 1;
/* Transmit next packet */
USBD_LL_Transmit(pdev,
CDC_IN_EP,
hcdc->TxBuffer,
hcdc->TxLength);
return USBD_OK;
}
else
{
return USBD_BUSY;
}
}
else
{
return USBD_FAIL;
}
}

The only location where this flag is unset can be found in USBD_CDC_DataIn()

static uint8_t USBD_CDC_DataIn (USBD_HandleTypeDef *pdev, uint8_t epnum)
{ 
USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef*) pdev->pClassData;
if(pdev->pClassData != NULL)
{
hcdc->TxState = 0;
return USBD_OK;
} 
else
{
return USBD_FAIL;
}
}

USBD_CDC_DataIn ist part of the CDC interface class callbacks structure Struct is:

typedef struct _Device_cb
{
uint8_t (*Init) (struct _USBD_HandleTypeDef *pdev , uint8_t cfgidx);
uint8_t (*DeInit) (struct _USBD_HandleTypeDef *pdev , uint8_t cfgidx);
/* Control Endpoints*/
uint8_t (*Setup) (struct _USBD_HandleTypeDef *pdev , USBD_SetupReqTypedef *req);
uint8_t (*EP0_TxSent) (struct _USBD_HandleTypeDef *pdev ); 
uint8_t (*EP0_RxReady) (struct _USBD_HandleTypeDef *pdev ); 
/* Class Specific Endpoints*/
uint8_t (*DataIn) (struct _USBD_HandleTypeDef *pdev , uint8_t epnum);
uint8_t (*DataOut) (struct _USBD_HandleTypeDef *pdev , uint8_t epnum);
uint8_t (*SOF) (struct _USBD_HandleTypeDef *pdev);
uint8_t (*IsoINIncomplete) (struct _USBD_HandleTypeDef *pdev , uint8_t epnum);
uint8_t (*IsoOUTIncomplete) (struct _USBD_HandleTypeDef *pdev , uint8_t epnum);
uint8_t *(*GetHSConfigDescriptor)(uint16_t *length); 
uint8_t *(*GetFSConfigDescriptor)(uint16_t *length);
uint8_t *(*GetOtherSpeedConfigDescriptor)(uint16_t *length);
uint8_t *(*GetDeviceQualifierDescriptor)(uint16_t *length);
#if (USBD_SUPPORT_USER_STRING == 1)
uint8_t *(*GetUsrStrDescriptor)(struct _USBD_HandleTypeDef *pdev ,uint8_t index, uint16_t *length);
#endif
} USBD_ClassTypeDef;

The implemetation of this pointer structure is

USBD_ClassTypeDef USBD_CDC =
{
USBD_CDC_Init,
USBD_CDC_DeInit,
USBD_CDC_Setup,
NULL, /* EP0_TxSent, */
USBD_CDC_EP0_RxReady,
USBD_CDC_DataIn,
USBD_CDC_DataOut,
NULL,
NULL,
NULL,
USBD_CDC_GetHSCfgDesc,
USBD_CDC_GetFSCfgDesc,
USBD_CDC_GetOtherSpeedCfgDesc,
USBD_CDC_GetDeviceQualifierDescriptor,
};

Looking at the ISR one can find

static HAL_StatusTypeDef PCD_EP_ISR_Handler(PCD_HandleTypeDef *hpcd)
...
/* TX COMPLETE */
HAL_PCD_DataInStageCallback(hpcd, 0);
...

void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
{
USBD_LL_DataInStage((USBD_HandleTypeDef*)hpcd->pData, epnum, hpcd->IN_ep[epnum].xfer_buff);
}

USBD_StatusTypeDef USBD_LL_DataInStage(USBD_HandleTypeDef *pdev ,uint8_t epnum, uint8_t *pdata)
if((pdev->pClass->EP0_TxSent != NULL)&&
(pdev->dev_state == USBD_STATE_CONFIGURED))
{
pdev->pClass->EP0_TxSent(pdev);
}
USBD_CtlReceiveStatus(pdev);

The docu UM1734 says: EP0_TxSent : This callback is called when the send status is finished. But EP0_TxSent is not implemented. The pointer to it is NULL. So TxState seems to be unset at the wrong location (not within EP0_TxSent)

[christoph2399]

Posted on August 31, 2016 at 21:38

mhh that deep in the core, i don't think there is the problem, because your 20ms delay is solving the problem of transmission?

still think its an ''buffer'' issue. i had one problem with USBD_malloc() which is used by the cdc core, my heap was to small, due the issue that the driver allocates 540bytes for a cdc-structure. --- just checked, core is setting txstate. the ''in'' and ''out'' is always seen from the point of the usb-host, so our tx is the host incoming endpoint

static
uint8_t USBD_CDC_DataIn (USBD_HandleTypeDef *pdev, uint8_t epnum)
{
USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef*) pdev->pClassData;
if
(pdev->pClassData != NULL)
{
hcdc->TxState = 0;
return
USBD_OK;
}
else
{
return
USBD_FAIL;
}
}

[A. M.]

Posted on December 30, 2017 at 14:23

Hardcoded / assumed delay() makes no sense... I've applied EP0_TxSent sugestion from Dierer and it works :)

I have not measured latencies, but it works in practice for my app purposes (a bridge between virtual com visible to windows software and real on-board usart). Here is the code:

------------------------------------------------

void HAL_PCD_MspInit(PCD_HandleTypeDef* pcdHandle) {

....

    /* Peripheral interrupt init */

    HAL_NVIC_SetPriority(USB_LP_CAN1_RX0_IRQn, 9, 0); where 9 is highest priority in my system

...

------------------------------------------------

uint8_t CDC_Transmit_FS(uint8_t* Buf, uint16_t Len)

{  uint8_t result = USBD_OK;  /* USER CODE BEGIN 7 */

  USBD_CDC_HandleTypeDef *hcdc = (USBD_CDC_HandleTypeDef*)hUsbDeviceFS.pClassData;

  if (hcdc->TxState != 0){return USBD_BUSY;}

  USBD_CDC_SetTxBuffer(&hUsbDeviceFS, (uint8_t*)Buf, Len);

  result = USBD_CDC_TransmitPacket(&hUsbDeviceFS);

  do{;} while (hcdc->TxState != 0);   //-> here is the conditional wait until buffer is really tx'ed

  /* USER CODE END 7 */

  return result;}

------------------------------------------------

static uint8_t  EP0_TxSentCallback (USBD_HandleTypeDef *pdev)

{    USBD_CDC_HandleTypeDef   *hcdc;    hcdc = (USBD_CDC_HandleTypeDef*) pdev->pClassData;

    hcdc->TxState = 0; // set 0 once tx is completed, above function will use/need it

    return USBD_OK;

}

------------------------------------------------

USBD_ClassTypeDef  USBD_CDC =

{  USBD_CDC_Init,  USBD_CDC_DeInit,  USBD_CDC_Setup,  //  NULL,                 /* EP0_TxSent, */ // -> original code, no callback defined  EP0_TxSentCallback,       /* EP0_TxSent, */ // -> new code, above callback is defined already  USBD_CDC_EP0_RxReady,  USBD_CDC_DataIn,  USBD_CDC_DataOut,  NULL,

.....

-------------------------------

CDC_Transmit_FS() is called from non-ISR context.

It may have sense to add it into a into usbd_cdc solution, so users do not need to reinvet it ;)

Additionally (the other direction):

static int8_t CDC_Receive_FS (uint8_t* Buf, uint32_t *Len)

{  /* USER CODE BEGIN 6 */  USBD_CDC_SetRxBuffer(&hUsbDeviceFS, &Buf[0]);  USBD_CDC_ReceivePacket(&hUsbDeviceFS);

  if(*Len>0) SendToSerial3((char*)Buf, *Len); // to ESP8266

  return (USBD_OK);

  /* USER CODE END 6 */ }

, where

extern 'C' int SendToSerial3(char* buf, int len)

{    volatile int state = huart3.gState;

    while( (state==HAL_UART_STATE_BUSY) || (state==HAL_UART_STATE_BUSY_TX) || (state==HAL_UART_STATE_BUSY_TX_RX) /*&& (state!=HAL_UART_STATE_TIMEOUT) && uart3txdone==0*/)

    {        state=huart3.gState;    };

    uart3txdone = 0;

    HAL_UART_Transmit(&huart3, (uint8_t*) buf, len, 5000);

    state = huart3.gState;

    return len;

}

, where uart3txdone is also updated/maintained by usart code

Not an ideal solution, but I hope it will help.