Standardise usage of rom_get_sys_info function#3094
Conversation
RP2350 Datasheet says "You should always check (the first word in the returned buffer) before interpreting the buffer.", so do that. Also some small unique_id tidy-ups.
| uint32_t result[5]; | ||
| rom_get_sys_info_fn func = (rom_get_sys_info_fn) rom_func_lookup_inline(ROM_FUNC_GET_SYS_INFO); | ||
| if (5 == func(result, count_of(result), SYS_INFO_BOOT_RANDOM)) { | ||
| int words_returned = rom_get_sys_info(result, 5, SYS_INFO_BOOT_RANDOM); |
There was a problem hiding this comment.
I think using count_of might be clearer - and then the uses in bootrom.h could be updated to count_of too
| int words_returned = rom_get_sys_info(result, 5, SYS_INFO_BOOT_RANDOM); | |
| int words_returned = rom_get_sys_info(result, count_of(result), SYS_INFO_BOOT_RANDOM); |
There was a problem hiding this comment.
Okay, I can change all of these. Is it still appropriate for count_of to continue being used in the words_returned == count_of(result) check on the next line too? This would mean that the number 5 only appears in the uint32_t result[5] line. (Which would be fine with me, I'm always happy to see the elimination of duplicated magic-numbers; I'm just double-checking that this is what's desired.)
And in that case I guess it might make sense to fold #3089 into this PR too.
There was a problem hiding this comment.
I think it's still appropriate - I also agree it's nice to remove the magic numbers.
Re folding the PRs, I don't mind - you could make the changes to use count_of in the existing bootrom.h functions in that PR, or fold it into this one and do it here
There was a problem hiding this comment.
Taking that idea of removing magic-numbers even further, perhaps it would also make sense to add e.g.
#define SYS_INFO_BOOT_RANDOM_WORDS_RETURNED _u(4)to bootrom_constants.h, and then we could do
uint32_t result[SYS_INFO_BOOT_RANDOM_WORDS_RETURNED + 1];?
There was a problem hiding this comment.
Yeah, that sounds good too - for all the SYS_INFO_XXX defines having a corresponding SYS_INFO_XXX_WORDS_RETURNED rather than just having it in the comment above.
| uint32_t words[4]; | ||
| uint8_t bytes[4 * 4]; | ||
| } out; | ||
| int words_returned = rom_get_sys_info(out.words, 4, SYS_INFO_CHIP_INFO); |
There was a problem hiding this comment.
Ditto on count_of
| int words_returned = rom_get_sys_info(out.words, 4, SYS_INFO_CHIP_INFO); | |
| int words_returned = rom_get_sys_info(out.words, count_of(out.words), SYS_INFO_CHIP_INFO); |
2 participants
I copied the
rom_get_sys_infousage-pattern frombootrom.h.RP2350 Datasheet says "You should always check (the first word in the returned buffer) before interpreting the buffer.", so do that too.
Also some small unique_id tidy-ups.