[SOLVED] I²C - [STM32CubeL4][1.13.0][stm32l4xx_hal_i2c] Wrong RD_WRN value

[phlf]

I may be mistaken but It appears that the HAL is not computing the correct value for the RD_RWN bit as defined line 6319 of the file stm32l4xx_hal_i2c.c:

/**
 * @brief Handles I2Cx communication when starting transfer or during transfer (TC or TCR flag are set).
 * @param hi2c I2C handle.
 * @param DevAddress Specifies the slave address to be programmed.
 * @param Size Specifies the number of bytes to be programmed.
 * This parameter must be a value between 0 and 255.
 * @param Mode New state of the I2C START condition generation.
 * This parameter can be one of the following values:
 * @arg @ref I2C_RELOAD_MODE Enable Reload mode .
 * @arg @ref I2C_AUTOEND_MODE Enable Automatic end mode.
 * @arg @ref I2C_SOFTEND_MODE Enable Software end mode.
 * @param Request New state of the I2C START condition generation.
 * This parameter can be one of the following values:
 * @arg @ref I2C_NO_STARTSTOP Don't Generate stop and start condition.
 * @arg @ref I2C_GENERATE_STOP Generate stop condition (Size should be set to 0).
 * @arg @ref I2C_GENERATE_START_READ Generate Restart for read request.
 * @arg @ref I2C_GENERATE_START_WRITE Generate Restart for write request.
 * @retval None
 */
static void I2C_TransferConfig(I2C_HandleTypeDef *hi2c, uint16_t DevAddress, uint8_t Size, uint32_t Mode, uint32_t Request)
{
 /* Check the parameters */
 assert_param(IS_I2C_ALL_INSTANCE(hi2c->Instance));
 assert_param(IS_TRANSFER_MODE(Mode));
 assert_param(IS_TRANSFER_REQUEST(Request));
 
 /* update CR2 register */
 MODIFY_REG(hi2c->Instance->CR2, ((I2C_CR2_SADD | I2C_CR2_NBYTES | I2C_CR2_RELOAD | I2C_CR2_AUTOEND | (I2C_CR2_RD_WRN & (uint32_t)(Request >> (31U - I2C_CR2_RD_WRN_Pos))) | I2C_CR2_START | I2C_CR2_STOP)), \
 (uint32_t)(((uint32_t)DevAddress & I2C_CR2_SADD) | (((uint32_t)Size << I2C_CR2_NBYTES_Pos) & I2C_CR2_NBYTES) | (uint32_t)Mode | (uint32_t)Request));
}

where:

(I2C_CR2_RD_WRN & (uint32_t)(Request >> (31U - I2C_CR2_RD_WRN_Pos)))

seems wrong as it shifts 21 bits to the rights the Request value which may be I2C_GENERATE_START_READ or I2C_GENERATE_START_WRITE as defined in stm32l4xx_hal_i2c.h.

Or I2C_GENERATE_START_READ is already "ORed" with I2C_CR2_RD_WRN by definition. So the function I2C_TransferConfig should be changed to:

static void I2C_TransferConfig(I2C_HandleTypeDef *hi2c, uint16_t DevAddress, uint8_t Size, uint32_t Mode, uint32_t Request)
{
 /* Check the parameters */
 assert_param(IS_I2C_ALL_INSTANCE(hi2c->Instance));
 assert_param(IS_TRANSFER_MODE(Mode));
 assert_param(IS_TRANSFER_REQUEST(Request));
 
 /* update CR2 register */
 MODIFY_REG(hi2c->Instance->CR2, ((I2C_CR2_SADD | I2C_CR2_NBYTES | I2C_CR2_RELOAD | I2C_CR2_AUTOEND | (I2C_CR2_RD_WRN & Request) | I2C_CR2_START | I2C_CR2_STOP)), \
 (uint32_t)(((uint32_t)DevAddress & I2C_CR2_SADD) | (((uint32_t)Size << I2C_CR2_NBYTES_Pos) & I2C_CR2_NBYTES) | (uint32_t)Mode | (uint32_t)Request));
}

where:

(I2C_CR2_RD_WRN & (uint32_t)(Request >> (31U - I2C_CR2_RD_WRN_Pos)))

is replaced with:

(I2C_CR2_RD_WRN & Request)

I am not 100% sure so any feedback about this is highly appreciated.

[Imen.D]

Hello @phlf​ ,

We will check your reported issue and come back to you as soon as possible.

Kind Regards,

Imen

[Imen.D]

Hello @phlf​ ,

In fact after double check the current code on L4 is correct.

To explain :

The "(I2C_CR2_RD_WRN & (uint32_t)(Request >> (31U - I2C_CR2_RD_WRN_Pos)))" is used in second parameter of the MODIFY_REG, where second parameter is the clearmask bit :

#define MODIFY_REG(REG, CLEARMASK, SETMASK) WRITE_REG((REG), (((READ_REG(REG)) & (~(CLEARMASK))) | (SETMASK)))

So goal of this "(I2C_CR2_RD_WRN & (uint32_t)(Request >> (31U - I2C_CR2_RD_WRN_Pos)))" is to not clear previous RD/WR in CR2 when update CR2.

Example, when driver use define "I2C_NO_STARTSTOP" which not request to read no write, goal is to not modified previous RD/WR bit.

Only for I2C_GENERATE_STOP, I2C_GENERATE_START_READ, and I2C_GENERATE_START_WRITE, CR2 RD/WR bit must be modified so cleared before update.

Now if you take time to convert/shift all request define, you have this result of clearbit:

(I2C_NO_STARTSTOP >> (31U - I2C_CR2_RD_WRN_Pos)) = 0 => so bit I2C_CR2_RD_WRN is not cleared

(I2C_GENERATE_STOP >> (31U - I2C_CR2_RD_WRN_Pos)) = 0x400 mean equal to I2C_CR2_RD_WRN => so bit I2C_CR2_RD_WRN is cleared before udpate

(I2C_GENERATE_START_READ >> (31U - I2C_CR2_RD_WRN_Pos)) = 0x400 mean equal to I2C_CR2_RD_WRN => so bit I2C_CR2_RD_WRN is cleared before udpate

(I2C_GENERATE_START_WRITE >> (31U - I2C_CR2_RD_WRN_Pos)) = 0x400 mean equal to I2C_CR2_RD_WRN => so bit I2C_CR2_RD_WRN is cleared before udpate

Hope that is clear now.

Please feel free to to share your comments or questions.

Kind Regards,

Imen

[phlf]

I had a mixed feeling about this issue.

Thank you very much for clearing things out.

I will try to take the time to investigate why I do not have the expected behavior on my I²C communication.