rp2/rp2_pio: Fix PIO support for pin wrapping and RP2350B upper-bank pins. #18343
Conversation
AJMansfield
force-pushed
the
rp2-pio64
branch
from
a5dc881 to
fe4a966
Compare
|
Code size report: |
|
@Gadgetoid @sfe-SparkFro @maholli Are any of you able to test if this resolves the issue? |
|
Thanks! Did |
It should be just as it was, only with upper-range GPIO pins now fully functional. |
|
Doh, my mistake! Was building for the Pico 2, which is not the RP2350B, so Can confirm that this works with the RP2350B! |
|
@dpgeorge Can this please be considered for v1.27? Or even #16985. We at SparkFun have an upcoming product that really needs #16199 to be resolved. We can of course include it in a release in our fork, though we strive to have minimal differences with our fork where possible. At least some indication of when #16199 will be resolved would be wonderful, thank you! |
dpgeorge
left a comment
dpgeorge
left a comment
There was a problem hiding this comment.
Thanks @AJMansfield for this, it's definitely good to support wrapping.
Some comments above, but overall this looks like a good approach.
AJMansfield
force-pushed
the
rp2-pio64
branch
from
fe4a966 to
198970c
Compare
|
@sfe-SparkFro I've pushed a new revision that should in theory behave identically, but I still lack means to test it directly. Could you test 198970c and report whether this still resolves the problem? |
|
Thanks @AJMansfield for updating the PR. It looks really good now, good not to depend on internal APIs. |
dpgeorge
left a comment
dpgeorge
left a comment
There was a problem hiding this comment.
I tested this on a PIMORONI_PICO_PLUS2_RP2350 (thanks @Gadgetoid !) using the script from #16199 (comment) modified to test with out_base=Pin(20) and out_base=Pin(36). Without the fix in this PR it does not work when using gpio_base(16) (the pin doesn't toggle). But with this PR it does work.
On RP2350B where there are more than 32 pins, using `pio_sm_set_pins_with_mask()` and `pio_sm_set_pindirs_with_mask()` is not correct because their arguments are `uint32_t` and higher bits get lost when `pio.gpio_base(16)` is used. This commit fixes the issue by using the 64-bit API functions on RP2350B. It also makes sure pin wrapping is supported, i.e. using [30, 31, 0, 1] or [46, 47, 16, 17] as contiguous pin ranges for a PIO program. Fixes issue micropython#16199. Signed-off-by: Anson Mansfield <[email protected]>
3 participants
Summary
The existing public pico-sdk API methods for
pio_sm_set_pins_with_mask/pio_sm_set_pindirs_with_mask, including the 64-bit variants, do not allow PIO "pin wrapping", i.e. using[30, 31, 0, 1]or[46, 47, 16, 17]as contiguous pin ranges for a PIO program, despite the documented support for this in the underlying hardware.This patch exposes the internal method that the existing 32- and 64-bit versions of these functions call to in order to skip the way the wrappers "helpfully" perform truncated bit-shifts on their arguments, and uses it to also resolve the outstanding issue with using upper-bank RP2350B pins for PIO.
Resolves: #16199
Closes: #16985
Testing
This still needs to be tested with real RP2350B hardware, not just the more common RP2350A --- as this is a key area where these two chips are different. At the moment I do not have suitable hardware to test on.