Skip to content

Standardize I2C initialization for libraries that use it. #340

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

Closed
caternuson opened this issue Oct 17, 2017 · 15 comments
Closed

Standardize I2C initialization for libraries that use it. #340

caternuson opened this issue Oct 17, 2017 · 15 comments
Milestone

Comments

@caternuson
Copy link

caternuson commented Oct 17, 2017

For libraries that are for I2C devices, come up with a standard __init__ to use for all of them.

@tannewt
Copy link
Member

tannewt commented Oct 17, 2017

@caternuson would you mind providing numbered options for people to weigh in on? Thanks!

@caternuson
Copy link
Author

caternuson commented Oct 17, 2017

OPTION 1

CODE

from adafruit_bus_device.i2c_device import I2CDevice

DEVICE_DEFAULT_I2C_ADDR = 0x42

class Some_Device():
    def __init__(self, i2c, address=DEVICE_DEFAULT_I2C_ADDR):
            self._device = I2CDevice(i2c, address)

USAGE

import board
import busio
import adafruit_some_device
i2c = busio.I2C(board.SCL, board.SDA)
sensor = adafruit_some_device.Some_Device(i2c)

EXAMPLE
https://github.com/adafruit/Adafruit_CircuitPython_AMG88xx/

OPTION 2

CODE

from adafruit_bus_device.i2c_device import I2CDevice

DEVICE_DEFAULT_I2C_ADDR = 0x42

class Some_Device():
    def __init__(self, i2c=None, address=DEVICE_DEFAULT_I2C_ADDR, **kwargs):
        if i2c is None:
            import board
            import busio
            i2c = busio.I2C(board.SCL, board.SDA)
        self.i2c_device = I2CDevice(i2c, address)

USAGE

import adafruit_some_device
sensor = adafruit_some_device.Some_Device()

EXAMPLE
None. But would work now if board.SCL and board.SDA exist.

OPTION 3

CODE

from adafruit_bus_device.i2c_device import I2CDevice

DEVICE_DEFAULT_I2C_ADDR = 0x42

class Some_Device():
    def __init__(self, i2c=None, address=DEVICE_DEFAULT_I2C_ADDR):
            self._device = I2CDevice(i2c, address)

USAGE

import adafruit_some_device
sensor = adafruit_some_device.Some_Device()

EXAMPLE
None. I2CDevice doesn't currently have a default behavior for a None input.

OPTION 4

CODE

from adafruit_bus_device.i2c_device import I2CDevice

DEVICE_DEFAULT_I2C_ADDR = 0x42

class Some_Device():
    def __init__(self, address=DEVICE_DEFAULT_I2C_ADDR, i2c=None, **kwargs):
        if i2c is None:
            import board
            import busio
            i2c = busio.I2C(board.SCL, board.SDA)
        self.i2c_device = I2CDevice(i2c, address)

USAGE

import adafruit_some_device
sensor = adafruit_some_device.Some_Device()

EXAMPLE
https://github.com/caternuson/CircuitPython_TSL2561/

OPTION 5

CODE

from adafruit_bus_device.i2c_device import I2CDevice

class Some_Device():
    def __init__(self, i2c):
        self.i2c_device = I2CDevice(i2c, 0x42)

USAGE

import board
import busio
import adafruit_some_device
i2c = busio.I2C(board.SCL, board.SDA)
sensor = adafruit_some_device.Some_Device(i2c)

EXAMPLE
https://github.com/adafruit/Adafruit_CircuitPython_DS3231/

@caternuson
Copy link
Author

caternuson commented Oct 17, 2017

I like an option that would allow the simple usage syntax of Options 2-4 without all the boiler plate imports and setup.

@ladyada
Copy link
Member

ladyada commented Oct 17, 2017

i think the issue with #2 is if the sensor has no args and thus creates an i2c device, then if they use that syntax to make another sensor, it will error out because the pins are in use. so id prefer requiring passing in an i2c device

@caternuson
Copy link
Author

caternuson commented Oct 17, 2017

That is an issue (same for #4) :(

import adafruit_this_device
import adafruit_that_device

this_sensor = adafruit_this_device.This_Device()
that_sensor = adafruit_that_device.That_Device()

Looks nice, but second sensor would error out.

@jerryneedell
Copy link
Collaborator

For options 2-4 you are forcing it to use SDA,SCL - this is fine for boards that have hardware I2C and SCL/SDA are fixed, but what about boards like the ESP8266 where it is not fixed to these pins. Would be possible to allow for optional arguments to set the pins in all options?

@ladyada
Copy link
Member

ladyada commented Oct 17, 2017

some boards have fixed i2c, some you can change, so i like the idea of a kwargs for the address.

also some, rare devices require another initializer when creating (since we do not use begin()) so kwargs seems best?

@tdicola
Copy link

tdicola commented Oct 17, 2017

I'm curious is there an issue with the current state of the world (option 1, passing in an explicit I2C bus)? If the issues is reducing boilerplate IMHO I'd push that complexity to another layer and not into the drivers (where they might have to make tradeoffs like picking to use busio vs. bitbangio for ESP8266, etc.). IMHO something nice would be in the board module add functions that give you the default I2C bus for the board and can know if they should be bitbangio, busio, etc. For example:

import board
import adafruit_sensor_device

device = adafruit_sensor_device.FooSensor(board.default_I2C_bus())

Then for each board they can choose how they implement default_I2C_bus(), perhaps returning a busio.I2C for the default SCL, SDA, etc. or returning a bitbangio.I2C on ESP8266, etc. You could even extend it further and just have explicit buses on the board module (board.I2C_0, board.I2C_1, etc.)

@caternuson
Copy link
Author

caternuson commented Oct 17, 2017

If the issues is reducing boilerplate

That's it really. Just thinking of target audience, it'd be nice if it could be as simple as:

import thing
thingee = thing.Thing()

@caternuson
Copy link
Author

@tdicola I think what you are suggesting is similar to what @tannewt mentioned, but bury it in I2CDevice, i.e. Option 3. I think this would also cover the case of software I2C like ESP8266 as @jerryneedell has pointed out.

Summarizing thoughts to date:
OPTION 1 = current approach, works fine, but requires boiler plate
OPTION 2 = no good, relies on .SCL and .SDA, wouldnt' allow multiple sensor, etc.
OPTION 3 = maybe?
OPTION 4 = no good, see option 2
OPTION 5 = only works for fixed address

@deshipu
Copy link

deshipu commented Oct 19, 2017

There is also option 6, make busio.I2C() without parameters default to board.SDA and board.SCL, but still require explicit creation of the I²C bus. And option 7, pass I2CDevice to the sensors, instead of i2c and address, and make the I2CDevice default to board.SDA and board.SCL. Not sure how it would know the default address for the sensor, though, maybe the driver code could initialize it if it wasn't specified by the user:

from adafruit_bus_device.i2c_device import I2CDevice

DEVICE_DEFAULT_I2C_ADDR = 0x42

class Some_Device():
    def __init__(self, i2c_device):
            self._device = i2c_device
            i2c_device.set_default_address(DEVICE_DEFAULT_I2C_ADDR)

The nice thing about this is that the adafruit_bus_device can then do the boilerplate imports of board and busio in the simplest case (of course if we want to, say, use bitbangio, we would need to pass the i2c object explicitly).

@tannewt
Copy link
Member

tannewt commented Oct 23, 2017

My preference would be @tdicola's suggestion because its just explicit enough in my mind. @deshipu's options 6 and 7 feel like they hide too much by hiding board.

So, how about @caternuson's Option 1 for drivers and an issue to support default i2c busses through board so we can do something like @tdicola's example?

@tannewt
Copy link
Member

tannewt commented Oct 25, 2017

I'll take the silence to mean we're ok with my proposal. Would someone mind:

  • Adding a section to the Design Guide doc about this.
  • Creating an issue to add default buses to board or simply making a PR for it.

@tannewt tannewt added this to the 3.0 milestone Oct 25, 2017
This was referenced Oct 25, 2017
@caternuson
Copy link
Author

Done and done.

@tannewt
Copy link
Member

tannewt commented Oct 26, 2017

Thanks @caternuson !

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

6 participants