Skip to content
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

Merged
merged 7 commits into from
Oct 14, 2024

Conversation

armandas
Copy link
Contributor

Part of #590

armandas added a commit to armandas/avr-hal that referenced this pull request Oct 12, 2024
Copy link
Owner

@Rahix Rahix left a 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!

Comment on lines +32 to +36
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();
Copy link
Owner

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.

Suggested change
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();
}

Copy link
Contributor Author

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™)

Copy link
Owner

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...

mcu/atmega-hal/src/adc.rs Outdated Show resolved Hide resolved
mcu/atmega-hal/src/usart.rs Outdated Show resolved Hide resolved
@Rahix Rahix added the documentation Documentation/Examples label Oct 13, 2024
mcu/atmega-hal/src/usart.rs Outdated Show resolved Hide resolved
mcu/atmega-hal/src/eeprom.rs Outdated Show resolved Hide resolved
mcu/atmega-hal/src/i2c.rs Outdated Show resolved Hide resolved
mcu/atmega-hal/src/port.rs Outdated Show resolved Hide resolved
mcu/atmega-hal/src/spi.rs Outdated Show resolved Hide resolved
mcu/atmega-hal/src/usart.rs Outdated Show resolved Hide resolved
mcu/atmega-hal/src/usart.rs Outdated Show resolved Hide resolved
@Rahix Rahix merged commit 7e21ea0 into Rahix:main Oct 14, 2024
25 checks passed
@armandas armandas deleted the atmega-doc-examples branch October 15, 2024 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation/Examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants