STMicroelectronics / stm32f4xx_hal_driver

Provides the STM32Cube MCU Component "hal_driver" of the STM32F4 series.
BSD 3-Clause "New" or "Revised" License
109 stars 47 forks source link

The TxE flag should never be cleared through the I2C's DR register at any time. #33

Closed HYP96 closed 3 weeks ago

HYP96 commented 1 month ago
static void I2C_MemoryTransmit_TXE_BTF(I2C_HandleTypeDef *hi2c)
{
  /* Declaration of temporary variables to prevent undefined behavior of volatile usage */
  HAL_I2C_StateTypeDef CurrentState = hi2c->State;

  if (hi2c->EventCount == 0U)
  {
    /* If Memory address size is 8Bit */
    if (hi2c->MemaddSize == I2C_MEMADD_SIZE_8BIT)
    {
      /* Send Memory Address */
      hi2c->Instance->DR = I2C_MEM_ADD_LSB(hi2c->Memaddress);

      hi2c->EventCount += 2U;
    }
    /* If Memory address size is 16Bit */
    else
    {
      /* Send MSB of Memory Address */
      hi2c->Instance->DR = I2C_MEM_ADD_MSB(hi2c->Memaddress);

      hi2c->EventCount++;
    }
  }
  else if (hi2c->EventCount == 1U)
  {
    /* Send LSB of Memory Address */
    hi2c->Instance->DR = I2C_MEM_ADD_LSB(hi2c->Memaddress);

    hi2c->EventCount++;
  }
  else if (hi2c->EventCount == 2U)
  {
    if (CurrentState == HAL_I2C_STATE_BUSY_RX)
    {
      /* Generate Restart */
      hi2c->Instance->CR1 |= I2C_CR1_START;

      hi2c->EventCount++;
    }
    else if ((hi2c->XferCount > 0U) && (CurrentState == HAL_I2C_STATE_BUSY_TX))
    {
      /* Write data to DR */
      hi2c->Instance->DR = *hi2c->pBuffPtr;

      /* Increment Buffer pointer */
      hi2c->pBuffPtr++;

      /* Update counter */
      hi2c->XferCount--;
    }
    else if ((hi2c->XferCount == 0U) && (CurrentState == HAL_I2C_STATE_BUSY_TX))
    {
      /* Generate Stop condition then Call TxCpltCallback() */
      /* Disable EVT, BUF and ERR interrupt */
      __HAL_I2C_DISABLE_IT(hi2c, I2C_IT_EVT | I2C_IT_BUF | I2C_IT_ERR);

      /* Generate Stop */
      SET_BIT(hi2c->Instance->CR1, I2C_CR1_STOP);

      hi2c->PreviousState = I2C_STATE_NONE;
      hi2c->State = HAL_I2C_STATE_READY;
      hi2c->Mode = HAL_I2C_MODE_NONE;
#if (USE_HAL_I2C_REGISTER_CALLBACKS == 1)
      hi2c->MemTxCpltCallback(hi2c);
#else
      HAL_I2C_MemTxCpltCallback(hi2c);
#endif /* USE_HAL_I2C_REGISTER_CALLBACKS */
    }
    else
    {
      /* Do nothing */
    }
  }
  else
  {
    /* Clear TXE and BTF flags */
    I2C_Flush_DR(hi2c);
  }
}

static void I2C_Flush_DR(I2C_HandleTypeDef *hi2c)
{
  /* Write a dummy data in DR to clear TXE flag */
  if (__HAL_I2C_GET_FLAG(hi2c, I2C_FLAG_TXE) != RESET)
  {
    hi2c->Instance->DR = 0x00U;
  }
}

This change causes a serious issue: when using the HAL_I2C_Mem_Read_IT(I2C_HandleTypeDef *hi2c, uint16_t DevAddress, uint16_t MemAddress, uint16_t MemAddSize, uint8_t *pData, uint16_t Size) function, it enters an interrupt to send DevAddress and MemAddress. However, when I2C_MemoryTransmit_TXE_BTF finishes sending and [hi2c->EventCount] is greater than 2, it enters the I2C_Flush_DR(I2C_HandleTypeDef *hi2c) function, causing an additional 0x00 to be sent, which prevents reading data from the EEPROM. Therefore, this place should not clear the DR, and all HAL library I2C functions should not perform the operation of clearing the DR register.

RJMSTM commented 1 month ago

Hello @HYP96,

Thank you for your contribution. I believe you have opened the issue twice, which is a duplication of https://github.com/STMicroelectronics/stm32f4xx_hal_driver/issues/32 . Please close one of them.

Thank you.

HYP96 commented 1 month ago

Have you tested the HAL library using the AT24 series EEPROM chip or the MB85RC series FRAM chip? In my tests with an oscilloscope, the code is completely unusable and should be discarded. The reason is that 0x00 is written to the chip by mistake. This is an obvious disaster, and you could have cleaned up after the data was sent and received, not in the middle of the transfer. I don't know why you would rather make your code unusable than fix it or roll it back. @RJMSTM @ALABSTM @KRASTM

ALABSTM commented 3 weeks ago

Hi @HYP96,

As explained by @KRASTM in #32, we could not reproduce the issue you described. Hence, we cannot consider our driver as faulty. Please allow us to close this thread again. Thank you for your comprehension.

With regards,