boards: Adding board definition for rpi_pico_w #57437
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @beechwoods-software, and thank you very much for your first pull request to the Zephyr project!
A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊
|
Sorry, didn't mean to create this PR (yet). |
|
We're welcome your contributions. https://docs.zephyrproject.org/latest/hardware/porting/board_porting.html#multiple-board-revisions |
|
Thanks for the pointer. I hadn’t noticed the “multiple board revisions” section in the documentation yet (I’d really just modeled this after the two variants of the “teensy” board).
I hadn’t quite meant to submit this as a PR yet this morning (I’d just intended to sync up with the main branch and clicked the wrong button), but will do so soon.
—Dave
… On May 1, 2023, at 9:19 AM, 'Tokita, Hiroshi' via Beechwoods Github ***@***.***> wrote:
@beechwoods-software <https://github.com/beechwoods-software>
We're welcome your contributions.
At first glance, it looks good in general.
IMO, if you plan to commonize the setting with original pico, it is good to consider using "board revision".
https://docs.zephyrproject.org/latest/hardware/porting/board_porting.html#multiple-board-revisions
—
Reply to this email directly, view it on GitHub <#57437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMZDOKIAZDHM7L7HGHPTLB3XD6Z5LANCNFSM6AAAAAAXRYAE3Y>.
You are receiving this because you were mentioned.
|
Hi, I just tried using the "board revision" in my local build tree to differentiate between the original pico and the pico W. It works, but I'm concerned that "board revision" isn't really the right way to handle different families of boards like the non-wireless and wireless version of the Pico. It seems that "board revision" is really intended for serial versioning of hardware (i.e. where each version of a board is denoted by an incremental number or letter). In the case of the current pico, it would require me to choose a version code for the non-wireless Pico. I could just choose an arbitrary letter like "A" and then call the wireless version "W". I could then make "A" the default so that people could still build the original pico with "west build -p -b rpi_pico" and people who want to build the wireless version could probably figure out that they need to do "west build -p -b rpi_pico@W". That would make sense for now, but what would happen when new revisions of each of these boards come out? My understanding is that you must either choose a numbering or a lettering version scheme (you can't combine the two schemes), so for future versions of the Pico I think there would be no choice other than to start naming them after arbitrary letters (maybe "B" and "X"?). Am I missing something here? Is there a way to combine numbering and lettering in the "board revision" style of versioning? It seems like a lot of the other families of boards that I see in the current Zephyr codebase just defined entirely new names for different categories of boards that are in the same family of boards like I did in the changeset that is associated with this PR. |
|
@beechwoods-software Maybe a better approach would be having a common base board that the two boards inherit from. An example of this is the |
Thanks. I'll look at that example. |
|
OK, I think what I had at first is pretty similar to how efr32_radio did this (although I'd originally used the teensy variants as a model). I'm going to re-open this is a submission with a few additional changes pushed (I abstracted the common parts of the dts into a dtsi and updated documentation). |
There was a problem hiding this comment.
Firstly, please clean up your commit log with rebase -i.
(Simply says, remove merge commits.)
And you need to update the description of this PR.
If PR contains only one commit, you can use the same description with the commit message. If not so, please summarize what changes with these commits.
beechwoods-software
changed the title
|
OK, I think I have the issues with the merge comments resolved and I've updated the PR description and also put my name on the one new file. |
drensber
force-pushed
the
rpi_pico_w
branch
2 times, most recently
from
bb79e47 to
6f7067b
Compare
|
offtopic: Please don't "Resolve conversation" button. Reviewers usually use it to remember "What I pointed out". |
Do I have the ability to do that? I'm not seeing that option anywhere. |
|
There was still one compliance issue with the commit comment. I pushed that fix and all of the automated tests passed, so hopefully we're finally good to go? |
|
@beechwoods-software |
Is the indentation what it's actually complaining about? I'll have to look into how to edit long commit messages like that. When I needed to rebase, it let me do it in a text editor, but when I was just fixing the commit message I had to do it on the command line and I guess it just inserted indentations for some reason. |
Sorry, It is my misjudgment. Following command to fix message. You will get open editor, change |
Where did the "7a946450e" come from? ...and also, what do I do after that? I don't understand the state that it left my repository in after I did that command. I tried to do a push and it first said "you're not on a branch", but when I checked out the rpi_pico_w branch, it seemed to undo my commit message edit. |
This is the hash of the one before commit from commit that do in this PR.
Oh, I forget last command. |
OK... I just pushed again. We'll see if it likes it this time. |
|
Is the compliance check still complaining about the current push? Does it want the entire comment to be all on one line? |
|
Second line must be blank line. |
|
OK, All one line with a blank line after. Just pushed. |
Actually I don't think it's going to like that one either. I just reviewed the guidelines again and I think I see how they want it. I'll try again. |
|
Actually, I still don't see how to do this with rebase. Do I need to put a "reword " in front of every line? ...or was I on the right track when I just made it all one line? |
|
OK, it looks kind of funny to me all on one line, but it passed, so I'm absolutely fine with that! |
one line must be under 75 characters. Don't put into one line! Please check CI error message and https://docs.zephyrproject.org/latest/contribute/guidelines.html#commit-guidelines. |
If I do multiple lines, then git rebase complains (says "error: invalid line" for every line except the first). I understand the guidelines, I don't understand how to make the comment conform to them using git. |
Effort has been made to abstract the features that are common to both the Pico and Pico W into a shared file when possible. This commit does not include the addition of bluetooth or wifi drivers for the W's Infineon module. Signed-off-by: Dave Rensberger <[email protected]>
|
OK, it seems that the problem was that I was changing the message as part of the reword command, whereas I was supposed to wait for the secondary editor (the one in the command is "just for reference" and actually gets ignored). https://stackoverflow.com/questions/50494713/how-can-i-insert-line-breaks-to-reworded-commit-messages-during-git-interactive Git's user interface truly is an abomination, but it's what the world has chosen. ;) Hopefully this one works! |
|
Great job! |
|
@yonsch |
Thanks for all your help. I'll try to help make sure that my Beechwoods colleagues make sure to do the initial PR from an account that has the "Author" set up correctly, because my experience here is that one is rather painful to correct. |
Great! It is giant step for human beings! 👍 |
This is a barebones implementation of the board definition for the Raspberry Pi
Pico W. Effort has been made to abstract the features that are common to both
the Pico and Pico W into a shared file when possible. This commit does not
include the addition of bluetooth or wifi drivers for the W's Infineon
module.