-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add a nice wrapper for get_sys_info
#877
Conversation
…er than copy pasted constant names from the datasheet
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.
Thank you for this PR. In general it looks great!
I would like to see an example of using this API though.
Sure! And I'll go ahead make the suggestions changes too. I'll probably do that by tonight (us time). Do you want the example to be added to the existing rom function example? And if so do you want it to replace the existing one? |
I'm ok with it going into the existing example binary. I'm not sure if we need to leave an example of the old API or if we expect people to only use the new one. |
I also went ahead and added some more documentation. It probably could use more work cause I was in a bit of a rush but it should make things easier to read. I also elaborated on the existing examples since we don't have settle for printing the hex representation of the bytes get_sys_info gives back now. |
_ = writeln!(uart, "get_sys_info(CPU_INFO/0x0004) -> {}", result); | ||
_ = writeln!(uart, "\tSupported Flags: {:#06x}", buffer[0]); | ||
let result = hal::rom_data::sys_info_api::cpu_info(); | ||
let result = match result { |
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.
These nested matches can be collapsed.
I'll get working on those changes. It just occurred to me that I didn't add an api for |
I held off on adding the ns version of the api for now but its ready (https://github.com/nahla-nee/rp-hal/tree/feature/get_sys_info_ns_api) for a pr whenever if you did want it included as well |
I guess it would be good to have that, although I haven't written much NS software for the 2350. Not a blocker for this PR though. |
Looks great! Thank you. |
ah, needs a |
I went ahead and merged the NS branch into my current branch. Hopefully that's not an issue, I glanced at the notification I got about your message which cut off the part about it not being a blocker. And cargo fmt has been ran 😅 forgot i made changes to the examples folder and only ran it in the hal folder |
Before:
After:
Looks great! |
Implements a nice safe wrapper for
get_sys_info
as described in #839 .