Skip to content

Add default buses to board #372

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 25, 2017 · 14 comments
Closed

Add default buses to board #372

caternuson opened this issue Oct 25, 2017 · 14 comments

Comments

@caternuson
Copy link

From the discussion raised in this issue, w.r.t I2C bus:
#340

@dhalbert
Copy link
Collaborator

I gently want to suggest that we revisit the idea of board.I2C(), board.UART(), board.SPI() as implemented recently in PR's #842, #844, and #845. I think it introduces a new, somewhat weak, shortcut API that doesn't work out well if you need to change your program to use different pins. I suggest instead that we add defaults to the pin args for busio.I2C(), busio.UART(), and busio.SPI().

I'll take board.I2C() as an example. It returns a singleton instance that is busio.l2C(board.SCL, board.SDA). So now how might it be used? Well, instead of

i2c = board.I2C(board.SCL, board.SDA)
...
i2c.write_to(...)

you can do:

board.I2C().write_to(...)

But suppose you later decide you want to use different pins? The singleton call doesn't take pin args, so you don't do this (which is too verbose anyway):

board.I2C(board.D0, board.D1).write_to(...)    # DOESN'T WORK

Instead you need to go back to the original way and rewrite it as i2c = busio.I2C(board.D0, board.D1) You have to change all the uses of board.I2C() throughout your program to the first way.

You might argue that instead one should just assign the value:

i2c = board.I2C()
...
i2c.write_to(...)

but in that case, I think the better API is to have busio.I2C() have default args (as suggested by @deshipu in #340 (comment), and @ladyada in an offline discussion), i.e. the equivalent of:

    def I2C(scl=board.SCL, sda=board.SDA, *, frequency=400000):

so that you can just do:

i2c = busio.I2C()     # Note busio, not board.
                      # Will use the defaults of board.SCL and board.SDA.

It's true you lose the convenience of the singleton, but you don't need to specify the pin names or remember their order, and you don't need to import board (vs. not having to import busio).

The motivation for the original is issue #340, which was much more about adafruit_bus_device than about providing a convenience singleton. Note also that (usually?) busio.SOMEBUS is implemented to invisibly fall over to bitbangio.SOMEBUS if busio.SOMEBUS doesn't exist on the particular board.

Tagging @matt-land, @tannewt, @deshipu, @caternuson, and @tannewt for comments.

(This bothered me enough that it woke me up. On the other hand, my middle-of-the-night thoughts might not be so coherent.)

@deshipu
Copy link

deshipu commented May 18, 2018

Of course I would much prefer the solution proposed here by @dhalbert. Not only will it use less memory when you are not using I2C, not only adding it will result in smaller code size, but also it has the advantage of using an existing Python mechanism, which everybody will have to learn about anyways, instead of being something entirely new created specially for the purpose, which has to be learned separately from the documentation.

@tannewt
Copy link
Member

tannewt commented May 18, 2018

One case this is designed to ease is initializing multiple I2C devices on the same bus, which is common when using multiple feather wings. With a singleton in board the device drivers can default to the same bus:

d1 = device1.Device1()
d2 = device2.Device2()

With the proposed default args we'd need:

bus = busio.I2C()
d1 = device1.Device1(bus)
d2 = device2.Device2(bus)

The second option does make it easier to change the pins but how common will this be? With featherwings in particular, this is highly unlikely. Are we ok with the extra step of the second option? Is the first too magical?

I think it'd be tricky to have a shared default bus like the first option without having it built into a core module such as board or busio. If we put it in adafruit_bus_device we prevent mixing bus device drivers and non-bus device drivers.

@deshipu
Copy link

deshipu commented May 18, 2018

Can't we cache the bus, so that creating a bus with the same parameters just gets you the same bus back?
Then the device can also default to it.

I think that's what MicroPython does, anyways.

@dhalbert
Copy link
Collaborator

I was composing a message with the same idea the @deshipu just posted.

Suppose busio took care of the singleton? Right now:

i2c1 = busio.I2C(board.SCL, board.SDA)
i2c2 = busio.I2C(board.SCL, board.SDA)

will throw a "Pin xxx in use" right now.

We could have busio.I2C keep track of the pins and return the same object. If this is an issue of upward compatibility, we could have a new SharedI2C class. But I think it's probably fine to create and return a singleton as necessary.

@tannewt
Copy link
Member

tannewt commented May 18, 2018

Ok, that's fine with me. I think it'll be a bit tricky because the memory is allocated in shared-bindings and not common-hal.

@dhalbert
Copy link
Collaborator

Yes, this would be easier to implement in Python than C 😃

@deshipu
Copy link

deshipu commented May 18, 2018

As an easier solution, we could cache only the argument-less call.

@dhalbert
Copy link
Collaborator

That might be kind of confusing, since then sometimes a double call would work and sometimes not. I think it might not be too hard to keep track. The I2C objects already keep the pins, so we just have to look over the existing objects. Just have to keep a list of them. We could even use list primitives to manage the storage if we don't want to manage it in a C way on the heap.

@deshipu
Copy link

deshipu commented May 18, 2018

And expose the list as busio.buses? That could actually help in debugging when you get errors about used pins and you are not sure where from.

@dhalbert
Copy link
Collaborator

dhalbert commented May 20, 2018

I'm now thinking about the details of implementing cached bus objects. The busio I2C and UART allow some settings (like frequency, baudrate, parity) when they are created. So perhaps I should throw an error if there's a matching cached bus object (pins match), but its current settings do not match the settings requested in the __init__ call?

@tannewt
Copy link
Member

tannewt commented May 21, 2018

Throwing an error on mismatched settings is ok with me.

@dhalbert
Copy link
Collaborator

dhalbert commented May 23, 2018

After thinking about the implementation of this for several days and talking with @tannewt, I've changed my mind and decided that board.I2C giving you a singleton busio.I2C(board.SCL, board.SDA) does make good sense. But it's true that it's not what we're used to if we've been using busio.I2C(...) for everything up to now. It's a habit we can change, with some effort. It also means that a lot of the example code in the Guides can be rewritten in a simpler way when 2.x is no longer supported.

I will add some documentation in the various busio constructors to refer to the board singletons. Otherwise the upshot is to do nothing.

Having the singletons come from board works well also when there are possibly multiple singletons for a single function. I'm thinking of a hypothetical "Mega" board which might have multiple labelled sets of pins to accommodate multiple busses, e.g. SCL/SDA, and also SCL2/SDA2, or TX/RX, TX2/RX2, TX3/RX3, etc. For such a board, there would be board.I2C and also board.I2C2 (This is reminiscent of Serial, Serial1, Serial2, etc., in Arduino .

Example from CircuitPython Essentials Guide:

# CircuitPython demo - I2C scan
 
import board
import busio
import time
 
i2c = busio.I2C(board.SCL, board.SDA)
 
while not i2c.try_lock():
    pass
 
while True:
    print("I2C addresses found:", [hex(device_address) for device_address in i2c.scan()])
    time.sleep(2)

becomes

# CircuitPython demo - I2C scan
 
import board
import time
 
while not board.I2C.try_lock():
    pass
 
while True:
    print("I2C addresses found:", [hex(device_address) for device_address in board.I2C.scan()])
    time.sleep(2)

@dhalbert
Copy link
Collaborator

Fixed by #841, #844, #845. Thanks @matt-land!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants