Skip to content

Commit

Permalink
fix well-known CDC RX/TX race condition by handling HAL_BUSY state as…
Browse files Browse the repository at this point in the history 
… discussed here: https://community.st.com/s/question/0D50X00009XkfIBSAZ/stm32cube-usb-cdc-possible-bug-found-rx-tx-race-condition
  • Loading branch information
@verybadsoldier
verybadsoldier committed Dec 2, 2018
1 parent e9ec3cb commit d85b0d9
Show file tree
Hide file tree
Showing 10 changed files with 99 additions and 27 deletions.
2 changes: 1 addition & 1 deletion culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Inc/stm32f1xx_hal_pcd.h
View file
Expand Up @@ -282,7 +282,7 @@ HAL_StatusTypeDef HAL_PCD_Stop(PCD_HandleTypeDef *hpcd);
void HAL_PCD_IRQHandler(PCD_HandleTypeDef *hpcd);

void HAL_PCD_DataOutStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum);
void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum);
HAL_StatusTypeDef HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum);
void HAL_PCD_SetupStageCallback(PCD_HandleTypeDef *hpcd);
void HAL_PCD_SOFCallback(PCD_HandleTypeDef *hpcd);
void HAL_PCD_ResetCallback(PCD_HandleTypeDef *hpcd);
Expand Down
15 changes: 11 additions & 4 deletions culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_hal_pcd.c
View file
Expand Up @@ -661,11 +661,12 @@ void HAL_PCD_IRQHandler(PCD_HandleTypeDef *hpcd)
* @param epnum: endpoint number
* @retval None
*/
__weak void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
__weak HAL_StatusTypeDef HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
{
/* Prevent unused argument(s) compilation warning */
UNUSED(hpcd);
UNUSED(epnum);
return HAL_OK;
/* NOTE : This function should not be modified, when the callback is needed,
the HAL_PCD_DataInStageCallback could be implemented in the user file
*/
Expand Down Expand Up @@ -1353,7 +1354,9 @@ static HAL_StatusTypeDef PCD_EP_ISR_Handler(PCD_HandleTypeDef *hpcd)
}
else
{
HAL_PCD_EP_Receive(hpcd, ep->num, ep->xfer_buff, ep->xfer_len);
HAL_StatusTypeDef rc = HAL_PCD_EP_Receive(hpcd, ep->num, ep->xfer_buff, ep->xfer_len);
if (rc == HAL_BUSY)
return HAL_BUSY;
}

} /* if((wEPVal & EP_CTR_RX) */
Expand Down Expand Up @@ -1404,11 +1407,15 @@ static HAL_StatusTypeDef PCD_EP_ISR_Handler(PCD_HandleTypeDef *hpcd)
if (ep->xfer_len == 0)
{
/* TX COMPLETE */
HAL_PCD_DataInStageCallback(hpcd, ep->num);
HAL_StatusTypeDef rc = HAL_PCD_DataInStageCallback(hpcd, ep->num);
if (rc == HAL_BUSY)
return rc;
}
else
{
HAL_PCD_EP_Transmit(hpcd, ep->num, ep->xfer_buff, ep->xfer_len);
HAL_StatusTypeDef rc = HAL_PCD_EP_Transmit(hpcd, ep->num, ep->xfer_buff, ep->xfer_len);
if (rc == HAL_BUSY)
return HAL_BUSY;
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Src/usbd_cdc.c
View file
Expand Up @@ -922,20 +922,19 @@ uint8_t USBD_CDC_ReceivePacket(USBD_HandleTypeDef *pdev, uint8_t cdc_num)
if(pdev->dev_speed == USBD_SPEED_HIGH )
{
/* Prepare Out endpoint to receive next packet */
USBD_LL_PrepareReceive(pdev,
return USBD_LL_PrepareReceive(pdev,
CDC_OUT_EP + cdc_num*2,
hcdc->RxBuffer,
CDC_DATA_HS_OUT_PACKET_SIZE);
}
else
{
/* Prepare Out endpoint to receive next packet */
USBD_LL_PrepareReceive(pdev,
return USBD_LL_PrepareReceive(pdev,
CDC_OUT_EP + cdc_num*2,
hcdc->RxBuffer,
CDC_DATA_FS_OUT_PACKET_SIZE);
}
return USBD_OK;
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Core/Inc/usbd_core.h
View file
Expand Up @@ -135,7 +135,7 @@ USBD_StatusTypeDef USBD_LL_Transmit (USBD_HandleTypeDef *pdev,
uint8_t *pbuf,
uint16_t size);

USBD_StatusTypeDef USBD_LL_PrepareReceive(USBD_HandleTypeDef *pdev,
USBD_StatusTypeDef USBD_LL_PrepareReceive(USBD_HandleTypeDef *pdev,
uint8_t ep_addr,
uint8_t *pbuf,
uint16_t size);
Expand Down
17 changes: 10 additions & 7 deletions culfw/STM32/hal_usart.c
View file
Expand Up @@ -283,13 +283,15 @@ void HAL_UART_TxCpltCallback(UART_HandleTypeDef *UartHandle)
UART_Tx_Callback();
#endif
} else if(UartHandle->Instance==USART2) {
CDC_Receive_next(CDC1);
if (CDC_Receive_next(CDC1) == USBD_BUSY)
CDC_rx_next[CDC1] = 1;
#ifdef HAS_WIZNET
NET_Receive_next(NET1);
#endif

} else if(UartHandle->Instance==USART3) {
CDC_Receive_next(CDC2);
if (CDC_Receive_next(CDC2) == USBD_BUSY)
CDC_rx_next[CDC2] = 1;
#ifdef HAS_WIZNET
NET_Receive_next(NET2);
#endif
Expand Down Expand Up @@ -332,19 +334,20 @@ uint32_t HAL_UART_Get_Baudrate(uint8_t UART_num) {
return 0;
}

void HAL_UART_Write(uint8_t UART_num, uint8_t* Buf, uint16_t Len) {
HAL_StatusTypeDef HAL_UART_Write(uint8_t UART_num, uint8_t* Buf, uint16_t Len) {
HAL_StatusTypeDef rc;
switch(UART_num) {
case 0:
HAL_UART_Transmit_IT(&huart2, Buf, Len);
rc = HAL_UART_Transmit_IT(&huart2, Buf, Len);
break;
case 1:
HAL_UART_Transmit_IT(&huart3, Buf, Len);
rc = HAL_UART_Transmit_IT(&huart3, Buf, Len);
break;
case UART_NUM:
HAL_UART_Transmit_IT(&huart1, Buf, Len);
rc = HAL_UART_Transmit_IT(&huart1, Buf, Len);
break;
}
return;
return rc;
}

void HAL_UART_init(uint8_t UART_num) {
Expand Down
2 changes: 1 addition & 1 deletion culfw/STM32/hal_usart.h
View file
Expand Up @@ -48,7 +48,7 @@ extern void Error_Handler(void);

void HAL_UART_Set_Baudrate(uint8_t UART_num, uint32_t baudrate);
uint32_t HAL_UART_Get_Baudrate(uint8_t UART_num);
void HAL_UART_Write(uint8_t UART_num, uint8_t* Buf, uint16_t Len);
HAL_StatusTypeDef HAL_UART_Write(uint8_t UART_num, uint8_t* Buf, uint16_t Len);
void HAL_UART_init(uint8_t UART_num);
uint8_t HAL_UART_TX_is_idle(uint8_t UART_num);

Expand Down
12 changes: 10 additions & 2 deletions culfw/STM32/usbd/usbd_cdc_if.c
View file
Expand Up @@ -95,6 +95,7 @@ uint8_t UserRxBufferFS[APP_RX_DATA_SIZE];
/* USER CODE BEGIN PRIVATE_VARIABLES */

uint8_t CDC_connected[CDC_COUNT];
uint8_t CDC_rx_next[CDC_COUNT];
/* USER CODE END PRIVATE_VARIABLES */

/**
Expand Down Expand Up @@ -135,6 +136,7 @@ USBD_CDC_ItfTypeDef USBD_Interface_fops_FS =
CDC_Receive_FS
};


/* Private functions ---------------------------------------------------------*/
/**
* @brief CDC_Init_FS
Expand All @@ -149,6 +151,9 @@ static int8_t CDC_Init_FS(void)
for (unsigned x=0; x<CDC_COUNT; x++) {
USBD_CDC_SetTxBuffer(&hUsbDeviceFS, NULL, 0,x);
USBD_CDC_SetRxBuffer(&hUsbDeviceFS, UserRxBufferFS,x);
CDC_rx_next[x] = 0;
CDC_UartTx_busy_buffer[x] = 0;
CDC_UartTx_busy_buffer_len[x] = 0;
}
return (USBD_OK);
/* USER CODE END 3 */
Expand Down Expand Up @@ -380,12 +385,15 @@ unsigned char CDC_isConnected(uint8_t cdc_num)
return 0;
}

void CDC_Receive_next (uint8_t cdc_num)
int doCdcReceive = 0;

uint8_t CDC_Receive_next (uint8_t cdc_num)
{
if((cdc_num < CDC_COUNT) && (CDC_isConnected(cdc_num))) {
USBD_CDC_SetRxBuffer(&hUsbDeviceFS, UserRxBufferFS,cdc_num);
USBD_CDC_ReceivePacket(&hUsbDeviceFS, cdc_num);
return USBD_CDC_ReceivePacket(&hUsbDeviceFS, cdc_num);
}
return USBD_OK;
}

/* USER CODE END PRIVATE_FUNCTIONS_IMPLEMENTATION */
Expand Down
7 changes: 6 additions & 1 deletion culfw/STM32/usbd/usbd_cdc_if.h
View file
Expand Up @@ -39,6 +39,7 @@
#endif
/* Includes ------------------------------------------------------------------*/
#include "usbd_cdc.h"
#include "board.h"
/* USER CODE BEGIN INCLUDE */
/* USER CODE END INCLUDE */

Expand All @@ -61,6 +62,10 @@
* @}
*/

extern uint8_t CDC_rx_next[CDC_COUNT];
extern uint8_t* CDC_UartTx_busy_buffer[CDC_COUNT];
extern uint32_t* CDC_UartTx_busy_buffer_len[CDC_COUNT];

/** @defgroup USBD_CDC_IF_Exported_Types
* @{
*/
Expand Down Expand Up @@ -118,7 +123,7 @@ uint8_t CDC_Transmit_FS(uint8_t* Buf, uint16_t Len, uint8_t cdc_num);
/* USER CODE BEGIN EXPORTED_FUNCTIONS */
unsigned char CDCDSerialDriver_Write(void *data, unsigned int size, void* dummy1, uint8_t CDC_num);
unsigned char CDC_isConnected(uint8_t cdc_num);
void CDC_Receive_next (uint8_t cdc_num);
uint8_t CDC_Receive_next (uint8_t cdc_num);
/* USER CODE END EXPORTED_FUNCTIONS */
/**
* @}
Expand Down
30 changes: 26 additions & 4 deletions culfw/STM32/usbd/usbd_conf.c
View file
Expand Up @@ -124,9 +124,19 @@ void HAL_PCD_DataOutStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
* @param epnum: Endpoint Number
* @retval None
*/
void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
HAL_StatusTypeDef 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 rc = USBD_LL_DataInStage((USBD_HandleTypeDef*)hpcd->pData, epnum, hpcd->IN_ep[epnum].xfer_buff);

switch(rc) {
case USBD_OK:
return HAL_OK;
case USBD_BUSY:
return HAL_BUSY;
case USBD_FAIL:
return HAL_ERROR;
}
return HAL_ERROR;
}

/**
Expand Down Expand Up @@ -446,12 +456,24 @@ USBD_StatusTypeDef USBD_LL_Transmit (USBD_HandleTypeDef *pdev,
* @param size: Data size
* @retval USBD Status
*/
USBD_StatusTypeDef USBD_LL_PrepareReceive(USBD_HandleTypeDef *pdev,
USBD_StatusTypeDef USBD_LL_PrepareReceive(USBD_HandleTypeDef *pdev,
uint8_t ep_addr,
uint8_t *pbuf,
uint16_t size)
{
HAL_PCD_EP_Receive((PCD_HandleTypeDef*) pdev->pData, ep_addr, pbuf, size);
HAL_StatusTypeDef rc = HAL_PCD_EP_Receive((PCD_HandleTypeDef*) pdev->pData, ep_addr, pbuf, size);

switch(rc) {
case HAL_OK:
return USBD_OK;
case HAL_TIMEOUT:
case HAL_ERROR:
return USBD_FAIL;
case HAL_BUSY:
return USBD_BUSY;
}

// we don't expect to get there
return USBD_OK;
}

Expand Down
34 changes: 31 additions & 3 deletions culfw/clib/cdc_uart.c
View file
Expand Up @@ -28,13 +28,24 @@ uint8_t CDC_Tx_buffer[CDC_COUNT-1][CDC_DATA_HS_MAX_PACKET_SIZE];
uint8_t CDC_Tx_len[CDC_COUNT-1];
rb_t UART_Rx_Buffer[CDC_COUNT-1];

uint8_t* CDC_UartTx_busy_buffer[CDC_COUNT];
uint32_t* CDC_UartTx_busy_buffer_len[CDC_COUNT];

uint8_t CDCDSerialDriver_Receive_Callback1(uint8_t* Buf, uint32_t *Len) {
HAL_UART_Write(0, Buf, (uint16_t)*Len);
HAL_StatusTypeDef rc = HAL_UART_Write(0, Buf, (uint16_t)*Len);
if (rc == HAL_BUSY) {
CDC_UartTx_busy_buffer[1] = Buf;
CDC_UartTx_busy_buffer_len[1] = Len;
}
return 0;
}

uint8_t CDCDSerialDriver_Receive_Callback2(uint8_t* Buf, uint32_t *Len) {
HAL_UART_Write(1, Buf, (uint16_t)*Len);
HAL_StatusTypeDef rc = HAL_UART_Write(1, Buf, (uint16_t)*Len);
if (rc == HAL_BUSY) {
CDC_UartTx_busy_buffer[2] = Buf;
CDC_UartTx_busy_buffer_len[2] = Len;
}
return 0;
}

Expand Down Expand Up @@ -96,9 +107,26 @@ void cdc_uart_init(void) {


void cdc_uart_task(void) {
CDCDSerialDriver_Write("abcde", 5, 0, 2);
// CDCDSerialDriver_Write("abcde", 5, 0, 2);

#if CDC_COUNT > 1

for(uint8_t x = 1;x<CDC_COUNT;x++) {
if (CDC_UartTx_busy_buffer_len[x] > 0) {
HAL_StatusTypeDef rc = HAL_UART_Write(x - 1, CDC_UartTx_busy_buffer[x], (uint16_t)*CDC_UartTx_busy_buffer_len[x]);
if (rc == HAL_OK) {
CDC_UartTx_busy_buffer[x] = 0;
CDC_UartTx_busy_buffer_len[x] = 0;
}
}

if(!CDC_rx_next[x])
continue;

if (CDC_Receive_next(x) != USBD_BUSY)
CDC_rx_next[x] = 0;
}

for(uint8_t x = 0;x<CDC_COUNT-1;x++) {
if(UART_Rx_Buffer[x].nbytes) {
while(UART_Rx_Buffer[x].nbytes && CDC_Tx_len[x] < CDC_DATA_HS_MAX_PACKET_SIZE) {
Expand Down

0 comments on commit d85b0d9

Please sign in to comment.