Skip to content

PR for Zephyr binding.#100

Open
drensber wants to merge 1 commit intogeorgerobotics:mainfrom
beechwoods-software:zephyr_build_pr
Open

PR for Zephyr binding.#100
drensber wants to merge 1 commit intogeorgerobotics:mainfrom
beechwoods-software:zephyr_build_pr

Conversation

@drensber
Copy link
Copy Markdown
Contributor

Hi, I suspect there will be a bit of back and forth here, but it has to start somewhere. This is my code to integrate the driver with the Zephyr OS and Zephyr's native IP stack.

The file zephyr_build/README.md explains how to do an out-of-tree build that will produce a Zephyr image with WiFi support. The example "app" is pretty minimal... it really just demonstrates which configuration items need to be in the prj.conf file to include the zephyr driver. Most interaction with the driver can be accomplished through Zephyr's network shell CLI.

We may want to consider maintaining this as a separate repository that pulls in georerobotics/cyw43-driver as a git submodule rather than as a subdirectory in main driver repository. That would be a little bit cleaner from a Zephyr perspective (it's the way that external Zephyr modules with out-of-tree builds are normally managed), but maintaining it as a subdirectory works too.

It currently only supports Raspberry Pi Pico W hardware, but the README in the zephyr_build directory explains how to provide support for other boards if someone wants to (although there may be limited interest in this, since Infineon has already provided their own driver to the Zephyr Project for boards that can use a regular SDIO interface).

I realize you'll need some time to review and try this, but please let me know as soon as you can if this at least looks good at a high level.

@dpgeorge
Copy link
Copy Markdown

Thanks for this! I'll have to take a good look.

Regarding the license: did you pick Apache 2 to match Zephyr, or because you took files from Zephyr that were already Apache 2? In other words, could it be licensed as MIT instead?

@drensber
Copy link
Copy Markdown
Contributor Author

drensber commented Nov 29, 2023 via email

@dpgeorge
Copy link
Copy Markdown

OK, thanks for the explanation. In principal Apache 2 is fine, but MIT is a bit simpler (although licensing is a tricky thing and "simple" is subjective!).

@peterharperuk
Copy link
Copy Markdown
Collaborator

We may want to consider maintaining this as a separate repository

Yes please. I don't think users of cyw43-driver will want all that stuff?

@dpgeorge
Copy link
Copy Markdown

Yes please. I don't think users of cyw43-driver will want all that stuff?

@peterharperuk do you have any specific concerns about merging this PR to this repo? There's no decision about it yet, and would be good to hear opinions. Text files are small and having them in a separate directory shouldn't affect other users of this driver.

@peterharperuk
Copy link
Copy Markdown
Collaborator

My comment was just referring to having to have a "zephyr_build" folder in this repo which just looks a bit odd. pico-sdk has a pico_cyw43_driver component and I wouldn't expect to add that to this repo. Having said that - I've not looked in detail at it.

@drensber
Copy link
Copy Markdown
Contributor Author

My comment was just referring to having to have a "zephyr_build" folder in this repo which just looks a bit odd. pico-sdk has a pico_cyw43_driver component and I wouldn't expect to add that to this repo. Having said that - I've not looked in detail at it.

Well, I was originally thinking of it as an alternative to LWIP as the IP stack (and the LWIP interface /is/ in cyw43-driver). ...but this is more than just an IP stack (it's an OS binding too) and it's much bigger than lwip.c. The code I've submitted here /will/ get somewhat smaller, because it currently carries some things that just aren't merged into the zephyrproject mainline yet, but it will always be at least a dozen files, I think. If you guys would be willing to create a separate repository for the Zephyr support, I think that would be great. I do think it would be beneficial if it's part of /your/ GitHub rather than Beechwoods, because it is a companion to cyw43-driver, and I think people would be much more likely to find it and use it if it's part of your GitHub.

@peterharperuk
Copy link
Copy Markdown
Collaborator

I just see this as a driver for some infineon wifi chips. Really lwip support should not be in here either. It does look like it should live elsewhere but that's just my opinion.

@drensber
Copy link
Copy Markdown
Contributor Author

I just see this as a driver for some infineon wifi chips. Really lwip support should not be in here either. It does look like it should live elsewhere but that's just my opinion.

I agree. When I was doing this Zephyr port, it was really a pleasant surprise to see how well you'd separated the generic driver from the parts that were specific to the RPi SDK or the IP stack (even though LWIP is included in the repo, it can be configured completely out). It was really great to find that I could integrate this driver with Zephyr without modifying any of the core cyw43-driver code at all. I think that separation is important to maintain, which was why I put the Zephyr specific stuff all in a separate directory, but a separate repository would probably be even better.

@drensber
Copy link
Copy Markdown
Contributor Author

I talked the licensing issue over with some people during our internal meeting today. We're perfectly fine with MIT license if that's what you prefer.

@drensber
Copy link
Copy Markdown
Contributor Author

We've published the Zephyr binding for your driver on Beechwoods's GitHub. It just pulls your driver in as a submodule. We'd still be happy to publish this on your GitHub instead if you'd like.

Our repo is: https://github.com/beechwoods-software/zephyr-cyw43-driver.git

I guess this PR can be closed, since I think that we all agree that adding it into the cyw43-driver repo doesn't make sense.

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.

3 participants