Raspberry Pi Pico smorgasboard of bugfixes and improvements #80707
Conversation
| @@ -11,4 +11,6 @@ include: raspberrypi,pico-clock.yaml | |||
| properties: | |||
| startup-delay-multiplier: | |||
| type: int | |||
| description: Startup delay multiplier | |||
| default: 1 | |||
There was a problem hiding this comment.
Since this is set to 1 here, I would suggest removing
so the default is consistent across RP2040 and RP2350.
There was a problem hiding this comment.
@ajf58 can you please address this comment.
Either by following the suggestion or reply why that's not a good idea in this case.
There was a problem hiding this comment.
Either by following the suggestion or reply why that's not a good idea in this case.
@xudongzheng was the original author of this change, so they're kinda arguing with their past self here, with me as a proxy. So the full suite of options are:
- Leave as I've done. This will not change the behaviour of the in-tree RP2040-based boards. This, implicitly, has been approved many many times over as part of another PR.
- Only remove
startup-delay-multiplierfrom rp2040.dtsi. This will effectively revert what @xudongzheng added in the first place. - Remove
startup-delay-multiplierfrom rp2040.dtsi and only add it to the in-tree boards where it is known to be needed, i.e. cross match against https://github.com/raspberrypi/pico-sdk/tree/master/src/boards/include/boards . This, IMHO is arguably what the original PR that introduced the delay should have done: it fixes things that are known to need this, and doesn't alter out-of-tree boards' behaviour.
There was a problem hiding this comment.
@xudongzheng was the original author of this change, so they're kinda arguing with their past self here,
Ever written code where you later discover this should probably have been done differently or at another location ?
I have done that many times.
Of course I cannot answer for @xudongzheng , but to me this looks like such a case, and when this PR broadly states improvements, then this seems to be one such improvement which can be taken in.
Though I agree it more seems like something which should be a regular comment, and not a change request.
Remove startup-delay-multiplier from rp2040.dtsi and only add it to the in-tree boards where it is known to be needed, i.e. cross match against https://github.com/raspberrypi/pico-sdk/tree/master/src/boards/include/boards . This, IMHO is arguably what the original PR that introduced the delay should have done: it fixes things that are known to need this, and doesn't alter out-of-tree boards' behaviour.
To me this also sounds like the ideal approach.
And as PR author you are free to decide if you want to include this change or not.
But thanks for replying to the comment, as that is the only way for others to know if it has been seen / considered.
There was a problem hiding this comment.
you are free to decide if you want to include this change or not.
I'm not going to be extending the scope of this change beyond what it currently does, i.e. "prevent out-of-tree boards based that use this DTS binding requiring to define a parameter."
It's a remarkable coincidence that in the "future" (subsequent commits)[1] it means that if another SoC uses this same driver it won't have to keep defining something to be equal to one. Someone should make one of those and put it on some boards... maybe get a PR to put them in-tree.
The value is board, not SoC specific, and really shouldn't be in rp2040.dtsi whatsoever. @xudongzheng , perhaps you can raise an issue referencing back to your own PR with these suggestions? I don't have time to do that, nor to action it.
[1] or ~weeks in the past for those humans who measure their life like this.
There was a problem hiding this comment.
For the author (me), it's less handle turning.
and for the reviewers ?
So whenever you push changes, do I then have to go through all commits again to see if my comments are now addressed or which other changes you have introduced in the meantime which I might or might not approve ?
While there can be reasons for having multiple commits in one PR, then if those commits are completely orthogonal to each other then please consider opening independent PRs.
Or do not open the PR until you believe all the changes are ready.
Please also take a look at the contribution guideline:
https://docs.zephyrproject.org/latest/contribute/guidelines.html#contribution-workflow
One general practice we encourage, is to make small, controlled changes. This practice simplifies review, makes merging and rebasing easier, and keeps the change history clear and clean.
I can't speak for everyone, but advantages that I can see are that Triage role people don't need to triage 7 PRs, reviewers can (if they wish) see one family of changes.
Please correct me if I'm wrong, but I don't think that any of the files modified in this draft PR are authored by you, nor are you a maintainer or assignee, so I feel like the very short answer is there's no obligation for you to review this.
I think yes, this is almost an inevitable side-effect of publicly reviewed PRs. It certainly happens that this situation occurs:
I feel that 8 commits pertaining to a single common SoC doesn't impose too much burden on the reviewer. Regardless of my opinion, if you disagree and will actively block/NACK this PR when it moves from Draft status, then let's have a concrete proposal, rather than a somewhat open-to-misunderstanding "do it different" How about if I create 7 (if my maths is correct) PRs (can't add 0954e44 without the preceding commit, [or if I do then the test will run and fail]). |
I'm maintainer of the build system, and author of HWMv2, so any PR which are working with SoCs and boards might have my interest. Especially PRs which are follow-ups on other PRs I have reviewed. Now, for this specific PR, I can't tell if one, two, three, or all commits in this PR will have my interest, but more commits added in a single PR increases the chance that I have to review the whole PR. If those are individual PRs, then it could be that most of those individual PRs will not have my interest, thus save me overall time. |
zephyrbot
added
area: Clock Control
area: DMA
zephyrbot
requested review from
decsny,
MaureenHelm,
nashif,
nordic-krch,
stephanosio and
teburd
This is fine, though probably not in your best interest if you want to slipstream the review, the system is going to tag the PR against multiple subsystems, notify all maintainers and probably assign it incorrectly. Then the reviewer see a PR touching multiple things and are naturally inclined to push it forward and wait for someone else to do an initial pass, then wait for the comment to be addressed and do their own part, which is exactly what you see here. Plus, I mean, the entire PR comment is basically acknowledging that you are trading your time for the reviewers one, how does you expect the other side to react? That conversation does not even belong to here, just leave a summary of the changes and let the reviewers go through them, if the PR spans across multiple subsystems it will need multiple rounds, if it's a new SoC this is inevitable and just part of the process. Anyway, folks, please let's focus on the change itself and get past the hump. |
|
Thanks for approving the PR, @fabiobaltieri . I asked if should split this into 7 PRs. I waited... 10 days, got no response, so moved this from draft to review ready. If the maintainers want/mandate that this is split into 7 PRs, please politely and clearly ask me to do so.
Thanks, but no, I'll exercise the Right Of Reply. I expect reviewers to respond as requested here. Sincerely, given that the content of these commits has repeatedly been approved and reviewed as part of another PR, I thought reviewers would think "yes, saving ~70 hours of CI compute time is pragmatic good use of time/compute resource. This is a small bunch of descriptive commits related to a single SoC, some of these commits are literally just whitespace changes, easy peasy, let's get some bugs fixed on
https://github.com/zephyrproject-rtos/zephyr/pull/80707/commits I can't think of anything better than the one line summary of each commit message. If the commit messages are confusing/misleading and reviewers have actionable feedback on that, then please do let me know. |
|
The PR stands up alright in my opinion, grouping by platform is as good as grouping by subsystem, either way both platform and subsystem maintainers have a voice on it, just give people time to go over it, we are in hard freeze it won't go until the release is cut anyway. Don't worry about CI compute time, that's a more systematic problem that needs to be solved at project level. Unless you actively want to work on the more systematic problem of project level CI compute time, Then, please, worry about it. |
People asked you to do a proper cleanup of your PR in #77368 and you decided to fight that request instead of following the proposed cleanup. I gave you a bit of advice in #80707 (review) but what you do with that advice is purely up to you. I'm not going to tell you what you should or should not do, but i'm going to tell you if I can approve a PR or if I can't and in that case also give you an explanation. One problem I have when looking at the PR description, is that I cannot judge if more commits will be added later when you find more bug to fix. This again results in an open-ended PR (when will more commits be added ?) and therefore I wait with further review until the PR seems stable (which it appears to be now). |
CODEOWNERS
Outdated
| @@ -422,7 +422,7 @@ | |||
| /dts/arm/nxp/ @mmahadevan108 @dleach02 | |||
| /dts/arm/nxp/nxp_s32* @manuargue | |||
| /dts/arm/microchip/ @franciscomunoz @albertofloyd @sjvasanth1 | |||
| /dts/arm/rpi_pico/ @yonsch | |||
| /dts/arm/raspberry/rpi_pico/ @yonsch | |||
There was a problem hiding this comment.
Wrong vendor prefix:
| /dts/arm/raspberry/rpi_pico/ @yonsch | |
| /dts/arm/raspberrypi/rpi_pico/ @yonsch |
There was a problem hiding this comment.
I would just drop this change altogether. See #82230
There was a problem hiding this comment.
My point was that the CODEOWNERS file is plain stale anyway and full of incorrect entries (it wasn't even covered with hwmv2 migration and nobody noticed/bothered... 8dc3f85#diff-fcf14c4b7b34fe7a11916195871ae66a59be87a395f28db73e345ebdc828085b). Dropping this change is effectively a simple way to avoid a possible future merge conflict IMO.
BTW if the change stays (which I really recommend it doesn't), why is the code owner not being updated to @soburi?
There was a problem hiding this comment.
Dropping this change is effectively a simple way to avoid a possible future merge conflict IMO.
Agreed. We should just wait the ~48 hours for #82230 . Absolute worse case scenario would be a known-to-be-wrong file is made a tiny bit more wrong, before being deleted.
if the change stays (which I really recommend it doesn't), why is the code owner not being updated to @soburi?
My assumption was the file was correct at the point I changed it, which it turns out was not the case, as you've pointed out.
|
Just a comment, This has already been explored in detail in #77368, and is in fact a collection of parts that can be rebased and extracted without being related to rp2350. |
@soburi now is it ? @ajf58 is now having two PRs opened, this one with 8 commits and #77368 with 28 commits, 8 of those being the same commits in both PRs. I cannot judge whether I should put my comments in this PR or in the other PR. I currently have on github (and there are many people in Zephyr assigned even more PRs): Properly crafted PRs and logical splitted PRs makes reviewers life so much easier. This is what I tried to explain to @ajf58 in my comment: #80707 (review) An example of two PRs, where one is a cleanup preparing for the work of the second, meaning the second depends on the first.: So that once #41301 got merged, then #40555 could be rebased and the Pros:
You may say that this PR as it stands today, makes it ready for review, and thus has the potential for approval, only outstanding comment is the CODEOWNERS comment, and that has only been addressed in #77368. But at the time I made the #80707 (review) comment, then the direction of this PR was not clear at all. |
|
I understand the rationality of what you say, but since it is a "recommendation" in the guidelines, I do not think it is completely legitimate. Regarding the background of this PR, I think that it is an excerpt from #77368 where the discussion has already converged to a certain extent, and that should be taken into consideration. I think that the point you pointed out should be pointed out as a carelessness on the part of reviewers, including myself. It is true that there may be some parts that are not explained well, but it is impossible for those outside of the inner circle (which can be rephrased as full-time contributors belonging to the sponsoring company of this project) to understand the degree of the binding force of the "recommendation" in the guidelines. It is too highly contextual. If that is the case, the guideline should be revised accordingly, and I think that @ajf58's current response is sufficiently rational. |
True it's only recommendation, which is also the reason i'm not blocking PRs on this ground, and i'm not blocking #77368. Reason I blocked this PR when it was still draft was due to how it was open-ended, and I wanted to avoid a situation where new changes are added later without proper review. It sometimes happens that when enough reviewers has approved then the PR gets merged and I wanted to avoid that situation for this PR, due to above.
This PR description just states:
It doesn't state whether content of this PR is already discussed elsewhere. (@soburi as assignee of the other PR you probably have followed the discussion in #77368 more closely than me)
True, it's only recommendations.
Minimizing the time spent overall by n-number of persons is imo more important then the time spent of a single individual. This is also in line with the Contributor Expectations:
Currently this PR is not nack'ed because of its structure or discussions like this comment but because #80707 (comment) haven't been addressed. |
@tejlmand , please reach out to me on Discord in the raspberrypi channel, I feel it's a better place for this kind of meta-chat. In the meantime, unless I click "re-request review", then there's no request/requirement/obligation from me to you for spending any more time on any PR I've raised. Thanks for all your time to date. Edit: various typos. |
Follow the wider directory convention of dts/<arch>/<vendor>/<family>. This is foundation work ahead of introducing support for the RP2350. Signed-off-by: Andrew Featherstone <[email protected]>
Rename rpi_pico_common.dtsi to rp2040_reset.h . This is more consistent with the wider Zephyr source tree, and is foundation work ahead of introducing the RP2350 SoC. Add missing include guard. This shouldn't be required, but it is consistent with other header files in the same directory. Signed-off-by: Andrew Featherstone <[email protected]>
RESETS_RESET_PLL_USB_BITS was logically or'd twice and 'unreset'ting PWM doesn't seem to be required, based on the contents of the SDK. Signed-off-by: Andrew Featherstone <[email protected]>
The Pico SDK defines a default value for its XOSC multiplier. Reflect this in the device tree binding so that it doesn't need to be repeated. Signed-off-by: Andrew Featherstone <[email protected]>
No in-tree board uses this driver's pinctrl functionality, and every RP2040-based board was configuring this to be an empty node in the device tree, so remove them. Signed-off-by: Andrew Featherstone <[email protected]>
Change whitespace to match the coding style for CMake files for all rp2040-based boards. This is foundation work ahead of adding support for boards based on the RP235XX SoCs. Signed-off-by: Andrew Featherstone <[email protected]>
From the API documentation, `dma_api_chan_filter`` can be given a value of NULL for `filter_param`. Match the behaviour of most implementations, and return true. This removes misleading error messages logged during tests (e.g. `test_tst_dma0_m2m_loop`). Signed-off-by: Andrew Featherstone <[email protected]>
Increase test coverage for Raspberry Pi's RP2040. Use the `socs` folder rather than `boards` to enable these tests to run on any boards with the same SoCs. Signed-off-by: Andrew Featherstone <[email protected]>
ajf58
dismissed stale reviews from kartben, soburi, and fabiobaltieri
via
6fce055
ajf58
force-pushed
the
bugfixes-for-rpi-pico
branch
from
0954e44
to
6fce055
Compare
Summary
The purpose of this PR is to get various bugfixes and enhancements found during development of #77368 approved an merged onto
main.They are, by design, entirely orthogonal to anything related to the Pico 2, RP2350 SoCs etc.
Why are there multiple bugfixes in a single PR?
At the extreme, almost every change in this PR could be a one commit : one PR. Creating a single PR has subjective advantages and disadvantages. For the author (me), it's less handle turning. For CI, one PR means less CI compute resource being used. This all works out ok because the project (quite rightly IMHO) doesn't try and automatically squash all commits in a PR.
Why not wait until #77368 is approved?
71298136232 drivers: dma: rpi_pico: Correct handling of NULL filter_param). Getting them ontomainfixes issues today, decoupling them from new features.