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

[SofLit]

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.

[SofLit · Accepted solution]

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;
}

[Andrew Neil]

---

@Robmar wrote:

The defined function has no return specification so the compiler assumes it's void

---

Really?

Wouldn't it assume int ?

:thinking_face: