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

proposal to i2c #19

Merged
merged 2 commits into from
Feb 21, 2018
Merged

proposal to i2c #19

merged 2 commits into from
Feb 21, 2018

Conversation

ESP32DE
Copy link

@ESP32DE ESP32DE commented Feb 3, 2018

see
talk: espressif#1061
PR on origin master : espressif#1073

see 
talk: espressif#1061
PR on origin master : espressif#1073
@stickbreaker
Copy link
Owner

@ESP32DE You are on to something. I spent some time with an oscilloscope. Your suggested modification reduced the spikes from:
esp32de_17

to this:
esp32de_16

Using your idea, I tried a changing the pinMode() options and reordering the digitalWrite(). This is what I achieved:

esp32de_18

No MORE SPIKES!, YEA!!

Change your code from:

pinMode(pin, OUTPUT_OPEN_DRAIN | PULLUP);
digitalWrite(pin, HIGH);

to:

digitalWrite(pin, HIGH);
pinMode(pin, OPEN_DRAIN | PULLUP);

Then I will merge it into my repo.

Chuck

@ESP32DE
Copy link
Author

ESP32DE commented Feb 21, 2018

@stickbreaker
:)
you are right,
i tested with an earier version of arduino esp32
and i tested with the actually version optional

// digitalWrite(scl, HIGH); // optional before
     pinMode(scl, OUTPUT_OPEN_DRAIN | PULLUP);
    digitalWrite(scl, HIGH); // tested

..

// digitalWrite(sda, HIGH); // optional before
     pinMode(sda, OUTPUT_OPEN_DRAIN | PULLUP);
    digitalWrite(sda, HIGH); // tested

cause not sure which version you use, i did optional code in first line instead.
you must then use the optional suggestion like you found
i did the same in Uart and have change the position to first line and setup the pin HIGH
also i did in actually version the PR like you sayed

digitalWrite(scl, HIGH); // optional, tested successful in actually version 
pinMode(scl, OUTPUT_OPEN_DRAIN | PULLUP);
// digitalWrite(scl, HIGH); // successful tested in a prev version, but fails in the actually so use optional!

...

digitalWrite(sda, HIGH); // optional, tested successful in actually version
pinMode(sda, OUTPUT_OPEN_DRAIN | PULLUP);
// digitalWrite(sda, HIGH); // successful tested in a prev version, but fails in the actually so use optional!

Ok i did not check with OSC just in time,
i see there is a different between using OPEN_DRAIN or OUTPUT_OPEN_DRAIN

#define OPEN_DRAIN        0x10
#define OUTPUT_OPEN_DRAIN 0x12

i have not tested "OPEN_DRAIN" instead "OUTPUT_OPEN_DRAIN"
but if you have successfull tested this second option too
i believe you, so yes will change the position of this code lines in the PR to yours and using
"OPEN_DRAIN" instead "OUTPUT_OPEN_DRAIN"

best wishes
rudi ;-)

edit:
( only for a reference info taken from esp idf )
GPIO mode : output only with open-drain mode
GPIO_MODE_OUTPUT_OD = ((GPIO_MODE_DEF_OUTPUT)|(GPIO_MODE_DEF_OD))
GPIO mode : output and input with open-drain mode
GPIO_MODE_INPUT_OUTPUT_OD = ((GPIO_MODE_DEF_INPUT)|(GPIO_MODE_DEF_OUTPUT)|(GPIO_MODE_DEF_OD))

@ifrew
Copy link

ifrew commented Mar 18, 2018

Glad you found this guys! I had started getting these timing errors again after syncing up to the latest ardunio esp32 core. I added manually in the change you stated but found it was still causing me issues. I had been using an earlier version of Chucks i2c recovery code (4files). I thought I would change the open_drain back to output_open_drain and all the timing issues disappeared! SO that made a difference. I then just saw now that chuck updated his i2c code in his master so I downloaded that. I noticed he used open_drain in there and thought it would fail but it doesn't! So between his updated code and using open_drain its now all seems to work!!! Just thought I'd post and let you know that.

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.

3 participants