I2C sw driver with support of multiple buses, Slow, Fast, FastPlus, and user-defined speed selection

[sonaux]

Fixes #2305.

• This PR is for the dev branch rather than for master.
• This PR is compliant with the [other contributing guidelines]
• I have thoroughly tested my contribution.
• The code changes are reflected in the documentation at docs/en/*.

Following @TerryE suggestions in #2305 for use of rsr.ccount instruction I was able to get good formula for calculation of delays.
Old delays did not take in account that running code takes some time too, and function i2c_master_writeByte contained extra unneeded (i hope) calls for setDC and delays, so overall speed was 35-40 kHz, not 100.

I tested module with all I have: good old analog oscilloscope, couple of displays (u8g2.ssd1306_i2c_128x64_noname, u8g2.ssd1306_i2c_128x32_univision), sensor BMP180, chip EEPROM 24LC01B.

SCL speed is independent of CPU clock speed until 250kHz, where delay reaches zero at CPU80MHz and ~400kHz clock achievable only at CPU160MHz.
Also I tried to replace gpio_output_set() with assembler code (s16i [gpio_mask], [gpio_addr]...), but only got 300 kHz SCL at CPU80MHz.

[TerryE]

Thanks Natalia. I've been meaning to do this, but too much on my plate. I will do a proper review of this in the next day or so.

[marcelstoer]

Is this in any way related to espressif/ESP8266_NONOS_SDK#160?

[sonaux]

Original driver in ESP8266_NONOS_SDK does not support clock stretching. Nodemcu version supports it since 2016, thanks to #1589

[TerryE]

@sonaux Natalia, I haven't commented inline because my comments on this pass are general.

• Precautionary principle. We need to be cautious about fundamental changes. This is why major changes (for example my LCD and LFS patches) are enabled by #define constants and when the change is first released these defines are disabled by default. That way developers can elect to use the patch, knowing that there might be some backwards compatibility issues. On a later release once we have a body of developers who have used the patch without issue, then at that stage we might flip the default. However individual developers still have the fallback of building the firmware with the patch disabled. In this case the I2C driver has been stable for years, so we should also be very careful to make a binary equivalent of the old algo buildable as a compile option. You don't do this.

There are various approaches to this, from simply having a large #ifdef / #else /#endif around the old and new implementations to using lots of inline macros (e.g. variant I2C_MASTER_WAIT(n) defines which either call the old i2c_master_wait() or generate nothing). My instinct here is to keep-it-simple-stupid and do the former, so that we can really strip down the new code.

• Making sure we conform to the spec. Here NXP are the patent and licence holder so its UM10204 is the standard reference.

• Optimising the code. IMO, 19 times out of 20, the best thing to do is to write the code clearly and leave it to the compiler. It will nearly always do a better job that we will, but this is probably a 1 in 20 scenario certainly as far as the i2c_master_setDC() and wait calls are concerned. So before I resort to assembly (which I do sometimes), I always look at the output from the C preprocessor (the -E option) and the generated assembly (the -s option) and also explore the impact of any rewrite on these -- just touch the c file, run the make and grab the xtensa-lx106-elf-gcc to tweak.

So here for example the approach to i2c_master_setDC() is just daft. SCL and SDL are in-file parameters and can be trusted; long if chains generate lots of code and branches. So for example using the existing dev variant, the simplest I could get this was (here pin_SDA_SCL_mask is pre-computed in the init code):

i2c_master_setDC(uint8 SDA, uint8 SCL) {
    uint32_t this_SDA_SCL_mask = (SDA<<pinSDA) | (SCL<<pinSCL);
    m_nLastSDA = SDA;
    m_nLastSCL = SCL;
    gpio_output_set(this_SDA_SCL_mask, pin_SDA_SCL_mask ^ this_SDA_SCL_mask, pin_SDA_SCL_mask, 0);
    if(1 == SCL) 
        while(!(gpio_input_get()&(1<<pinSCL))) {}
}

This a lot shorter and faster than the current version. (This was a source only exercise and need validating). And your rsr loop compiles down to

        rsr a6, ccount
.L3:
        rsr a4, ccount
        sub     a4, a4, a6
        bltu    a4, a5, .L3

So big tick there.

• Validating the edge cases. There are a couple of places where the old code deliberately stretches timing and yours doesn't. We need cross check against the standard to see if we are on safe ground.

So overall well done and thanks, but I do think that we need another iteration.

Terry 😄

PS. I am not sure if that clock stretch algo is correct. This needs review.

[sonaux]

Thanks for a feedback! It's my first time coding in C since university classes 15 years ago, so i'm really new to this. Had to reread some textbooks at first. I've got the idea to #ifdef code to allow compile-time selection of old/new version.
I've read that pdf from NXP, though not to last page...
About optimizing: my opinion too, there is too much complexity with macros and aliases for SDK functions, so I thought, "Maybe there is some reason to code complex projects that way, I just do not understand it, so better leave it as it is". I used precalculated masks when I tried assembly version of gpio_output_set()
I looked at assembly code by launching xtensa-lx106-elf-objdump [-d/-S] i2c_master.o. C preprocessor output is interesting to look at.
Also, I finally got a cheap logic analyzer from Aliexpress, and now can look at samples of real transmissions, not a endlessly repeated piece of code on an analog oscilloscope.
So, I'm out to do next iteration.

[TerryE]

If you want to work more informally, then you can use the Gitter IM for nodemcu/nodemcu-firmware or we can ping-pong emails. I am happy to help anyone get over the leaning hump -- if they are here to contribute constructively.

I am sorry to say that macros are part of Lua and the SDK, so that's life. If you do want to get a handle on the Xtensa architecture then something like this useful synopsis An short guide to Xtensa assembly language would help, or the biggy: Xtensa Instruction Set Architecture (ISA) Reference Manual.

Just look at the generated code before you think that macros hurt, for example you couldn't beat the 3 instruction wait loop.

PS. I use a BitScope logic analizer.

[TerryE]

done in comments

[devsaurus]

From a quick code review it seems that certain omissions potentially violate the I2C protocol standard. See my inline comments.

[sonaux]

Where should I put //#define I2C_MASTER_OLD_VERSION? Currently it is in the beginning of i2c_master.h, but maybe there is a special place for such defines?

[nwf]

@sonaux Usually configuration knobs go in ./app/include/user_config.h, I think.

[sonaux]

new commit adds features:

• Multiple buses (up to 10) with different speeds on each bus
• Standard(Slow, 100kHz), Fast(400kHz) and FastPlus(1MHz) modes or an arbitrary clock speed
• Sharing SDA line over multiple I²C buses to save available pins
• GPIO16 pin can be used as SCL pin, but it does not support clock stretching and selected bus will be limited to FAST speed.

My concern is memory allocation for array of i2c state structures. Now is defined as static i2c_master_state i2c[NUM_I2C]; where NUM_I2C = 10 (that much you can squeeze out of ESP-12 module using shared SDA line and GPIO16.
Size of array is 10 * 22 bytes. Should I care about it?

[sonaux]

New version is disabled by default by #define I2C_MASTER_OLD_VERSION in app/include/user_config.h

[devsaurus]

My concern is memory allocation for array of i2c state structures. Now is defined as static i2c_master_state i2c[NUM_I2C]; where NUM_I2C = 10

You could define it as an array of pointers and have the structures allocated dynamically on demand by the i2c.setup() function. With that you're getting closer to an OOP API style, similar to what we've done with the tmr module in #1416. But that would be out of scope for this PR IMO.

[TerryE]

Some general comments that if we are going to do a major rework of this driver that we should also consider fixing. And to this end, I've been doing a review of I²C use across all of our modules; none use the direct interface to the I²C driver. Instead they all go through the platform_i2C_XX interface where XX is one of the following functions:

Module	setup	send_start	send_stop	send_address	send_byte	recv_byte
platform u8x8_nodemcu_hal		1	1	1	1	0
tsl2561 tsl2561	1	5	5	5	4	2
i2c	1	1	1	1	3	1
si7021		6	6	6	6	2
adxl345		5	4	5	4	3
ads1115		4	4	4	5	2
bmp085		4	4	4	5	1
mcp4725		2	2	2	3	1
tcs34725		3	3	3	3	1
hmc5883l		5	5	5	4	3
hdc1080		5	5	5	5	2
am2320		3	3	3	3	3
bme680		5	5	5	3	1
bme280		5	5	5	3	1
l3g4200d		5	4	5	4	3

This platform_i2C interface only invokes a subset of the I²C calls: i2c_master_gpio_init(), i2c_master_start(), i2c_master_stop(), i2c_master_readByte(), i2c_master_getAck(), i2c_master_setAck(), i2c_master_readByte(), i2c_master_writeByte().

The driver header app/include/driver/i2c_master.h should only present the publicly callable interface, and macros that will be used publicly, but not declarations and macros that are really private to the i2c_master.c code. The I2C_MASTER defines therefore belong in the C module. i2c_master_wait() is definitely internal to the driver and shouldn't be exposed.

We also have 5 other entries that aren't used anywhere: init(), get_pinSCL(), get_pinSDA(), checkAck(), send_nack(). Of these, the one that surprises me the most is the init() code which seems to be a standard way of initialising the I²C bus to a known state, but is never used.

Lastly I did find an error in int platform_i2c_send_address(). The direction check is optimised out and direction isn't validated anywhere so i2c.address(0,dev,27) will not error, but produce bizarre results. At a minimum direction should be clamped to 0/1, so the body of this function should be

  i2c_master_writeByte( (uint8_t) (
    (address << 1) + (direction == PLATFORM_I2C_DIRECTION_TRANSMITTER ? 0 : 1)
  );
  // Low-level returns nack (0=acked); we return ack (1=acked).
  return ! i2c_master_getAck(); 

[sonaux]

We also have 5 other entries that aren't used anywhere: init(), get_pinSCL(), get_pinSDA(), checkAck(), send_nack(). Of these, the one that surprises me the most is the init() code which seems to be a standard way of initialising the I²C bus to a known state, but is never used.

init() is called from gpio_init() at the end of routine.
As for other functions - I do not know, since they are very small, maybe let them be, and get_speed() could be perfect addition to them.

Lastly I did find an error in int platform_i2c_send_address(). The direction check is optimised out and direction isn't validated anywhere so i2c.address(0,dev,27) will not error, but produce bizarre results. At a minimum direction should be clamped to 0/1

I do not know how Will change to your version.

[sonaux]

And I haven't looked at ESP32 code yet... I see i2c master software driver is the same and used same way, except that bus IDs 1 and 2 routed to hadrware i2c. It would be very easy to integrate new driver into dev-esp32 branch.

[devsaurus]

except that bus IDs 1 and 2 routed to hadrware i2c. It would be very easy to integrate new driver into dev-esp32 branch.

I agree, and the constants for the hardware drivers could be mapped to higher IDs (e.g. 100, 101) to free up 0-9 for the SW driver.

[TerryE]

@sonaux Natalia, I've pulled your last push version (which is suspect is now behind your local dev PC version) and had a play / looked at the code. My main concern here is the code for GPIO16. I understand and support your goals of (i) supporting fast or better I²C; and (ii) supporting multiple I²C buses. A major issue here is the lack of usable IOs exposed on the ESP8266, and so your desire to have GPIO16 as an option is also understandable.

However, the constraints on using GPIO16 are poorly documented by Espressif. GPIO0-15 all share a unified H/W interface and can be set to open-drain and in this case an internal pull-up isn't mandatory (though I use an extra 10K external pull-up resistor anyway.) GPIO16 has a separate H/W interface and it's main purpose allow the RTC to wake the CPU after deep sleep, but it can be used a GPIO so long as interrupts aren't required. It's also used for the key LED. I believe that also has an internal pull-down permanently attached and we therefore configure it as input and must use an external pull-up (and since we are connecting GND to 3.3V through a couple of pull-ups is this creating a ~mA current draw). It is also problematic toggling between output and input, so we can't implement clock stretching if SCL is on GPIO16.

So IMO the whole area of using GPIO16 is a potential minefield. It's implementation needs to be properly researched and documented (both in the code and some form of application note. My vote would be to defer this work to a later PR. For example, you've cut and pasted the GPIO16.c functions into the code, but in this case would it not be better just leaving them out of line?

Also since including the testing for GPIO16 add extra runtime code paths it would be nice to ensure that this can be optimized out during compile, eg.

#ifdef I2C_MASTER_GPIO16_ENABLED
#define IS_PIN16(n) ((n)==16)
#else
#define IS_PIN16(n) (0)
#endif

[sonaux]

Sorry for delay, at this time new driver works in all configurations, but some TODOs and improvements I wanted is not done yet. For example:

• Check SCL for low after pin configuration - if low, then throw error, otherwise will stuck in clock stretching loop
• Check SDA for low after clearing bus by init - if low, then throw error for bus failure
  Change if interface - move ack into read/write functions
• Sharing of SDA line works for devices I tested, but it may not work with some, and I didn't test with shared SCL. If it works too, it would be cool 😄

[TerryE]

@sonaux Natalia, don't worry about the timescales. We are all doing this pro-bono so other priorities can intrude. The main thing is that you are working this, so others don't need to step in.

Thanks Terry

[devsaurus]

Looks good.
Please help clarify correct NUM_I2C for old version.

Issues resolved

[marcelstoer]

Fine for me. I dropped it for the next cut. Ideally @sonaux would fix the conflicts in i2c.md (it's now in docs/modules/) so we can then merge it right after the next release.

[TerryE]

If will be trailing spaces. You are going to see a few of these. Easy to resolve on the merge.

[sonaux]

Rebased and changed path to source file in documentation

[HHHartmann]

@TerryE I currently only have hardware that supports slow speed. Will see if I can try it on the weekend with several of these devices if it helps.

[pjsg]

I just tried this on a 128x64 oled display and it ran at 400kHz just fine (the old code was at 48kHz). It isn't quite that much faster as the device itself inserts delays, but the display updates much better now. (I was using a logic analyzer to look at the speeds on the wire).

The only issue that I have with this PR is that you need to comment out a #define in user_config.h to enable the new driver. If this really is backwards compatible with the old code (except that it is much faster), then it probably ought to be the standard.

[TerryE]

I've tried it on my current (slow) I²C devices and this doesn't seem to have broken anything, so I suggest that we merge this into dev. Any objections?

[devsaurus]

IMO ok to merge after master drop.

[Michal78S]

I have tried all avaiable speeds: i2c.SLOW, i2c.FAST and i2c.FASTPLUS with firmware builded by cloud builder but always 47kHz clk speed appeared.

[galjonsfigur]

@Michal78S That's because the default option in the firmware is to use old I2C driver. See https://github.com/nodemcu/nodemcu-firmware/blob/dev/app/include/user_config.h#L175 .
I don't see an option to enable it in cloud builder so I think the only way to use this driver for now is to build the firmware yourself. But an option to enable this driver would be a nice feature to add to @marcelstoer cloud builder.

[marcelstoer]

an option to enable this driver would be a nice feature to add to the cloud builder

True, I can do that once it's available on master. Please ping me should I forget that.

[mnpaslay]

This is working well for me with a ADS1115 and ADS1015. The faster bus speeds allow me to sample at maximum rates. Thanks!

[Michal78S]

an option to enable this driver would be a nice feature to add to the cloud builder

True, I can do that once it's available on master. Please ping me should I forget that.

Did You add this option to the cloud builder? I'm still waiting for it ...

[marcelstoer]

I can do that once it's available on master

You need to keep an eye on that. It depends on https://github.com/nodemcu/nodemcu-firmware/milestone/13

[Michal78S]

an option to enable this driver would be a nice feature to add to the cloud builder

True, I can do that once it's available on master. Please ping me should I forget that.

Ping, ping...

[Michal78S]

Is there any chance to put this solution to the cloud builder?

[HHHartmann]

Maybe we should undefine I2C_MASTER_OLD_VERSION by default.
But that would be in another bug.
Or @marcelstoer fixes it in his builder.

[Code1110]

I have just tried this new driver with 3x mcp23017 at i2c.FASTPLUS and it works fine and much faster. Just do not forget the pull-up resistors, which were not required in this case, when running with the old driver.