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

Double I2C read in one transaction skips a clock pulse (#5528) #6579

Closed
wants to merge 2 commits into from

Conversation

TD-er
Copy link
Contributor

@TD-er TD-er commented Oct 1, 2019

See #5528 and the more elaborate description
where @maarten-pennings did all the hard work, but as far as I could see, no PR was made.

I also noticed some code duplication, which I moved to separate functions.

According to this documentation on I2C clock stretching it is not allowed to have some slave keep the clock low during transmission of a byte, only after an ack.
So that's why it is not done in the while loop.
But I wondered if there should be an extra delay between the sequence of pulses and the extra added clock "valey". See my comment in the code.

Have not tested it myself, this is just the long awaited PR.

@Jason2866
Copy link
Contributor

@TD-er Have you seen #6326 ?
We use this PR for our core pre2.6 Tasmota. We have until now NO feedback with issues with.
Could you adapt you PR to this. This I2C saves a lot of iRam!

@TD-er
Copy link
Contributor Author

TD-er commented Oct 3, 2019

Have seen it and indeed it may be useful to base this PR on that one.
I will have a look at it to see how hard it is to do the same based on that branch.

@TD-er
Copy link
Contributor Author

TD-er commented Oct 3, 2019

@Jason2866 See PR #6592

@earlephilhower
Copy link
Collaborator

Minor niggle, but in English it is spelled "valley" (two ells).

According to this documentation on I2C clock stretching it is not allowed to have some slave keep the clock low during transmission of a byte, only after an ack.

NXP (spinoff from Philips IIRC) says that clock stretching can be on a bit-by-bit basis, and that's the way it's (theoretically) done w/the existing I2C code in the repo: https://www.nxp.com/docs/en/user-guide/UM10204.pdf

@TD-er
Copy link
Contributor Author

TD-er commented Oct 5, 2019

@earlephilhower In the other PR #6592 I had also changed it into "valley". Noticed the same strange notation :)

It makes more sense to allow clock stretching per bit, so the device can make sure it will be able to handle the stream.
But I wonder if it will then also be able to read if something is addressed to him/her/it or that it may stretch all bits on the bus even when not addressed to that device.
That would be bad for performance of the whole bus and maybe that's the reason it should not stretch the clock during transfers.

@TD-er
Copy link
Contributor Author

TD-er commented Oct 19, 2019

Superseded by #6654

@TD-er TD-er closed this Oct 19, 2019
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.

None yet

3 participants