Skip to content

Implement flash read/erase/program #239

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Add initial DMA support
- Allow specification of ADC reference voltage in ADC configuraton structure
- Added support for hardware-based CRC32 functionality
- Added flash driver

### Fixed
- Stability fixes related to SD card write
Expand Down
165 changes: 165 additions & 0 deletions src/flash.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
use crate::signature::FlashSize;
use crate::stm32::FLASH;
use core::{ptr, slice};

/// Flash erase/program error
#[derive(Debug, Clone, Copy)]
pub enum Error {
ProgrammingSequence,
ProgrammingParallelism,
ProgrammingAlignment,
WriteProtection,
Operation,
}

impl Error {
fn read(flash: &FLASH) -> Option<Self> {
let sr = flash.sr.read();
if sr.pgserr().bit() {
Some(Error::ProgrammingSequence)
} else if sr.pgperr().bit() {
Some(Error::ProgrammingParallelism)
} else if sr.pgaerr().bit() {
Some(Error::ProgrammingAlignment)
} else if sr.wrperr().bit() {
Some(Error::WriteProtection)
} else if sr.operr().bit() {
Some(Error::Operation)
} else {
None
}
}
}

/// Flash methods implemented for `stm32::FLASH`
pub trait FlashExt {
/// Memory-mapped address
fn address(&self) -> usize;
/// Size in bytes
fn len(&self) -> usize;
/// Returns a read-only view of flash memory
fn read(&self) -> &[u8] {
let ptr = self.address() as *const _;
unsafe { slice::from_raw_parts(ptr, self.len()) }
}
/// Unlock flash for erasing/programming until this method's
/// result is dropped
fn unlocked(&mut self) -> UnlockedFlash;
}

impl FlashExt for FLASH {
fn address(&self) -> usize {
0x0800_0000
Copy link

@TheRealBluesun TheRealBluesun Dec 31, 2020

Choose a reason for hiding this comment

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

Could this be a static fn instead? Same question applies to len().

This could allow for FlashExt::read() to also be static.

Copy link
Member

Choose a reason for hiding this comment

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

read needs to borrow self, otherwise the user would be able to have a & and &mut to the same memory.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, not &mut, but they would be able to mutate the memory while it has a live immutable reference to it.

Copy link

@TheRealBluesun TheRealBluesun Jan 11, 2021

Choose a reason for hiding this comment

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

read needs to borrow self, otherwise the user would be able to have a & and &mut to the same memory.
Sorry, not &mut, but they would be able to mutate the memory while it has a live immutable reference to it.

EDIT: Disregard

I don't think you're carrying around a reference to memory, though; only the results of a read (i.e. copied bytes). I believe I see what you mean if you think of the entirety of Flash as an object to which you have a slice into, but I don't think very much is gained from treating it that way. I'm really curious to hear what you/others think about it. I do see how this could become a problem in environments where two threads could simultaneously access flash, though.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean if you think of the entirety of Flash as an object to which you have a slice into, but I don't think very much is gained from treating it that way.

read is doing exactly that, it gives the user a &[u8] pointing directly to flash memory. Without the borrow, the user would be able to create an UnlockedFlash and modify the flash contents, and then use the same &[u8] after, which is UB in rust, since you can't mutable alias with references.

Copy link
Member

Choose a reason for hiding this comment

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

read doesn't copy. But even if it did (which isn't really desirable), without borrowing Self you couldn't prevent the user from using read on main and program in an interrupt, which is a data race.

Choose a reason for hiding this comment

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

You are right -- I was thinking of my own implementation for read(), not the one included here 🤦‍♂️ . Apologies.

}

fn len(&self) -> usize {
FlashSize::get().bytes()
}

fn unlocked(&mut self) -> UnlockedFlash {
unlock(self);
UnlockedFlash { flash: self }
}
}

const PSIZE_X8: u8 = 0b00;

/// Result of `FlashExt::unlocked()`
pub struct UnlockedFlash<'a> {
flash: &'a mut FLASH,
}

/// Automatically lock flash erase/program when leaving scope
impl Drop for UnlockedFlash<'_> {
fn drop(&mut self) {
lock(&self.flash);
}
}

impl UnlockedFlash<'_> {
/// Erase a flash sector
///
/// Refer to the reference manual to see which sector corresponds
/// to which memory address.
pub fn erase(&mut self, sector: u8) -> Result<(), Error> {
let snb = if sector < 12 { sector } else { sector + 4 };

#[rustfmt::skip]
self.flash.cr.modify(|_, w| unsafe {
w
// start
.strt().set_bit()
.psize().bits(PSIZE_X8)
// sector number
.snb().bits(snb)
// sectore erase
.ser().set_bit()
// no programming
.pg().clear_bit()
});
self.wait_ready();
self.ok()
}

/// Program bytes with offset into flash memory,
/// aligned to 128-bit rows
pub fn program<I>(&mut self, mut offset: usize, mut bytes: I) -> Result<(), Error>
where
I: Iterator<Item = u8>,
Copy link

@TheRealBluesun TheRealBluesun Dec 31, 2020

Choose a reason for hiding this comment

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

I think this may be more easily used as IntoIterator<Item = &'a u8>.

That or just change bytes to a slice?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't Iterator more flexible than IntoInterator ?

Copy link
Member

Choose a reason for hiding this comment

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

No. IntoIterator allows you to work with collection types as well as Iterator (which automatically implements IntoIterator) which is super-handy (but probably not that important in this particular usecase).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, just realized that the suggestion was with Item = &'a u8

Choose a reason for hiding this comment

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

It may be best to just pass a &[u8], even. Ultimately, I don't think you gain much by using IntoIter<Item = &u8> really. Definitely willing to be convinced (insofar as my opinion matters, hah!).

{
let ptr = self.flash.address() as *mut u8;
let mut bytes_written = 1;
Copy link
Member

Choose a reason for hiding this comment

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

What is the use of bytes_written ?

while bytes_written > 0 {
bytes_written = 0;
let amount = 16 - (offset % 16);

#[rustfmt::skip]
self.flash.cr.modify(|_, w| unsafe {
w
.psize().bits(PSIZE_X8)
// no sector erase
.ser().clear_bit()
// programming
.pg().set_bit()
});
for _ in 0..amount {
match bytes.next() {
Some(byte) => {
unsafe {
ptr::write_volatile(ptr.add(offset), byte);
}
offset += 1;
bytes_written += 1;
}
None => break,
}
}
self.wait_ready();
self.ok()?;
}
self.flash.cr.modify(|_, w| w.pg().clear_bit());

Ok(())
}

fn ok(&self) -> Result<(), Error> {
Error::read(&self.flash).map(Err).unwrap_or(Ok(()))
}

fn wait_ready(&self) {
while self.flash.sr.read().bsy().bit() {}
}
}

const UNLOCK_KEY1: u32 = 0x45670123;
const UNLOCK_KEY2: u32 = 0xCDEF89AB;

fn unlock(flash: &FLASH) {
flash.keyr.write(|w| unsafe { w.key().bits(UNLOCK_KEY1) });
flash.keyr.write(|w| unsafe { w.key().bits(UNLOCK_KEY2) });
assert!(!flash.cr.read().lock().bit())
}

fn lock(flash: &FLASH) {
flash.cr.modify(|_, w| w.lock().set_bit());
}
2 changes: 2 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ pub mod dma;
#[cfg(feature = "device-selected")]
pub mod dwt;
#[cfg(feature = "device-selected")]
pub mod flash;
#[cfg(feature = "device-selected")]
pub mod prelude;
#[cfg(feature = "device-selected")]
pub mod pwm;
Expand Down