-
Notifications
You must be signed in to change notification settings - Fork 226
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 examples to atmega-hal documentation #591
Conversation
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.
Very nice, thanks a lot!
let mut boot_count = eeprom.read_byte(BOOT_COUNT_OFFSET); | ||
boot_count = boot_count.wrapping_add(1); | ||
eeprom.write_byte(BOOT_COUNT_OFFSET, boot_count); | ||
|
||
ufmt::uwriteln!(&mut serial, "Boot count: {}", boot_count).unwrap(); |
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 minor thing, but because people like copying example code for production use, let's talk about it:
I think this should be using a saturating_add()
instead of wrapping_add()
so the value doesn't cycle back to zero after 256 boots. The print statement should then display a different message on overflow. E.g.
let mut boot_count = eeprom.read_byte(BOOT_COUNT_OFFSET); | |
boot_count = boot_count.wrapping_add(1); | |
eeprom.write_byte(BOOT_COUNT_OFFSET, boot_count); | |
ufmt::uwriteln!(&mut serial, "Boot count: {}", boot_count).unwrap(); | |
let mut boot_count = eeprom.read_byte(BOOT_COUNT_OFFSET); | |
boot_count = boot_count.saturating_add(1); | |
eeprom.write_byte(BOOT_COUNT_OFFSET, boot_count); | |
if boot_count == u8::MAX { | |
ufmt::uwriteln!(&mut serial, "Boot count: >={} (overflow)", boot_count).unwrap(); | |
} else { | |
ufmt::uwriteln!(&mut serial, "Boot count: {}", boot_count).unwrap(); | |
} |
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 agree, examples are how people learn, so we should try to show the best code.
The issue with EEPROM is that the data is 0xFF
in the default/erased state, so we need to "initialize" it on first boot. A couple of possible solutions would be:
- Check and stop counting at
u8::MAX - 1
- Use a larger integer (
u32
should be enough for everyone™)
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.
Hmm, I see it is getting even more difficult... Tell you what, let's merge it as is and then maybe think about improvements in the future. I smell the solution being a custom "driver" to manage a bootcount, to contain all this complexity...
Co-authored-by: Rahix <[email protected]>
Part of #590