Skip to content

Conversation

@waveshare
Copy link
Contributor

Added some weveshare board support,PTAL.

@waveshare waveshare changed the base branch from master to develop December 21, 2024 07:44
@lurch lurch self-requested a review December 24, 2024 17:31
@kilograham kilograham added this to the 2.1.1 milestone Jan 11, 2025
@lurch
Copy link
Contributor

lurch commented Jan 16, 2025

@waveshare Apologies for the delay in getting round to reviewing this.
My check_board_header.py script doesn't report any problems with any of these new header files 👍

However if I compare the waveshare_rp2350_one.h in this PR to the one in #2174 , I can see that your PR says that RP2350-One has:

// no PICO_DEFAULT_WS2812_PIN

but the version in #2174 says:

// --- WS2812 ---
#ifndef PICO_DEFAULT_WS2812_PIN
#define PICO_DEFAULT_WS2812_PIN 16
#endif

and https://www.waveshare.com/wiki/RP2350-One#Pinout_Definition also says that there's a WS2812 connected to GP16 ? 🤔
Also, the version in this PR says

#define PICO_DEFAULT_SPI_RX_PIN 16

but your pinout diagram seems to show that GP16 isn't available on any of the user-accessible pins?

@lurch lurch changed the title Added some weveshare board support Added some Waveshare board support Jan 16, 2025
@waveshare
Copy link
Contributor Author

Sorry, this is our mistake, #2174 is right, we will fix it soon
GP16 is only used for WS2812

@waveshare waveshare force-pushed the BSP branch 4 times, most recently from 4ad6a03 to 6224c6f Compare January 17, 2025 03:20
@waveshare
Copy link
Contributor Author

Hello,
After a long wait, we have corrected the corresponding pin definitions.Please take a look.

@lurch
Copy link
Contributor

lurch commented Jan 17, 2025

Thanks for the updates, my check_board_header.py now doesn't report any problems 👍

However I see that in various different files you've defined:

WAVESHARE_RP2040_TOUCH_LCD_SPI
WAVESHARE_RP2350_TOUCH_LCD_SPI
WAVESHARE_RP2350_LCD_SPI

(along with the associated pin-defines for each of those SPI interfaces).
You obviously know your own hardware much better than I do, but is there a reason for all these different names, or might it makes sense to just name them all WAVESHARE_LCD_SPI ?

Similarly, perhaps it would make sense for both

WAVESHARE_RP2040_BAT_ADC_PIN
WAVESHARE_RP2350_BAT_ADC_PIN

to be named just WAVESHARE_BAT_ADC_PIN ?

These suggestions are entirely optional; if you disagree I'm happy to approve this PR as-is.

@waveshare
Copy link
Contributor Author

I think your suggestion is very good. Give me some time and I will solve it.

@waveshare
Copy link
Contributor Author

Please take a look.

Copy link
Contributor

@lurch lurch left a comment

Choose a reason for hiding this comment

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

Fantastic 👍

Thank you for taking my comments into account, and thank you so much for adding all these board headers! ❤️

@lurch lurch linked an issue Jan 17, 2025 that may be closed by this pull request
@kilograham kilograham merged commit 61e2712 into raspberrypi:develop Jan 18, 2025
4 checks passed
will-v-pi pushed a commit to will-v-pi/pico-sdk that referenced this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for Waveshare RP2350 One board

4 participants