Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A clock phase correction in write_start(void) (I2C) #8383

Merged
merged 4 commits into from
Nov 28, 2021

Conversation

xpalaa00
Copy link
Contributor

Some devices require the data wire SDA to be held down at the moment while the clock wire is pulled down too to execute the start condition (e.g. devices using "TinyWires.h" library). This change follows a behaviour of Arduino Wire.h library, where the SCL signal is pulled down in half of period of start condition.

obrazek

Some devices require the data wire SDA to be held down at the moment while the clock wire is pulled down too to execute the start condition (e.g. devices using "TinyWires.h" library). This change follows a behaviour of Arduino Wire.h library, where the SCL signal is pulled down in half of period of start condition.
@xpalaa00 xpalaa00 changed the title A clock phase correction in write_start(void) A clock phase correction in write_start(void) (I2C) Nov 22, 2021
@TD-er
Copy link
Contributor

TD-er commented Nov 23, 2021

Is this variation also allowed according to the standard?
Are there devices known that may not like this behavior? Or is it in general a good idea to do it like this?

@xpalaa00
Copy link
Contributor Author

xpalaa00 commented Nov 23, 2021

What I have looked around the I2C protocol, the declaration of "start and stop condition" is defined as pulling down or releasing SDA wire while SCL wire is released, nothing more, but a lot of diagrams in a protocol specification shows the SCL line is pulled down earlier than the data line is set to send a bit (when sending start flag) and is released earlier than the communication stops (when sending stop flag ... this one is correctly implemented in current protocol). There is one point in specification which can help, the "start condition" can be achieved by sending 0x01 byte (https://www.pololu.com/file/0J435/UM10204.pdf page 19). While transmission is running, the clock signal and data signal changes are delayed between themself by half of period (data bit is changed on falling edge of clock and transmitted on rising edge). So, current implementation of "start condition" is out of phase when sending the 1st bit (other ones are in phase) if it was done by sending 0x01 byte.

How I have described above, I have tried to communicate via I2C between ESP8266 and device what is using the TinyWires library (specificaly it was Attiny85 controller and library from https://playground.arduino.cc/Code/USIi2c/ -> https://github.com/rambo/TinyWire/blob/master/TinyWireS/ ). I have tried to resolve a reason, why Attiny85 answers only sometimes (rarely) by sending ACK, when calling it's address, in other tries just received NACK, and just only if frequency of the bus was slow. I have made a lot of changes in my own code, also tried example code to communicate with ESP8266, I have tried to look for any bugs in libraries I used, but nothing helped with rising of reliability of communication, solution came after moving the phase in start condition of the clock in ESP8266 I2C Master protocol. I don't know true cause, but I think there wasn't a lot of time between clock falling edge and the first sended bit, if data bit is logic one, it perhaps causes detection of stop condition on slower devices, or just it doesn't execute a start condition.

I have tried this modification also to communicate with some sensors and without any complications, although I'm not able to verify this change among every combination of devices, I think the communication standard isn't harmed. This modification is in accordance with Arduino I2C Master's implementation (picture above again) and delay calculation takes into consideration a possible odd number of entered value of delay, that could cause a dephasing.

@TD-er
Copy link
Contributor

TD-er commented Nov 25, 2021

Found Understanding the I2C Bus - Texas Instruments

2.1.1 START and STOP Conditions
I2C communication with this device is initiated by the master sending a START condition and terminated by the master sending a STOP condition. A high-to-low transition on the SDA line while the SCL is high defines a START condition. A low-to-high transition on the SDA line while the SCL is high defines a STOP condition.

image

Just based on this explanation, it seems like some delay of the SCL is indeed needed.
What happens according to your logic analyzer screenshot is that SDA goes up while SCL may still be high and thus that would mean it is technically a STOP condition.
Depending on the capacitance in the wires and the used pull-up resistors, the SDA may rise in a shorter amount of time than the SCL may fall, which may cause the connected I2C devices to actually experience a STOP condition.

@xpalaa00
Copy link
Contributor Author

xpalaa00 commented Nov 25, 2021

So, my hypothesis was partially right, thank you for your working around.

Also I have been looking into TinyWires library a little deeper and I have founded another possible reason, why Attiny devices aren't compatible with current implementation related on unwanted call of stop condition. After start condition there is a check if the communication hasn't been terminated after 'while loop' checking if SCL wire is pulled down and SDA wire is still released (need to specify that Attiny devices has stop condition detection, but it isn't an interrupt vector but only a flag, so stop detection has to be realised by software). Upcomming condition tests if SCL is pulled down also until SDA is still pulled down, if not, the communication is terminated. This check is technically right, because it filters wire cut off situation for exaple, but the software realization of this check brings a delay that helps to cause this error, am I right? (rows 493-546 in https://github.com/rambo/TinyWire/blob/master/TinyWireS/usiTwiSlave.c )

But, if would be possible to use my modification as fix of this problem, I noticed on Thuesday, I thing, has failed a workflow test at Style and formatting point, because near dividion operators there were missing some spaces. It should be fixed now, I hope.

@d-a-v
Copy link
Collaborator

d-a-v commented Nov 26, 2021

Given above TI documentation and write_stop() in current master, and despite I'm not an i2c specialist, I'd approve this PR after clarification about why dcount/2 is used instead of dcount (stop vs start, write_stop() vs write_start())

@xpalaa00
Copy link
Contributor Author

In fact, I didn't want to modify a length of current start bit, because I wasn't sure, if it is possible, but if stop bit can be twice wider, why the start bit couldn't be too.

My variant of modification use dividion, but it is a good point, the dividion can cause error when delay is equal to 1. That cause a fall down of both signals at the same time and not executing a start condition.

Not using the dividion and making start condition wider by using plain delay without any mathematical operators may be a better way. Should I modify it by this way instead?

I noticed the stop bit contains clock stretching detection, I wanted to ask, if in the start bit it should be inserted too, but if I am right, clock stretching is not possible unless addressign proccess is completed (it is possible only if real data, not address, are received or transmitted what I read here: https://onlinedocs.microchip.com/pr/GUID-CA6B1F8F-E09D-4780-A52A-CBBF61902464-en-US-2/index.html?GUID-5CCAB0DB-28BD-4095-B2E2-2F3CF0FC6966 ).

@TD-er
Copy link
Contributor

TD-er commented Nov 26, 2021

Clock stretching is done by the connected I2C devices, not the I2C master.
It is a way for the connected device to tell the master to slow down a bit.

So it does not make sense to have clock stretching in the start sequence.

@TD-er
Copy link
Contributor

TD-er commented Nov 27, 2021

Don't forget to add the comments:

bool Twi::write_start(void)
{
    SCL_HIGH(twi_scl);
    SDA_HIGH(twi_sda);
    if (!SDA_READ(twi_sda))
    {
        return false;
    }
    busywait(twi_dcount);
    // A high-to-low transition on the SDA line while the SCL is high defines a START condition.
    SDA_LOW(twi_sda);
    busywait(twi_dcount);
    SCL_LOW(twi_scl);
    busywait(twi_dcount);
    return true;
}

bool Twi::write_stop(void)
{
    SCL_LOW(twi_scl);
    SDA_LOW(twi_sda);
    busywait(twi_dcount);
    SCL_HIGH(twi_scl);
    WAIT_CLOCK_STRETCH();
    busywait(twi_dcount);
    // A low-to-high transition on the SDA line while the SCL is high defines a STOP condition.
    SDA_HIGH(twi_sda);
    busywait(twi_dcount);
    return true;
}

@xpalaa00
Copy link
Contributor Author

I've added a reason comment of additional delay too, if never minds?

Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking through the code this effectively just adds twi_dcount setup delay for the SDA which seems safe to do. Existing code has the START sequence correct, but only a few ESP clock cycles between SCL-> and SDX->x transitions which may not be enough for some systems. I can't seem top find a good reference for tSETUP here, but half a clock seems like a good #.

@mcspr mcspr merged commit d5444c4 into esp8266:master Nov 28, 2021
hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
* A clock phase correction in write_start(void)

Some devices require the data wire SDA to be held down at the moment while the clock wire is pulled down too to execute the start condition (e.g. devices using "TinyWires.h" library). This change follows a behaviour of Arduino Wire.h library, where the SCL signal is pulled down in half of period of start condition.

* Formatting of modification restyled

* Removing mathematical operations from delay

* Comments added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants