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.

Sign up for GitHub

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

Changes to Flash.rs #83

Merged
merged 6 commits into from
Nov 29, 2023
Merged

Changes to Flash.rs #83

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d99ba9d
Changes to Flash.rs
amcelroy Sep 14, 2023
9b31810
Added changes systec-ms
amcelroy Nov 14, 2023
b6466d7
FlashWriter.write requires additional user input
amcelroy Nov 16, 2023
883ca94
Merge branch 'stm32-rs:main' into flash_patches
amcelroy Nov 28, 2023
fcc39d3
Merge branch 'stm32-rs:main' into flash_patches
amcelroy Nov 28, 2023
f5fdae3
Post review changes
amcelroy Nov 28, 2023
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
86 changes: 73 additions & 13 deletions src/flash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ const _RDPRT_KEY: u16 = 0x00A5;
const KEY1: u32 = 0x45670123;
const KEY2: u32 = 0xCDEF89AB;

const OPT_KEY1: u32 = 0x08192A3B;
const OPT_KEY2: u32 = 0x4C5D6E7F;

pub const SZ_1K: u32 = 1024;

pub type Result<T> = core::result::Result<T, Error>;
Expand All @@ -26,7 +29,9 @@ pub enum Error {
WriteError,
VerifyError,
UnlockError,
OptUnlockError,
LockError,
OptLockError,
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
Expand All @@ -53,6 +58,34 @@ pub struct FlashWriter<'a, const SECTOR_SZ_KB: u32> {
verify: bool,
}
impl<'a, const SECTOR_SZ_KB: u32> FlashWriter<'a, SECTOR_SZ_KB> {
fn unlock_options(&mut self) -> Result<()> {
// Check if flash is busy
while self.flash.sr.sr().read().bsy().bit_is_set() {}

// Unlock
self.unlock()?;

// Write the OPT Keys to the OPTKEYR register
unsafe {
self.flash
._optkeyr
.optkeyr()
.write(|w| w.optkeyr().bits(OPT_KEY1));
}
unsafe {
self.flash
._optkeyr
.optkeyr()
.write(|w| w.optkeyr().bits(OPT_KEY2));
}

// Verify success
match self.flash.cr.cr().read().lock().bit_is_clear() {
true => Ok(()),
false => Err(Error::OptUnlockError),
}
}

fn unlock(&mut self) -> Result<()> {
// Wait for any ongoing operations
while self.flash.sr.sr().read().bsy().bit_is_set() {}
Expand Down Expand Up @@ -111,8 +144,6 @@ impl<'a, const SECTOR_SZ_KB: u32> FlashWriter<'a, SECTOR_SZ_KB> {
> self.flash_sz.kbytes() as u32
{
Err(Error::LengthTooLong)
} else if length & 0x1 != 0 {
Err(Error::LengthNotMultiple2)
} else {
Ok(())
}
Expand All @@ -139,7 +170,7 @@ impl<'a, const SECTOR_SZ_KB: u32> FlashWriter<'a, SECTOR_SZ_KB> {
self.flash
.cr
.cr()
.write(|w| w.pnb().bits(page.try_into().unwrap()));
.modify(|_, w| w.pnb().bits(page.try_into().unwrap()));
}

// Start Operation
Expand Down Expand Up @@ -224,22 +255,47 @@ impl<'a, const SECTOR_SZ_KB: u32> FlashWriter<'a, SECTOR_SZ_KB> {
// Unlock Flash
self.unlock()?;

for idx in (0..data.len()).step_by(2) {
for idx in (0..data.len()).step_by(8) {
amcelroy marked this conversation as resolved.
Show resolved Hide resolved
// Check the starting address
self.valid_address(offset + idx as u32)?;
// Check the ending address to make sure there is no overflow
self.valid_address(offset + 8 + idx as u32)?;

let write_address = (FLASH_START + offset + idx as u32) as *mut u16;
let write_address1 = (FLASH_START + offset + idx as u32) as *mut u32;
let write_address2 = (FLASH_START + offset + 4 + idx as u32) as *mut u32;

let mut word1: u32 = 0;
let mut word2: u32 = 0;
amcelroy marked this conversation as resolved.
Show resolved Hide resolved

// Check if there is enough data to make 2 words, if there isn't, pad the data with 0xFF
if idx + 8 > data.len() {
let mut tmp_buffer = [255 as u8; 8];
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be 255u8?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, why this buffer length use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That buffer is used to pad non-divisible-by-8 data if the user wants an "easy" way to write data to flash. The STM32G4 requires 8 bytes to be written at a time. The user can use the force_data_padding flag so that data that isn't divisible by 8 is buffered into tmp_buffer and appropriately padded on the fly.

I can change it to 255u8 if that is preferred.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, also don't forget remove TODO for flash register structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think it is complete, thank you for reviewing!

for jdx in idx..data.len() {
tmp_buffer[jdx] = data[idx + jdx];
}
let tmp_dword = u64::from_le_bytes(tmp_buffer);
word1 = tmp_dword as u32;
word2 = (tmp_dword >> 32) as u32;
Copy link

Choose a reason for hiding this comment

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

A note from my colleague:

For me this is a bit to much intelligence/"magic" within the driver. In my opinion we should check whether the user passes data to us whose length is a multiple of 8 and return an error, if this is not the case.
Background:
Imagine, a user doesn't know, that we only can write multiples of 8. This user passes blocks of 4 bytes (this is valid for a lot of MCUs). The first write succeeds, because you added the missing 4 bytes. The next write will fail, because writing 0xFF is internally another state than a deleted flash block and the MCU's flash routine will return an error, if the user tries to write his next block of 4 bytes.
We should inform the user that he does not fulfill the boundary conditions as soon as he writes the first time (via an error). This is because the error in the second write is de facto only a subsequent 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.

Yes, I see y'alls perspective. On a previous MCU I worked with, as long as the alignment was fine, a flash bits could go from 1 -> 0, so padding with 0xFF didn't cause an issue and made writing data easier since the user didn't need to worry about data length, only alignment of the start address and that the flash block was already empty.

Let me write some test cases to cover this and see what happens. If there are any issues with this test I will add a check to ensure incoming arrays are divisible by 8 and return an error (or add a new one) that makes this condition clear.

Thank you for the feedback.

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 did a bit more testing and recognize your colleagues complaint. However, I still would like writing data to be easy in most cases. A hopeful solution is adding a flag to FlashWriter.write called force_data_padding, along with function comments, that makes it clear that data padding is going on.

Copy link

Choose a reason for hiding this comment

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

I have no objections in this respect, since the padding with 0xFF is then at least obvious.
Maybe, instead of a Boolean flag, two methods would be more suitable, just an idea.

} else {
word1 = (data[idx] as u32)
| (data[idx + 1] as u32) << 8
| (data[idx + 2] as u32) << 16
| (data[idx + 3] as u32) << 24;

word2 = (data[idx + 4] as u32)
| (data[idx + 5] as u32) << 8
| (data[idx + 6] as u32) << 16
| (data[idx + 7] as u32) << 24;
}

// Set Page Programming to 1
self.flash.cr.cr().modify(|_, w| w.pg().set_bit());

while self.flash.sr.sr().read().bsy().bit_is_set() {}

// Flash is written 16 bits at a time, so combine two bytes to get a
// half-word
let hword: u16 = (data[idx] as u16) | (data[idx + 1] as u16) << 8;

// NOTE(unsafe) Write to FLASH area with no side effects
unsafe { core::ptr::write_volatile(write_address, hword) };
unsafe { core::ptr::write_volatile(write_address1, word1) };
unsafe { core::ptr::write_volatile(write_address2, word2) };

// Wait for write
while self.flash.sr.sr().read().bsy().bit_is_set() {}
Expand All @@ -261,8 +317,9 @@ impl<'a, const SECTOR_SZ_KB: u32> FlashWriter<'a, SECTOR_SZ_KB> {
} else if self.verify {
// Verify written WORD
// NOTE(unsafe) read with no side effects within FLASH area
let verify: u16 = unsafe { core::ptr::read_volatile(write_address) };
if verify != hword {
let verify1: u32 = unsafe { core::ptr::read_volatile(write_address1) };
let verify2: u32 = unsafe { core::ptr::read_volatile(write_address2) };
if verify1 != word1 && verify2 != word2 {
self.lock()?;
return Err(Error::VerifyError);
}
Expand Down Expand Up @@ -374,7 +431,10 @@ impl Parts {
feature = "stm32g491",
feature = "stm32g4a1",
))]
pub fn writer(&mut self, flash_sz: FlashSize) -> FlashWriter<{ 4 * SZ_1K }> {
pub fn writer<const PAGE_SIZE_KB: u32>(
&mut self,
flash_sz: FlashSize,
) -> FlashWriter<PAGE_SIZE_KB> {
FlashWriter {
flash: self,
flash_sz,
Expand Down