raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.68k stars 917 forks source link

The sm_config_set_in_pins() set only one pin while it is named "pins". #1893

Closed suikan4github closed 1 month ago

suikan4github commented 1 month ago

The sm_config_set_in_pins() function configures only one pin while it is named "pins". Please kindly check.

The definition of the sm_config_set_in_pins() is like this : https://github.com/raspberrypi/pico-sdk/blob/efe2103f9b28458a1615ff096054479743ade236/src/rp2_common/hardware_pio/include/hardware/pio.h#L381-L383

This function receives the in_base parameter, and just passes it to the sm_config_set_in_pin_base();

On the other hand, sm_config_set_out_pins() receives not only out_base but also out_count parameter. Then, calls sm_config_set_out_pin_base() sm_config_set_out_pin_count() internally, to configure the multiple pins.

https://github.com/raspberrypi/pico-sdk/blob/efe2103f9b28458a1615ff096054479743ade236/src/rp2_common/hardware_pio/include/hardware/pio.h#L304-L307

I think the implementation of the sm_config_set_out_pins() is right, and the implementation of the sm_config_set_in_pins() is wrong.

lurch commented 1 month ago

The sm_config_set_in_pins() function configures only one pin while it is named "pins". Please kindly check.

Perhaps sm_config_set_in_pins() is just a "convenience function" which is named like that for commonality with sm_config_set_out_pins(), sm_config_set_set_pins(), etc.? If you don't like the name, I guess you could just ignore it and only use sm_config_set_in_pin_base() instead? :wink:

the implementation of the sm_config_set_in_pins() is wrong.

I think it's right - the ability to set the "count" of IN pins was a feature added in RP2350 (see section 11.1.1. in https://datasheets.raspberrypi.com/rp2350/rp2350-datasheet.pdf), and so is configured via a separate sm_config_set_in_pin_count function (which doesn't do anything on RP2040). And I assume that this wasn't added to sm_config_set_in_pins() in order to preserve backwards-compatibility?

suikan4github commented 1 month ago

Thank you for the comment.

If you don't like the name, I guess you could just ignore it and only use sm_config_set_in_pin_base() instead?

Yes. This is what am doing.

The ability to set the "count" of IN pins was a feature added in RP2350

Ah, I could not notice it. So, I don't need to call sm_config_set_in_pin_count() for RP2040...

I understand current sm_config_set_in_pins() is left as is to provide backward compatibility. I agree not to change the parameters.