From d85b0d96c5d16fc8c1c27bbed18545755b72ac2f Mon Sep 17 00:00:00 2001 From: verybadsoldier Date: Sun, 2 Dec 2018 14:02:46 +0100 Subject: [PATCH] fix well-known CDC RX/TX race condition by handling HAL_BUSY state as discussed here: https://community.st.com/s/question/0D50X00009XkfIBSAZ/stm32cube-usb-cdc-possible-bug-found-rx-tx-race-condition --- .../Inc/stm32f1xx_hal_pcd.h | 2 +- .../Src/stm32f1xx_hal_pcd.c | 15 +++++--- .../Class/CDC/Src/usbd_cdc.c | 5 ++- .../Core/Inc/usbd_core.h | 2 +- culfw/STM32/hal_usart.c | 17 ++++++---- culfw/STM32/hal_usart.h | 2 +- culfw/STM32/usbd/usbd_cdc_if.c | 12 +++++-- culfw/STM32/usbd/usbd_cdc_if.h | 7 +++- culfw/STM32/usbd/usbd_conf.c | 30 +++++++++++++--- culfw/clib/cdc_uart.c | 34 +++++++++++++++++-- 10 files changed, 99 insertions(+), 27 deletions(-) diff --git a/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Inc/stm32f1xx_hal_pcd.h b/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Inc/stm32f1xx_hal_pcd.h index 9489bd34..d5832e7e 100644 --- a/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Inc/stm32f1xx_hal_pcd.h +++ b/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Inc/stm32f1xx_hal_pcd.h @@ -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); diff --git a/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_hal_pcd.c b/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_hal_pcd.c index 91461a39..e675af24 100644 --- a/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_hal_pcd.c +++ b/culfw/STM32/Drivers/STM32F1xx_HAL_Driver/Src/stm32f1xx_hal_pcd.c @@ -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 */ @@ -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) */ @@ -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; } } } diff --git a/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Src/usbd_cdc.c b/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Src/usbd_cdc.c index d3007afb..93bc0a98 100644 --- a/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Src/usbd_cdc.c +++ b/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Class/CDC/Src/usbd_cdc.c @@ -922,7 +922,7 @@ 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); @@ -930,12 +930,11 @@ uint8_t USBD_CDC_ReceivePacket(USBD_HandleTypeDef *pdev, uint8_t cdc_num) 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 { diff --git a/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Core/Inc/usbd_core.h b/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Core/Inc/usbd_core.h index 013a5c14..8f47c4cb 100644 --- a/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Core/Inc/usbd_core.h +++ b/culfw/STM32/Middlewares/ST/STM32_USB_Device_Library/Core/Inc/usbd_core.h @@ -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); diff --git a/culfw/STM32/hal_usart.c b/culfw/STM32/hal_usart.c index 7c517507..fa0f814d 100644 --- a/culfw/STM32/hal_usart.c +++ b/culfw/STM32/hal_usart.c @@ -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 @@ -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) { diff --git a/culfw/STM32/hal_usart.h b/culfw/STM32/hal_usart.h index c83a186f..2802c2ca 100644 --- a/culfw/STM32/hal_usart.h +++ b/culfw/STM32/hal_usart.h @@ -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); diff --git a/culfw/STM32/usbd/usbd_cdc_if.c b/culfw/STM32/usbd/usbd_cdc_if.c index 252036e4..cb7737cb 100644 --- a/culfw/STM32/usbd/usbd_cdc_if.c +++ b/culfw/STM32/usbd/usbd_cdc_if.c @@ -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 */ /** @@ -135,6 +136,7 @@ USBD_CDC_ItfTypeDef USBD_Interface_fops_FS = CDC_Receive_FS }; + /* Private functions ---------------------------------------------------------*/ /** * @brief CDC_Init_FS @@ -149,6 +151,9 @@ static int8_t CDC_Init_FS(void) for (unsigned x=0; xpData, 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; } /** @@ -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; } diff --git a/culfw/clib/cdc_uart.c b/culfw/clib/cdc_uart.c index 8780eecf..d34da8e7 100644 --- a/culfw/clib/cdc_uart.c +++ b/culfw/clib/cdc_uart.c @@ -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; } @@ -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 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