-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 build issues when building for RP2040 from pico-examples/pico-sdk. #2151
Conversation
* include_guard requires GLOBAL as family.cmake is included in multiple non child places * the following recently added check is suprfluous (family_configure_host_example for rp2040 should do this already), and breaks if pico_pio_usb is not avaialble, so i have removed # Add pico-pio-usb for rp2040 since user can choose to run on bit-banging host if(FAMILY STREQUAL "rp2040") family_add_pico_pio_usb(${PROJECT}) endif() * added new familt_example_missing_dependency functino to print missing dependency warning, so pico-examples can override it to be less in your face, and also more contextual to pico-examples
oops, sorry, I am still getting used to cmake. Though you are right, I will try to include building a couple of usb examples from pico-examples into the ci as well. |
hw/bsp/family_support.cmake
Outdated
if (IPO_SUPPORTED) | ||
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION TRUE) | ||
endif () | ||
if (NOT TINYUSB_OPT_SKIP_CHECK_IPO_SUPPORTED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this is spot-on. Actualy I think we should skip this entirely for rp2040. LTO is global option, and it should only be set by top level application cmakelists.txt. It is ok for other ports since it only applies to examples within tinyusb. But I don't really want to force this on user application at all.
hmm, I pushed some update to your branch, but look like github doesn't pick that up. Will try to close and re-open to force an update PS: work like a charm :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skip the LTO option entirely for rp2040 since this should be decision by application top level cmake file. @kilograham let me know you are with the changes.
update: and PR building pico-examples will be added soon (after this one). CI will only build 2 usb example in the pico-examples repo dev_hid_composite and host_cdc_msc_hid (currently disabled due to usb host api changes). let me know if this is sufficient enough. tinyusb/.github/workflows/cmake_arm.yml Lines 92 to 109 in 2e7192c
|
LGTM - not sure if it is better to just have the job fail if there are any issues (e.g. the host API changing is a valid reason for failure), though we could add a second job for that - perhaps i'll add that on our end |
maybe you could update the pico-examples on dev branch, and we can use dev branch here. otheriwse a failed ci will prevent any merge on this repo. We could fix and enable it in follow up PR. Thank you. |
PS: let me know if you need to set up an hook when there is pushed to tinyusb for running ci on pico-examples. |
family_configure_host_example
on rp2040 should do this already), and breaks if pico_pio_usb is not avaialble, so i have removed the rp2040 checkfamily_example_missing_dependency
function to print missing dependency warning, so pico-examples can override it to be less in your face, and also more contextual to pico-examplesTINYUSB_OPT_SKIP_CHECK_IPO_SUPPORTED
var to skip the recently added IPO check which produces a bunch of warnings on RP2040note: we really should set up an action to build pico-sdk/pico-examples periodically - either on pico- end, or tinyusb- end... there will be times when it is genuinely broken, but it would be nice to catch these breaks.