Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pico_Plus_2: cleanup pin-names #9803

Merged
merged 2 commits into from
Nov 19, 2024
Merged

Conversation

bablokb
Copy link

@bablokb bablokb commented Nov 11, 2024

This PR does some cleanup on pin-names:

  • renamed SPICE_xxx to SPCE_xxx (Pimoroni calls their connector SP/CE)
  • removed SPI (board.SPI() throws NotImplementedError anyways)
  • added normal GPxx names since all pins are normal GPIOs as well
  • removed SPICE_TX and SPICE_RX. RX/TX pin names are UART-related on other boards. Here, they are only inclusive names for MOSI/MISO. It turns out that SPICE_TX is an UART-RX and vice versa. So to avoid confusion, I removed the names.

Adding a board_spi_obj (thus preventing the NotImplementedError) would be possible, but I don't think this makes sense. The SP/CE connector lacks a DC-pin and busy-pin. So to drive a SPI-screen, it is better to create an SPI object without MISO and use the pin for DC.

I would even suggest to remove SCK, MISO and MOSI as well. They are already defined as SPCE_SCK (and so on). The pure names suggest that there is something like a default SCK/MISO/MOSI on the Pico, which is not the case and is not implemented for other Pico-boards.

@ZodiusInfuser: maybe you could chime in and share your opinion regarding the pin-names

@ZodiusInfuser
Copy link

I believe the SPICE name was copied from what @Gadgetoid set within our C header for the product (e.g. https://github.com/pimoroni/pimoroni-pico-rp2350/blob/feature/wireless/micropython/board/PIMORONI_PICO_PLUS2/pimoroni_pico_plus2w_rp2350.h), though why he included the I I don't know.

The other changes sound sensible to me.

@Gadgetoid
Copy link

"SPICE" flows better and is easier to say, I will die on this hill.

image

@dhalbert
Copy link
Collaborator

dhalbert commented Nov 11, 2024

Whatever is consistent for your pin names elsewhere (MicroPython, Arduino, board silk, etc.) is OK with us. The pin can have multiple names if that would be helpful.

@bablokb
Copy link
Author

bablokb commented Nov 11, 2024

Although I use SPICE for simulation and when I am cooking, I have no problem reverting to SPICE naming again to make all spice boys and girls happy. Just let me know.

@dhalbert
Copy link
Collaborator

dhalbert commented Nov 18, 2024

@ZodiusInfuser @Gadgetoid Is Pimoroni in agreement that it should be SPICE? I have found only one use of that term on pimoroni.com, in the forums. We can also have an alias with SP_CE as part of the name.

@Gadgetoid
Copy link

I'm going to defer to @ZodiusInfuser on this one 👀

We can probably change the Pico SDK headers to SPCE without too much hassle since it's still early days.

@ZodiusInfuser
Copy link

Thanks for that .....

Go with SPCE like this PR already does. We'll (cough @Gadgetoid ) fix it in other places.

Gadgetoid added a commit to Gadgetoid/pico-sdk that referenced this pull request Nov 18, 2024
As per discussion on adafruit/circuitpython#9803

Change "SPICE" to "SPCE" for consistency.

Signed-off-by: Phil Howard <github@gadgetoid.com>
kilograham pushed a commit to raspberrypi/pico-sdk that referenced this pull request Nov 18, 2024
As per discussion on adafruit/circuitpython#9803

Change "SPICE" to "SPCE" for consistency.

Signed-off-by: Phil Howard <github@gadgetoid.com>
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have settled on the names. Thanks everyone.

@dhalbert dhalbert merged commit 132f1af into adafruit:main Nov 19, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants