STMicroelectronics / STM32CubeH7

STM32Cube MCU Full Package for the STM32H7 series - (HAL + LL Drivers, CMSIS Core, CMSIS Device, MW libraries plus a set of Projects running on all boards provided by ST (Nucleo, Evaluation and Discovery Kits))
https://www.st.com/en/embedded-software/stm32cubeh7.html
Other
497 stars 305 forks source link

I2C: Mixed transfers methods causing Interrupts to hang MCU #260

Closed Corderbollie closed 11 months ago

Corderbollie commented 1 year ago

Setup

Bug Description A testprogramm acting as I2C Master is connected via I2C1 to a Slave. The slave emulates an EEPROM via I2C and offers 128 Byte to read/write using combined transactions (with repeated start) using the HAL_I2CMem... functions. The address to read from or to write to is sent by the master as a 16-Bit value. When HAL_I2C_Mem_Read_DMA is executed as first read function from the master, then any following HAL_I2C_Mem_Write... function except the HAL_I2C_Mem_Read_DMA function will cause I2C interrupts to hang the MCU.

How To Reproduce

  1. Perform a call to HAL_I2C_Mem_Read_DMA followed by a call to HAL_I2C_Mem_Write (i.e. DMA then Polling)

  2. I2C (stm32h7xx_hal_i2c.c)

  3. The use case is to mix different transaction methods (DMA, Interrupt, Polling) in an application and to perform e.g. Reads using DMA and writes using Interrupt.

  4. Perform a call to HAL_I2C_Mem_Read_DMA followed by a call to HAL_I2C_Mem_Write (i.e. DMA then Polling) or HAL_I2C_Mem_Write_IT

Additional context I found that after execution of HAL_I2C_Mem_Read_DMA TXIE is still enabled in the I2C 1->CR1 register. In I2C1->ISR register TXE and TXIS are set causing the interrupt to fire. However the call to HAL_I2C_Mem_Write (Polling) sets the function pointer XferISR in the I2C handle to zero and the interrupt is not handled. Upon execution of HAL_I2C_Mem_Read_IT after a DMA Write, the transaction hangs after the master has sent the address and one byte of the 2 byte read location (see screenshot 2)

Screenshots Screenshot 1: Debug View with call stack and registers HalI2cIrqProblem

Screenshot 2: Logic Analyzer HAL_I2C_Mem_Read_IT stops after sending the first address Byte HalI2CIrqProblem2

HBOSTM commented 1 year ago

Hello @Corderbollie ,

Thank you for this report. We will get back to you as soon as we analyze it further. This may take some time. Thank you for your comprehension.

With regards, Houssine

HBOSTM commented 1 year ago

Hello @Corderbollie ,

Thank you for this contribution. There is no problem from my side. So could you please give us more details about how you got this issue? And share part of the code shows this issue.

Best Regards, Houssine

Corderbollie commented 1 year ago

Hello @HBOSTM

I've attached the example code of my test application. The problem is, that it is not possible to to do a I2C DMA Read followed by an Interrupt or Polling Write.

/*
 * UserApp.c
 *
 */
#include "main.h"
#include <stdbool.h>

typedef bool bool_t;

// Size of the emulated EEPROM in Bytes
#define EEPROM_SIZE (128U)

// Handle of used I2C peripheral with:
//  - 7-Bit Addressing
//  - 100 KHz
extern I2C_HandleTypeDef hi2c1;

static bool isBusy = false;

enum eI2cMethod
{
  i2cMethodPolling,
  i2cMethodInterrupt,
  i2cMethodDMA
};

/**
 * @brief User application main function
 * @details Function to test I2C functions. The application runs as I2C Master
 * and is connected to an EEPROM simulation (I2C Slave) with I2C address 0x40.
 * EEPROM max. size is 128 Bytes and EEPROM addresses have 16-Bit.
 *
 * This is to proof that mixed I2C methods (i.e. Read with DMA then write with
 * polling or interrupt, read with IT then write with polling) will not work
 * with STM32Cube FW_H7 V1.11.0 on STM32H753I-EVAL2.
 *
 */
void UserAppRun(void)
{
  uint8_t rxBuffer[EEPROM_SIZE];
  uint8_t txBuffer[EEPROM_SIZE];
  HAL_StatusTypeDef status;
  uint16_t eepromAdr = 10;    // Emulated eeprom address to read/write from/to

  // Read 5 Bytes from eeprom addr. 10
  isBusy = true;
  status = HAL_I2C_Mem_Read_DMA(&hi2c1,             // I2C Handle
                                (0x40 << 1),        // I2C addr. of Slave (7 Bit)
                                eepromAdr,          // EEPROM Addr to read from
                                sizeof(eepromAdr),  // Address Size
                                rxBuffer,           // Receive buffer
                                5);                 // Number of bytes to receive
  if (status != HAL_OK) Error_Handler();
  while (isBusy){};
  HAL_Delay(1);

  // Write 5 Bytes to eeprom addr. 10
  txBuffer[0] = 5;
  txBuffer[1] = 4;
  txBuffer[2] = 3;
  txBuffer[3] = 2;
  txBuffer[4] = 1;
  // Select transfer method to use, only i2cMethodDMA will do the job for the
  // next transaction.
  // i2cMethodPolling causes the I2C interrupt to fire permanently so that the
  // MCU permanently will call the I2C Interrupt handler.
  // i2cMethodInterrupt will stop transfer after three Bytes.
  enum eI2cMethod method = i2cMethodDMA;

  if (method == i2cMethodDMA)
  {
    // This works!
    isBusy = true;
    status = HAL_I2C_Mem_Write_DMA(&hi2c1, (0x40 << 1), eepromAdr,
                                   sizeof(eepromAdr), txBuffer, 5);
    if (status != HAL_OK) Error_Handler();
    while (isBusy){};
  }
  else
  if (method == i2cMethodPolling)
  {
    // Write using polling (blocks until data transfered), won't work!
    status = HAL_I2C_Mem_Write(&hi2c1, (0x40 << 1), eepromAdr, sizeof(eepromAdr),
                               txBuffer, 5, 2000);
    if (status != HAL_OK) Error_Handler();
  }
  else
  {
    if (method == i2cMethodInterrupt)
    {
      // Write using Interrupt, won't work
      isBusy = true;
      status = HAL_I2C_Mem_Write_IT(&hi2c1, (0x40 << 1), eepromAdr,
                                    sizeof(eepromAdr), txBuffer, 5);
      if (status != HAL_OK) Error_Handler();
      while (isBusy){};
    }
  }
  HAL_Delay(1);

  // Execution will come here only if method is DMA!

  // Read 5 Bytes from eeprom addr. 10
  isBusy = true;
  status = HAL_I2C_Mem_Read_DMA(&hi2c1, (0x40 << 1), eepromAdr, sizeof(eepromAdr),
                                rxBuffer, 5);
  if (status != HAL_OK) Error_Handler();
  while (isBusy){};

}

/**
  * @brief  Memory Tx Transfer completed callback.
  * @param  hi2c Pointer to a I2C_HandleTypeDef structure that contains
  *                the configuration information for the specified I2C.
  * @retval None
  */
void HAL_I2C_MemTxCpltCallback(I2C_HandleTypeDef *hi2c)
{
  isBusy = false;
}

/**
  * @brief  Memory Rx Transfer completed callback.
  * @param  hi2c Pointer to a I2C_HandleTypeDef structure that contains
  *                the configuration information for the specified I2C.
  * @retval None
  */
void HAL_I2C_MemRxCpltCallback(I2C_HandleTypeDef *hi2c)
{
  isBusy = false;
}

/**
  * @brief  I2C error callback.
  * @param  hi2c Pointer to a I2C_HandleTypeDef structure that contains
  *                the configuration information for the specified I2C.
  * @retval None
  */
void HAL_I2C_ErrorCallback(I2C_HandleTypeDef *hi2c)
{
  Error_Handler();
}
HBOSTM commented 1 year ago

Hello @Corderbollie ,

Thank you for this report. This problem is noticed by customers, After examining I propose the following hal_driver fix: In the I2C_Mem_ISR_DMA function here


/* Disable Interrupt related to address step */
I2C_Disable_IRQ(hi2c, I2C_XFER_TX_IT); 

And here


/* Disable Interrupt related to address step */
I2C_Disable_IRQ(hi2c, I2C_XFER_TX_IT);

/* Enable only Error and NACK interrupt for data transfer */
I2C_Enable_IRQ(hi2c, I2C_XFER_ERROR_IT);

In the I2C_Mem_ISR_IT function here


/* Disable Interrupt related to address step */
I2C_Disable_IRQ(hi2c, I2C_XFER_TX_IT);

/* Enable ERR, TC, STOP, NACK and RXI interrupts */
I2C_Enable_IRQ(hi2c, I2C_XFER_RX_IT);

Could you please try this modification and come back to me if you need any further help? Thank you again for this contribution.

Best Regards,

Corderbollie commented 1 year ago

Hello @HBOSTM I've applied the above modifications and now it works like expected.

HBOSTM commented 1 year ago

ST Internal Reference: 147666

carlo-dev-git commented 1 year ago

Hi HBOSTM, the fix you introduced after releasing v1.11.1 is very important but frankly I'm not clear what the release model of these drivers is. I thought the files in the STM32H7xx_HAL_Driver folder were taken from the stm32h7xx_hal_driver repository but the fixed file stm32h7xx_hal_i2c.c seems several commits behind (e.g. does not follow MISRA-C rules).

https://github.com/STMicroelectronics/STM32CubeH7/blob/master/Drivers/STM32H7xx_HAL_Driver/Src/stm32h7xx_hal_i2c.c https://github.com/STMicroelectronics/stm32h7xx_hal_driver/blob/master/Src/stm32h7xx_hal_i2c.c

Makin-Things commented 1 year ago

Hi @HBOSTM,

I believe that both HAL_I2C_Mem_Read_DMA and HAL_I2C_Mem_Write_DMA are broken. They only ever send the first DMA (write/address) but never send the payload or the restart/read and the clocks to get the data. On top of this they then leave the I2C bus in a bad state. If you look at the original posters logic analyzer image you will see the I2C transaction does not have a stop but even worse afterwards the SCL is left pulled low. This is wrong.

I am observing the same problem.

image HAL_I2C_Mem_Read_DMA

image HAL_I2C_Mem_Write_DMA

I have tested the recent commit (https://github.com/STMicroelectronics/STM32CubeH7/commit/60267215a6c4dab597566e9e7988909797bcd1f8) and it does not resolve the problem.

Cheers Simon

Corderbollie commented 1 year ago

I still have problems with HAL_I2C_Mem_Write_IT and HAL_I2C_Mem_Read_IT. hi2c->XferSize should be set to 0 in these functions else the app runs into error HAL_I2C_ERROR_SIZE. `/ Prepare transfer parameters /

hi2c->pBuffPtr    = pData;
hi2c->XferSize    = 0;
hi2c->XferCount   = Size;
hi2c->XferOptions = I2C_NO_OPTION_FRAME;
hi2c->XferISR     = I2C_Mem_ISR_IT;
hi2c->Devaddress  = DevAddress;

`

carlo-dev-git commented 1 year ago

I still have problems with HAL_I2C_Mem_Write_IT and HAL_I2C_Mem_Read_IT. hi2c->XferSize should be set to 0 in these functions else the app runs into error HAL_I2C_ERROR_SIZE. `/ Prepare transfer parameters /

hi2c->pBuffPtr    = pData;
hi2c->XferSize    = 0;
hi2c->XferCount   = Size;
hi2c->XferOptions = I2C_NO_OPTION_FRAME;
hi2c->XferISR     = I2C_Mem_ISR_IT;
hi2c->Devaddress  = DevAddress;

The fix was made in stm32h7xx_hal_driver/ branch: https://github.com/STMicroelectronics/stm32h7xx_hal_driver/commit/e0c69e16fc5792c3fe38b9fc3e13197f1945af74

I also reported these problems to @HBOSTM in my comment above...

wheregone commented 11 months ago

seems FW_G0 V1.6.1 has this bug too. I just encountered same problem, search, and this link is found.

ALABSTM commented 11 months ago

Fixed in commit 97d545d37f2a02d51db857185813a80bf08c2841