RP2350: hardware issue: stuck inputs

todbot commented 3 weeks ago

CircuitPython version

Adafruit CircuitPython 9.2.0-alpha.2350-19-gc53a0cb864 on 2024-08-19; Raspberry Pi Pico 2 with rp2350a

Code/REPL

import time, board, touchio
touchin = touchio.TouchIn( board.GP2 )
while True:
    print( touchin.value )
    time.sleep(0.1)

Behavior

Pin GP2 has a 1M resistor going to GND.

code.py output:
Traceback (most recent call last):
  File "code.py", line 2, in <module>
ValueError: No pulldown on pin; 1Mohm recommended

Description

The standard touchio technique doesn't seem to work.

Happens for at least these pins: board.GP0, board.GP1, board.GP2, board.GP3, board.GP4, board.GP5, board.GP6, board.GP7 ,board.GP8, board.GP9, board.GP10, board.GP11, board.GP12, board.GP13, board.GP14, board.GP15 on a known-good board that works with Pico RP2040.

Additional information

The three functions touchio uses:

common_hal_digitalio_digitalinout_switch_to_output(self->digitalinout, true, DRIVE_MODE_PUSH_PULL)
common_hal_digitalio_digitalinout_switch_to_input(self->digitalinout, PULL_NONE)
common_hal_digitalio_digitalinout_get_value(self->digitalinout)

Haven't had any changes for RP2350 except for one-line to select pin drive strength. Since other normal digitalio uses seem to work, I'm at a loss .

But for that drive-strength setting in DigitalInOut.c, why is it this:

    hw_write_masked(&pads_bank0_hw->io[pin],
        PADS_BANK0_GPIO0_DRIVE_VALUE_12MA << PADS_BANK0_GPIO0_DRIVE_LSB,
            PADS_BANK0_GPIO0_DRIVE_BITS);

and not:

gpio_set_drive_strength(pin, GPIO_DRIVE_STRENGTH_12MA);

as described in the Pico-SDK docs?

tannewt commented 3 weeks ago

as described in the Pico-SDK docs?

That code may pre-date the pico-sdk having an API for it. :-)

There is an errata around pull downs and input too. That will likely impact this too.

todbot commented 3 weeks ago

A small update: here are two scope traces of this code:

import time, board, touchio
while True:
    try:
        touchin = touchio.TouchIn(board.GP5)
        touchin.deinit()
    except ValueError as e:
        pass

The first one (RP2040 Pico) shows what I would expect: signal goes high for 10 us, then nice exponential decay to ground. The second one (RP2350 Pico2) shows the weirdness. After the 10us wait, it holds at ~2.2V until the pin is deinited. rp2040_touchio rp2350_touchio

This seems to match the "RP2350-E9" errata in the RP2350 datasheet for the RP2350 A2, which is the version of the RP2350s I have across all the devices I have.

dhalbert commented 3 weeks ago

I was just going to mentioned errata E9, which I first saw mentioned here: https://github.com/orgs/micropython/discussions/15621#discussioncomment-10423602

dhalbert commented 2 weeks ago

But for that drive-strength setting in DigitalInOut.c, why is it this:
    hw_write_masked(&pads_bank0_hw->io[pin],
        PADS_BANK0_GPIO0_DRIVE_VALUE_12MA << PADS_BANK0_GPIO0_DRIVE_LSB,
            PADS_BANK0_GPIO0_DRIVE_BITS);
and not:
gpio_set_drive_strength(pin, GPIO_DRIVE_STRENGTH_12MA);
as described in the Pico-SDK docs?

I think that API might not have been present in early versions of pico-sdk. I'm preparing a PR for this and will include that change.

dhalbert commented 2 weeks ago

I am not sure that the erratum RP2350-E9 is the cause of this after all, even though we are seeing the 2.1V "stuck" value. TouchIn doesn't use the internal pulldown at all. Yet I am seeing stuckness even on simple DigitalInOut:

On RP2350:

    [attach jumper between A0 and 3.3V]
>>> import digitalio, board
>>> a0 = digitalio.DigitalInOut(board.A0)
>>> a0.value
True
    [remove jumper]
>>> a0.value
True

RP2040 does not show this.

There is a fundamental difference between RP2040 and RP2350:

The RP2040 datasheet says:

By default, all pads come out of reset ready to use, with their input enabled and output disable set to 0.

The RP2350 datasheet says (for BANK0):

IMPORTANT The pad reset state is different from RP2040, which only disables digital inputs on GPIOs 26 through 29 (as of version B2) and does not have isolation latches. Applications must enable the pad input (GPIO0.IE = 1) and disable pad isolation latches (GPIO0.ISO = 0) before using the pads for digital I/O. The gpio_set_function()SDK function performs these tasks automatically.

EDIT: WRONG. gpio_set_function() is called by gpio_init(), which we do call. ~~gpio_set_function() is not called at all in DigitalInOut.c. I'll do that and see what happens.~~

I already wrote code to address E9, which I wrote first, but didn't fix the TouchIn problem.

dhalbert commented 2 weeks ago

I was wrong above: gpio_set_function() is called by gpio_init(), which we do call.

jepler commented 2 weeks ago

just peeping at the datasheet there's also the new "pad isolation" bit for each GPIO (but this bit should also be correctly set by gpio_set_function())

dhalbert commented 2 weeks ago

I am seeing this 2.2V latchup (actually 2.4V here) with the pulls not set at all, ever, and even with a 1M pull-down on the pin. I am testing with GP0 on the Tiny2350 instead of A0, which has the ADC attached and might have different reset characteristics.

Example: Create a DigitalInOut on GP0. GP0.value is False. Pull GP0 high with a jumper direct to 3.3V, or a 1M resistor to 3.3V GP0.value is True. Remove jumper. GP0.value remains True, even if 1M resistor is then connected to ground. If you measure the pin with a voltmeter, it reads 2.4V. Pulling it hard to ground sets it back to False and the 2.4V disappears.

One other thing: the power-on reset state of the pins sets a pull-down. See 14.8.2.2, page 1319, in the RP2350 datasheet and 5.5.2.2, page 615 in the RP2040 datasheet. We never clear this explicitly that I see (we should have). I changed our code so we clear both pulls immediately on DigitalInOut construction, to avoid erratum E9, but I still see the 2.4V. Maybe the latch-up condition is being enabled happening somewhere earlier before for all pins.

dhalbert commented 2 weeks ago

I am going to try to reproduce these problems with some simple pico-sdk programs.

dhalbert commented 2 weeks ago

I filed an issue with raspberry-pi: https://github.com/raspberrypi/pico-feedback/issues/401

dhalbert commented 1 week ago

@todbot Addressing the original issue: I was able to implement TouchIn using a 1M pull-up instead of a pull-down, testing on an RP2040. The sense code just pulls the pin down instead of up and then measures the time to flip from logical low to high. The code changes are simple. The calibration may need to change due to the transition voltage not being exactly in the middle, but in principle it works.

However, this method doesn't yet work on RP2350 because of the latch-up. I need to implement something like the workaround described in RP2350-E9: enabling and disabling the input using IE. I could fiddle with that, but I'd prefer to wait for further guidance from RPi on the best way to do the workaround, and see if they add support in pico-sdk to make it easier.

Anyway, maybe we have a path forward on TouchIn.

todbot commented 1 week ago

@dhalbert, agreed, this is what I found when I was hacking too. I didn't consider it a viable alternative since it changed the API of touchio and would break existing examples. My concern was if the API changed to support both directions, shared_module/touchio size would grow, putting it at risk of being excluded from SAMD builds.

dhalbert commented 1 week ago

The API is the same: it's just that you use an external pullup instead of a pulldown. The code difference is very slight, and can be determined at compile-time. See https://github.com/adafruit/circuitpython/compare/main...dhalbert:circuitpython:rp2350-touch. Also, SAMD21 doesn't use the generic touchio: it uses the proprietary SAMD21 hardware touch, so it's not affected by this.

todbot commented 1 week ago

Apologies, I wasn't very clear. I was assuming the touchio API needs to stay consistent with the many existing guides (and, selfishly, the 100+ already-assembled PCBs I sell). Thus, to support pull-ups and pull-downs at run time, touchio would need an optional new argument to TouchIn() or a new class. This would make shared-module/touchio change, which would impact all implementations of touchio, not just RP2.

dhalbert commented 1 week ago

The RP2350 datasheet has been updated: the RP2350-E9 erratum has been rewritten and includes a lot more information. I'll study it to see what we need to do in DigitalInOut and TouchIn. No word yet on any workaround support in pico-sdk.

dhalbert commented 6 days ago

@todbot The plan I had in mind was to switch at compile-time, for RP2350 only, to supporting a weak pull-up rather than a weak pull-down. That would be documented, so any RP2350 hardware would need to use it that way. It would not be an option to switch at run-time between the two methods. I just was testing it on RP2040 to see if the idea worked.

We might support an informational TouchIn.pull_needed read-only parameter, which would return digitalio.DigitalInOut.Pull.DOWN, .UP, or None. None would be for ports that have a native implementation: ports/*/common-hal/touchio/ exists. All others, like SAMD51, would use DOWN, except for RP2350, which would return UP. So any supporting library could check if a pull was needed.

It's true that any existing external boards that added external pull-downs wouldn't work with RP2350, but that's already the case, and we couldn't fix that. The boards would need a redesign, to allow the pull direction to be set by a jumper. Which board(s) of yours are affected by the RP2350 issue?

todbot commented 6 days ago

Which board(s) of yours are affected by the RP2350 issue?

All of these I've sold, currently sell, or are planning to sell. As Raspberry Pi has advertised the Pico2 as a drop-in replacement for Pico, I now have to make sure my customers don't inadvertently solder down a part that can never work.

dhalbert commented 6 days ago

That's a lot of boards to respin. I don't see any way around it :frowning_face: if you want to support RP2350. I can make the software side easier for you, like .pull_needed, but the E9 issue is certainly a showstopper for a certain class of use cases.

todbot commented 6 days ago

If touchio behaves differently between RP2040 and RP2350, none of these PCBs can work for both cases. I sell these as kits, with all SMD parts soldered except for the Pico, to give people the option of installing one of the nice 16MB USB-C Pico clones. I'm updating product pages to let people know to not try a Pico2 on them, but I'm sure someone will try

dhalbert commented 3 days ago

On Monday we discussed internally what to do about the E9 workaround for CircuitPython.

Without adding additional API specifically to support the RP2350, there is no way to inform CircuitPython in various use cases, "well, yes, this is a weak input signal, so do the workaround". The only hint would be if you enabled the internal pulldown, but there are other cases as well. We do not right now want to make a special-case API for the RP2350.

So our conclusion is to not implement the workaround, at least for now, but simply to document the restrictions that make it unnecessary: don't expect the internal pulldown to work, use an external pulldown 8.2k or lower if you need a pulldown, or make sure your input signal can sink enough current to override the leakage current. This is defining the problem away.

So this now becomes an issue to address via documentation.

In addition, we will disable touchio on RP2350 for now, because implementing it via an external pull-up is non-standard compared with all the other implementations, and that will be a support issue. (I personally think we could live with a non-standard impl, but I can see the argument the other way as well.) This might be revisited later.

adafruit / circuitpython