CubeMX LWIP ethernet_link_thread bug

[el_d]

I am using stm32f407zg board with DP83848 ethernet PHY chip. CubeMX generates code with LWIP for FreeRTOS. App used to answer ping successfully when it starts with connected eth cable only and stops after its reconnecting. After some investigation i have noticed some suspicious place in code(see below). After adding "_IT" postfix it started to work, so looks like cube used wrong handler to stop/start ETH periphery(see lines 7 and 30). So my question is: is it a CubeMX bug or i just missed some delails?

Here is a cycle from ethernet_link_thread entry in ethernetif.c file:

for(;;)
  {
  PHYLinkState = DP83848_GetLinkState(&DP83848);
 
  if(netif_is_link_up(netif) && (PHYLinkState <= DP83848_STATUS_LINK_DOWN))
  {
    HAL_ETH_Stop_IT(&heth); //Here we stop ETH 
    netif_set_down(netif);
    netif_set_link_down(netif);
  }
  else if(!netif_is_link_up(netif) && (PHYLinkState > DP83848_STATUS_LINK_DOWN))
  {
    switch (PHYLinkState)
    {
    case DP83848_STATUS_100MBITS_FULLDUPLEX:
      duplex = ETH_FULLDUPLEX_MODE;
      speed = ETH_SPEED_100M;
      linkchanged = 1;
      break;
   ...
    }
 
    if(linkchanged)
    {
      /* Get MAC Config MAC */
      HAL_ETH_GetMACConfig(&heth, &MACConf);
      MACConf.DuplexMode = duplex;
      MACConf.Speed = speed;
      HAL_ETH_SetMACConfig(&heth, &MACConf);
      HAL_ETH_Start(&heth); // HERE IS A POSSIBLE BUG by using wrong handle to start ETH
      netif_set_up(netif);
      netif_set_link_up(netif);
    }
  }

[federic0]

Can someone at ST listen to @Piranha and fix these bugs?

[Garnett.Robert]

I second the motion.

[Garnett.Robert]

Hi Piranha,

I had a look at the ethernet_link_thread an it seems to me that it only calls lwip if the link is down as detected by the phy.  I can't see how this violates the Lwip thread rules? If the link is sown the system has to be re initialised in any event, with auto negotiation etc. What am I missing?

I do agree with you that it would be better to do away with this thread and do the checking in the ethernetif_input() sem timeout.  And I agree they should use task notifications for deferred interrupts as they are faster and use less memory.  I use notifications whenever I can.  I suspect they don't because they want to use the cmsis wrappers for everything which are a pain because they don't provide the functionality of the raw FreeRTOS API.

eg. for a timer 2 interrupt

/* This  */
void TIM2_IRQHandler(void)
{
  BaseType_t xHigherPriorityTaskWoken = pdFALSE;
  __HAL_TIM_CLEAR_FLAG(htim, TIM_FLAG_UPDATE);
  vTaskNotifyGiveFromISR(procCntrlTaskHandle, &xHigherPriorityTaskWoken);
  portYIELD_FROM_ISR(xHigherPriorityTaskWoken);
}

/* Instead of this */
void HAL_TIM_IRQHandler(TIM_HandleTypeDef *htim)
{
... 70 lines of code
}

void HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *handlerEth)
{
  osSemaphoreRelease(RxPktSemaphore);
}

/* Calls this */
osStatus_t osSemaphoreRelease (osSemaphoreId_t semaphore_id) {
  SemaphoreHandle_t hSemaphore = (SemaphoreHandle_t)semaphore_id;
  osStatus_t stat;
  BaseType_t yield;
  stat = osOK;
  if (hSemaphore == NULL) {
    stat = osErrorParameter;
  }
  else if (IS_IRQ()) {
    yield = pdFALSE;

    if (xSemaphoreGiveFromISR (hSemaphore, &yield) != pdTRUE) {
      stat = osErrorResource;
    } else {
      portYIELD_FROM_ISR (yield);
    }
  }
  else {
    if (xSemaphoreGive (hSemaphore) != pdPASS) {
      stat = osErrorResource;
    }
  }
  return (stat);
}

Regards

Rob

void ethernet_link_thread(void* argument)
{
  ETH_MACConfigTypeDef MACConf = {0};
  int32_t PHYLinkState = 0;
  uint32_t linkchanged = 0U, speed = 0U, duplex = 0U;

  struct netif *netif = (struct netif *) argument;
/* USER CODE BEGIN ETH link init */

/* USER CODE END ETH link init */

  for(;;)
  {
  PHYLinkState = LAN8742_GetLinkState(&LAN8742);

  if(netif_is_link_up(netif) && (PHYLinkState <= LAN8742_STATUS_LINK_DOWN))
  {
    HAL_ETH_Stop_IT(&heth);  /****** ONLY DOES THIS WHEN THE LINK IS DOWN ****/
    netif_set_down(netif);
    netif_set_link_down(netif);
  }
  else if(!netif_is_link_up(netif) && (PHYLinkState > LAN8742_STATUS_LINK_DOWN))
  {
    switch (PHYLinkState)
    {
    case LAN8742_STATUS_100MBITS_FULLDUPLEX:
      duplex = ETH_FULLDUPLEX_MODE;
      speed = ETH_SPEED_100M;
      linkchanged = 1;
      break;
    case LAN8742_STATUS_100MBITS_HALFDUPLEX:
      duplex = ETH_HALFDUPLEX_MODE;
      speed = ETH_SPEED_100M;
      linkchanged = 1;
      break;
    case LAN8742_STATUS_10MBITS_FULLDUPLEX:
      duplex = ETH_FULLDUPLEX_MODE;
      speed = ETH_SPEED_10M;
      linkchanged = 1;
      break;
    case LAN8742_STATUS_10MBITS_HALFDUPLEX:
      duplex = ETH_HALFDUPLEX_MODE;
      speed = ETH_SPEED_10M;
      linkchanged = 1;
      break;
    default:
      break;
    }

    if(linkchanged)
    {
      /* Get MAC Config MAC */
      HAL_ETH_GetMACConfig(&heth, &MACConf);
      MACConf.DuplexMode = duplex;
      MACConf.Speed = speed;
      HAL_ETH_SetMACConfig(&heth, &MACConf);
      HAL_ETH_Start_IT(&heth);
      netif_set_up(netif);
      netif_set_link_up(netif);
    }
  }

/* USER CODE BEGIN ETH link Thread core code for User BSP */

/* USER CODE END ETH link Thread core code for User BSP */

    osDelay(100);
  }