I2C SCL frequency 10% less than it should be at 400kHz

Hans Dorn
Posts: 62
Joined: Tue Feb 21, 2017 2:21 am

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby Hans Dorn » Sun Mar 26, 2017 1:13 am

MartyMacGyver wrote:Hans,

Others have been having this problem as well and I shared your post with them. Thanks to your discovery, the problem may be resolved. An issue has been raised on Github regarding this: https://github.com/espressif/arduino-esp32/issues/286. You might want to subscribe to that issue on there to further track it.

I'm curious if you're seeing this issue with the ESP IDF or with Arduino-ESP32? A corresponding fix may be needed for the IDF.


I'll have a look if something pops up.

Honestly, I have no idea why there's 2 different addresses for the same register, one that works and one that doesn't, and why expressif didn't just switch everything over to the working ones...

WiFive
Posts: 1420
Joined: Tue Dec 01, 2015 7:35 am

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby WiFive » Sun Mar 26, 2017 5:12 am

Hans Dorn wrote:why expressif didn't just switch everything over to the working ones...


Yes I wonder about that too. Is there any penalty to use the alt address?

MartyMacGyver
Posts: 56
Joined: Sun Dec 18, 2016 9:17 pm

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby MartyMacGyver » Sun Mar 26, 2017 7:42 am

Note that the change was made and merged into Arduino-ESP32 pretty quickly. Not sure what the state of esp-idf is though.

User avatar
rudi ;-)
Posts: 1208
Joined: Fri Nov 13, 2015 3:25 pm

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby rudi ;-) » Sun Mar 26, 2017 11:44 pm

I2C0
https://github.com/espressif/esp-idf/bl ... soc.h#L167
https://github.com/espressif/esp-idf/bl ... rals.ld#L9

I2C1
https://github.com/espressif/esp-idf/bl ... soc.h#L181
https://github.com/espressif/esp-idf/bl ... als.ld#L19

base looks unchanged

best wishes
rudi ;-)

edit:

wounder me how the address is knowed?..
0x60013000
0x60027000

looks deeper, got an address append ( 16 Dez. 16 ) for the first moment ( never saw this base address before ) :
https://github.com/espressif/esp-idf/co ... 19e749R264

Code: Select all

 
 #define I2C_DATA_APB_REG(i)      (0x60013000 + (i) * 0x14000 + 0x001c)


1. modify i2c_set_pin function

2. update example comments and other minor changes
3. rename API: i2c_cmd_link_create/i2c_cmd_link_delete (+4 squashed commits)

Squashed commits:
[2e0ac3e] 1. coding style: add one space after condition key words.
2. modify i2c.h, use gpio_num_t instead of int, improve comments of return values
3. add i2c index in index.rst
4. add readme for i2c example
[4991d92] update i2c.doc
[88b672e] driver: i2c

1. add mux and spin lock to run in a thread-safe way.
2. modify example code
[4eb15fe] driver: i2c code

1. add i2c master code
2. add i2c slave code
3. add i2c example code
4. add DRAM_ATTR for I2C array



https://github.com/espressif/esp-idf/bl ... reg.h#L264

good to know - we have "HW" reserve addresses?
freespace ( 0x60013000 ) do ( 0x6002701C -1 )

(0x60013000 + (i) * 0x14000 + 0x001c)


what good things are still hidden / not published?
:mrgreen:
-------------------------------------
love it, change it or leave it.
-------------------------------------
問候飛出去的朋友遍全球魯迪

ESP_Angus
Posts: 757
Joined: Sun May 08, 2016 4:11 am

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby ESP_Angus » Mon Mar 27, 2017 12:44 am

WiFive wrote:
Hans Dorn wrote:why expressif didn't just switch everything over to the working ones...


Yes I wonder about that too. Is there any penalty to use the alt address?


There is a performance penalty. The peripheral address registers from 0x3FF40000 are mirrored at the 0x600000 address, but this second address is via a slower bus where the CPU can't issue the writes via its write buffer.

ECO 3.3 was due to a bug where sometimes subsequent writes to the same address were lost (the bug happens as part of the same write buffer behaviour that is a performance boost for all other peripheral writes). Adding a peripheral bus read before the second write, or writes to different peripheral addresses interleaved, means the bug does not trigger. Which is why the ECO document only mentions moving the addresses used for FIFO registers (where data is often copied word-at-a-time to the same address.) It seems like an oversight that the I2C data FIFO register is not mentioned in the ECO document.

In Arduino, I had a quick look but I don't yet see where the FIFO data loss would have been happening except in 10bit mode. Writing "fifo_data.data" twice in succession here may cause the second write to be lost in 10 bit mode:
https://github.com/espressif/arduino-es ... i2c.c#L171

However the loop which writes "normal" i2c data writes to the 'val' 8 bit bitfield rather than writing the full register, which should be a register read-modify-write cycle each time (having the read there should mean that the bug doesn't trigger):
https://github.com/espressif/arduino-es ... i2c.c#L181

It seems from the reports here that there is some data loss somewhere though, possibly to do with timing between some of the read/writes when interrupts are enabled. Or maybe some other software behaviour that changing to the slower peripheral bus mitigates.

Not sure what the state of esp-idf is though.


In IDF, the i2c driver already uses the "slow" address at 0x6000xxxx when writing to the FIFO register:
https://github.com/espressif/esp-idf/bl ... i2c.c#L380

(The I2C_DATA_APB_REG macro returns the correct register address.)

Overall, I'm a little confused now reading such a long thread that seems to be be discussing multiple quasi-related issues. Marty, are you still having I2C integrity problems if you use the latest Arduino master branch? Is it possible to differentiate between these problems (ideally in separate threads)?

MartyMacGyver
Posts: 56
Joined: Sun Dec 18, 2016 9:17 pm

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby MartyMacGyver » Mon Mar 27, 2017 1:05 am

This I2C thread is the result of one problem: I have a normal SSD1306 display that works utterly fine on other MCUs, but glitches to the point of being non-functional on the ESP32. That led me to examine the I2C frequency (very sensitive and inexact), the waveform (the 50% duty cycle is naive and may be exacerbating the issue), and the electrical characteristics of the I2C hardware itself (which leads us back to the unusual sensitivity to RC load (both from noise/proximity as well as other devices).

As I've delved into this, we've seen other reports of similar glitchiness, and I'm seeing it myself on other I2C devices as I look more closely at this.

That this register fix seems to help others is positive news, though if it has a performance penalty then that's a possible negative.

Finally, as I've been investigating the other "wandering bug" problem (that turned out not to be specific to I2C), I'm led to wonder whether latent defects in either the base platform, toolchain, or the chip itself are manifesting in various places. Interrupt processing certainly seems to be a common thread here.

I look forward to updating to the very latest on Arduino-ESP32 (with this fix) and trying it out against the OLED to see if it stabilizes it.

Hans Dorn
Posts: 62
Joined: Tue Feb 21, 2017 2:21 am

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby Hans Dorn » Mon Mar 27, 2017 10:20 pm

Hi Angus, thanks for the feedback!

I had a look at the performance penalty of using the slower address.
I counted the cycles used for a call to "i2cSetCmd" for both variants.

Using the 3xxx address, I get at 0.733µs,while the 6xxx address makes i2cSetCmd take 1.046µs or 142.7% of the original version.
That's not too bad, but generally a bit slow for both variants, considering we're only updating a single peripheral register.

Since the 6xxx address doesn't require a read-modify-write-cycle to work, there's some room for optimizations now:
I modified i2cSetCmd to update an I2C command in memory and write the resulting word into the hardware

Code: Select all

typedef  struct {
   union {
        struct {
            uint32_t byte_num:      8;             
            uint32_t ack_en:        1;             
            uint32_t ack_exp:       1;             
            uint32_t ack_val:       1;             
            uint32_t op_code:       3;             
            uint32_t reserved14:   17;
            uint32_t done:  1;                     
        };
        uint32_t val;
    } command;
} i2c_cmd_t;

i2c_cmd_t cmd = { .command.val=0 };

void i2cSetCmd(i2c_t * i2c, uint8_t index, uint8_t op_code, uint8_t byte_num, bool ack_val, bool ack_exp, bool ack_check)
{
   cmd.command.ack_en = ack_check;
   cmd.command.ack_exp = ack_exp;
   cmd.command.ack_val = ack_val;
   cmd.command.byte_num = byte_num;
   cmd.command.op_code = op_code;
    i2c->dev->command[index].val = cmd.command.val;
}




The new version clocks in @ 0.092µs , or about 8x faster than the original.

I'll let this run until tomorrow to see if something broke, and then do the same for the remaining i2c functions.

Cheers

Edit: forgot an initializer...

ESP_Angus
Posts: 757
Joined: Sun May 08, 2016 4:11 am

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby ESP_Angus » Mon Mar 27, 2017 11:33 pm

Hans Dorn wrote:I had a look at the performance penalty of using the slower address.
I counted the cycles used for a call to "i2cSetCmd" for both variants.

Using the 3xxx address, I get at 0.733µs,while the 6xxx address makes i2cSetCmd take 1.046µs or 142.7% of the original version.
That's not too bad, but generally a bit slow for both variants, considering we're only updating a single peripheral register.


Hi Hans,

Nice results. This is one of the downsides of the "struct" style of register access (lots of read-modify-write cycles), compared to using REG_READ/REG_WRITE. Your approach of assembling the register value and then writing it back in one move is a nice one, though - keeps most of the readability of struct registers while maintaining performance!

I think that optimisation should work in both the "slow" and the "fast" register space (it will be even better in the "fast" register space because the writes can be pushed through the write buffer). You may want to look into taking a similar approach to the IDF i2c driver, where only the data fifo register is updated via the "slow" space and the other registers are updated via buffer-able writes. (In the case of the command registers, you're writing a new address each time so the write buffer bug shouldn't trigger.)

MartyMacGyver wrote:: I have a normal SSD1306 display that works utterly fine on other MCUs, but glitches to the point of being non-functional on the ESP32


Hi Marty,

Someone else has reported that this fix stopped their SSD1306 from glitching. So it's possible the clock speed/waveform itself was a red herring. Although we'll continue to look into what we can do to square up the i2c clock speed, in any case.

Hans Dorn
Posts: 62
Joined: Tue Feb 21, 2017 2:21 am

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby Hans Dorn » Mon Mar 27, 2017 11:51 pm

Hi Angus, thx.


It might be a while until I get up to speed with the IDF. I'm working on a little arduino project right now, and rather would finish this first.

Cheers

MartyMacGyver
Posts: 56
Joined: Sun Dec 18, 2016 9:17 pm

Re: I2C SCL frequency 10% less than it should be at 400kHz

Postby MartyMacGyver » Tue Mar 28, 2017 5:43 am

ESP_Angus wrote:
MartyMacGyver wrote:: I have a normal SSD1306 display that works utterly fine on other MCUs, but glitches to the point of being non-functional on the ESP32

Someone else has reported that this fix stopped their SSD1306 from glitching. So it's possible the clock speed/waveform itself was a red herring. Although we'll continue to look into what we can do to square up the i2c clock speed, in any case.


The speed issue and waveform issue are things, but they aren't evidently involved here. Besides, these can compensated for and improved in code (which I was working on here (https://github.com/MartyMacGyver/arduino-esp32/tree/bugfix-i2c-timings) before all this).

What IS surprising is how dramatically different things are with this code change (I'm using the latest arduino-esp32 on master). I will soak test it more thoroughly, but it seems much less prone to glitching.

However, it glitched and froze once already in these few minutes in a more complex test setup. I'm going to test more without the logic analyzer - the ESP32 seems unusually sensitive to it, though less-so now. Waveform and speed optimizations may help with this, but for now I'll be testing under typical conditions.

In general I2C seems more electrically stable on the alternate registers, though I don't understand why the other registers would have greater sensitivity to noise as they appeared to. Thoughts?

Who is online

Users browsing this forum: No registered users and 1 guest