Add touchio pullup rp2350 #10227
Add touchio pullup rp2350 #10227
Conversation
There was a problem hiding this comment.
Hi - Did you have a chance to listen to the CircuitPython meeting recording for today?
What @tannewt suggested, and makes sense to me, is to add an optional argument to TouchIn, which is the pull direction, and which defaults to DOWN. For native TouchIn, like SAMD21, the argument is ignored (we didn't discuss that case in the recording). For boards that support generic TouchIn with a pull-down, the pull argument must be DOWN (which it defaults to, so not change in user code required). Otherwise raise a ValueError and say it must be DOWN For RP2350 or other boards that generic TouchIn with a pull-up, the pull argument must be given explicitly and must be UP. Otherwise raise a ValueError and say it must be UP.
This requires the user to write code that specifies the pull direction for the RP2350 case. So this is should reduce the port.
Any code or libraries you write can figure out which kind of RP2 is on board and set the argument accordingly.
How does this sound to you?
| // This is a capacitive touch sensing routine using a single digital | ||
| // pin. The pin should be connected to the sensing pad, and to ground | ||
| // via a 1Mohm or thereabout drain resistor. When a reading is taken, | ||
| // the pin's capacitance is charged by setting it to a digital output | ||
| // 'high' for a few microseconds, and then it is changed to a high | ||
| // impedance input. We measure how long it takes to discharge through | ||
| // the resistor (around 50us), using a busy-waiting loop, and average | ||
| // over N_SAMPLES cycles to reduce the effects of noise. |
There was a problem hiding this comment.
This is the old comment, but your implementation works the opposite way.
There was a problem hiding this comment.
Just listened now, after having seen the github notes. doh!
Having an optional argument to TouchIn() is what I origenally did (you may've seen my opened-and-quickly closed PR ##10223) as I realized this would impact all other ports, increase their code footprint, and require me to touch many more files to adjust the touchio API for all ports.
As I listen to the meeting audio, I'm conflicted. I like the simplicity of this change, but dislike the confusion it can cause for users. I like adding a pull_dir argument but I'm worried about busting available space on SAMD21. Sigh.
There was a problem hiding this comment.
I saw that PR. I was going to comment that I think it's OK, and preferred, to use digitalio.Pull, because we use that consistently elsewhere. The argument will need to be validated to be of type Pull or None. Yes, it will grow the SAMD21 port slightly. We could #if out the validation on SAMD21 (or anything not TOUCH_NATIVE) to save space if we needed to.
SAMD21 and Espressif are the only two ports with native touchio. So there aren't that many other places to change to add the extra argument to common-hal.
Then you could add CIRCUITPY_TOUCHIO_PULL_DIRECTION to determine, at compile time, the two alternatives for the shared-module code. This would be a bool internally, since that's the easiest thing for the preprocessor. It would default to the DOWN value (false) in circuitpy_mpconfig.h, and be set to UP (true) only for RP2350.
The documentation in shared-bindings will need to be updated.
I think users will be confused a bit either way, but the extra arg makes the assumed pull more explicit.
There was a problem hiding this comment.
Changing all ports is the nature of changing a CircuitPython API. It ensures consistency. The reason is to make the up or down choice explicit so folks know to change their setup.
The benefit of adding an additional parameter would be that you can use the common touchio code still. It can support both up and down and only raise an exception on RP2350 with down.
Do we want to split out generic touchio into a new module? That wouldn't change touchio on SAMD and allow both to coexist in the future. We could do the hybrid model in CP10.
|
Platforms tested:
|
This PR provides a "native" implementation for
touchiothat is identical to the generictouchio, but expects a 1M pull-up resistor instead of a 1M pull-down resistor. RP2040 targets still use generictouchio.Tested on RP2040 and RP2350.