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

Add SparkFun Thing Plus RP2350 #2605

Merged

Conversation

sfe-SparkFro
Copy link
Contributor

Upcoming board from us!

Note - currently not compiling when including the radio stuff. Not sure why, because it's an exact copy from the Pico W board definition. Help would be appreciated, thanks!

C:\Users\Dryw\AppData\Local\Arduino15\packages\rp2040\hardware\rp2040\4.2.0\variants\sparkfun_thingplusrp2350\picow_init.cpp:21:10: fatal error: pico/cyw43_arch.h: No such file or directory
   21 | #include <pico/cyw43_arch.h>
      |          ^~~~~~~~~~~~~~~~~~~
compilation terminated.
C:\Users\Dryw\AppData\Local\Arduino15\packages\rp2040\hardware\rp2040\4.2.0\variants\sparkfun_thingplusrp2350\picow_digital.cpp:21:10: fatal error: pico/cyw43_arch.h: No such file or directory
   21 | #include <pico/cyw43_arch.h>
      |          ^~~~~~~~~~~~~~~~~~~

@earlephilhower
Copy link
Owner

Thanks for the addition!

I think this is down to the SDK build and includes. For the RP2040 based boards we use -Dboard=picow and include the CYW43 includes in the platform_inc.txt file. On the RP2350 we're using one of the RP2350B boards which is not going to include either the CYW43 libs nor the includes.

There are also a bunch of places where we've got #ifdef ARDUINO_RASPBERRY_PI_PICO_W which was fine before but now there needs to be some abstraction so we can key off of #ifdef HAS_CYW43... or something similar and ensure code gets included on all boards w/that specific WiFi chip.

Oddly enough, the fact that the CYW43 driver is in the SDK makes this more of a pain here. The wireless ESP32 and WINC1500 and other wired Etherned LWIP based drivers "just work" on the RP2350 because they're fully contained here.

For now, is there some special SDK you're building with to get the magic bits for the CYW43 library while on your board? I don't think the release version can make that happen on any RP2350 board...

@sfe-SparkFro
Copy link
Contributor Author

Thanks for the explanation, that makes sense now!

For now, is there some special SDK you're building with to get the magic bits for the CYW43 library while on your board?

No. Everything builds and runs fine without the radio stuff, then I just copied from the Pico W variant. After seeing the include error, I decided to just make the PR in the hopes someone would know what the problem was so I didn't have to dig. Saved me a lot of head scratching! 😄

now there needs to be some abstraction so we can key off of #ifdef HAS_CYW43... or something similar and ensure code gets included on all boards w/that specific WiFi chip.

Yes, that would be a great feature! Given where things are going (cough and cough), best to have something generic for any board to include the radio stuff as needed (some relevant work here and here for the Pico SDK v2.1.0 release).

I imagine that will take some time. Would it make sense for me to remove the radio stuff from this board for now? I can then make a separate PR to add the radio stuff back, which can get merged once more generic radio support is done.

@sfe-SparkFro
Copy link
Contributor Author

Would you like us to send you one of these boards to help with development? If so, can follow up via email.

@earlephilhower
Copy link
Owner

earlephilhower commented Nov 14, 2024

I did some investigation and there are several things that need to fall into place to get CYW43 running on non-RP2040-PicoW boards.

libpico needs to be updated on the RP2350 builds to set the proper config and libraries. Right now only the RP2040 will include the cyw43 driver stack. tools/libpico/Cmakelists.txt and rebuild, which leads to the first issue.

The 2.0.0 SDK has hardcoded defines for the pinout of the CYW43 chip. On the RP2040/PicoW that's no big deal, there is only one physical setup to worry about. On the RP2350 with multiple Wifi PCBs, that will need to be abstracted (we need to build the SDK using Cmake outside of Arduino, so only build a single .A for all CPU archs, not for all PCBs). Defines like CYW43_PIN_WL_HOST_WAKE and CYW43_PIN_WL_SDIO_1, etc. This can probably be handled with some careful cores/rp2040/sdkoverrides but I'd need to think about how to safely implement it...

Once that's running, then an additional define, PICO_CYW43_SUPPORTED needs to be added to the extra list in tools/makeboards.py for the PicoW and RP2350 boards w/the CYW43 chip. Maybe including the physical CYW43_PIN_xxxx from above. And then an almost search/replace of s/ifdef ARDUINO_RASPBERRY_PI_PICO_W/ifdef PICO_CYW43_SUPPORTED/ through the libraries and you're within spitting distance...

So, I see a couple options. You may want to consider going radio-free to start, and then I can work on my own to get the CYW43 stuff up later. Or, I can start a new dev branch/PR with the changes above and you can rebase this PR to that branch. When it's good, we can merge this Pr into that branch, then merge that branch to master. Whatever makes sense for you, just let me know...

--edit-- One other option would be to split out the cyw43 Cmake into its own library and pull that in on an individual basis. If the constant definitions become too much of a hangup, then a hardcoded list of pinouts in the CMakefile.txt could be built and then linked in via makeboards.py adding the appropriate flags.

@sfe-SparkFro
Copy link
Contributor Author

Thanks for working on this, much appreciated! Perhaps this should be tracked in a dedicate issue?

You may want to consider going radio-free to start

If we do this, would you be able to cut a v4.2.1 release so our board is at least accessible for users right away? If so, I can remove the radio stuff from this PR and make a new one to add it back against your new dev branch.

@earlephilhower
Copy link
Owner

Sure, I was planning on doing a release anyway since this could be a little bit involved. Releases are automated and only take about 1 minute of my time to clean up the notes.

Do you have pinouts for the CYW43 interface on your board in the SDK develop branch yet?

Will need to add back once full radio support is added, see earlephilhower#2605
@earlephilhower
Copy link
Owner

#2608 for the tracking issue...

@sfe-SparkFro
Copy link
Contributor Author

Great, thanks so much! I've updated this PR to remove the radio stuff. Can open a new PR against your development branch for #2608 once that's ready.

Do you have pinouts for the CYW43 interface on your board in the SDK develop branch yet?

See here: raspberrypi/pico-sdk#2038 This board uses the same pinout for the radio as the Pico W.

@earlephilhower
Copy link
Owner

Looks like we're passing. Good to merge this one?

@sfe-SparkFro
Copy link
Contributor Author

Yep, good with me! Thank you!

@earlephilhower earlephilhower merged commit 17aab7e into earlephilhower:master Nov 14, 2024
26 checks passed
@sfe-SparkFro sfe-SparkFro deleted the add_sparkfun_thingplus_rp2350 branch December 4, 2024 22:06
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.

2 participants