Skip to content

Allow multiple board buses #5422

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

Merged
merged 7 commits into from
Jan 21, 2022
Merged

Conversation

Neradoc
Copy link

@Neradoc Neradoc commented Oct 2, 2021

This adds an optional second I2C singleton, referred to as "SECOND_I2C".

  • Define SECOND_I2C_BUS_SCL and SECOND_I2C_BUS_SDA
  • Declare it in pins.c like this for example:
    { MP_ROM_QSTR(MP_QSTR_STEMMA_I2C), MP_ROM_PTR(&board_second_i2c_obj) },

board.STEMMA_I2C is implemented on the QTPY RP2040, the only board that I know of where the STEMMA port does not share the default board.I2C pins.

Other boards that have a STEMMA port don't need the second I2C and can just use an alias, which is why I chose to leave that to a future PR.

    { MP_ROM_QSTR(MP_QSTR_STEMMA_I2C), MP_ROM_PTR(&board_i2c_obj) },

gamblor21
gamblor21 previously approved these changes Oct 2, 2021
Copy link
Member

@gamblor21 gamblor21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Didn't test it as I don't have the board.

@jfurcean
Copy link

jfurcean commented Oct 3, 2021

Tested this with the I2C QT Rotary Encoder.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. We might discuss it in Monday's meeting.

Copy link
Collaborator

@microdev1 microdev1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this using the following method:

def I2C(instance: Optional[int] = 0) -> busio.I2C
# for primary bus:
board.I2C() = board.I2C(0)
# for secondary bus:
board.I2C(1) 
# and so on...

@jfurcean
Copy link

jfurcean commented Oct 3, 2021

I like microdev's method because it could apply to other boards with more than one I2C whether stemma or not.

@dhalbert
Copy link
Collaborator

dhalbert commented Oct 3, 2021

I think we might consider doing both. The board.STEMMA_I2C() has affordance.

@dhalbert
Copy link
Collaborator

dhalbert commented Oct 4, 2021

# for primary bus:
board.I2C() = board.I2C(0)
# for secondary bus:
board.I2C(1) 
# and so on...

Let me be describe further our naming scheme for the board.SOMEBUS() singletons, and on pin or pin-group names in general. board pin names try to correspond to the silkscreen names on the board. They may also refer to specific-function pins that are not broken out to pads, but are attached to some on-board functionality, like board.LED, board.ACCELEROMETER_INTERRUPT, board.SD_CS, etc.

A few boards have defined "standard" pins to use for certain groups of pins. These usually correspond to clearly labelled pins, such as board.I2C() for SCL and SDA. Very occasionally there are pins that are not labelled, such as the SPI 2x3 header on Arduino-shaped boards, but there is a strong convention there, and the pins should be labelled. And the STEMMA/QT connectors define an I2C bus connection standard, though they are not literally labelled.

So for instance if a board had SCL, SDA, SCL1, and SDA1 pins, then it would might make sense to have board.I2C() and board.I2C1(). However, we have not tried to do board.SOMEBUS() singletons for weaker groups that are only convenience conventions. We discussed this in #5041 and 4121, specifically about the Pi Pico default buses, which actually vary between languages.

My general thought about this is that if you have to refer to documentation to figure out which pins correspond to SOMEBUSx, then you may as well just list the pins. If I were reading the code, I would have to refer to that documentation, so if instead the code uses the actual silkscreen pin names, I am better off without the pin group abstractions.

@jfurcean
Copy link

jfurcean commented Oct 4, 2021

Here is an earlier discussion about this on Scott's Deep Dive with Lady Ada - https://youtu.be/5UKr0fT51ro?t=5957

@microdev1
Copy link
Collaborator

Is it possible to point board.STEMMA_I2C() to board.I2C(1)? This way we can have board.BUS(instance) at the board module level and board specific names matching silk screen or documentation can be set in the board's config file.

@dhalbert
Copy link
Collaborator

dhalbert commented Oct 6, 2021

Is it possible to point board.STEMMA_I2C() to board.I2C(1)? This way we can have board.BUS(instance) at the board module level and board specific names matching silk screen or documentation can be set in the board's config file.

That is certainly be possible. I assume you have some specific use case in mind, probably for your own boards. Could you describe how you want to use and designate multiple buses?

@microdev1
Copy link
Collaborator

I did a crude implementation of my proposal and it can be found here.

I assume you have some specific use case...

I am proposing this method for a couple of reasons:

  • Reduced code duplication.
  • Provides more flexibility in terms of compatibility with boards.

@Neradoc
Copy link
Author

Neradoc commented Oct 12, 2021

I am looking at an implementation like that. My original intent was that the new code did not take any more space that the old one when not used, which led me to do an ugly duplication, but that's not good... Also I was not sure how to implement some of that. I'll commit an update soon.

I am not in favor of adding a parameter to the python API of board.I2C(). The idea is to add named dedicated I2C ports, in particular the STEMMA port in the case of the QT PY 2040, for discoverability. It's better for that if singletons are named in pins.c.

@microdev1
Copy link
Collaborator

I am not in favor of adding a parameter to the python API of board.I2C() ...

I agree about discoverability of the bus and thus in the implementation above I have set up following redirect at board level:

// in pins.c
STATIC mp_obj_t board_stemma(void) {
    return board_uart(mp_obj_new_int(0));
}
MP_DEFINE_CONST_FUN_OBJ_0(board_stemma_obj, board_stemma);

STATIC const mp_rom_map_elem_t board_module_globals_table[] = {
    { MP_ROM_QSTR(MP_QSTR_STEMMA), MP_ROM_PTR(&board_stemma_obj) },
};
# in this case
board.STEMMA() = board.UART(0) = board.UART()

@dhalbert
Copy link
Collaborator

@microdev1 board.STEMMA_UART, right, to distinguish STEMMA_UART and STEMMA_I2C?

@microdev1
Copy link
Collaborator

@microdev1 board.STEMMA_UART, right, to distinguish STEMMA_UART and STEMMA_I2C?

Yup, my implementation is just a poc... any name or even multiple names can be pointed to same instance at board level.

@microdev1
Copy link
Collaborator

@Neradoc Are you still looking into this? If not, I can work on maturing the poc.

@Neradoc Neradoc force-pushed the nera-secondary-I2C branch from 08c294a to 77ee965 Compare November 4, 2021 22:52
@Neradoc
Copy link
Author

Neradoc commented Nov 5, 2021

I pushed a new version, you declare the list of I2C pins and their number like this:

#define DEFAULT_I2C_BUS (2)
// SCL, SDA, SCL1, SDA1
#define DEFAULT_I2C_PINS { &pin_GPIO25, &pin_GPIO24, &pin_GPIO23, &pin_GPIO22 }

And declare a new I2C object with a name and then number like this:

ADD_SINGLETON_I2C_OBJ(board_stemma_i2c_obj, 1)
...
    { MP_ROM_QSTR(MP_QSTR_STEMMA_I2C), MP_ROM_PTR(&board_stemma_i2c_obj) },

It is compatible with the old style, thus not requiring any changes in boards that only have board.I2C.

@microdev1
Copy link
Collaborator

I made some tweaks in the implementation and also converted SPI and UART to allow multiple instances.
I'll try and merge my branch with yours.

@microdev1 microdev1 requested a review from dhalbert November 20, 2021 17:56
@microdev1 microdev1 changed the title Implement a secondary I2C singleton if the board needs it Allow multiple board buses Nov 20, 2021
microdev1
microdev1 previously approved these changes Nov 21, 2021
Copy link
Collaborator

@microdev1 microdev1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I implemented the suggestions that I had, need a second opinion before merging.

@evildave666
Copy link

evildave666 commented Nov 21, 2021

This might also be interesting for use on boards like the Maker Pi RP2040 that provide multiple I2Cs on grove connectors.

Actually on further thought, that board might be more trouble than its worth to do that, as it provides I2C0 and I2C1 on 3 different ports each.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@microdev1 Very nice, how you have made it backward-compatible! In the long run it would be nice to change all the existing definitions to use the _PIN struct initialization rather than the separate pins.

I suggested some minor clarifications in the doc.

@tannewt are you liking this?

@Neradoc
Copy link
Author

Neradoc commented Dec 14, 2021

I'm still not sold on the idea of a user-facing parameter to board.I2C (and others). For me it is a name for the pins marked SDA and SCL on the silkscreen for convenience, like other entries in board are names for pins, not a function for making I2C buses. board.I2C(1) looks more like it's giving a parameter to the default I2C bus, not a whole different instance on a different set of pins which have a different name.
Also I have seen confusion between board.I2C and busio.I2C, and I feel like that could increase that issue.

And I don't see a use case for it. It doesn't bring any portability since the number doesn't carry meaning. While board.STEMMA_I2C could be used to make portable code for a stemma QT breakout (pending a PR to alias it to board.I2C on relevant boards), board.I2C(1) doesn't do that. I don't see it used to loop through ports either (for scanning for example) since it's not a list, and does not provide a len().

@dhalbert
Copy link
Collaborator

Perhaps we can use the underlying mechanism, but not expose the parameter. Another example of multiple buses is Grand Central M4.

@microdev1 microdev1 requested a review from dhalbert December 14, 2021 18:32
@tannewt
Copy link
Member

tannewt commented Dec 14, 2021

I agree with @Neradoc.

board.I2C() and friends shouldn't take in a parameter because instance ID is meaningless. (It could also be confused with the peripheral instance number that MP uses too.) The purpose of board isn't to replace busio.I2C(). board is about reflecting the physical API of the board and shared board design pattern benefits. I like board.STEMMA_I2C() instead because it clearly designates a feature of a board that may be uniform across multiple boards and therefore makes the code more portable.

I'm ok using numbers internally but would prefer a text identifier. Perhaps use the QSTR instead of an instance ID.

I'd suggest adding a _COUNT suffix to the defines like CIRCUITPY_BOARD_SPI so it's clearer.

Thanks for iterating on this! It'll be nice to have multiple standard names for buses.

@microdev1
Copy link
Collaborator

microdev1 commented Dec 14, 2021

A lot of boards have buses with pin names TX0/RX0, TX1/RX1 and so on, the instance id parameter could work nicely here.

I'm ok using numbers internally but would prefer a text identifier. Perhaps use the QSTR instead of an instance ID.

Yes, we are already doing this, for example on adafruit_qtpy_rp2040:

board.STEMMA_I2C() = board.I2C(1)

The numerical parameter is a secondary and hidden way of interacting with the buses.

Which of the following is a better way for handling of multiple bus instances?

board.BUS() = board.BUS(0), board.BUS(1)...
board.BUS() = board.BUS0(), board.BUS1()...

- minor typo fix
- update documentation for board module
@tannewt
Copy link
Member

tannewt commented Dec 15, 2021

My preference is BUS0 and BUS1 because it emphasizes that these are names.

@dhalbert
Copy link
Collaborator

I would choose names related to the board nomenclature or silkscreen: I2C, I2C1, etc.

@microdev1
Copy link
Collaborator

microdev1 commented Dec 30, 2021

Thanks for the suggestions everyone, I haven't been able to get back to this and may not able to complete this for a while.
@Neradoc would you like to pitch in?

@kattni
Copy link

kattni commented Jan 4, 2022

@microdev1 Hello! We're looking at finishing this up, but I'm unclear on exactly what is left to be done. We need to add the QT Py ESP32-S2, but I believe from the conversation above, there's at least one other thing that needs to be done? I'm hoping you can clarify so we can get this finished up. Thanks!

@microdev1
Copy link
Collaborator

The instance parameter is now removed so this should be good to go.

dhalbert
dhalbert previously approved these changes Jan 5, 2022
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thanks for persevering on this. I have not tested this.

The next step is to add some more extra board buses for those boards where multiple named buses make sense. And then eventually all the boards could be changed to use the new scheme, and the backwards-compatibility code could be removed.

@dhalbert dhalbert requested a review from tannewt January 5, 2022 15:43
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for removing the instance argument. I think there is a bit more cleanup to simplify and clarify what the code is doing. The main task is to remove the argument parsing bits since they are no longer needed.

@microdev1 microdev1 requested a review from tannewt January 21, 2022 04:43
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for persisting on this! It looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
board New board or update to a single board circuitpython api enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants