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

Run i2c on < 100 kHz #2524

Closed
vlast3k opened this issue Sep 16, 2016 · 11 comments
Closed

Run i2c on < 100 kHz #2524

vlast3k opened this issue Sep 16, 2016 · 11 comments
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.

Comments

@vlast3k
Copy link

vlast3k commented Sep 16, 2016

Basic Infos

Hardware

Hardware: ESP-12F
Core Version: github-current

Description

I wanted to try running I2C on a lower speed, e.g. 20khz, as i have a sensor that claims it needs <25khz. (though it works pretty well on 100 khz as well). But then i noticed that the minimum speed is limited to 100 khz as per

void twi_setClock(unsigned int freq){
#if F_CPU == FCPU80
  if(freq <= 100000) twi_dcount = 19;//about 100KHz
  else if(freq <= 200000) twi_dcount = 8;//about 200KHz
  else if(freq <= 300000) twi_dcount = 3;//about 300KHz
  else if(freq <= 400000) twi_dcount = 1;//about 400KHz
  else twi_dcount = 1;//about 400KHz

is there any reason for this?
Else i could check the twi_dcount for ~25khz and compile with it.

@devyte
Copy link
Collaborator

devyte commented Oct 12, 2017

@vlast3k I still see the same code in latest git. The reason seems to be "standard speed", per this. In other words, if your device specifies a lower speed, it's apparently not compliant.
Still, that is supposed to be a theoretical max. The timing is supposed to be dicated by the clock, not the standard, and the clock is sent by the master. In noisy envs, I don't see harm in working at a lower speed.
Did you ever find the value for 25KHz?

@devyte devyte added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label Oct 12, 2017
@teliot
Copy link

teliot commented Feb 13, 2018

I have to lower the I2C clock for SMBus to work

https://www.totalphase.com/support/articles/200349186-Differences-between-I2C-and-SMBus
https://en.wikipedia.org/wiki/System_Management_Bus
http://smbus.org/specs/

#if F_CPU == FCPU80
  if(freq <= 50000) twi_dcount = 52;      //about 40KHz
  else if(freq <= 100000) twi_dcount = 19;//about 100KHz
  else if(freq <= 200000) twi_dcount = 8; //about 200KHz
  else if(freq <= 300000) twi_dcount = 3; //about 300KHz
  else if(freq <= 400000) twi_dcount = 1; //about 400KHz
  else twi_dcount = 1;//about 400KHz
#else```

@Tech-TX
Copy link
Contributor

Tech-TX commented Oct 28, 2019

If it comes up in the future, here's a fix for those old, slow parts. Note that SMBus has a minimum speed of 10KHz. I2C has no lower limit; it can theoretically go to DC. The code below is for the current git; the specific entry points and values for twi_dcount may be different for older releases.

If you want to drop the I2C clock below 50KHz (the current minimum), you'll have to find core_esp8266_si2c.cpp and edit it. Sorry, I can't tell you where that file is on your system; google 'everything' by voidtools, and it'll find it for you (and anything else). The patches below will give you 5KHz increments below 50KHz. twi_dcount is an unsigned char, so it can't be bigger than 255. That limits the bottom end with 160MHz CPU, but not with 80MHz CPU.

for 80MHz CPU clock, replace

#if F_CPU == FCPU80
    if (freq <= 50000)

with this:

#if F_CPU == FCPU80
    if (freq <= 10000)
        twi_dcount = 229;  //about  9.99KHz
    }
    else if (freq <= 15000)
    {
        twi_dcount = 150;  //about 14.95KHz
    }
    else if (freq <= 20000)
    {
        twi_dcount = 110;  //about 20.00KHz
    }
    else if (freq <= 25000)
    {
        twi_dcount =  88;  //about 24.88KHz
    }
    else if (freq <= 30000)
    {
        twi_dcount =  73;  //about 29.59KHz
    }
    else if (freq <= 35000)
    {
        twi_dcount =  60;  //about 34.72KHz
    }
    else if (freq <= 40000)
    {
        twi_dcount =  53;  //about 39.53KHz
    }
    else if (freq <= 45000)
    {
        twi_dcount =  45;  //about 44.66KHz
    }
    else if (freq <= 50000)

and for 160MHz CPU clock, down below the section above replace

#else
    if (freq <= 50000)

with

#else
    if (freq <= 15000)
        twi_dcount = 236;  //about 15.02KHz; 13.9 is the slowest we can go
    }
    else if (freq <= 20000)
    {
        twi_dcount = 176;  //about 19.94KHz
    }
    else if (freq <= 25000)
    {
        twi_dcount = 140;  //about 24.85KHz
    }
    else if (freq <= 30000)
    {
        twi_dcount = 115;  //about 29.94KHz
    }
    else if (freq <= 35000)
    {
        twi_dcount =  98;  //about 34.90KHz
    }
    else if (freq <= 40000)
    {
        twi_dcount =  85;  //about 39.84KHz
    }
    else if (freq <= 45000)
    {
        twi_dcount =  75;  //about 44.74KHz
    }
    else if (freq <= 50000)

That's a lot of extra code for very few people, so I don't recommend adding it to the core.

@devyte
Copy link
Collaborator

devyte commented Oct 28, 2019

@Tech-TX could you please make a PR with that?

@Tech-TX
Copy link
Contributor

Tech-TX commented Nov 2, 2019

@devyte Sorry, I've been off-line with Real Life for a bit, and I'm busy this next week preparing for 3 classes sessions I'm teaching; it's been a year since I last taught Red Cross First Aid, CPR & AED, and I want to do it well for the students. I'll do a PR soon.

I checked the bus clock speeds with my HP 54645D mixed-signal scope, and the scope still reports a 10MHz OCXO reference oscillator as 10.00MHz, so it's reasonably within calibration.

I finally have a clean git pull of the master on my Win7 desktop.

@TD-er
Copy link
Contributor

TD-er commented Nov 2, 2019

Can't we just compute these values? Then code changes will not be as elaborate.

@Tech-TX
Copy link
Contributor

Tech-TX commented Nov 3, 2019

It not a simple progression, so the effect from smaller counts has a much larger effect at the high end of the speed. The offset between any two speeds down in the sub-50KHz range isn't constant, it's widest at the 10-15KHz change, smallest at the 45-50KHz range, decreasing as you go up in speed.
Here's the full file, my changes start around line 168. Down below my initial additions, I noticed that all of the current clock speeds were off, some by 20% (600KHz was actually close to 500KHz), so I tweaked them as close as possible. Where the previous speeds were, I've initially left the original line on top, and the change below that. It's easier to see the tweaks that way. I'll have to remove the duplicates before the real pull request.

There's a fixed delay from the instructions in the various functions, a fixed delay from calling Twi::busywait at line 341, plus the push-pop, then a variable delay from the loop. I can add some pin wiggles in there to try and determine how much time each bit takes. The highest speed for both CPU speeds is using twi_dcount = 0, so that should be the overall fixed part of the delay.
At 80MHz CPU and 363KHz (twi_dcount = 0) the low time of SCL is 1.14us, and the high time is 1.64us (it's not symmetrical due to what's going in in the calling function). At 160MHz CPU low time of SCL is 610ns, and high time is 1.12us. That's the fixed delay for 160MHz. At 160MHz, twi_dcount = 1 adds 100ns exactly. At 80MHz, twi_dcount = 0 is 1.15us for the low, 1.61us for the high, and twi_dcount = 2 adds 230ns per loop, 460ns total.

Hmmm. The delay from program instructions (high time of SCL) will get worse if I add in arbitration / collision tests. That will screw up all of the clock timing.

One option is to cut out the 5KHz additions, and just add 40, 30, 20 and 10KHz. That would likely suit most people that need slow speed.

Would you mind looking at the branch I've done to see if it's been done correctly? I tried to follow simple directions, but this is my first full clone-change-commit-push and I'd rather not cause devyte any more work undoing any obvious mistakes. It looks OK to me per his instructions, but... I'm a virgin.
https://github.com/Tech-TX/Arduino/tree/issue2524

I noticed another bug (minutes ago) in Twi::writeTo in two places, may also be elsewhere. If you request Repeated Start, it won't send a STOP if there's an error, it just exits after the NACK. I verified that it really does that. That leaves the bus locked to you when it exits, instead of freeing the bus as it should. On error, it must always send a STOP before exiting.
"All transactions begin with a START (S) and are terminated by a STOP (P)" from the spec. I don't know why nobody has ever noticed it.

@devyte
Copy link
Collaborator

devyte commented Nov 3, 2019

@Tech-TX your procedure for implementing looks good: your code is in a branch in your fork. You just seem to have missed actually making the pull request. To do that, you should see a green button for it here in this repo and in your fork. Either one works.
I do have one comment about your code, but I'll wait for the pr to walk you through the review process, so that you get to know that as well.

@TD-er
Copy link
Contributor

TD-er commented Nov 3, 2019

I noticed another bug (minutes ago) in Twi::writeTo in two places, may also be elsewhere. If you request Repeated Start, it won't send a STOP if there's an error, it just exits after the NACK. I verified that it really does that. That leaves the bus locked to you when it exits, instead of freeing the bus as it should. On error, it must always send a STOP before exiting.
"All transactions begin with a START (S) and are terminated by a STOP (P)" from the spec. I don't know why nobody has ever noticed it.

I guess it does make sense to have this as a separate issue, although it may make the PR-process a bit more confusing for a "first time" PR.
But it does seem like an important bug which would be nice if it could be included in the 2.6.0 release which is planned for as I understood.

@Tech-TX
Copy link
Contributor

Tech-TX commented Nov 3, 2019

I'll have to recalculate the clock timing after adding the new overhead for collision/arbitration tests, so this is a stub branch. I've saved a copy locally, but I've already reset my local to the upstream/master.

I've only ever seen one part (long ago) that couldn't run at 100KHz and didn't do clock stretching, so the low-speed options are a good idea, but not as critical as the multi-master arbitration and the other bug I noticed. I'll switch over to [issue 5645] for those more critical changes.

@devyte
Copy link
Collaborator

devyte commented Dec 28, 2019

Closing via #6934.

@devyte devyte closed this as completed Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback Waiting on additional info. If it's not received, the issue may be closed.
Projects
None yet
Development

No branches or pull requests

5 participants