Added the repeated-start feature in I2C driver

[jmchiappa]

This solves the repeated-start issue #583 . Tested on F411RE using a MMA8451 accelerometer and OLED SSD1306 I2C peripherals. This seems to be ok but it's a little bit tricky.
It must hugely tested by the community with several I2C peripherals and several platforms.

[uzi18]

I see just whitespaces changes.

[jmchiappa]

Hi @uzi18, yes the second commit concerns style issue (aka whitespaces) only. it fixes some errors during buld on Travis.
But major changes are in my first commit (adding HAL_... seq_....).
Thank you for reviewing it.

[uzi18]

looks ok

[LMESTM]

Thank you for your proposal.
I have 1 question included in the review.

[fpistm]

@jmchiappa any update on this? (I do not know if you came back from vacation 😉 )

[ABOSTM]

Hi @jmchiappa,
Thanks for your PR.
I had a look at the source code and it seems that using I2C_NO_OPTION_FRAME may cause issue:

1. Call to assert() function in STM32 HAL (if assert is activated) will fail because I2C_NO_OPTION_FRAME is not consider a valid option for external API.
2. Also, on F3 family for example, in HAL_I2C_Master_Seq_Receive_IT() the XferOptions can be written in xfermode which is then written in hardware register (I2C_CR2) through call to I2C_TransferConfig()
   {
   hi2c->XferSize = hi2c->XferCount;
   xfermode = hi2c->XferOptions;
   }
   . . .
   /* Send Slave Address and set NBYTES to read */
   I2C_TransferConfig(hi2c, DevAddress, hi2c->XferSize, xfermode, xferrequest);

But 0xFFFF0000 is not a valid value to write to register.

So I discussed with our I2C expert, and we finally reach a new proposal:
for both RX and TX:
if (sendStop == 0) {
_i2c.handle.XferOptions = I2C_OTHER_FRAME;
} else {
_i2c.handle.XferOptions = I2C_OTHER_AND_LAST_FRAME;
}

Unfortunately F0 and F2 do not yet have those defines but it should come in the future,
also F1/F3 will have it soon (next release).
So we propose to update your proposal with the solution from our expert, only when I2C_OTHER_FRAME define is available.
And keep former solution for other families (i.e. stop condition always present, and uncompatible for now with device like your accelerometer).
I made a Pull Request on top of yours, in your fork:
jmchiappa#1

I tested this with I2C EEPROM 24LC256 and with Thermal sensor LM75 on NUCLEO_L476RG to be sure there is no regression. Test are passed.

Would it be possible for you to test with your MMA8451 accelerometer ... if you get it ?

[fpistm]

@jmchiappa
is it ok for you ?

[jmchiappa]

@jmchiappa
is it ok for you ?

Hello Frédéric,
Yes, it is. I should receive the MMA for this weekend.

[fpistm]

OK.
Then we will wait your feedback.
I'm currently preparing release 1.7.0 and this should be fine to get it. 😉

[jmchiappa]

OK.
Then we will wait your feedback.
I'm currently preparing release 1.7.0 and this should be fine to get it. wink

Hi,
So after a lot of tests, here are some results which are mitigated:
Number of tested frameworks: 3
-1.6.1 - official
-1.6.1 - JM changes
-1.6.1 - ABO proposal

Number of test board : 2

• NUCLEO F411RE
• NUCLEO L476RG

Number of i2c devices : 2

• MMA 8451 (repeated start feature required)
• monochrome OLED 128x64 based on SSD1306 controller (standard I2C driver)

I2C libraries : 3
-Adafruit_MMA8451_Library (MMA library)
-Adafruit_SSD1306 (OLED library)
-U8g2 (OLED library)

Here is the list of tested configurations with their results :
1.6.1 + MMA + F411RE -> KO
1.6.1 + OLED Adafruit + L476RG-> OK, high number of frame per second (FPS)
1.6.1 + OLED u8g2 +L476RG -> OK, high number of frame per second (FPS)

1.6.1-JM + MMA + F411RE -> OK, no dead time between each i2c frame
1.6.1-JM + Adafruit + L476RG -> OK, high number of frame per second (FPS)
1.6.1-JM + U8g2 + L476RG -> OK, BUT very very low FPS (110 ms of dead time between each i2c frame)
1.6.1-JM + U8g2 + F411RE -> OK, high number of frame per second (FPS)

1.6.1-ABO + MMA + F411RE -> OK, no dead time between each i2c frame
1.6.1-ABO + Adafruit + L476RG -> OK, high number of frame per second (FPS)
1.6.1-ABO + U8g2 + L476RG -> OK, BUT very very low FPS (110 ms of dead time between each i2c frame)
1.6.1-ABO + U8g2 + F411RE-> OK, high number of frame per second (FPS)

So, I think that i2c driver behavior is different for each board.
I don't go further for now, but I think it is not mature enough to be a part of 1.7.0.
your thoughts ?

[fpistm]

@jmchiappa
right, I will not add it to 1.7.0.
Thanks for your quick feedback.
We need to perform further tests.
Anyway, my understanding is you get the same result btw your implementation and @ABOSTM one ? Right?

[jmchiappa]

@fpistm, right.
Behavior is the same for both implementations.
Please note that all i2c drivers are not consistent between boards. BTW, could you please clarify the origin of I2C_NO_OPTION_FRAME ? Up to now, this value is present in most HAL i2c drivers from ST.

Thank you for your support and I hope this feature will be available soon.
Let me know if you need more tests.

[ABOSTM]

Hi @jmchiappa,
We will try to get a U8g2 compatible device by end of the week, so we could test.
About I2C_NO_OPTION_FRAME, it is used internally (internally to STM32 cube HAL driver) to handle non sequential transfert, in case of "non sequential transfer" (HAL_I2C_Master_Receive_xx() or HAL_I2C_Master_Transmit_xx()) . And it is not aimed to be used in external API and specially not to use "sequential API" for a non sequential transfert.

[ABOSTM]

Hi @jmchiappa,
I tested with same OLED kind of display than yours. And face the same difference in measurement.
The Problem comes from U8g2 library that calls for each transfert TwoWire::setClock().
The difference in measurement comes from different I2C hardware versions:

• one is able to compute by hardware the I2C timing with simply the I2C frequency required (like STM32F4)
• the other need complex software computation... which is of course slower (like STM32L4).
  Normally it should be done only once for an application, but in case of U8gé, it is done for each transfert.
  I made a PR Avoid I2C timing computation when input clock is unchanged #646 to workaround this issue: computation is done only once, timings are saved, and reused later (in condition input clock is the same).

I now have high FPS on STM32L4.

There is still one point I would like to clarify with you:
The I2C timing computation which cause issue, is not inegrated in 1.6.1. (timing were hardcoded at that time).
So can you confirm that in fact you made the tests not exactly on 1.6.1, but rather in between 1.6.1 and head of the master ?

[jmchiappa]

Hi @ABOSTM, thank you for your quick fix, it sounds good.
My changes have been made on the "pure" 1.6.1 checkout.
I don't have any changes in twi.c file in my repo.
For now, TwoWire::setclock is calling i2c_setTiming(...)
I do agree that in my stm32duino lib, timings are hardcoded:

void i2c_setTiming(i2c_t *obj, uint32_t frequency)
{
  uint32_t f = I2C_10KHz;
  __HAL_I2C_DISABLE(&(obj->handle));

  if (frequency <= 10000) {
    f = I2C_10KHz;
  } else if (frequency <= 50000) {
    f = I2C_50KHz;
  } else if (frequency <= 100000) {
    f = I2C_100KHz;
  } else if (frequency <= 200000) {
    f = I2C_200KHz;
  } else if (frequency <= 400000) {
    f = I2C_400KHz;
  }

#if defined (STM32F0xx) || defined (STM32F3xx) || defined (STM32F7xx) ||\
    defined (STM32G0xx) || defined (STM32H7xx) || defined (STM32L0xx) ||\
     defined (STM32L4xx) || defined (STM32WBxx)
  obj->handle.Init.Timing = f;
#else
  obj->handle.Init.ClockSpeed = f;
  /* Standard mode (sm) is up to 100kHz, then it's Fast mode (fm)     */
  /* In fast mode duty cyble bit must be set in CCR register          */
  if (frequency > 100000) {
    obj->handle.Init.DutyCycle       = I2C_DUTYCYCLE_16_9;
  } else {
    obj->handle.Init.DutyCycle       = I2C_DUTYCYCLE_2;
  }
#endif
  /*
    else if(frequency <= 600000)
      g_i2c_init_info[i2c_id].i2c_handle.Init.ClockSpeed = I2C_600KHz;
    else if(frequency <= 800000)
      g_i2c_init_info[i2c_id].i2c_handle.Init.ClockSpeed = I2C_800KHz;
    else
      g_i2c_init_info[i2c_id].i2c_handle.Init.ClockSpeed = I2C_1000KHz;
  */
  HAL_I2C_Init(&(obj->handle));
  __HAL_I2C_ENABLE(&(obj->handle));
}

[fpistm]

Waiting jmchiappa#1 to be merged

[fpistm]

Thanks @jmchiappa and @ABOSTM

[fpistm]

Fixes #583

[LieBtrau]

Hello, will this also fix the same issue on the STM32F103?
I have issues using the VEML7700 ambient light sensor (as sold by DFRobot & Adafruit). That sensor requires a repeated start. Currently I worked around it by using bitbanged I²C.

[fpistm]

Probably.