cancel
Showing results for 
Search instead for 
Did you mean: 

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

Robmar
Senior III

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

 

 

1 ACCEPTED SOLUTION

Accepted Solutions
SofLit
ST Employee

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

 

 

 

 

To give better visibility on the answered topics, please click on "Accept as Solution" on the reply which solved your issue or answered your question.

View solution in original post

14 REPLIES 14
Robmar
Senior III

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)

 

 

 

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?

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
ST Employee

Escalated internally to be fixed.

Internal ticket number for tracking 189599.

To give better visibility on the answered topics, please click on "Accept as Solution" on the reply which solved your issue or answered your question.

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.
Evangelist III

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.

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
ST Employee

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

 

 

 

 

To give better visibility on the answered topics, please click on "Accept as Solution" on the reply which solved your issue or answered your question.

@Robmar wrote:

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


Really?

Wouldn't it assume int ?