SDA can be driven high (violates i2c specification)

[XMOS-JoeG]

lib_i2c/lib_i2c/src/i2c_master.xc

Line 181 in 648351b

p_sda <: data & 0x1;

Both I2C bus lines should only ever be driven in open drain mode i.e. either drive low or drive high-z (input). This is the case for SCL but not the case for SDA when driving data on to the bus - it is driven high if the data is high. This is contrary to the specification and could lead to multiple hardware issues. (driving current into pullup supply voltage, multi voltage compatibility issues etc.)

Instead of:
p_sda <: data & 0x1;
Correct function here would be:
if (data & 1)
p_sda :> void;
else
p_sda <: 0;

I have made this change and done quick testing with success.

A similar change would be needed to the other i2c sources e.g. i2c_slave.xc and i2c_master_async.xc etc.

[ACascarino]

Assigning @mbanth to bring to your attention - Joe proposes a simple fix for the 1b port implementation, where each line is a separate port. Will need regression testing but looks watertight on a quick inspection. The multibit port implementation is slightly trickier, since we can't make single pins on a port HiZ while others are outputting, but Joe suggests using the internal pullup mode of the port to at least mititate the issue - will probably need a note in documentation/in the release changelog that this behaviour has changed so that customers aren't surprised when the chip starts pulling up, as well as an option to toggle the old behaviour.

[mbanth]

At a meeting on 15 January 2024, @andrewdewhurst, @mbanth, @xross, @timmace-xmos, and @tomblackie-xmos agreed to defer work on this item until the work occurs to incorporate fwk_io's I2C functionality with lib_i2c (Jira AP-241).

[mfreeborn]

Just stumbled across this issue after noticing this irregularity on my scope. Picture one clearly shows SDA being driven high, as documented in the first post. The top 2 traces are digital, the bottom 2 are analogue.

Updating the code results in a nice smooth rising edge from the action of the pull up resistor:

Would be nice to see this small change merged so that we don't have to maintain a different version of this library ourselves.

[jseaber]

Confirmed. We've forked lib_i2c with this change, and it has tested well throughout the year.

[xross]

Fixed in #98