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

Eeprom #51

Merged
merged 8 commits into from
May 20, 2023
Merged

Eeprom #51

merged 8 commits into from
May 20, 2023

Conversation

amcelroy
Copy link
Contributor

Howdy,

I recently had a requirement to store some variables in EEPROM on the TM4C123 and added code to:

  • Enable the peripheral
  • Address the EEPROM
  • Write data (in the form of &[u8]) to the EEPROM
  • Read data (in the form of &[u8]) from the EEPROM
  • Erase data (in the form of number of bytes to erase) from the EERPOM
  • Erase a block from the EEPROM

I tested the code in another project using RTIC with a Tiva C Series Launchpad and included the relevant test and example code in tiva-c-launchpad/src/main.rs. Please let me know if any changes need to be made or if any issues arise during testing.

Thanks,
Austin

amcelroy added 5 commits May 16, 2023 14:40
- Added example / test cases to main.rs in tiva-c-launchpad
- Cleaned up EepromError enum
- Added impl Display to EepromError
- Added increment() method to EepromAddress, used to increment
the block and offset.
- Improved documentation and function names
- Improved the Eeprom peripheral initialization to match the datasheet
- Added a delay function to introduce clock cycle delays as called for in
the datasheet. See code comments for datasheet sections that point
out the delays.
- Added erase and erase_block impl.
- Much improved test code.
fn set_block_and_offset()
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!
Would you mind adding an entry to the changelog about this and fixing the code formatting?

- Updated Changlog, added EEPROM section
- Linter / format fixes
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
@thejpster could you have a look at this?

@amcelroy
Copy link
Contributor Author

Sure no problem! I'm still pretty new to Rust and and grappling with the format checking. Sorry about that.

If any other internet stranger stumbled across this post helped set things to right to configured VS Code to auto format on save.

Copy link
Member

@thejpster thejpster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Just a couple of minor comments but it's generally very clearly written.

I don't have a TM4C to hand for testing so if it works for you (please post screenshots of that test app running) I'm happy!

tm4c123x-hal/src/eeprom.rs Outdated Show resolved Hide resolved

// 3. Read PRETRY and ERETRY
if final_eeprom.eeprom.eesupp.read().eretry().bit_is_set() {
panic!("Eeprom ERETRY bit set, please investigate or stop using the EEPROM peripheral");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a panic or should it return an error?

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 can change to an error, but chose a panic because of the datasheet (section 8.2.4.2):

If the PRETRY or ERETRY bits are set in the EESUPP register, the EEPROM was unable to recover its state. If power is stable when this occurs, this indicates a fatal error and is likely an indication that the EEPROM memory has exceeded its specified lifetime write/erase specification. If the supply voltage is unstable when this return code is observed, retrying the operation once the voltage is stabilized may clear the error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! Can you add a comment explaining this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I had added code comments, but did you mean function comment? I have just added a comment to the Eeprom::new() functions that informs users that the function may panic, why, and with the quote from the datasheet explaining the issue.

tm4c123x-hal/src/eeprom.rs Outdated Show resolved Hide resolved
tm4c123x-hal/src/eeprom.rs Outdated Show resolved Hide resolved
- Renamed validate_byte_array_bounds to
is_access_valid for improved readability
- Removed custom delay function, using the cortex_m::asm::delay function
now.
- Renamed _pc to pc since it was being used in Eeprom::new()
- Simplified branching for functions that invoke the
is_access_valid(...) function.
- Comment improvement
@thejpster
Copy link
Member

Just the comment to go, then ready to merge. Thank you!

@thejpster thejpster merged commit 78ff567 into rust-embedded-community:master May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants