-
Notifications
You must be signed in to change notification settings - Fork 905
Add a sample to read the partition table #627
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: develop
Are you sure you want to change the base?
Add a sample to read the partition table #627
Conversation
@oyama You might find some useful partition-related code in https://github.com/raspberrypi/picotool ? 🤷♂️ |
@lurch Thank you for the pointer! It looks like the relevant code is around here: https://github.com/raspberrypi/picotool/blob/de8ae5ac334e1126993f72a5c67949712fd1e1a4/bintool/metadata.h#L227 – I'll take a look. |
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.
A few comments from a first pass reviewing this
…tions and variable-length field sections
Based on the comments, the read process of the partition table was divided and organized into a fixed-length field portion and a variable-length field portion. |
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.
LGTM, just a few minor things
Co-authored-by: will-v-pi <[email protected]>
Co-authored-by: will-v-pi <[email protected]>
Co-authored-by: will-v-pi <[email protected]>
Co-authored-by: will-v-pi <[email protected]>
Co-authored-by: will-v-pi <[email protected]>
Co-authored-by: will-v-pi <[email protected]>
Co-authored-by: will-v-pi <[email protected]>
Co-authored-by: will-v-pi <[email protected]>
Thanks suggestions. Very helpful. |
#define PARTITION_LOCATION_AND_FLAGS_SIZE 2 | ||
#define PARTITION_ID_SIZE 2 | ||
#define PARTITION_NAME_MAX 127 |
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.
@will-v-pi Is it worth hoisting any of these defines into pico-sdk, or is it fine for each project to define them itself?
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'll probably be sensible to add them to picobin.h
, along with more partition table handling in bootrom.c
- @kilograham might have opinions too?
@oyama I'd just like to say thank you very much for putting up with all my niggly comments. I sometimes get a bit carried away when reviewing code 😆 |
@lurch Thank you so much for your detailed feedback, it was incredibly helpful and inspiring. I truly appreciate your attention to every little detail, and your comments have motivated me to improve further. Thanks again for your support! |
Now that you've added all the code to display extra family IDs, perhaps it's worth exercising that code by putting an extra family ID in your |
printf("un-partitioned_space: S(%s%s) NSBOOT(%s%s) NS(%s%s) uf2 { %s }\n", | ||
(pt.flags_and_permissions & PICOBIN_PARTITION_PERMISSION_S_R_BITS ? "r" : ""), | ||
(pt.flags_and_permissions & PICOBIN_PARTITION_PERMISSION_S_W_BITS ? "w" : ""), | ||
(pt.flags_and_permissions & PICOBIN_PARTITION_PERMISSION_NSBOOT_R_BITS ? "r" : ""), | ||
(pt.flags_and_permissions & PICOBIN_PARTITION_PERMISSION_NSBOOT_W_BITS ? "w" : ""), | ||
(pt.flags_and_permissions & PICOBIN_PARTITION_PERMISSION_NS_R_BITS ? "r" : ""), | ||
(pt.flags_and_permissions & PICOBIN_PARTITION_PERMISSION_NS_W_BITS ? "w" : ""), |
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.
Similarly to how you have a function (uf2_family_ids_join
) for creating the formatted family-ids string, I wonder if it might be worth pulling out a separate function for creating the S(%s%s) NSBOOT(%s%s) NS(%s%s)
part of this string? (which I believe you could then also use in the per-partition loop?) 🤔
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.
Haha, I was about to suggest that you could use %c
instead of %s
in your printf
formatting, but then I realised that you can't do "empty string" when using char
😂
Add a sample that reads the partition table from runtime.
I would appreciate comments on the partition table operation procedure itself, as I did not know the correct way to do it and coded it by trial and error.