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

View more

[Robmar]

The function has not return specification, on top of that one path returns nothing, the other a status value, so how does the compiler even compile it, it would have to assume int for example, then assume to return 0 for the path with no return.

Do you think that complies with C Standards, or that its safe programming?

Of course different compilers/parsers may do different things, but my point is that a $16 billion turnover company like STM should not be pushing sloppy code on their commercial clients.

Or do are you a sloppy Joe programmer? :)

[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 ...

[Robmar]

Reply edited to align with community guidelines.

You are ignoring the fact the the GNU compiler is report a warning that the function is void yet returns a value.

[Andrew Neil]

That warning would refer to the enclosing function - not the macro.

As @Bob S said earlier, where DID you see that compiler warning?