STMicroelectronics / stm32l4xx_hal_driver

Provides the STM32Cube MCU Component "hal_driver" of the STM32L4 series.
BSD 3-Clause "New" or "Revised" License
30 stars 14 forks source link

`HAL_I2C_Master_Transmit` timeout parameter behaves unexpectedly #3

Closed zfields closed 2 years ago

zfields commented 3 years ago

Describe the set-up

Describe the bug The timeout parameter begins counting from the instant the transaction begins, and it doesn't appear that the watchdog is being reset with each incoming byte. Therefore, when you have a long I2C transaction, it will timeout, even if it is working perfectly.

Here is an example with a 250ms timeout:

Invocation: HAL_I2C_Master_Transmit(&hi2c4, (dev_addr << 1), query_request, strlen(query_request), 250);

image

How To Reproduce

  1. Select a slow I2C speed (i.e. 100kHz).
  2. Select a easy timeout value that is easy to test (e.g. 100ms).
  3. Send a long I2C transaction that is guaranteed to surpass the given timeout.
  4. The transaction will stop sending midstream, within a few milliseconds of the timeout expiration.
  5. I assume this is isolated to the I2C Timeout watchdog

Expected Behavior I would expect the timeout to be related to the amount of time the slave device holds the line low, or time between incoming bytes, but certainly not the length of an in progress I2C transaction.

ASELSTM commented 3 years ago

Hi @zfields,

Thank you for this 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 3 years ago

Hi @zfields,

Sorry for the delayed answer.

Referring to our development team, the timeout value should be tuned by the user and referring the use case of the application so that it can run without error. Actually, we de not have a timer counter enabled during the low period of the line, this mean that we cannot associated a notion of timeout with a latency of the hardware or distant device.

The development team, recommends increasing the timeout value so that the application could runs without issues.

Hope this would help you.

With regards,

zfields commented 3 years ago

No, this does not help. In fact, if this is true, then this implementation does not match the implementation of any other I2C bus that I'm aware of.

A timeout (or watchdog) does NOT typically fire, when a device is operating under "normal" or successful conditions. In this case the I2C driver is actively sending information, and it should NOT "timeout". There must be some misunderstanding of my request.

"timeout value should be tuned by the user and referring the use case of the application so that it can run without error."

Given the way your timeout functions, a static system would be required, so you would need to know the length of every single transaction. This would leave HAL_MAX_DELAY as the only option to handle data of arbitrary length.

Please request for the "development team" to take another look a the following section specifically:

In the implementation of HAL_I2C_Master_Transmit(), you make repeated calls to I2C_WaitOnFlagUntilTimeout() and I2C_WaitOnTXISFlagUntilTimeout() without adjusting tickstart for each byte transferred.

The bug likely stems from the fact tickstart is set outside the transmission while loop, and is not updated with each iteration.

stm32l4xx_hal_i2c.c

1078 /* Init tickstart for timeout management*/
1079 tickstart = HAL_GetTick();
     ...
1108 while (hi2c->XferCount > 0U)
1109 {
         ... (never updated here)
1143 }
ASELSTM commented 2 years ago

Hi @zfields,

The HAL_I2C_Master_Transmit() function is designed so that the timeout value corresponds to the duration set by the user to ensure that the system don't hang during the transfer. The timeout value does not correspond to one character but for the full communication and it should be computed by the user depending on the application and on the amount of data to transfer. This value corresponds the duration estimated by the user after which transfer should be carried out.

Hence, it is up to the application to compute the maximum timeout possible for the number of characters transferred during the communication.

Since this is the normal behavior and that HAL_I2C_Master_Transmit() works as designed for, please allow me thus to close this thread.

With regards,

zfields commented 2 years ago

Does this mean STMicroelectronic's chips do NOT support variable length I2C transmissions, by design?

This would effectively eliminate "full I2C support" of several ICs, by disallowing any reasonable usage of the timeout parameter for variable length transmissions.

This is very disappointing news.