Nrf internal pullup support

[scath999]

Addresses #5689.

Not a full PR - only outlining suggested edits to limited number of files in support of internal pullups on certain NRF boards.

@dhalbert notified me that all instances of common_hal_busio_i2c_construct would need an extra parameter added. Accepting this as true, I didn't feel comfortable making that many changes before receiving guidance from the project owners.

I wasn't able to figure out the proper parameter to pass - @tannewt suggested MP_QSTR_internal_pullup, but this isn't a recognized type and I'm too new to this project to start creating types (or even know where to do that).

Hoping for input.

[microdev1]

Thanks for the PR @scath999. Few suggestions but you are on the right track.

[dhalbert]

@microdev1 thanks for this quick review

[dhalbert]

My comments would have been very similar to @microdev1's. I suggest using internal_pullup, to make clear which pullup we're talking about.

As you note, this is a job, because to make the API consistent, we have to add the new argument in the common-hal impl in all the ports. It should also be added to bitbangio.I2C.

[scath999]

@dhalbert @microdev1 Thank you so much!

If I can ask a couple of questions, to be answered when you guys have time:

1. I'm confused about MP_QSTR s - these look like predefined types that are set somewhere else in the code. Is this true, or are they more-or-less arbitrary (like variable names)?

2. @dhalbert has mentioned that this feature requires the new parameter be added in many places, even to bitbangio.I2C. Is there a way I can find where the changes need to be made, other than ask you (and/or CTRL+F) ? (I don't wanna waste your time).

I was afraid you'd just ignore this draft PR or get back to me in a few months - I'm really grateful for the input! Cheers!

[dhalbert]

1. I'm confused about MP_QSTRs - these look like predefined types that are set somewhere else in the code. Is this true, or are they more-or-less arbitrary (like variable names)?

MP_QSTR is a tricky way of providing predefined "interned" strings for parameter names, etc. There is a pre-processing step that searches for all of them and generates a big unique table of these strings. The part of the name after MP_QSTR_ is used as the string, with special escape hacks for non-name characters.

You can just think of it as a way of creating the name internal_pullups (or whatever) for use in argument lists, etc.

2. @dhalbert has mentioned that this feature requires the new parameter be added in many places, even to bitbangio.I2C. Is there a way I can find where the changes need to be made, other than ask you (and/or CTRL+F) ? (I don't wanna waste your time).

All the definitions and uses of the routines below will need to add the extra argument. Also the inline documentation in shared-bindings will need to document the new argument.
common_hal_busio_i2c_construct
shared_module_bitbangio_i2c_construct
busio_i2c_make_new
bitbangio_i2c_make_new

Someone will have to figure out how to turn on the internal pullups for each port, which will of course vary based on that chip family's hardware details.

The CI builds will not lie, and will error out on the places where you missed :)

[scath999]

MP_QSTR is a tricky way of providing predefined "interned" strings for parameter names, etc. There is a pre-processing step that searches for all of them and generates a big unique table of these strings. The part of the name after MP_QSTR_ is used as the string, with special escape hacks for non-name characters.

You can just think of it as a way of creating the name internal_pullups (or whatever) for use in argument lists, etc.

I see - thank you! If I can ask a quick followup, what do underscores in variable names do in this context? I'm concerned that this will create and expose a variable with a space in the API. (e.g. internal_pullup becomes internal pullup) That can't be correct, but it would increase understanding to know.

All the definitions and uses of the routines below will need to add the extra argument. Also the inline documentation in shared-bindings will need to document the new argument. common_hal_busio_i2c_construct shared_module_bitbangio_i2c_construct busio_i2c_make_new bitbangio_i2c_make_new

I understand, and will use your list - I have bandwidth for this right now, so I'm actively working on this.

Someone will have to figure out how to turn on the internal pullups for each port, which will of course vary based on that chip family's hardware details.

Yikes! I take it I'm not putting other people's work in danger here - is there some preventative measure I can take to keep this safe? I'm terrified of breaking anything (as silly as that might sound).

The CI builds will not lie, and will error out on the places where you missed :)

That's a huge relief. I did see some when building the project with changes initially, and was grateful to see that extremely rigorous tests were being performed by github.

Thanks again, @dhalbert - you've been extremely kind and patient.

[dhalbert]

I see - thank you! If I can ask a quick followup, what do underscores in variable names do in this context? I'm concerned that this will create and expose a variable with a space in the API. (e.g. internal_pullup becomes internal pullup) That can't be correct, but it would increase understanding to know.

Underscores become underscores. We have plenty of MP_QSTRs with underscores in them, don't worry. There are a bunch of escapes for such characters: see py/makeqstr*.py

Yikes! I take it I'm not putting other people's work in danger here - is there some preventative measure I can take to keep this safe? I'm terrified of breaking anything (as silly as that might sound).

That's why we do code reviews, and testing (manual in this case).

[scath999]

Not sure what to make of the call in shared-module/board/__init__.c lines 59-68:

common_hal_board_create_i2c takes no parameters, but passes DEFAULT_I2C_BUS_SCL/SDA to common_hal_busio_i2c_construct, which I think will error because the function definition is changed.

Does this mean I'll need to change calls for common_hal_board_create_i2c()? I hope not, but unsure how to proceed.

[dhalbert]

Does this mean I'll need to change calls for common_hal_board_create_i2c()? I hope not, but unsure how to proceed.

There's only one call to that routine anyway. You'll need to add the extra argument inside this call. But you don't need to add an extra argument to common_hal_board_create_i2c() itself. The value of that argument would be a compile-time constant which defaults to false except for boards that need to use internal pullups.

Now you have to understand how various compile options are passed along. You can study py/circuitpy_mpconfig.{h,mk}, the various mpconfigport.{h,mk} files, and the various mpconfigboard.{h,mk} files.

[scath999]

Super helpful.

Seems like I'll need to explicitly set CIRCUITPY_BOARD to 0, since it appears to default to 1 in circuitpy_mpconfig.mk.

Am I misreading that?

Also, is it wise to #define another constant like CURCUITPY_INTERNAL_PULLUPS in the appropriate .mk files, defaults to 0, and reference that?

Example (py/circuitpy_mpconfig.mk, at EoF):

# support for internal pullups
CIRCUITPY_INTERNAL_PULLUPS ?= 0
CFLAGS += -DCIRCUITPY_INTERNAL_PULLUPS=$(CIRCUITPY_INTERNAL_PULLUPS)

In the case of the 52833, I would then add something like CURCUITPY_INTERNAL_PULLUPS = 1 when detecting the MCU_CHIP. For example, at line 82 of ports/nrf/mpconfigport.mk:

ifeq ($(MCU_CHIP),nrf52833)
MCU_SERIES = m4
MCU_VARIANT = nrf52
MCU_SUB_VARIANT = nrf52833
CIRCUITPY_INTERNAL_PULLUPS = 1

SD ?= s140
SOFTDEV_VERSION ?= 7.0.1

BOOT_SETTING_ADDR = 0x7F000
NRF_DEFINES += -DNRF52833_XXAA -DNRF52833

endif

I don't know enough to tell if this will cause other important #defines to break (SPI, UART, etc.).

[dhalbert]

Seems like I'll need to explicitly set CIRCUITPY_BOARD to 0, since it appears to default to 1 in circuitpy_mpconfig.mk.

Don't turn off CIRCUITPY_BOARD -- everyone expects it to be there. You need it for pins.c.

Also, is it wise to #define another constant like CIRCUITPY_INTERNAL_PULLUPS in the appropriate .mk files, defaults to 0, and reference that?

Yes, make it more specific: CIRCUITPY_I2C_INTERNAL_PULLUPS.

In the case of the 52833, I would then add something like CURCUITPY_INTERNAL_PULLUPS = 1 when detecting the MCU_CHIP. For example, at line 82 of ports/nrf/mpconfigport.mk:

Don't do this for all 52833 builds. I thought the reason for this was just that your particular board had no pullups for on-board I2C peripherals. Is that the case?

Ideally the board would have on-board pullups. But we put pullups on our I2C breakouts, and most other mfrs do too. The microcontroller boards don't in general have pullups except when there are on-board sensors.

Taking a step back here, is this the previous paragraph resetting one of your original assumptions?

[scath999]

Don't turn off CIRCUITPY_BOARD -- everyone expects it to be there. You need it for pins.c.

Yes, I see that now. Better to have a different define to work with and leave that high-level define alone.

Yes, make it more specific: CIRCUITPY_I2C_INTERNAL_PULLUPS.

Can do. That said, I found something interesting the busio/I2C.c belonging to the espressif port (lines 61-64):

#if CIRCUITPY_I2C_ALLOW_INTERNAL_PULL_UP 
    gpio_pullup_en(sda->number);
    gpio_pullup_en(scl->number);
#endif

Makes me wonder if someone has already solved this problem for that board in a simpler way, or if this is undesired functionality because it may be hidden from the developer. Not calling fault here, just confused by this choice and unsure if I should take the same approach - even use the same #define?

Further, there appear to be boards that set the pull of passed SDA and SCL pins up without checking to see if they're the default pins, though this is likely just me not understanding what's written.

For example, mimxrt10xx/common-hal/busio/I2C.c has a call to i2c_check_pin_config on lines 95-96, but when you examine the method itself it calls IOMUXC_SetPinConfig with the pull set up (line 80 of that file). Later in that file, it uses the same method to pull the pins down.

Am I understanding this correctly?

Don't do this for all 52833 builds. I thought the reason for this was just that your particular board had no pullups for on-board I2C peripherals. Is that the case?

I see your point. It was my understanding that all 52833 boards have this functionality, which appears to be a wrong assumption.

My next question is (of course) how can I indicate that my particular board has this capability, and have CPY expose that functionality? Seems like I need to just straight out #define USE_INTERNAL_PULLUPS = true for my own build, but that seems brittle, and doesn't meaningfully contribute to CPY. I imagine this would upset you, @dhalbert - it might upset me, too.

Ideally the board would have on-board pullups.

I totally get this, and I admit I'm concerned that the internal pullups won't be strong enough, but I'll need to show that adding them on is necessary before I ask the team to rework the board.

The microcontroller boards don't in general have pullups except when there are on-board sensors.

A bit confused by this - do you mean sensors that are integrated into the chipset, or 3rd-party sensors that have been wired into the system? Are you asking because this is important to this case, or important for CPY and this feature? This is an honest question; I'm not doubting you, I just want to understand the implications of what I'm trying to add for my project, and CPY in general.

Thanks again :)

[dhalbert]

Let's discuss the basic requirements of pullups in the issue you opened for this #5689, instead of here in the PR. I brought this up in general in a comment there. I'll paste your relevant comments from above there, in this new comment: #5689 (comment)

[scath999]

I appreciate it @dhalbert, and thank you. I'll join you there on that count.

That said, I'm still puzzled by how to proceed given the other issues outlined in my previous post. I want to build this feature out in a way you'll appreciate and that will be useful for the community, but I'm unsure what to make of those items.

Thoughts?

[dhalbert]

Initial suggestions

[dhalbert]

Is this for Windows?

[scath999]

For Windows bash, yes. I thought I had added a comment indicating this.

I understand this can't stay, but needed it to build on my side.

Looking for input on this - no idea how to resolve.

[dhalbert]

We would recommend that you build on Linux if you can. Alternatively you could define an $(NRFJPROG) that is selected to be the right thing by some platform detection.

[dhalbert]

Move this down to line 124, and add an else for lines 125-136; do the check for external pullups only if internal_pullup is false.

Take away the #if CIRCUITPY_REQUIRE_I2C_PULLUPS; it is superseded by the passed-in boolean.

You don't need the hal_pull variable: you can use the constant:

Suggested change

	// Set pulls on I2C pins up if setpullup is 1 - this supports boards with internal pullups
	if (internal_pullup){
	nrf_gpio_pin_pull_t hal_pull = NRF_GPIO_PIN_PULLUP;
	nrf_gpio_cfg_input(scl->number, hal_pull);
	nrf_gpio_cfg_input(sda->number, hal_pull);
	}
	// Set pulls on I2C pins up if setpullup is 1 - this supports boards with internal pullups
	if (internal_pullup){
	nrf_gpio_cfg_input(scl->number, NRF_GPIO_PIN_PULLUP);
	nrf_gpio_cfg_input(sda->number, NRF_GPIO_PIN_PULLUP);
	}

[dhalbert]

Will be redundant with the changes above; remove all this.

Suggested change

	// Set pulls up if internal_pullup is true
	if (internal_pullup){
	nrf_gpio_pin_pull_t hal_pull = NRF_GPIO_PIN_PULLUP;
	nrf_gpio_cfg_input(scl->number, hal_pull);
	nrf_gpio_cfg_input(sda->number, hal_pull);
	}

[dhalbert]

Make a similar change as suggested in the nrf port:

if (internal_pullup) {
    // set the pullups
} else {
    // test for pullups
}

[dhalbert]

Don't do this; we will only enable internal pullups on the boards that you want to use them on, which will be your own company's board, not any of the existing boards.

[scath999]

Ok, you're saying leave this out of git add?

[dhalbert]

The idea of this PR is to be able to specify internal pullups at run-time, instead of compile time. Conceivably you would want board.I2C() or similar to use internal pullups, but that would be for a specialized board (e.g. your company board), not for any current board. That would be a reason to use a compile-time setting, and the reason I suggested BOARD_I2C_USE_INTERNAL_PULLUPS

[scath999]

I hear you. So you're saying move that #define to the board's .mk?

The only reason I've added a constant at all is to accommodate common_hal_board_create_i2c(void): It callscommon_hal_busio_i2c_construct which will include the internal_pullups param. I figured the least invasive change was to declare a constant rather than change yet another method signature and any calls to it.

Doing the latter would require me to get into shared-bindings/board/__init__.c and play with board_i2c() which would now need the internal_pullups param... that's starting to get messy.

Thoughts?

[dhalbert]

Right, I'm just saying check for the constant CIRCUITPY_BOARD_I2C_USE_INTERNAL_PULLUPS in common_hal_board_create_i2c(void). But define that constant directly, and then I think you can eliminate CIRCUITPY_I2C_INTERNAL_PULLUPS and I2C_USE_INTERNAL_PULLUPS.

[scath999]

Not to be thick-headed, but confused about what you mean by "define that constant directly".

To clarify, I assume you mean something like

mp_obj_t common_hal_board_create_i2c(void) {
    // All callers have either already verified this or come so early that it can't be otherwise.
    assert(i2c_singleton == NULL || common_hal_busio_i2c_deinited(i2c_singleton));
    busio_i2c_obj_t *self = &i2c_obj;
    self->base.type = &busio_i2c_type;

    // ---> Check if board supports internal pullups
    //         and define the constant. e.g.
    if (internal_pullups_are_supported)
          const bool I2C_USE_INTERNAL_PULLUPS = true;
    else
          const bool I2C_USE_INTERNAL_PULLUPS = false;
    

    common_hal_busio_i2c_construct(self, DEFAULT_I2C_BUS_SCL, DEFAULT_I2C_BUS_SDA, I2C_USE_INTERNAL_PULLUPS, 100000, 255);
    i2c_singleton = (mp_obj_t)self;
    return i2c_singleton;
}

My question is how can I know at runtime if the board has support for internal pullups?

This isn't an issue if it's just for my own board, but if it's for the community at large (which is what I'm assuming) it would need to be more robust - I'd need to know if the board supports this.

[dhalbert]

My question is how can I know at runtime if the board has support for internal pullups?

With the addition of the internal_pullups argument to busio.I2C(), all boards now support internal pullups. You just have to ask for them by setting internal_pullups to be true in the constructor.

The question now for a particular board is whether board.I2C() asks for internal pullups by default. So I'm sayin to remove the older compile time constants, and add a CIRCUITPY_BOARD_I2C_USE_INTERNAL_PULLUPS, which defaults to 0.

Then in the code above, you would just do

mp_obj_t common_hal_board_create_i2c(void) {
  ...
    common_hal_busio_i2c_construct(self, DEFAULT_I2C_BUS_SCL, DEFAULT_I2C_BUS_SDA, CIRCUITPY_BOARD_I2C_USE_INTERNAL_PULLUPS, 100000, 255);
   ...
}

[dhalbert]

Did you post and then delete (retract) a comment there? To answer that, nearly all chips have always had the capability of supporting internal pullups, we just didn't expose that. Now we are. It's a choice of the user about whether they are going to connect some I2C devices without pullups. That could happen on any board.

And, there might be some boards, with board-mounted I2C devices, without external pullups. In that case, we might want board.I2C() to use internal pullups by default. It might actually be better not to do that, since it hides whether internal pullups are used or not. On code written for your board, you might just go ahead and set internal_pullups=True explicitly when creating a busio.I2C() for use with the board-mounted I2C cervices. Then we don't need any compile-time constants at all.

There may be some chips that don't have pull-ups; I don't know. For those chips, the common-hal constructor would raise NotImplementedError if internal_pullups=True, to indicate it was not possible.

[scath999]

Ok. I'm confused about some of this, mainly because I'm trying to relate now to set your suggested CIRCUITPY_BOARD_I2C_USE_INTERNAL_PULLUPS at runtime, but perhaps it's better to put a pin in this?

In the interim, I can simply pass true to that part of the method and move on to the bigger issue: i2c.scan() is not returning any devices, even after I set the pulls up and read a proper voltage (3.3V) on them at runtime. At this point, I need to figure out what's going wrong there. After all, if going through all of this trouble still yields no usable result, what's the point?

[dhalbert]

Ok. I'm confused about some of this, mainly because I'm trying to relate now to set your suggested CIRCUITPY_BOARD_I2C_USE_INTERNAL_PULLUPS at runtime, but perhaps it's better to put a pin in this?

I'm suggesting that the compile-time constant need not exist at all. The user code will decide whether or not to use internal pullups.

In the interim, I can simply pass true to that part of the method and move on to the bigger issue: i2c.scan() is not returning any devices, even after I set the pulls up and read a proper voltage (3.3V) on them at runtime. At this point, I need to figure out what's going wrong there.

I'll build this and try it out.

Are you trying this with a PCA10100 and an external sensor without pullups, or are you trying it on your own board?

[dhalbert]

The default for the pca10100 should not be changed: you are using it as a test board, but that's for your own use. It is your company's board that will turn this on; no existing boards will do this.

[dhalbert]

Make a similar change as suggested in the nrf port:

if (internal_pullup) {
    // set the pullups
} else {
    // test for pullups
}

[dhalbert]

Make a similar change as suggested in the nrf port:

if (internal_pullup) {
    // set the pullups
} else {
    // test for pullups
}

[scath999]

Ok, so do away with the existing code beneath this comment?

// This appears to be a board-specific way of supporting internal pullups without 
    // going through all of the changes I've made - @dhalbert can you comment?
    // Not going to change anything here before discussion - will just throw internal_pullup away
    #if CIRCUITPY_I2C_ALLOW_INTERNAL_PULL_UP
    gpio_pullup_en(sda->number);
    gpio_pullup_en(scl->number);
    #endif

[scath999]

This is in ports/espressif/common-hal/busio/I2C.c around line 64.

[dhalbert]

not do away with it, but move it into the then clause of the if statement above

[scath999]

Ok, so remove the #if and wrap the contents into the if-else. Can do.

[dhalbert]

Good catch, I believe yes.

[scath999]

Thumbsup.

[dhalbert]

Make a similar change as suggested in the nrf port:

if (internal_pullup) {
    // set the pullups
} else {
    // test for pullups
}

[dhalbert]

HI, replied to your queries, and deleted a few erroneous comments of mine.

[dhalbert]

not do away with it, but move it into the then clause of the if statement above

[dhalbert]

We would recommend that you build on Linux if you can. Alternatively you could define an $(NRFJPROG) that is selected to be the right thing by some platform detection.

[dhalbert]

The idea of this PR is to be able to specify internal pullups at run-time, instead of compile time. Conceivably you would want board.I2C() or similar to use internal pullups, but that would be for a specialized board (e.g. your company board), not for any current board. That would be a reason to use a compile-time setting, and the reason I suggested BOARD_I2C_USE_INTERNAL_PULLUPS

[dhalbert]

I am going to close this for now because the original motivation I believe has become moot. We can revisit this at some point in the future if we see more demand. I am reluctant to add it now because it increases code size on even the small builds.