Add some missing pio functions #2074

2024-11-20T09:15:14-06:00

peterharperuk commented

2024-11-20 09:15:14 -06:00

(Migrated from github.com)

Adds some 64 versions to work with rp2350b and more than 32 pins. Change cyw43 code to use the new function.

Adds some 64 versions to work with rp2350b and more than 32 pins. Change cyw43 code to use the new function. Fixes #1834

lurch (Migrated from github.com) reviewed

2024-11-20 09:31:15 -06:00

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

					
				@ -1727,0 +1755,4 @@

				 * before restoring the state machine's pin configuration to what it was.

				 *

				 * This method is provided as a convenience to set initial pin states, and should not be used against a state machine that is enabled.

				* Note: This method only works for pins < 32. To use with pins >= 32 call pio_sm_set_pins_with_mask64

... or \ref pio2 on an RP2350? 😉

... ` or \ref pio2` on an RP2350? :wink:

lurch (Migrated from github.com) reviewed

2024-11-20 09:35:29 -06:00

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

					
				@ -1741,0 +1788,4 @@

				 * This method is provided as a convenience to set initial pin directions, and should not be used against a state machine that is enabled.

				 * Note: This method only works for pins < 32. To use with pins >= 32 call pio_sm_set_pindirs_with_mask64

				 *

				 * \param pio The PIO instance; e.g. \ref pio0 or \ref pio1

I suspect you meant to add this "Note:" to the doxygen for pio_sm_set_pindirs_with_mask, not to the doxygen for pio_sm_set_pindirs_with_mask64 ?

I suspect you meant to add this "Note:" to the doxygen for `pio_sm_set_pindirs_with_mask`, not to the doxygen for `pio_sm_set_pindirs_with_mask64` ?

lurch (Migrated from github.com) reviewed

2024-11-20 09:39:37 -06:00

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

					
				@ -708,6 +708,15 @@ static inline uint pio_get_gpio_base(PIO pio) {

				#endif

In pio.c the use of pio_get_gpio_base is always guarded by #if PICO_PIO_USE_GPIO_BASE - does something similar need to be done here too?

In `pio.c` the use of `pio_get_gpio_base` is always guarded by `#if PICO_PIO_USE_GPIO_BASE` - does something similar need to be done here too?

lurch (Migrated from github.com) reviewed

2024-11-20 09:42:55 -06:00

src/rp2_common/hardware_pio/pio.c

					
				@ -253,0 +271,4 @@

				    pio_sm_set_pindirs_with_mask64(pio, sm, pindirs, pin_mask);

				}

				void pio_sm_set_pindirs_with_mask64(PIO pio, uint sm, uint64_t pindirs, uint64_t pin_mask) {

Should it also do check_pio_pin_mask64(pio, pinvals); ?

Should it also do `check_pio_pin_mask64(pio, pinvals);` ?

lurch (Migrated from github.com) reviewed

2024-11-20 09:44:07 -06:00

src/rp2_common/hardware_pio/pio.c

Should it also do check_pio_pin_mask64(pio, pindirs); ?

Should it also do `check_pio_pin_mask64(pio, pindirs);` ?

lurch (Migrated from github.com) reviewed

2024-11-20 10:10:55 -06:00

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

					
				@ -708,6 +708,15 @@ static inline uint pio_get_gpio_base(PIO pio) {

				#endif

Also, with all the nested brackets and both an || and and && being used here, it's very hard to tell what it's doing!
I may be totally wrong (bit-manipulation isn't my strong point), but would something like this do the trick?

#if PICO_PIO_USE_GPIO_BASE
    valid_params_if(HARDWARE_PIO, pinmask & ~(((1ull << 32) - 1) << pio_get_gpio_base(pio)) == 0);
#else
    valid_params_if(HARDWARE_PIO, pinmask & ~((1ull << 32) - 1) == 0);
#endif

I created the above by starting with

    uint64_t allowed_pins = (1ull << 32) - 1;
#if PICO_PIO_USE_GPIO_BASE
    allowed_pins <<= pio_get_gpio_base(pio);
#endif
    valid_params_if(HARDWARE_PIO, pinmask & ~allowed_pins == 0);

and then "inlining" the various parts.

Also, with all the nested brackets and both an `||` and and `&&` being used here, it's very hard to tell what it's doing! I may be _totally_ wrong (bit-manipulation isn't my strong point), but would something like this do the trick? ```c #if PICO_PIO_USE_GPIO_BASE valid_params_if(HARDWARE_PIO, pinmask & ~(((1ull << 32) - 1) << pio_get_gpio_base(pio)) == 0); #else valid_params_if(HARDWARE_PIO, pinmask & ~((1ull << 32) - 1) == 0); #endif ``` I created the above by starting with ``` uint64_t allowed_pins = (1ull << 32) - 1; #if PICO_PIO_USE_GPIO_BASE allowed_pins <<= pio_get_gpio_base(pio); #endif valid_params_if(HARDWARE_PIO, pinmask & ~allowed_pins == 0); ``` and then "inlining" the various parts.

peterharperuk (Migrated from github.com) reviewed

2024-11-20 10:14:54 -06:00

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

					
				@ -1727,0 +1755,4 @@

				 * before restoring the state machine's pin configuration to what it was.

				 *

				 * This method is provided as a convenience to set initial pin states, and should not be used against a state machine that is enabled.

				* Note: This method only works for pins < 32. To use with pins >= 32 call pio_sm_set_pins_with_mask64

The whole file is missing this. Can we please address in a separate PR if it's important?

peterharperuk (Migrated from github.com) reviewed

2024-11-20 10:17:29 -06:00

src/rp2_common/hardware_pio/pio.c

					
				@ -253,0 +271,4 @@

				    pio_sm_set_pindirs_with_mask64(pio, sm, pindirs, pin_mask);

				}

				void pio_sm_set_pindirs_with_mask64(PIO pio, uint sm, uint64_t pindirs, uint64_t pin_mask) {

no

peterharperuk (Migrated from github.com) reviewed

2024-11-20 10:17:46 -06:00

src/rp2_common/hardware_pio/pio.c

no

peterharperuk (Migrated from github.com) reviewed

2024-11-20 10:18:38 -06:00

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

					
				@ -708,6 +708,15 @@ static inline uint pio_get_gpio_base(PIO pio) {

				#endif

yes that does look simpler

lurch (Migrated from github.com) reviewed

2024-11-20 10:29:35 -06:00

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

					
				@ -1727,0 +1755,4 @@

				 * before restoring the state machine's pin configuration to what it was.

				 *

				 * This method is provided as a convenience to set initial pin states, and should not be used against a state machine that is enabled.

				* Note: This method only works for pins < 32. To use with pins >= 32 call pio_sm_set_pins_with_mask64

Sorry, I hadn't noticed that.

peterharperuk (Migrated from github.com) reviewed

2024-11-20 10:42:00 -06:00

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

					
				@ -1741,0 +1788,4 @@

				 * This method is provided as a convenience to set initial pin directions, and should not be used against a state machine that is enabled.

				 * Note: This method only works for pins < 32. To use with pins >= 32 call pio_sm_set_pindirs_with_mask64

				 *

				 * \param pio The PIO instance; e.g. \ref pio0 or \ref pio1

fixed

peterharperuk (Migrated from github.com) reviewed

2024-11-20 10:43:18 -06:00

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

					
				@ -708,6 +708,15 @@ static inline uint pio_get_gpio_base(PIO pio) {

				#endif

I don't want variables in here because valid_params_if compiles to nothing by default. But I think I've more or less fixed as suggested.

kilograham commented

2024-11-20 16:37:44 -06:00

(Migrated from github.com)

merging this prior to some more PIO api cleanup I'm doing for > 32 pins, so it is possible it may change slightly

kilograham (Migrated from github.com) approved these changes

2024-11-20 16:37:54 -06:00

lurch commented

2024-11-20 19:47:24 -06:00

(Migrated from github.com)

@kilograham When I do a full-build with arm-none-eabi-gcc it's complaining that:

...../pico-sdk/src/rp2_common/hardware_pio/pio.c: In function 'pio_sm_set_pins_with_mask64':
...../pico-sdk/src/rp2_common/hardware_pio/pio.c:259:41: error: conversion from 'uint64_t' {aka 'long long unsigned int'} to 'unsigned int' may change value [-Werror=conversion]
  259 |         uint base = (uint)__builtin_ctz(pin_mask);
      |                                         ^~~~~~~~
compilation terminated due to -Wfatal-errors.
cc1: all warnings being treated as errors

And indeed https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_platform_compiler/include/pico/platform/compiler.h#L63 says:

static inline uint __builtin_ctz(uint x) {

@kilograham When I do a full-build with `arm-none-eabi-gcc` it's complaining that: ``` ...../pico-sdk/src/rp2_common/hardware_pio/pio.c: In function 'pio_sm_set_pins_with_mask64': ...../pico-sdk/src/rp2_common/hardware_pio/pio.c:259:41: error: conversion from 'uint64_t' {aka 'long long unsigned int'} to 'unsigned int' may change value [-Werror=conversion] 259 | uint base = (uint)__builtin_ctz(pin_mask); | ^~~~~~~~ compilation terminated due to -Wfatal-errors. cc1: all warnings being treated as errors ``` And indeed https://github.com/raspberrypi/pico-sdk/blob/develop/src/rp2_common/pico_platform_compiler/include/pico/platform/compiler.h#L63 says: ``` static inline uint __builtin_ctz(uint x) { ```

kilograham commented

2024-11-20 20:25:46 -06:00

(Migrated from github.com)