Support for nrf52833 internal pull ups and pull downs.

[scath999]

The nrf52833 series of boards uses internal pull ups and pull downs to configure its pins for use with peripherals.

Currently, there does not appear to be a way to set these internal pull ups on selected pins to make use of them using busio.I2C.

Pending the outcome of this feature request, does anyone have any workarounds or ideas?

[dhalbert]

We discussed this to some extent in #4488, which was to use the RP2040 internal pullups, but make it a compile-time. Those pullups are considerably weaker (50-80kohms), and so they weren't as useful for I2C. The nRF pullups are 11-16kohms.

You could swipe the code idea from that PR, though think you want to make it a run-time option in the long run.

[scath999]

@dhalbert Thanks for directing me to that issue.

Is it ok to leave this open while I do some research and test that PR out?

[dhalbert]

Is it ok to leave this open while I do some research and test that PR out?

Sure, it's an interesting feature request.

[scath999]

I've taken a look at the issue(s) outlined, and the PR submitted. Couple of questions when you have a moment:

1. I'm sure it's my own lack of familiarity with git and the CPY repo itself, but I can't find where those changes are in the code. They don't appear on my side, nor do they appear in mpconfigboard.h, which is where issue #4873 appears to make changes.

2. I can find the section of I2C.c that checks for the pullups, and I'm thinking those checks are good to keep in place. They'll alert me (or someone else) if I've forgotten to set the pull up on a pin being used with i2c. Am I thinking about this right?

3. The proposed solution appears to make use of bitbangio - a module not included in nrf/common-hal/. Guidance on how to include this is welcome, as I don't think it will be as simple as copying that module over from some other port.

Cheers!

[dhalbert]

Re 1.: There were some proposed changes in one of the commits in #4488: 29bf9e7. That code was never added.

Re 2.: If internal pullups are enabled, we don't need to check for pullups. See the code in 1.

Re 3.: The bitbangio stuff is separate, it's about a flaw in the RP2040 I2C peripheral, and isn't relevant to nRF. By the way, bitbangio is in shared-module, because it's not port-dependent.

[scath999]

Thanks for getting back @dhalbert. Still a little confused, and thinking I'm not being clear. Apologies - I'll try to do better.

Thanks for the reference - looking now :)

It's my understanding that the board I'm using requires us to set pulls up or down in code - they aren't set up by default.

The feature I'm requesting is to be able to set these pin pulls up or down in code, not to have all pins set up all the time. Not sure if I'm being clear here, or if I always was and am just repeating myself - apologies.

I'm getting this from Nordic's forums, and their manuals. There are a ton of questions, but a really high-level one can be found here, containing this reference to their own internal documentation.

With that in mind, does my second question (and the feature request) make more sense overall?

Thanks again for your kindness and patience :)

[dhalbert]

That's fine. I understand, and the example I pointed to you was for "all time" setting. So what would make sense is to add an optional pullup argument to busio.I2C() and bitbangio.I2C(), that defaults to False. It would get passed to all the common_hal I2C creation routines.

Is there any case where you want a pull-down? For I2C, no, but are there other peripherals for which you want a pull-down?

[scath999]

I agree with your thoughts here (and would defer to your better knowledge and experience anyway).

I'm trying to imagine a place where someone might want to change a pullup to a pulldown, but I'm not experienced enough here. I defer to the better experience and knowledge of the community - would love to hear their thoughts on this.

I'm going to clone a fresh copy of CPY (mine is quite dirty as I've played with the code over the past month). I'll play around with it and see if I can come up with something that makes sense. If I do, I'll put a PR together.

[scath999]

On a side note, I'm really blown away by the support from the community. It isn't your job (or anyone elses's) to help some neophyte like me with a board adafruit didn't create, but I've encountered nothing but patience and kindness, and a willingness to help.

You guys are fantastic, and have my deepest thanks!

[scath999]

Ok. I've reviewed what I think is the relevant code.

If I'm understanding the process correctly, i2c implementations rely on definitions for the DEFAULT_I2C_BUS_ values for the Data and Clock channels to be declared in the boards mpconfigboard.h file.

That doesn't happen in the pca10100 mpconfig, which makes sense when there are no predefined i2c channels on the board.

As @dhalbert mentioned, an ideal next step is to pass an optional parameter for the i2c on pca10100 that allows us to define the SDA and SCL channels we want to use, set the pull on those channels to up, and return the interface.

The busio_i2c_obj_t struct contains two digitalio_digitalinout_obj structs, each referencing the indicated pin (SDA and SCL). Those structs contain a mcu_pin_obj_t pointer, which (presumably) references the physical pin indicated.
digitalio_digitalinout_obj_ts can be given a pull, apparently via common_hal_digitalio_digitalinout_set_pull( digitalio_digitalinout_obj_t *self, digitalio_pull_t pull).

This makes Dan's strategy seem reasonable, but I wonder if it's overly complicated.

Is it easier create a solution that makes use of the existing conceptual logic/workflow?

For example, we expect the following:

1. Declare two pins (one each for SDA and SCL) using something like SDA/SCL = digitalio.DigitalInOut(microcontroller.pin.<pin>)
2. Set the pull on those pins to up using something like SDA.pull = digitalio.PULL.UP
3. Pass those pins into the board_i2c() method, expecting an interface to return.

Maybe it's just me, but that makes sense given what I've seen and used so far in CPY.

If we accept that programmers of all levels might adopt this kind of straight-line thinking, then is it reasonable to make the following changes:

1. Change the board_i2c method to accept digitalio objects as optional parameters, and check for a specific MCU (like the pca10100).
2. Pending the result, extract and pass the mcu_pin_obj_t pointers from each digitalio parameter into the call to common_hal_board_create_i2c()
3. Change common_hal_busio_i2c_construct()s parameters to accept those pointers as arguments (if presented) or use the DEFAULT_I2C_BUS_ arguments if not.
4. Allow execution to proceed from that point onward as normal.

I don't think this is what @dhalbert really envisioned, but perhaps it's a starting point?

Cheers.

[dhalbert]

I was thinking of something simpler:

i2c = busio.I2C(board.SCL, board.SDA, pullup=True)

So the Python interface in shared_bindings/busio/I2C.c would add an optional keyword argument. Pass that on to common_hal_busio_i2c_construct(), which would take care of setting the pullups when it set up the pins, using the specific mechanisms provided by the HAL for the chip.

The busio_i2c_obj_t struct contains two digitalio_digitalinout_obj structs

You are suggesting adding that, is that correct? It doesn't have those objects now. I would not do that. We can just set the pullups when we set up the I2C (TWIM) peripheral.

We would add this to all ports, not just nRF. We want the interface to be uniform across all ports. On some ports (RP2040), it would not work very well because the builtin pullups have quite a high resistance for I2C.

For board.I2C(), yes, there would be some specific compile-time flag to set the value of pullup for the default pins.

Also we would add it to bitbangio.I2C. That does use DigitalInOut objects, in a port-independent way.