CubeMX - Full duplex communication in USB CDC

[sincoon]

Posted on April 24, 2015 at 16:30

So, I have finally achieved sending capability over CDC USB. BUT...

I have this CDC_Receive_FS function

/**
* @brief CDC_Receive_FS
* Data received over USB OUT endpoint are sent over CDC interface 
* through this function.
* 
* @note
* This function will block any OUT packet reception on USB endpoint 
* untill exiting this function. If you exit this function before transfer
* is complete on CDC interface (ie. using DMA controller) it will result 
* in receiving more data while previous ones are still not sent.
* 
* @param Buf: Buffer of data to be received
* @param Len: Number of data received (in bytes)
* @retval Result of the operation: USBD_OK if all operations are OK else USBD_FAIL
*/
static int8_t CDC_Receive_FS (uint8_t* Buf, uint32_t *Len)
{
/* USER CODE BEGIN 7 */ 
uint8_t result = USBD_OK;
int i;
USBD_CDC_SetRxBuffer(hUsbDevice_0, &UserRxBufferFS[0]); 
result = USBD_CDC_ReceivePacket(hUsbDevice_0); 
for (uint32_t i = 0; i < 1000000; i++);
HAL_GPIO_TogglePin(GPIOD, GPIO_PIN_14);
CDC_Transmit_FS(''Hello'', 5);
for (uint32_t i = 0; i < 1000000; i++);
HAL_GPIO_TogglePin(GPIOD, GPIO_PIN_14);
CDC_Transmit_FS(''Gello'', 5);
for (uint32_t i = 0; i < 1000000; i++); 
return result;
/* USER CODE END 7 */ 
}

As said in comments to this function, this function will block any out packet :( So, I cannot send multiple packets at this function? Only one? #vcp #stm32 #usb #stm32f0 #cdc

[noobee]

Posted on April 24, 2015 at 17:05

i always thought it is never safe to call CDC_Transmit_FS() in the context of an ISR (or the callback in this case). it you follow CDC_Transmit_FS() in the code, it'll call HAL_PCD_EP_Transmit() eventually, which does a  __HAL_LOCK(hpcd); to acquire the hpcd lock.

the same issue applies to the many of the stm32 device drivers since they follow the same design pattern. calling HAL_UART_Transmit_IT() within the HAL_UART_RxCpltCallback() is not interrupt-safe, because of the same lock contention when you call HAL_UART_Transmit_IT() from non-ISR mode and a receive interrupt occurs leading to calling HAL_UART_Transmit_IT() from ISR mode.

[diabolo38]

Posted on April 24, 2015 at 18:10

just got to found that issue  myslef ...

use critical section or disable interrupt prior to call the HAL driver functon when not in isr. if that function also use time base ofn tick then ther's ,no solution   :( HAL lock policy is quite incorrect ... not to say uselless (lock testing and taking must be atomic or its can be broken)  

[sincoon]

Posted on April 24, 2015 at 18:22

Ok, so what do I need to do? My task is to send a lot of data as response on some command(''READ_BUF''). I've made this as loop in CDC_Read_FS function, but it does not work :(

[noobee]

Posted on April 25, 2015 at 02:33

in general, i think you should keep processing in the ISR context ''as short as possible''.

in this case, you could post an event to an event queue, or set a volatile global variable to indicate you have received data, and then immediately return from the callback.then, in the ''main'' processing context, process the event that you received (through the event queue or global flag) and call the transmit function, which, in this case, would not be in the ISR context.

[hybridalienrob]

Posted on January 29, 2016 at 23:52

I just came across a very similar issue using a MIDI USBD driver (based on the AUDIO USBD driver from STMCubeMX.

The issue I had is that both the HAL_PCD_EP_Transmit and HAL_PCD_EP_Receive functions both use the same hal lock variable, and I found it quite easy to lock up the USB IRQ if I try to send and receive at the same time : EP_Receive is called from the IRQ and EP_Transmit is called from a user thread.Obviously the whole HAL_LOCK macro is a horribly bad implementation as it returns HAL_BUSY instead of waiting and the return code is effectively ignored a lot of the time.My solution was to add a LockRx and LockTx variable to the PCD_HandleTypeDef struct and create two new sets of LOCK / UNLOCK macros which I can use to protect the IN / OUT endpoints. I left the 'setup' code, or EP0 handling locks using the original lock and now I have reliable duplex comms.

[richardst9]

Posted on September 24, 2016 at 09:32

I've hit exactly this issue (HAL_PCD_EP_Receive in interrupt receive fails if HAL_LOCK is set in User thread). So I 'm really keen to hear more details about how you fixed it.

Can you provide more details about your improved LOCK and UNLOCK mechanisms? Many thanks, Richard

[slimen]

Posted on September 30, 2016 at 13:56

Hello 

Your issue is mentioned in this [DEAD LINK /public/STe2ecommunities/mcu/Lists/STM32Java/Flat.aspx?RootFolder=/public/STe2ecommunities/mcu/Lists/STM32Java/STM32Cube%20USB%20CDC%20%28possible%29%20Bug%20Found%20-%20Rx%20Tx%20Race%20condition&FolderCTID=0x01200200770978C69A1141439FE559EB459D758000F9A0E3A95BA69146A17C2E80209ADC21&currentviews=122]thread and reported to ST team.So, ST team is aware about this issue.Regards

[hybridalienrob]

Here was my solution :-

In file stm32f4xx_hal_def.h :-

// workaround for terrible design of HAL lock use in USB driver
#define __HAL_LOCK_RX(__HANDLE__)         \
do{                                       \
  if((__HANDLE__)->LockRx == HAL_LOCKED)  \
  {                                       \
      return HAL_BUSY;                    \
  }                                       \
  else                                    \
  {                                       \
   (__HANDLE__)->LockRx = HAL_LOCKED;     \
  }                                       \
} while (0)
 
#define __HAL_UNLOCK_RX(__HANDLE__)       \
do{                                       \
  (__HANDLE__)->LockRx = HAL_UNLOCKED;    \
}while (0)
 
#define __HAL_LOCK_TX(__HANDLE__)         \
do{                                       \
    if((__HANDLE__)->LockTx == HAL_LOCKED)\
    {                                     \
       return HAL_BUSY;                   \
    }                                     \
    else                                  \
    {                                     \
       (__HANDLE__)->LockTx = HAL_LOCKED; \
    }                                     \
  }while (0)
 
#define __HAL_UNLOCK_TX(__HANDLE__)       \
do{                                       \
    (__HANDLE__)->LockTx = HAL_UNLOCKED;  \
  }while (0)

Add the following members to the PCD_HandleTypeDef struct :-

 HAL_LockTypeDef         LockTx;         /*!<  workaround for terrible HAL lock design - if try to send from user thread while USB is receiving - USB IRQ locks up! */
  HAL_LockTypeDef         LockRx;         /*!< workaround for terrible HAL lock design - if try to send from user thread while USB is receiving - USB IRQ locks up! */
 

In stm32f4xx_hal_pcd.c , add the following lines to HAL_PCD_Init..

HAL_StatusTypeDef HAL_PCD_Init(PCD_HandleTypeDef *hpcd)
{ 
  uint32_t i = 0U;
  
  /* Check the PCD handle allocation */
  if(hpcd == NULL)
  {
    return HAL_ERROR;
  }
  
  //  init locks (missing from original code!)
  hpcd->Lock = HAL_UNLOCKED;
  hpcd->LockRx = HAL_UNLOCKED;
  hpcd->LockTx = HAL_UNLOCKED;
 
<snip>

Change HAL_PCD_EP_Receive to :-

HAL_StatusTypeDef HAL_PCD_EP_Receive(PCD_HandleTypeDef *hpcd, uint8_t ep_addr, uint8_t *pBuf, uint32_t len)
{
  USB_OTG_EPTypeDef *ep;
  
  ep = &hpcd->OUT_ep[ep_addr & 0x7F];
  
  /*setup and start the Xfer */
  ep->xfer_buff = pBuf;  
  ep->xfer_len = len;
  ep->xfer_count = 0U;
  ep->is_in = 0U;
  ep->num = ep_addr & 0x7F;
  
  if (hpcd->Init.dma_enable == 1U)
  {
    ep->dma_addr = (uint32_t)pBuf;  
  }
  
  // Now uses "Rx" lock so we don't stall in here in IRQ context if we happen to be transmitting in the user thread
  __HAL_LOCK_RX(hpcd); 
 
  if ((ep_addr & 0x7FU) == 0U)
  {
    USB_EP0StartXfer(hpcd->Instance , ep, hpcd->Init.dma_enable);
  }
  else
  {
    USB_EPStartXfer(hpcd->Instance , ep, hpcd->Init.dma_enable);
  }
  __HAL_UNLOCK_RX(hpcd); 
  
  return HAL_OK;
}

Change HAL_PCD_EP_Transmit to :-

HAL_StatusTypeDef HAL_PCD_EP_Transmit(PCD_HandleTypeDef *hpcd, uint8_t ep_addr, uint8_t *pBuf, uint32_t len)
{
  USB_OTG_EPTypeDef *ep;
  
  ep = &hpcd->IN_ep[ep_addr & 0x7F];
  
  /*setup and start the Xfer */
  ep->xfer_buff = pBuf;  
  ep->xfer_len = len;
  ep->xfer_count = 0U;
  ep->is_in = 1U;
  ep->num = ep_addr & 0x7F;
  
  if (hpcd->Init.dma_enable == 1U)
  {
    ep->dma_addr = (uint32_t)pBuf;  
  }
  
  // Now uses "Tx" lock so we don't cause the IRQ to stall 
  __HAL_LOCK_TX(hpcd); 
  
  if ((ep_addr & 0x7FU) == 0U)
  {
    USB_EP0StartXfer(hpcd->Instance , ep, hpcd->Init.dma_enable);
  }
  else
  {
    USB_EPStartXfer(hpcd->Instance , ep, hpcd->Init.dma_enable);
  }
  
  __HAL_UNLOCK_TX(hpcd);
     
  return HAL_OK;
}

Change HAL_PCD_EP_ClrStall to

HAL_StatusTypeDef HAL_PCD_EP_ClrStall(PCD_HandleTypeDef *hpcd, uint8_t ep_addr)
{
  USB_OTG_EPTypeDef *ep;
  
  if ((0x80 & ep_addr) == 0x80)
  {
    ep = &hpcd->IN_ep[ep_addr & 0x7F];
  }
  else
  {
    ep = &hpcd->OUT_ep[ep_addr];
  }
  
  ep->is_stall = 0U;
  ep->num   = ep_addr & 0x7F;
  ep->is_in = ((ep_addr & 0x80) == 0x80);
  
  if ( ep->is_in) 
      __HAL_LOCK_TX(hpcd); 
  else
      __HAL_LOCK_RX(hpcd);
  USB_EPClearStall(hpcd->Instance , ep);
  if ( ep->is_in) 
      __HAL_UNLOCK_TX(hpcd); 
  else
      __HAL_UNLOCK_RX(hpcd);
    
  return HAL_OK;
}

Change HAL_PCD_EP_Flush to

HAL_StatusTypeDef HAL_PCD_EP_Flush(PCD_HandleTypeDef *hpcd, uint8_t ep_addr)
{
  __HAL_LOCK(hpcd); 
  
  if ((ep_addr & 0x80) == 0x80)
  {
      __HAL_LOCK_TX(hpcd);
      USB_FlushTxFifo(hpcd->Instance, ep_addr & 0x7FU);
      __HAL_UNLOCK_TX(hpcd); 
  }
  else
  {
      __HAL_LOCK_RX(hpcd); 
    USB_FlushRxFifo(hpcd->Instance);
      __HAL_UNLOCK_RX(hpcd); 
  }
  
  __HAL_UNLOCK(hpcd); 
    
  return HAL_OK;
}