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

feat: add logic to hardware/rp2040/prebuild.cmake to properly add cyw… #9

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

Kintar
Copy link
Contributor

@Kintar Kintar commented Jun 5, 2024

Setting the board to "pico_w" in project.cmake as directed in the README does not add the necessary libraries to utilize the cyw43 wifi chip on the pico_w. This PR updates the hardware/rp2040/prebuild.cmake to conditionally add pico_cyw43_arch_none to the hardware_libs CMAKE variable, and to add a project define "WITH_CYW43" to enable conditional compilation of -- for example -- onboard LED code, which does not function on the pico_w due to the change in GPIO pinouts for the wireless module.

@Kintar
Copy link
Contributor Author

Kintar commented Jun 5, 2024

Forgot to rebase to your latest changes before opening the PR. 🤦 Fixed that.

Copy link
Contributor

@biot biot left a comment

Choose a reason for hiding this comment

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

You can actually just add pico_cyw43_arch_none to hardware_libs, which at this point is a LIST variable. I found this page helpful.

@Kintar
Copy link
Contributor Author

Kintar commented Jun 5, 2024

You can actually just add pico_cyw43_arch_none to hardware_libs, which at this point is a LIST variable. I found this page helpful.

Yes...that's what this pull does, it automatically adds that library to the hardware_libs list automatically if the board is set to pico_w.

@mcknly mcknly self-requested a review June 6, 2024 14:42
@mcknly
Copy link
Owner

mcknly commented Jun 17, 2024

@Kintar would you still like to get this merged in? Just check my comments above. Would love to see what else you come up with for the wireless module on Pico in the future too.

@Kintar
Copy link
Contributor Author

Kintar commented Jun 20, 2024

@Kintar would you still like to get this merged in? Just check my comments above. Would love to see what else you come up with for the wireless module on Pico in the future too.

@mcknly , yes I would, but I don't see any comments from you other than the automated GitHub notification that you self-requested a review, and this comment that I'm replying to. Admittedly, I've been using BitBucket for the past 8 years at work, so I might be out of practice with GitHub, but I didn't think I'd forgotten THAT much! What am I missing? :)

@mcknly
Copy link
Owner

mcknly commented Jun 20, 2024

Ah, maybe it looks different from my dashboard, I thought the comments were visible inline in this thread but maybe they are "review comments" in a different view. Does this work? I just requested a couple of minimal changes before I merge.

@mcknly
Copy link
Owner

mcknly commented Jun 20, 2024

Sorry the link disappeared

@Kintar
Copy link
Contributor Author

Kintar commented Jun 20, 2024

Sorry the link disappeared

@mcknly : That link just takes me back to this comment page. I at least see a "view reviewed changes" button for biot's comment, above, but I don't see any links to show me the files you've reviewed or any recommended changes. In fact, the final part of the page after "add more commets by..." is showing you as a PENDING reviewer. Maybe you need to take an additional action before I can see the changes you requested, since I'm not an actual member of this project?

@mcknly
Copy link
Owner

mcknly commented Jun 20, 2024

Okay seems like maybe a permissions issue then. Thanks for letting me know. Basically just rebase and remove line 11 - here's a screenshot of my comments:
Screenshot_20240620-154427.png

@Kintar
Copy link
Contributor Author

Kintar commented Jun 20, 2024

I think that big green "Finish Your Review" button is why I can't see the comments. :D But I've made the requested changes now.

hardware/rp2040/prebuild.cmake Outdated Show resolved Hide resolved
@mcknly mcknly merged commit c27aa27 into mcknly:main Jun 21, 2024
@mcknly
Copy link
Owner

mcknly commented Jun 21, 2024

Seems like I need some GH training too! Yeah I guess I need to "finish review" before any of it is visible. Sorry for the confusion. Everything looks good with the cyw43 driver build on my end, merged.

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.

None yet

3 participants