rp2/rp2_pio: Switch to mask64 for pins/pindirs. #16985
Conversation
Use pio_sm_set_pins_with_mask64 and pio_sm_set_pindirs_with_mask64 in lieu of manually shifting by the pin base. These functions were added in SDK 2.1.0. Signed-off-by: Phil Howard <[email protected]>
Gadgetoid
force-pushed
the
patch-rp2-pio-maskpins
branch
from
e5950ae to
d452e14
Compare
|
Code size report: |
I guess I should have seen that coming 🤦 |
|
Out of curiosity I looked into why it's such a huge increase, and it's because all of those 64-bit operations are done as Compare the original disassembly: versus the new disassembly: |
There was a problem hiding this comment.
That seems like an absurd size increase for a feature that adds literally no functionality. If this were the thing needed to add some missing corner-case to fully support upper bank pin PIO, that would be one thing (even if I'd still suggest gating it on #if PICO_PIO_USE_GPIO_BASE) -- but the previous code already offers complete support for the upper bank access.
|
Looks like these same 64bit ops might be leaked into Pico 2 and Pico 2 W builds: via: micropython/ports/rp2/mphalport.h Lines 184 to 190 in f1018ee Gating them probably wouldn't help to accomplish the original goal of clarity, but I wouldn't consider 64 bytes "huge" enough to weigh so heavily in a correctness vs size tradeoff. Would this even have been a debate if these SDK functions were available when the original PIO changes were made? |
The only correctness issue that this solves is one it replaces with a functional deficiency. The hardware supports using pin ranges that wrap at one of the two possible 32-pin upper boundaries. By which I mean, it's completely valid and supported to use The previous 32-bit version had a mild correctness bug, where it would blindly truncate those bit masks, and as a result only configure In the new code, though, that is no longer a possible configuration at all; an attempt to set up a configuration like that would instead fault on For the 64-bit version to even itself be correct, it would need to check that parameter and raise an appropriate python exception; but the actual correct version is to just use the 32-bit operations but replace the regular bitshifts with bit rotations. EDIT: Errors of that sort will become recoverable if #16991 is merged, so there's an argument to be made that with that added, the 64-bit version does have the more correct behavior. It's still not ideal, though; the wrapping use-case ought to be supported. |
|
Here's what I'd propose: static inline uint32_t rotl32a(uint32_t x, uint32_t n) {
return (x<<n) | (x>>(32-n));
}
static void asm_pio_init_gpio(PIO pio, uint32_t sm, asm_pio_config_t *config) {
uint32_t gpio_base = pio_get_gpio_base(pio);
uint32_t pinmask = rotl32a( ~(-1 << config->count), config->base - gpio_base);
pio_sm_set_pins_with_mask(pio, sm, rotl32a(config->pinvals, config->base - gpio_base), pinmask);
pio_sm_set_pindirs_with_mask(pio, sm, rotl32a(config->pindirs, config->base - gpio_base), pinmask);
for (size_t i = 0; i < 32; ++i) {
if (pinmask & (1<<i)) {
gpio_set_function(gpio_base + i, GPIO_FUNC_PIO0 + pio_get_index(pio));
}
}
} |
|
Regarding the code size and calls to Regarding this PR itself: I tend to agree with @AJMansfield above that it's probably a good idea to have the ability to wrap the pin configuration around back to pin 0 or pin 16. Would need to first check that that's actually allowed by all the other bits of MicroPython PIO code. |
|
Hi folks, I believe this would be a fix for #16199 according to my testing with this replication code: #16199 (comment) Would appreciate this PR being re-evaluated. 64 bytes sounds like a reasonable trade-off to me for an important bug fix! |
|
fwiw in circuitpython we made some typedefs and convenience macros to adapt to whichever situation... // pio_pinmask_t can hold ANY pin masks, so it is used before selection of gpiobase
#if NUM_BANK0_GPIOS > 32
typedef struct { uint64_t value;
} pio_pinmask_t;
... #else
typedef struct { uint32_t value;
} pio_pinmask_t;
typedef uint32_t pio_pinmask_value_t;this is vagely similar to mp's here was my PR if you want more of the little details: adafruit#9901 We decided to punt on wraparound. I think this was based on discord discussion, but basically we found no evidence that wraparound had ever worked on rp2040 in circuitpython and that meant nobody had ever asked for it either. |
|
@dpgeorge @AJMansfield @Gadgetoid Can this PR please be re-evaluated? This appears to fix #16199 from my own testing. I'm working on a product that requires use of PIO on GPIO >=32, and the product won't function without #16199 being fixed. Thanks! |
|
I've been meaning to get some proper RP2350B hardware to test things like this on anyway... |
5 participants
Use pio_sm_set_pins_with_mask64 and pio_sm_set_pindirs_with_mask64 in lieu of manually shifting by the pin base.
These functions were added in SDK 2.1.0.
I matched, to a certain extent, the form of #16191
Somewhat related to #16199
Testing
I'll get 'round to it, I swear.
Trade-offs and Alternatives
None, these functions should be used for clarity and brevity. Though sheesh I don't like the uint64_t casts 🫠