Flash Read/Write Support #141

luctius · 2019-11-22T11:12:51Z

This pr adds support for flash reading and writing.

I think the API can be improved and I am open for suggestions on that front.

thalesfragoso

Thanks for the work, I was actually thinking about this yesterday, heh. I just have some questions and suggestions. Also, I think the NOTE(unsafe) should be used to explain why the operation is correct.

thalesfragoso · 2019-11-22T19:58:15Z

src/flash.rs

+
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)]
+pub enum Error {
+    Success,


Is this used ?

thalesfragoso · 2019-11-22T20:08:51Z

src/flash.rs

+        if FLASH_START + offset > FLASH_END {
+            Err(Error::AddressLargedThanFlash)
+        }
+        else if offset % 2 != 0 {


I wonder if the compiler will always do the right thing and change this to offset & 1, otherwise it might be better if we use bitwise AND instead of mod.

I had a look at the LLVM-IR produced by rustc, and it looks like it doesn't change it to an and in debug mode, but does in release mode. That is without LLVM optimizations though

thalesfragoso · 2019-11-22T20:09:27Z

src/flash.rs

+        if length > self.sector_sz.size() as u32 {
+            Err(Error::LengthTooLong)
+        }
+        else if length % 2 != 0 {


Same as above

thalesfragoso · 2019-11-22T20:13:15Z

src/flash.rs

+        // Re-lock flash
+        self.lock()?;
+
+        for idx in 0..self.sector_sz.size() -1 {


It might be good to add a way to ignore verification, like a verify: bool argument.

thalesfragoso · 2019-11-22T20:17:25Z

src/flash.rs

+        self.lock()?;
+
+        for idx in 0..self.sector_sz.size() -1 {
+            let write_address = (FLASH_START + start_offset + idx as u32) as *mut u16;


Don't you have to correct this to not go pass a section boundary ? i.e start_offset might be in the middle of a sector. Also I don't think it needs to be mut.

thalesfragoso · 2019-11-22T20:31:19Z

src/flash.rs

+        Ok(())
+    }
+
+    /// Retrieve an slice of at most 1 sector of data from FLASH_START +offset


Why have this limitation ?

thalesfragoso · 2019-11-22T20:35:37Z

src/flash.rs

+        }
+    }
+
+    /// Write at most 1 sector of data to FLASH_START +offset


Why have this limitation ?

thalesfragoso · 2019-11-22T20:37:33Z

src/flash.rs

+
+            // Check for errors
+            let sr = self.flash.sr.sr().read();
+            if self.flash.sr.sr().read().pgerr().bit_is_set() {


Reset bit if set ?

thalesfragoso · 2019-11-22T20:37:51Z

src/flash.rs

+                self.lock()?;
+                return Err(Error::ProgrammingError);
+            }
+            else if self.flash.sr.sr().read().wrprterr().bit_is_set() {


Same as above

thalesfragoso · 2019-11-22T20:38:43Z

src/flash.rs

+            }
+
+            // Verify written word
+            let verify: u16 = unsafe { core::ptr::read_volatile(write_address) };


Add option to avoid verification ?

luctius · 2019-11-22T22:19:47Z

You make excellent points. It was a while since I really worked on the code and although I gave it a final look-over I apparently missed quite a bit.

Thanks for the thorough check.

Most of it is easily fixed which I will probably do in the coming week.

TheZoq2

Thanks for the PR! I had a quick read through, and the code looks very nice over all. I added a few nitpicks. which would be nice if you could fix. I also agree with the comments @thalesfragoso, especially about the unsafe comments

TheZoq2 · 2019-11-24T09:03:50Z

src/flash.rs

+    Sz1M   = 1024,
+}
+impl FlashSize {
+    fn size(self) -> u32 {


I would prefer a more descriptive name for this, perhaps as kbytes

TheZoq2 · 2019-11-24T09:16:06Z

src/flash.rs

+        if FLASH_START + offset > FLASH_END {
+            Err(Error::AddressLargedThanFlash)
+        }
+        else if offset % 2 != 0 {


I had a look at the LLVM-IR produced by rustc, and it looks like it doesn't change it to an and in debug mode, but does in release mode. That is without LLVM optimizations though

TheZoq2 · 2019-11-24T09:18:47Z

src/flash.rs

+    _0: (),
+}
+
+#[allow(dead_code)]


Why are these allow(dead_code) needed? Is rustc missing the usages, or are they currently unused?

Those entries are currently unused; I added them for completeness.

- Added FlashReaderGuard - Removed modulo operations - Added option to avoid verify operations - Cleanup

luctius · 2019-11-29T19:16:55Z

The comments made should have been fixed, though perhaps some unsafe code comments could still be better.

I have verified that single byte read operations from flash are possible.

TheZoq2

Some minor nitpicks, but other than this I think it looks good

TheZoq2 · 2019-12-01T09:24:37Z

src/flash.rs

+        // Wait for any ongoing operations
+        while self.flash.sr.sr().read().bsy().bit_is_set() {}
+
+        // NOTE(unsafe) write Keys to the key register


Still doesn't explain why unsafe is safe

TheZoq2 · 2019-12-01T09:25:15Z

src/flash.rs

+        self.flash.cr.cr().modify(|_, w| w.per().set_bit() );
+
+        // Write address bits
+        // NOTE(unsafe) This sets the sector address in the Address Register


TheZoq2 · 2019-12-01T09:26:31Z

src/flash.rs

+            let hword: u16 =
+                  (data[idx]   as u16)
+                | (data[idx+1] as u16) << 8;


Weird formatting. Though I'm not sure what the best way to format it is

it's better to use copy_from_slice and u16::from_be/le_bytes for such manipulations

irwineffect · 2020-03-30T03:54:33Z

@luctius, @TheZoq2, if the remaining comments are addressed, would this be ready to merge in?

TheZoq2 · 2020-03-31T06:23:42Z

It has been a while since I looked at it, but IIRC, yes

DerFetzer · 2020-07-06T22:55:48Z

@luctius Any news on this? Is there help needed somewhere?

irwineffect · 2020-07-09T17:44:17Z

I had been working on addressing the comments for the unsafe blocks (irwineffect@0a997b0), although when I started to do more testing of the branch I had noticed some occasional issues when erasing the flash. It mostly worked, but occasionally I think the hardware was getting stuck in a weird state, I think the erase logic as it exists right now isn't quite correct (the reference manual PM0075 also is somewhat ambiguous).
It's been a while since I had looked at it, but I'll dig up what I had been working on and post more details.

irwineffect · 2020-07-24T02:44:21Z

Well, I've been testing and I haven't been able to replicate the issues I had seen earlier, everything seems ok...perhaps my hardware was just wigging out? Sorry for the doubt, @luctius.

irwineffect · 2020-07-24T02:49:21Z

It seems that @luctius hasn't signed on in a long time...should I create a pull request for irwineffect/stm32f1xx-hal@0a997b0? This is based on @luctius's branch, plus I've tried to address the comments.

TheZoq2 · 2020-08-12T19:03:21Z

Sorry for the late reply.

should I create a pull request for irwineffect/stm32f1xx-hal@0a997b0? This is based on @luctius's branch, plus I've tried to address the comments.

I'd say you should, it's pointless to have a stale PR waiting when one that can make progress is available :)

irwineffect · 2020-08-16T02:49:08Z

This PR can now be continued on #257

TheZoq2 · 2020-08-22T16:30:54Z

I'll go ahead and close this then :)

Added Flash Read/Write Support

a658443

thalesfragoso suggested changes Nov 22, 2019

View reviewed changes

TheZoq2 reviewed Nov 24, 2019

View reviewed changes

Incorporated Comments

b34c111

- Added FlashReaderGuard - Removed modulo operations - Added option to avoid verify operations - Cleanup

luctius force-pushed the flash_support branch from 7f55672 to b34c111 Compare November 29, 2019 19:14

TheZoq2 reviewed Dec 1, 2019

View reviewed changes

irwineffect mentioned this pull request Aug 16, 2020

Flash Read/Write Support #257

Merged

TheZoq2 closed this Aug 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flash Read/Write Support #141

Flash Read/Write Support #141

luctius commented Nov 22, 2019

thalesfragoso left a comment •

edited

Loading

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

TheZoq2 Nov 24, 2019

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

thalesfragoso Nov 22, 2019

luctius commented Nov 22, 2019

TheZoq2 left a comment

TheZoq2 Nov 24, 2019

TheZoq2 Nov 24, 2019

TheZoq2 Nov 24, 2019

luctius Nov 29, 2019 •

edited

Loading

luctius commented Nov 29, 2019

TheZoq2 left a comment

TheZoq2 Dec 1, 2019

TheZoq2 Dec 1, 2019

TheZoq2 Dec 1, 2019

burrbull Apr 11, 2020

irwineffect commented Mar 30, 2020

TheZoq2 commented Mar 31, 2020

DerFetzer commented Jul 6, 2020

irwineffect commented Jul 9, 2020 •

edited

Loading

irwineffect commented Jul 24, 2020

irwineffect commented Jul 24, 2020

TheZoq2 commented Aug 12, 2020 •

edited

Loading

irwineffect commented Aug 16, 2020

TheZoq2 commented Aug 22, 2020

Flash Read/Write Support #141

Flash Read/Write Support #141

Conversation

luctius commented Nov 22, 2019

thalesfragoso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luctius commented Nov 22, 2019

TheZoq2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luctius Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

luctius commented Nov 29, 2019

TheZoq2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irwineffect commented Mar 30, 2020

TheZoq2 commented Mar 31, 2020

DerFetzer commented Jul 6, 2020

irwineffect commented Jul 9, 2020 • edited Loading

irwineffect commented Jul 24, 2020

irwineffect commented Jul 24, 2020

TheZoq2 commented Aug 12, 2020 • edited Loading

irwineffect commented Aug 16, 2020

TheZoq2 commented Aug 22, 2020

thalesfragoso left a comment •

edited

Loading

luctius Nov 29, 2019 •

edited

Loading

irwineffect commented Jul 9, 2020 •

edited

Loading

TheZoq2 commented Aug 12, 2020 •

edited

Loading