Display uf2 combine --offset default-value in hexadecimal - #353
Conversation
Co-authored-by: will-v-pi <108662275+will-v-pi@users.noreply.github.com>
| --minor <minor> | ||
| Add Minor Version | ||
| --rollback <rollback> [<rows>..] | ||
| Add Rollback Version |
There was a problem hiding this comment.
The rollback stuff is stored in OTP, right? So why is this using hex numbers when https://github.com/raspberrypi/picotool#setget appears to be using decimal numbers? (Or do the OTP docs also need to be updated to specify hexadecimal numbers? 🤷 )
There was a problem hiding this comment.
I'm not sure where the decimal row numbers are in the docs? Rows can be indexed by hex or decimal for set/get, but the output always prints hex:
$ picotool otp list 0x40
ROW 0x0040: OTP_DATA_CRIT1
"Page 1 critical boot flags (RBIT-8)"
field GLITCH_DETECTOR_SENS (bits 5-6)
field GLITCH_DETECTOR_ENABLE (bit 4)
field BOOT_ARCH (bit 3)
field DEBUG_DISABLE (bit 2)
field SECURE_DEBUG_DISABLE (bit 1)
field SECURE_BOOT_ENABLE (bit 0)
$ picotool otp list 64
ROW 0x0040: OTP_DATA_CRIT1
"Page 1 critical boot flags (RBIT-8)"
field GLITCH_DETECTOR_SENS (bits 5-6)
field GLITCH_DETECTOR_ENABLE (bit 4)
field BOOT_ARCH (bit 3)
field DEBUG_DISABLE (bit 2)
field SECURE_DEBUG_DISABLE (bit 1)
field SECURE_BOOT_ENABLE (bit 0
Same goes for PAGE:PAGE_ROW_NUMBER selectors - 1:0x11, 1:17, 0x1:17 etc all target the same row
This --rollback argument is using hex rows for simplicity, as the default rows are OTP_DATA_DEFAULT_BOOT_VERSION0_ROW=0x4e and OTP_DATA_DEFAULT_BOOT_VERSION1_ROW=0x51
There was a problem hiding this comment.
Rows can be indexed by hex or decimal
Ahhh okay, that's the bit I was missing. As it doesn't say "hexadecimal", that's why I was assuming it was decimal-only. But perhaps it's worth tweaking the OTP section of the docs to clarify that the row and page numbers can be specified in either decimal or hex?
There was a problem hiding this comment.
All regular integer arguments in picotool can be specified as hex or dec (or binary) (see integer::parse_string in cli.h, so I don't think that it is necessary to state that these selector fields also support hex or dec, given the output prints row numbers in hex anyway (for set/get/list and dump)
There was a problem hiding this comment.
Is that actually mentioned in the README, or do people need to read the source-code to discover that? 😉
There was a problem hiding this comment.
Not that I can see in the README, but anyway, I think that's a separate issue - this PR is about hex arguments which always treat values as hex (i.e. 1234 is treated as 0x1234), whereas this discussion is about integer arguments which are treated as decimal unless a 0x or 0b prefix. The hex ones are the only ones with non-default behaviour, hence being good to have explicit documentation, integer behaves like you would expect it to behave.
There was a problem hiding this comment.
Not that I can see in the README, but anyway, I think that's a separate issue
Haha, fair enough, sorry for going off-topic.
Possible fix for #352