STMicroelectronics / stm32l0xx_hal_driver

Provides the STM32Cube MCU Component "hal_driver" of the STM32L0 series.
BSD 3-Clause "New" or "Revised" License
8 stars 2 forks source link

An I2C NACK during memory address transfer goes undetected. #3

Closed jelledevleeschouwer closed 2 years ago

jelledevleeschouwer commented 4 years ago

Dear @ALABSTM,

It seems you released a new HAL with a fix for #1, great! :+1: I do have another bug to report, however:

Set-up

Description When a NACK is observed during memory address transfer, it goes undetected until the next I2C transaction, and the I2C HAL produces a HAL_I2C_ERROR_TIMEOUT error rather than a HAL_I2C_ERROR_AF error.

We have observed a situation in which a I2C slave device NACK's a memory address (see sreenshot 1 below). However it seems the I2C hal cannot cope with this situation. When this occurs, the I2C driver is waiting for the TC bit to high in case a of a MemoryRead operation:

stm32l0xx_hal_i2c.c:5266:

  /* Wait until TC flag is set */
  if (I2C_WaitOnFlagUntilTimeout(hi2c, I2C_FLAG_TC, RESET, Timeout, Tickstart) != HAL_OK)
  {
    return HAL_ERROR;
  }

However, since the NACK occurs this operation times out. If we inspect the I2C peripheral register contents when this occurs, we observe:

 CR1 = 0x1,
 CR2 = 0x10032,
 OAR1 = 0x8000,
 OAR2 = 0x0,
 TIMINGR = 0xd61329,
 TIMEOUTR = 0x0,
 ISR = 0x131,           # Bit 8: BERR = 1 (Bus error), Bit 5: STOPF = 1 (Stop detected), Bit 4: NACKF = 1 (Not Acknowledge received), Bit 1: TXE = 1 (Transmit data register empty )
 ICR = 0x0,
 PECR = 0x0,
 RXDR = 0x3c,
 TXDR = 0x37

But we don't notice any of that because the I2C HAL doesn't check for that NACKF anymore, we just observe a timeout and we get on with our business, but the next time we request an I2C read transaction the following occurs: the HAL I2C driver requests a memory read with I2C_RequestMemoryRead() wherein the transaction is initiated by configuring the transfer and generating a start condition, after which the driver waits for the Transmit interrupt status bit to become set (TXIS). In the subroutine responsible for this the NACKF status bit does get tested:

stm32l0xx_hal_i2c.c:6256:

static HAL_StatusTypeDef I2C_WaitOnTXISFlagUntilTimeout(I2C_HandleTypeDef *hi2c, uint32_t Timeout, uint32_t Tickstart)
{
  while (__HAL_I2C_GET_FLAG(hi2c, I2C_FLAG_TXIS) == RESET)
  {
    /* Check if a NACK is detected */
    if (I2C_IsAcknowledgeFailed(hi2c, Timeout, Tickstart) != HAL_OK)
    {
      return HAL_ERROR;
    }
    // ...

Since the NACKF is still set from the previous I2C transaction, this new I2C transaction will be prematurely aborted, while the start condition and slave address will already be transmitted on the wire. If we inspect the register contents if has happened we observe:

  CR1 = 0x1,
  CR2 = 0x0,
  OAR1 = 0x8000,
  OAR2 = 0x0,
  TIMINGR = 0xd61329,
  TIMEOUTR = 0x0,
  ISR = 0x8141,       # Bit 15: BUSY = 1 (Busy), Bit 8: BERR = 1 (Bus error), Bit 6: TC = 1 (Transfer Complete), Bit 1: TXE = 1 (Transmit data register empty)
 ICR = 0x0,
  ICR = 0x0,
  PECR = 0x0,
  RXDR = 0x0,
  TXDR = 0x0

Since the NACK was already detected during previous I2C transaction the STOP condition was already generated during previous I2C transaction (see register contents above). But now the current I2C transaction is prematurely aborted, in combination with the fact that the START condition and the I2C device address has already been transmitted on the wire, the BUSY flag is set high! Which prevents any further I2C transactions to be request from the HAL. This will generate timeout issues at each subsequent i2c_mem_read request:

stm32l0xx_hal_i2c.c:2450:

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

So I propose to please add a NACKF detection to memory address transfer phase as well, either by adding an additional call to I2C_WaitOnTXISFlagUntilTimeout() or by extending the I2C_WaitOnFlagUntilTimeout() function with a test for NACKF.

What do you think is best?

Thanks, Best regards,

Jelle De Vleeschouwer

Screenshots

  1. nack_received

ALABSTM commented 4 years ago

Hi @jelledevleeschouwer,

Glad to read from you again. Glad also to know the previous HAL release addressed the EXTI reset issue.

As for I2C topic, @ASELSTM is the one in charge and will handle this issue.

With regards,

ASELSTM commented 3 years ago

Hi @jelledevleeschouwer,

First we would like to thank you for your contribution and for this detailed report. It has been forwarded to our development teams. We will get back to you as soon as they provide us with their feedback.

With regards,

ASELSTM commented 2 years ago

ST Internal Reference: 120754.

ASELSTM commented 2 years ago

Hi @jelledevleeschouwer,

A fix has been implemented and made available in the frame of the latest stm32l0_hal_driver V1.10.5 release. Please allow me then to close this thread. You can reopen it at any time if you still detect this issue.

With regards,