-
Notifications
You must be signed in to change notification settings - Fork 2.1k
CPU/Boards: Add Raspberry Pi Pico 2 (RP2350) support #21545
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
base: master
Are you sure you want to change the base?
Conversation
Also Murdock is "failing" because of the example that will not be merged, please ignore that 😛 |
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.
Some mostly minor style comments. Speaking of comments, a lot of comments are still in the //
style, which we don't use: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#comments
Some defines and functions are not documented yet, but I'll have to review what Doxygen complains about. Also I have not checked the generated Doxygen documentation yet.
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 think RIOT already has a library of atomic commands. Perhaps @maribu knows more about that?
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 did not know that this was something universal, the more you know
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.
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.
No, I had this in mind: https://github.com/RIOT-OS/RIOT/tree/master/sys/include/atomic_utils.h
Sorry for not posting the link initially.
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 wont lie, that header is a bit overwhelming and complicated 😰 Is it something where you still need to write the functions for or is this just something that works out of the box.
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.
It will work out of the box and provides atomic access, assuming all (potentially) concurrent accesses go via those functions.
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.
Here is the doc btw: https://doc.riot-os.org/group__sys__atomic__utils.html
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.
If I understand this correctly this would mean Id also have to adjust the shared UART code in the followup so Id rather avoid it and simply use the same method signatures that the rp2040 uses for atomic operations
After investigating the feasibility of using shared components such as UART I have decided that it probably makes more sense to limit the scope of this PR. (Also mentioned here: #21545 (comment)) The idea of this PR is to add minimal support, and then later in a followup PR add support for the shared components such as the RP2040 UART Driver. Doing this in this PR would most likely heavily impact reviewability, which I'd like to avoid, esp. when moving to a shared component would require major adjustments to some parts of the code. |
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.
Again, mostly small stuff.
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.
No, I had this in mind: https://github.com/RIOT-OS/RIOT/tree/master/sys/include/atomic_utils.h
Sorry for not posting the link initially.
Please also check the Doxygen warnings/errors from the Static Test. Now that the Doxygen headers are included, Doxygen actually evaluates many of the headers which previously weren't. |
I removed the example from the PR, for historic context, this was the example: https://github.com/RIOT-OS/RIOT/blob/4d7cc7dd9aa31ca371a1f627f98f24dfd4bc32ce/examples/basic/minimal/main.c |
You can also run the static tests locally, you know? :D |
Hey, just as a small status update, no pressure, is there anything that I still need to do for this PR? |
Co-authored-by: crasbe <[email protected]>
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.
Hey, just as a small status update, no pressure, is there anything that I still need to do for this PR?
Probably not much, but I don't have much time and no hardware yet for testing.
Thank you for the reviews (as always 😄) |
#include <stdbool.h> | ||
#include <stdint.h> | ||
|
||
#include "RP2350.h" |
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.
Where does this file come from?
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.
Ah from the pico sdk. That is not pulled automatically.
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 know why that is...
The Makefile adds picosdk
to the USEPKG
, however picosdk
is not a package. It is stored in dist/tools
, along with picotool
. This was introduced in #21269.
Therefore the USEPKG
does not do anything.
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.
The best approach would probably to move picosdk
from dist/tools
to pkg
(in a separate PR, as that might also require changes to the picotool
Makefile).
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.
Okay it's not that easy apparently, but I don't have time to look into it right now.
|
|
||
## Flashing the Board | ||
|
||
The Raspberry Pi Pico 2 has a built-in bootloader that allows flashing via USB. However, you can also use OpenOCD for flashing the board. |
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.
can you add a few words on how the bootselect works?
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.
With that, do you mean how to get into the flashing state via the bootselect button?
Was this using picotool or using openocd? With picotool you might have to reinsert the pico 2 for the program to execute. The picosdk stuff is a bit unfortunate, will try to fix this in a separate PR as @crasbe suggested. |
That was using picotool. I tried reinserting the pico but without success. |
Man 😞 Will have to investigate |
Little status update, the two culprits appear to be:
|
Contribution description
This PR introduces the Raspberry Pi Pico 2 to Riot.
Currently the periph list is rather limited, it has GPIO and non-configurable extremely basic write only UART support. It does however, already configure the clocks using the XOSC and works well within the RIOT Ecosystem.
It supports OpenOCD, which I am currently using and (as introduced in #21269) Picotool for debugging/flashing. (And JLink though I did not test this)
One other big note is that we currently only support the Cortex M33 cores of the Pico 2, however, most of the code should reusable when adding support for the Pico 2s RISC-V cores.
There is still a lot to be done, the documentation is still "limited", however, I felt like it would make more sense to at least open the PR itself before continuing. Esp. to avoid potentially duplicated efforts from occurring.
I also took the liberty to already use #21515 for the license headers. I am not sure whether this is already the consensus.
Testing procedure
To showcase the current features I included a very minimal example, this is not intended to get merged but merely to showcase what is available as of now.
Issues/PRs references