Bug in HAL I2C code - V1.11.2 HAL_I2C_Mem_Write() function

[Robmar]

Can you see the glaring bug, or is it?  I left a clue *** some fail returns do not unlock HAL

 

 

HAL_StatusTypeDef HAL_I2C_Mem_Write(I2C_HandleTypeDef *hi2c, uint16_t DevAddress, uint16_t MemAddress,
 uint16_t MemAddSize, uint8_t *pData, uint16_t Size, uint32_t Timeout)
{
 uint32_t tickstart;

 /* Check the parameters */
 assert_param(IS_I2C_MEMADD_SIZE(MemAddSize));

 if (hi2c->State == HAL_I2C_STATE_READY)
 {
 if ((pData == NULL) || (Size == 0U))
 {
 hi2c->ErrorCode = HAL_I2C_ERROR_INVALID_PARAM;
 return HAL_ERROR;
 }

 /* Process Locked ***/
 __HAL_LOCK(hi2c);

 /* Init tickstart for timeout management*/
 tickstart = HAL_GetTick();

 if (I2C_WaitOnFlagUntilTimeout(hi2c, I2C_FLAG_BUSY, SET, I2C_TIMEOUT_BUSY, tickstart) != HAL_OK)
 {
 return HAL_ERROR;
 }

 hi2c->State = HAL_I2C_STATE_BUSY_TX;
 hi2c->Mode = HAL_I2C_MODE_MEM;
 hi2c->ErrorCode = HAL_I2C_ERROR_NONE;

 /* Prepare transfer parameters */
 hi2c->pBuffPtr = pData;
 hi2c->XferCount = Size;
 hi2c->XferISR = NULL;

 /* Send Slave Address and Memory Address */
 if (I2C_RequestMemoryWrite(hi2c, DevAddress, MemAddress, MemAddSize, Timeout, tickstart) != HAL_OK)
 {
 /* Process Unlocked */
 __HAL_UNLOCK(hi2c);
 return HAL_ERROR;
 }
...

 

 

[Robmar]

There is also another fail here in this function definition, one arm has a return, the other does not.  Sloppy really as the compiler even reports the issue "Return has value in function returning void".

 

 

 #define __HAL_LOCK(__HANDLE__) \
 do{ \
 if((__HANDLE__)->Lock == HAL_LOCKED) \
 { \
 return HAL_BUSY; \
 } \
 else \
 { \
 (__HANDLE__)->Lock = HAL_LOCKED; \
 } \
 }while (0)

 

 

 

[Bob S]

I admit up front I am no fan of the "return hidden in a macro" coding style of the __HAL_LOCK() macro.

That said, you should not see that compiler warning for the __HAL_LOCK() call in HAL_I2C_Mem_Write() as that function DOES have a return value.  So where DID you see that compiler warning?

Which CPU?  I presume the "V1.11.2" refer to the Cube library version?

[Robmar]

The defined function has no return specification so the compiler assumes its void, then one arm of the "if" returns a value, of course its going to give a warning!  I´m using the latest version of CubeIDE and the warning is on the tool tip when hovering over the use of the defined function __HAL_LOCK and in the compiler warnings.

MCU is H743, Repository is the latest V1.11.2 at -> STM32Cube\Repository\STM32Cube_FW_H7_V1.11.2\Drivers\STM32H7xx_HAL_Driver\Src

[mƎALLEm]

Escalated internally to be fixed.

Internal ticket number for tracking 189599.

[Robmar]

There were quite a lot of changes from HAL V1.11.1 to 1.11.2, stm32h7xx_hal_i2c.c has quite a few changes, and our application is no longer stable communicating over I2C to a PLL SI5351, though we are still investigating the reason.

Is there any information on I2C bugs in this release?

[Pavel A.]

Do you mean that return on line 25 after I2C_WaitOnFlagUntilTimeout does not call __HAL_UNLOCK?

No bug there.  I2C_WaitOnFlagUntilTimeout calls unlock when it returns error.

Neither __HAL_LOCK macro will cause  "Return has value in function returning void" - when used properly (in functions that return HAL_StatusTypedef.

/* It's only a matter of taste, as one my friend said after watching the Paris Olympics opening */

> Is there any information on I2C bugs in this release?

STM32H7 has some I2C related errata. Please check the errata document. For the HAL library bugs - see open issues on github.

[Robmar]

As far as I see there are three error return points in HAL_I2C_Mem_Write, only 1 unlocks HAL.

No reference to any I2C bugs in HAL on the link you sent, what a pain this all is.

[mƎALLEm]

Checked this point internally and seems the Unlock is done on each function call in return of HAL_I2C_Mem_Write() in this list of function:

I2C_WaitOnFlagUntilTimeout()
I2C_WaitOnTXISFlagUntilTimeout()
I2C_WaitOnSTOPFlagUntilTimeout()

Example. In this function the unlock is done in line 24 before returning error.

static HAL_StatusTypeDef I2C_WaitOnFlagUntilTimeout(I2C_HandleTypeDef *hi2c, uint32_t Flag, FlagStatus Status,
 uint32_t Timeout, uint32_t Tickstart)
{
 while (__HAL_I2C_GET_FLAG(hi2c, Flag) == Status)
 {
 /* Check if an error is detected */
 if (I2C_IsErrorOccurred(hi2c, Timeout, Tickstart) != HAL_OK)
 {
 return HAL_ERROR;
 }

 /* Check for the Timeout */
 if (Timeout != HAL_MAX_DELAY)
 {
 if (((HAL_GetTick() - Tickstart) > Timeout) || (Timeout == 0U))
 {
 if ((__HAL_I2C_GET_FLAG(hi2c, Flag) == Status))
 {
 hi2c->ErrorCode |= HAL_I2C_ERROR_TIMEOUT;
 hi2c->State = HAL_I2C_STATE_READY;
 hi2c->Mode = HAL_I2C_MODE_NONE;

 /* Process Unlocked */
 __HAL_UNLOCK(hi2c);
 return HAL_ERROR;
 }
 }
 }
 }
 return HAL_OK;
}

 

 

 

 

[Robmar]

That allows recovery, but standard practice is to leave interface in the same state as when entered, and not depend on external routines to recover the status.

You've also overlooked the failing in the HAL_Lock code as #defined, where the return type is not specified, and one path return a status value, which prompts a GNU warning.

 

[Andrew Neil]

---

@Robmar wrote:

 in the HAL_Lock code as #defined, where the return type is not specified

---

Because __HAL_Lock is just a macro - not a function.

Therefore the return within the macro acts as a return for the function in which the macro is used. It's the enclosing function which provides the return type.

 

Whether this is a great coding style is another question ...