Issue on I2C HAL generated by CubeMX

[Noirim DIH]

Hello,

I think that I found an issue in the HAL code generated by CubeMx 6.6.1 with STM32L4.

I2C HAL in file stm32l4xx_hal_i2c.c line 6855 there is an infinite loop :

     while (__HAL_I2C_GET_FLAG(hi2c, I2C_FLAG_STOPF) == RESET)

     {

      /* Check for the Timeout */

      if ((HAL_GetTick() - tickstart) > I2C_TIMEOUT_STOPF)

      {

       hi2c->ErrorCode |= HAL_I2C_ERROR_TIMEOUT;

       hi2c->State = HAL_I2C_STATE_READY;

       hi2c->Mode = HAL_I2C_MODE_NONE;

       /* Process Unlocked */

       __HAL_UNLOCK(hi2c);

       status = HAL_ERROR;

      }

     }

when the time out is reached, states are updated, I2C is released but the loop still continue if (__HAL_I2C_GET_FLAG(hi2c, I2C_FLAG_STOPF) == RESET) remains true.

a break is missing. For me a correct implementation can be something like

     while (__HAL_I2C_GET_FLAG(hi2c, I2C_FLAG_STOPF) == RESET)

     {

      /* Check for the Timeout */

      if ((HAL_GetTick() - tickstart) > I2C_TIMEOUT_STOPF)

      {

       hi2c->ErrorCode |= HAL_I2C_ERROR_TIMEOUT;

       hi2c->State = HAL_I2C_STATE_READY;

       hi2c->Mode = HAL_I2C_MODE_NONE;

       /* Process Unlocked */

       __HAL_UNLOCK(hi2c);

       status = HAL_ERROR;

break; //break the while loop

      }

     }

Thanks

[Piranha]

Either the __HAL_UNLOCK(hi2c); must be removed and break; added as you suggested, or status = HAL_ERROR; must be replaced with return HAL_ERROR; . To me it seems that the latter was the original intent (if there was such at all...).

Also just look at that code. Two loops one inside the other and both waiting on the same thing. That code can be refactored into a single loop with a less code and simpler logic. Then there is this variable. It's there just because those beginners don't even know that the function parameter also is a normal variable and it's value can be modified. And the name, which differs just by a case of the first letter, is such a bad choice.

Edited by moderation team to adhere to community guidelines.

[Pavel A.]

> must be replaced with return HAL_ERROR; . To me it seems that the latter was the original intent 

This maybe is an ill effect of MISRAfication. They impose a requirement to have only one exit point from a function, forbidding returns in the middle.

[Piranha]

Not in this case. Scroll the first link up and right above you will see 4 similar I2C_WaitOn***() functions, all of which do multiple returns.

The more I hear about some strange MISRA rules, the more it sounds to be exaggerated and written by academics without a real life experience.

https://stackoverflow.com/questions/4838828/why-should-a-function-have-only-one-exit-point

Totally agreed with the sane answers. And the last down-voted answer is a good example on how a blind religious obsession can reach idiotic levels.

[Noirim DIH]

Thank you for your answers. Indeed __HAL_UNLOCK(hi2c) macro is called in I2C_IsErrorOccurred() finction without __HAL_LOCK(hi2c). it is not a big deal since the macro update the state

[Bob S]

With I2C_IsErrorOccurred(), the calling functions invoke the __HAL_LOCK() macros. There are several (many?) internal I2C functions that have similar calling patterns - the caller does the LOCK and the internal functions UNLOCK if it encounters an error.

[S.Ma]

Instead of implementing lock or mutex because a multidrop bus has multiple devices witg their unique thread, one alternate way around it is the spooler which nerwork printer given as model. Queue the unique i2c transactions from any requestor and keep low level code clean, simple and easy to maintain. Any SW wait is calling for waste of energy and taking core bandwidth thrown out of the window....

[Piranha]

With RTOS a waiting on mutex, semaphore and even a simple delay does not take a CPU cycles. But overall yes - a single central dispatcher thread is also a viable solution. Actually such a scenario is another argument against implementing a protection against multiple threads in the driver.

[Amel NASRI]

Hi @Noirim DIH​ ,

I confirm the issue. The fix will be available in next STM32CubeL4 package revision.

Internal ticket number: 128190 (This is an internal tracking number and is not accessible or usable by customers).

-Amel