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

Allow setting I2C bus speed #36

Closed
caternuson opened this issue Sep 23, 2020 · 22 comments
Closed

Allow setting I2C bus speed #36

caternuson opened this issue Sep 23, 2020 · 22 comments

Comments

@caternuson
Copy link
Contributor

Currently the default of 400kHz is being used. This is fine for all the on board sensors, but some external sensors may require slower speeds. For example, the PM25:
https://github.com/adafruit/Adafruit_CircuitPython_PM25/blob/0940f715aa29f965f2b18339ae57b4cd7c03921d/examples/pm25_simpletest.py#L39
which can cause issues:
https://forums.adafruit.com/viewtopic.php?f=65&t=169662

Suggest adding a new kwarg to init to allow specifying I2C bus speed. Default it to a conservative 100kHz.

@kevinjwalters
Copy link

kevinjwalters commented Sep 23, 2020

The library is using board.I2C() so this may need features in circuitpython to change (lower) the bus speed, assuming it's at 400kHz? This would also apply to other boards particularly those with a shared i2c bus with onboard i2c devices. Other libraries for i2c devices could also be loaded before or after adafruit_clue.

Is there a data sheet that confirms it's 100kHz only, i.e. to rule out this being a transmission line or pull-up issue?

@jtobinart
Copy link

jtobinart commented Sep 23, 2020

The datasheets for the CLUE sensors permit 100kHz. I used it on my cutebot library and had no problems. Also, micro:bit devices also expect a 100kHz i2c connection and it would be nice not to have to use an edited version of the repository to get them to work.

@caternuson
Copy link
Contributor Author

@kevinjwalters from datasheet:
https://cdn-shop.adafruit.com/product-files/4505/4505_PMSA003I_series_data_manual_English_V2.6.pdf
pm25_i2c

@kevinjwalters
Copy link

kevinjwalters commented Sep 23, 2020

This raises the question about specification / "plug and play" for STEMMA. If there's no feature for an i2c host to detect/adapt the i2c speed when one slave device isn't capable of that speed then perhaps it needs to be lowest common denominator for a bus with external connectivity. Hot swapping is another factor to take into account here.

@ladyada
Copy link
Member

ladyada commented Sep 23, 2020

arduino is 100khz default, im ok making circuitpython 100khz default too

@caternuson
Copy link
Contributor Author

Should we make it happen at the CP level then? If so, we could close this and open a new issue over there to change current 400kHz default:
https://circuitpython.readthedocs.io/en/5.3.x/shared-bindings/busio/I2C.html#busio.I2C

@ladyada
Copy link
Member

ladyada commented Sep 23, 2020

@tannewt any reason not to change default to match arduino's 100khz?

@kevinjwalters
Copy link

kevinjwalters commented Sep 23, 2020

On a related note, would be useful to be able to query the frequency of an i2c object. It doesn't look like it's visible on 5.3.1 (on a CLUE)?

>>> i2c=board.I2C()
>>> dir(i2c)
['__class__', '__enter__', '__exit__', 'deinit', 'readfrom_into', 'scan', 'try_lock', 'unlock', 'writeto', 'writeto_then_readfrom']

@kevinjwalters
Copy link

If the default becomes 100Hz what would be the mechanism for a user to change it to 400Hz for board.I2C() where that function call might be embedded in a library? Would control via boot.py be appropriate here?

@ladyada
Copy link
Member

ladyada commented Sep 24, 2020

for the few that have embedded I2C() we'd have a kwarg on initialization. boot.py is not appropriate

@caternuson
Copy link
Contributor Author

hmm...might need something other than a kwarg since the library creates an instance internally?

clue = Clue() # pylint: disable=invalid-name

@tannewt
Copy link
Member

tannewt commented Sep 24, 2020

@tannewt any reason not to change default to match arduino's 100khz?

The busio.I2C constructor default or the speed of board.I2C()?

If the default becomes 100Hz what would be the mechanism for a user to change it to 400Hz for board.I2C() where that function call might be embedded in a library? Would control via boot.py be appropriate here?

Libraries should only use board.I2C() if they haven't been passed an I2C object. That way the user can pass in busio.I2C(board.SCL, board.SDA, frequency=400000) with whatever settings they want.

@ladyada
Copy link
Member

ladyada commented Sep 24, 2020

The busio.I2C constructor default or the speed of board.I2C()?

both - the i2c standard is 'technically' 100khz - 400khz is 'speedy' and is not guaranteed the same way

@kevinjwalters
Copy link

Libraries should only use board.I2C() if they haven't been passed an I2C object.

How does that work for libraries like this one (adafruit_clue)?

@tannewt
Copy link
Member

tannewt commented Sep 25, 2020

Libraries should only use board.I2C() if they haven't been passed an I2C object.

How does that work for libraries like this one (adafruit_clue)?

We need to add an option i2c argument here: https://github.com/adafruit/Adafruit_CircuitPython_CLUE/blob/master/adafruit_clue.py#L203-L206

@kevinjwalters
Copy link

What's the clue object going to be pass in https://github.com/adafruit/Adafruit_CircuitPython_CLUE/blob/master/adafruit_clue.py#L976 then?

tannewt added a commit to tannewt/circuitpython that referenced this issue Sep 25, 2020
Greater that 100khz is technically out of the original spec.

Background here: adafruit/Adafruit_CircuitPython_CLUE#36
@tannewt
Copy link
Member

tannewt commented Sep 25, 2020

PR created for CircuitPython: adafruit/circuitpython#3471

@tannewt
Copy link
Member

tannewt commented Sep 25, 2020

What's the clue object going to be pass in https://github.com/adafruit/Adafruit_CircuitPython_CLUE/blob/master/adafruit_clue.py#L976 then?

We'll either have to remove that or not use the Clue object at all. At some point, these high level libraries aren't the right way of doing advanced things.

@kevinjwalters
Copy link

Another datapoint, the Cytron Edu:bit works for a short period at 400kHz and then gives a wide range of errors in CircuitPython after an unpredictable number of i2c operations. Oddly both require a power cycle to revert to normal operation.

@kevinjwalters
Copy link

A bit more peripheral information. One interesting, subtle change on the V2 micro:bit is there are two i2c buses: an internal one for onboard devices and an external one presented on the edge connector: https://tech.microbit.org/latest-revision/

@caternuson
Copy link
Contributor Author

I think maybe we can close this? The fix was implemented at the CP level.

@tannewt tannewt closed this as completed Oct 14, 2020
@kevinjwalters
Copy link

kevinjwalters commented Apr 11, 2021

It struck me today that these mismatches are trivial to detect and prevent in the libraries. I posted Adafruit Forums: Checking current i2c speed in libraries against each device to discuss this.

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

No branches or pull requests

5 participants