Skip to content
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

RP2350: hardware issue: stuck inputs #9541

Closed
todbot opened this issue Aug 20, 2024 · 20 comments · Fixed by #9617
Closed

RP2350: hardware issue: stuck inputs #9541

todbot opened this issue Aug 20, 2024 · 20 comments · Fixed by #9617

Comments

@todbot
Copy link

todbot commented Aug 20, 2024

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?

@todbot todbot added the bug label Aug 20, 2024
@tannewt tannewt added the rp235x label Aug 21, 2024
@tannewt tannewt added this to the 9.2.0 milestone Aug 21, 2024
@tannewt
Copy link
Member

tannewt commented Aug 21, 2024

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
Copy link
Author

todbot commented Aug 24, 2024

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
Copy link
Collaborator

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

@dhalbert dhalbert self-assigned this Aug 25, 2024
@dhalbert
Copy link
Collaborator

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
Copy link
Collaborator

dhalbert commented Aug 26, 2024

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
Copy link
Collaborator

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

@jepler
Copy link
Member

jepler commented Aug 26, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@dhalbert
Copy link
Collaborator

I filed an issue with raspberry-pi: raspberrypi/pico-feedback#401

@dhalbert dhalbert changed the title RP2350 touchio does not work RP2350: stuck inputs: touchio does not work + other problems Aug 26, 2024
@dhalbert dhalbert modified the milestones: 9.2.0, Third-party Aug 28, 2024
@dhalbert dhalbert changed the title RP2350: stuck inputs: touchio does not work + other problems RP2350: hardware issue: stuck inputs Aug 29, 2024
@dhalbert
Copy link
Collaborator

dhalbert commented Sep 5, 2024

@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
Copy link
Author

todbot commented Sep 6, 2024

@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
Copy link
Collaborator

dhalbert commented Sep 6, 2024

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 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
Copy link
Author

todbot commented Sep 6, 2024

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
Copy link
Collaborator

dhalbert commented Sep 6, 2024

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
Copy link
Collaborator

dhalbert commented Sep 7, 2024

@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
Copy link
Author

todbot commented Sep 7, 2024

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
Copy link
Collaborator

dhalbert commented Sep 7, 2024

That's a lot of boards to respin. I don't see any way around it ☹️ 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
Copy link
Author

todbot commented Sep 7, 2024

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
Copy link
Collaborator

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.

@dhalbert dhalbert modified the milestones: Third-party, 9.2.0 Sep 10, 2024
@tannewt tannewt assigned tannewt and unassigned dhalbert Sep 12, 2024
@tannewt tannewt linked a pull request Sep 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants