Added the repeated-start feature in I2C driver #590

jmchiappa · 2019-08-03T11:55:25Z

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 · 2019-08-03T14:08:13Z

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 · 2019-08-03T18:34:29Z

looks ok

LMESTM

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

libraries/Wire/src/Wire.cpp

fpistm · 2019-08-30T06:41:15Z

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

Co-Authored-By: Frederic Pillon <[email protected]>

…o#590

ABOSTM · 2019-09-04T15:09:19Z

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:

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.
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 · 2019-09-06T12:10:39Z

@jmchiappa
is it ok for you ?

jmchiappa · 2019-09-06T12:49:08Z

@jmchiappa
is it ok for you ?

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

fpistm · 2019-09-06T12:53:10Z

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

jmchiappa · 2019-09-07T16:52:23Z

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 · 2019-09-09T09:10:39Z

@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 · 2019-09-09T09:19:10Z

@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 · 2019-09-09T15:31:11Z

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 · 2019-09-11T16:00:55Z

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 · 2019-09-11T16:33:33Z

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

update on PR Added the repeated-start feature in I2C driver stm32duino#590

fpistm

Thanks @jmchiappa and @ABOSTM

fpistm · 2019-09-13T09:48:45Z

Fixes #583

This reverts commit 2280453.

LieBtrau · 2019-10-28T20:54:14Z

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 · 2019-10-29T04:44:36Z

Probably.

Fixes stm32duino#583

jmchiappa added 2 commits August 3, 2019 13:50

Added the repeated-start feature in I2C driver

760f7aa

fixed Astyle issues

7b63e8c

uzi18 reviewed Aug 3, 2019

View reviewed changes

fpistm added enhancement New feature or request help wanted 🙏 Extra attention is needed labels Aug 19, 2019

LMESTM reviewed Aug 19, 2019

View reviewed changes

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

fpistm added the waiting feedback Further information is required label Aug 21, 2019

fpistm assigned jmchiappa Aug 21, 2019

fpistm requested changes Aug 21, 2019

View reviewed changes

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

libraries/Wire/src/Wire.cpp Outdated Show resolved Hide resolved

Update libraries/Wire/src/Wire.cpp

22379d1

Co-Authored-By: Frederic Pillon <[email protected]>

fpistm requested a review from ABOSTM September 4, 2019 07:32

jmchiappa and others added 3 commits September 4, 2019 10:43

Update libraries/Wire/src/Wire.cpp

b76a180

Co-Authored-By: Frederic Pillon <[email protected]>

Update libraries/Wire/src/Wire.cpp

551f908

Co-Authored-By: Frederic Pillon <[email protected]>

update on PR Added the repeated-start feature in I2C driver stm32duin…

1bb59af

…o#590

ABOSTM mentioned this pull request Sep 11, 2019

Avoid I2C timing computation when input clock is unchanged #646

Merged

fpistm requested changes Sep 12, 2019

View reviewed changes

Merge pull request #1 from ABOSTM/PR_590

9a113e8

update on PR Added the repeated-start feature in I2C driver stm32duino#590

fpistm approved these changes Sep 13, 2019

View reviewed changes

fpistm added this to the 1.7.0 milestone Sep 13, 2019

fpistm added New feature and removed help wanted 🙏 Extra attention is needed waiting feedback Further information is required labels Sep 13, 2019

fpistm merged commit 2280453 into stm32duino:master Sep 13, 2019

fpistm mentioned this pull request Sep 13, 2019

STM32 I2C driver "repeated start" issue #583

Closed

fpistm added a commit that referenced this pull request Sep 23, 2019

Revert "Added the repeated-start feature in I2C driver (#590)"

14d071d

This reverts commit 2280453.

fpistm mentioned this pull request Sep 23, 2019

Revert "Added the repeated-start feature in I2C driver" #663

Merged

fpistm added a commit that referenced this pull request Sep 23, 2019

Revert "Added the repeated-start feature in I2C driver (#590)"

0c87941

This reverts commit 2280453.

fpistm pushed a commit to fpistm/Arduino_Core_STM32 that referenced this pull request Oct 31, 2019

Added the repeated-start feature in I2C driver (stm32duino#590)

f56b7a6

Fixes stm32duino#583

fpistm mentioned this pull request Oct 31, 2019

I2C Fix #748

Merged

fpistm removed the New feature label Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the repeated-start feature in I2C driver #590

Added the repeated-start feature in I2C driver #590

jmchiappa commented Aug 3, 2019

uzi18 left a comment

jmchiappa commented Aug 3, 2019

uzi18 commented Aug 3, 2019

LMESTM left a comment

fpistm commented Aug 30, 2019

ABOSTM commented Sep 4, 2019

fpistm commented Sep 6, 2019

jmchiappa commented Sep 6, 2019

fpistm commented Sep 6, 2019

jmchiappa commented Sep 7, 2019

fpistm commented Sep 9, 2019

jmchiappa commented Sep 9, 2019

ABOSTM commented Sep 9, 2019

ABOSTM commented Sep 11, 2019

jmchiappa commented Sep 11, 2019

fpistm left a comment •

edited

Loading

fpistm left a comment

fpistm commented Sep 13, 2019

LieBtrau commented Oct 28, 2019

fpistm commented Oct 29, 2019

Added the repeated-start feature in I2C driver #590

Added the repeated-start feature in I2C driver #590

Conversation

jmchiappa commented Aug 3, 2019

uzi18 left a comment

Choose a reason for hiding this comment

jmchiappa commented Aug 3, 2019

uzi18 commented Aug 3, 2019

LMESTM left a comment

Choose a reason for hiding this comment

fpistm commented Aug 30, 2019

ABOSTM commented Sep 4, 2019

fpistm commented Sep 6, 2019

jmchiappa commented Sep 6, 2019

fpistm commented Sep 6, 2019

jmchiappa commented Sep 7, 2019

fpistm commented Sep 9, 2019

jmchiappa commented Sep 9, 2019

ABOSTM commented Sep 9, 2019

ABOSTM commented Sep 11, 2019

jmchiappa commented Sep 11, 2019

fpistm left a comment • edited Loading

Choose a reason for hiding this comment

fpistm left a comment

Choose a reason for hiding this comment

fpistm commented Sep 13, 2019

LieBtrau commented Oct 28, 2019

fpistm commented Oct 29, 2019

fpistm left a comment •

edited

Loading