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

Fix compilation for PICO_BOARD=pico_w when cyw43_arch is missing #525

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

lurch
Copy link
Contributor

@lurch lurch commented Aug 22, 2024

Currently, if you try to build all of pico-examples for the Pico W and you haven't yet init-ed the cyw43-driver submodule in pico-sdk, then the build of pico-examples will halt with a fatal build-error. This PR fixes that 🙂

I'm not very familiar with CMake though, so perhaps there's a "neater" way of doing what I'm doing here?

@kilograham
Copy link
Contributor

pretty sure this is supposed to work so @peterharperuk may want to revist this

@peterharperuk
Copy link
Contributor

peterharperuk commented Aug 22, 2024

If ${PICO_CYW43_DRIVER_PATH}/${CYW43_DRIVER_TEST_FILE} is missing then we don't add the cyw43_driver_picow library. If cyw43_driver_picow doesn't exist we don't add the pico_cyw43_arch library. We have this guarding the pico_w examples, so I guess we should have it here as well.

if (PICO_CYW43_SUPPORTED) # set by PICO_BOARD=pico_w
if (NOT TARGET pico_cyw43_arch)
message("Skipping Pico W examples as support is not available")
else()

But having said that. I don't like the change. It looks brittle having the repetition in the cmake files. I'd rather just fix the examples to do the right thing.

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -43,7 +43,11 @@ function(add_subdirectory_exclude_platforms NAME)
endfunction()

# Add blink example
add_subdirectory_exclude_platforms(blink)
if (PICO_CYW43_SUPPORTED AND NOT TARGET pico_cyw43_arch)
message("Wi-Fi driver is not available")
Copy link
Contributor

Choose a reason for hiding this comment

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

I had meant "Skipping blink example as Wi-Fi driver is not available"

Can you also move this check into the blink CMakeLists.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you also move this check into the blink CMakeLists.txt

By the time we're "in" the blink CMakeLists.txt, is it not then too late to call add_subdirectory_exclude_platforms(blink) ?

@@ -5,7 +5,11 @@ if (TARGET hardware_adc)
add_subdirectory_exclude_platforms(joystick_display)
add_subdirectory_exclude_platforms(onboard_temperature)
add_subdirectory_exclude_platforms(microphone_adc)
add_subdirectory_exclude_platforms(read_vsys)
if (PICO_CYW43_SUPPORTED AND NOT TARGET pico_cyw43_arch)
message("Skipping read_vsys for Pico W as support is not available")
Copy link
Contributor

Choose a reason for hiding this comment

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

and fix this up to be "as Wi Fi driver is not available"

@seamusdemora
Copy link

just a "throw-away" comment:
Not sure what all the comments were about, but as someone who got "tripped up" with the cyw43 thing, I hope this issue has been fixed - and thanks!

@lurch
Copy link
Contributor Author

lurch commented Sep 4, 2024

as someone who got "tripped up" with the cyw43 thing, I hope this issue has been fixed - and thanks!

The issue will be fixed after this PR is merged, but we're still discussing the finer details 🙂

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