pio_encoding fixes + added PIO_VERSION to platform_defs.h #2958

2026-05-19T17:19:06-05:00

kilograham commented

2026-05-19 17:19:06 -05:00

(Migrated from github.com)

pio_encode_mov(pio_pindirs) was wrong because the mov destination for pindirs is a different value
Noticed that pio_instructions.h is included before PICO_PIO_VERSION is set; didn't want to make a circular include, and previously pio_instructions.h is somewhat standalone include-able, so decided to add PIO_VERSION to platform_defs.h... TBH it should have been there already as the init of PICO_PIO_VERSION was a bit of a hack. I doubt anyone wants to override PICO_PIO_VERSION anyway
I saw that we have pio_exec_mov and pio_exec_out - kept these for backwards compatibility, but decided to just allow pio_exec and have pio_encode_mov take care of it. as i imagine most of uses of pio_encode_mov take a constant arg, this isn't much of an overhead (the alternative would have been to encode the two different values in the enum value, which is fine, but likely going to lead to more code if done at runtime vs just the ifs)

* `pio_encode_mov(pio_pindirs)` was wrong because the `mov` destination for pindirs is a different value * Noticed that `pio_instructions.h` is included before `PICO_PIO_VERSION` is set; didn't want to make a circular include, and previously pio_instructions.h is somewhat standalone include-able, so decided to add `PIO_VERSION` to `platform_defs.h`... TBH it should have been there already as the init of `PICO_PIO_VERSION` was a bit of a hack. I doubt anyone wants to override PICO_PIO_VERSION anyway * I saw that we have `pio_exec_mov` and `pio_exec_out` - kept these for backwards compatibility, but decided to just allow `pio_exec` and have `pio_encode_mov` take care of it. as i imagine most of uses of `pio_encode_mov` take a constant arg, this isn't much of an overhead (the alternative would have been to encode the two different values in the enum value, which is fine, but likely going to lead to more code if done at runtime vs just the ifs)

kilograham commented

2026-05-19 17:21:55 -05:00

(Migrated from github.com)

fixes #2754

lurch commented

2026-05-20 04:40:27 -05:00

(Migrated from github.com)

I doubt anyone wants to override PICO_PIO_VERSION anyway

Maybe it's worth removing the PICO_CONFIG: PICO_PIO_VERSION line? 🤔

> I doubt anyone wants to override PICO_PIO_VERSION anyway Maybe it's worth removing the `PICO_CONFIG: PICO_PIO_VERSION` line? :thinking:

lurch (Migrated from github.com) reviewed

2026-05-20 04:49:22 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

					
				@ -66,13 +66,20 @@ enum pio_src_dest {

				    pio_x = 1u,

For consistency, should this be using PICO_PIO_VERSION?
Or should some of the places in pio_instructions.h, pio.h and pio.c that are currently doing #if PICO_PIO_VERSION > 0 and #if PICO_PIO_VERSION == 0 be updated to use PIO_VERSION instead?

For consistency, should this be using `PICO_PIO_VERSION`? Or should some of the places in `pio_instructions.h`, `pio.h` and `pio.c` that are currently doing `#if PICO_PIO_VERSION > 0` and `#if PICO_PIO_VERSION == 0` be updated to use `PIO_VERSION` instead?

lurch (Migrated from github.com) reviewed

2026-05-20 05:01:40 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

I'm not really awake-enough yet to figure out what this code is doing, but (for symmetry) should pio_encode_out be doing if (dest == pio_exec) dest = pio_exec_out;?

I'm not really awake-enough yet to figure out what this code is doing, but (for symmetry) should `pio_encode_out` be doing `if (dest == pio_exec) dest = pio_exec_out;`?

will-v-pi (Migrated from github.com) reviewed

2026-05-20 13:14:01 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

					
				@ -66,13 +66,20 @@ enum pio_src_dest {

				    pio_x = 1u,

I would say either the PICO_PIO_VERSION define should be moved into pio_instructions.h and used everywhere, or it should be removed entirely and PIO_VERSION should be used everywhere?

Or is it the case that the version used in pio_instructions.h must match the chip being used (hence using PIO_VERSION directly), but the rest of the code can be compiled for version 0 on both RP2040 and RP2350?

If we're keeping PICO_PIO_VERSION then there should also be a check that PICO_PIO_VERSION <= PIO_VERSION, otherwise you get lots of compile errors when trying to build RP2040 code with PICO_PIO_VERSION=1.

I would say either the `PICO_PIO_VERSION` define should be moved into `pio_instructions.h` and used everywhere, or it should be removed entirely and `PIO_VERSION` should be used everywhere? Or is it the case that the version used in `pio_instructions.h` must match the chip being used (hence using `PIO_VERSION` directly), but the rest of the code can be compiled for version 0 on both RP2040 and RP2350? If we're keeping `PICO_PIO_VERSION ` then there should also be a check that `PICO_PIO_VERSION <= PIO_VERSION`, otherwise you get lots of compile errors when trying to build RP2040 code with `PICO_PIO_VERSION=1`.

kilograham (Migrated from github.com) reviewed

2026-05-20 16:08:36 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

					
				@ -66,13 +66,20 @@ enum pio_src_dest {

				    pio_x = 1u,

I guess I'll just rename the one in platform_defs to PICO_PIO_VERSION (I had been avoiding that because other h/w stuff doesn't have a PICO_) we have PICO_PIO_VERSION in the build too, so I think it is probably confusing to introduce another

kilograham (Migrated from github.com) reviewed

2026-05-20 16:09:01 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

i could add an assert i guess!

lurch (Migrated from github.com) reviewed

2026-05-21 06:34:41 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

					
				@ -310,7 +317,7 @@ static inline uint pio_encode_wait_irq(bool polarity, bool relative, uint irq) {

				    return _pio_encode_instr_and_args(pio_instr_bits_wait, 2u | (polarity ? 4u : 0u), _pio_encode_irq(relative, irq));

#if PICO_PIO_VERSION > 0

```suggestion #if PICO_PIO_VERSION > 0 ```

lurch (Migrated from github.com) reviewed

2026-05-21 06:56:55 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

					
				@ -66,13 +66,20 @@ enum pio_src_dest {

				    pio_x = 1u,

Just because I was curious - f4e041b is what this would look like with PICO_PIO_VERSION replaced by PIO_VERSION

Just because I was curious - f4e041b is what this would look like with `PICO_PIO_VERSION` replaced by `PIO_VERSION`

will-v-pi (Migrated from github.com) approved these changes

2026-05-22 10:34:55 -05:00

lurch (Migrated from github.com) reviewed

2026-05-27 04:28:33 -05:00

src/rp2_common/hardware_pio/include/hardware/pio_instructions.h

Probably doesn't matter now, but I guess this could have been:

    else if (dest == pio_pindirs) dest = pio_pindirs_mov;

Probably doesn't matter now, but I guess this could have been: ```c else if (dest == pio_pindirs) dest = pio_pindirs_mov; ```

Sign in to join this conversation.

No reviewers

will-v-pi

No labels

compiler version issues

needs reporter detail

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-raspberrypi/pico-sdk!2958

				`@ -310,7 +317,7 @@ static inline uint pio_encode_wait_irq(bool polarity, bool relative, uint irq) {`
				`return _pio_encode_instr_and_args(pio_instr_bits_wait, 2u \| (polarity ? 4u : 0u), _pio_encode_irq(relative, irq));`

Rows
Columns