Flash Read/Write Support #257

irwineffect · 2020-08-16T02:47:20Z

This PR is a followup on #141.

burrbull · 2020-08-16T13:46:20Z

Please, use git rebase instead of merge for PR.

irwineffect · 2020-08-21T00:41:27Z

Alright, I've rebased on master, it also looks like I need to run rustfmt over the code?

TheZoq2 · 2020-08-22T16:10:43Z

Yea, that would be nice. Should just require doing cargo fmt if it is installed :)

TheZoq2

Thanks for putting in the work to finishing this!

Over all I think the changes look good on the surface though I'm not read up on the details of the flash device so these comments are mostly overall code quality. Since this has seen testing by at least 2 people, I think it's safe to merge despite this :)

src/flash.rs

irwineffect · 2020-08-23T14:53:39Z

@TheZoq2, thanks for the review feedback! I agree about squashing the warnings and the various name changes, but I did have comments/questions on two other parts, let me know what you think!

irwineffect · 2020-08-25T04:54:35Z

I believe I've addressed all of the review findings, with the exception of handling the lock/unlock, still working on that!

src/flash.rs

thalesfragoso · 2020-08-26T19:08:08Z

Actually, I don't think we need the Guard at all, since

pub fn read(&self, offset: u32, length: usize) -> Result<&[u8]>

should be the same as

pub fn read(&'c self, offset: u32, length: usize) -> Result<&'c [u8]>

So it has to extend the borrow as long as the output lives if my understanding is correct, see this playground example:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=ce7f1c92470429693d99969d87f985d5

irwineffect · 2020-08-29T17:58:32Z

@thalesfragoso, yeah, I think you're right, we don't need to have the separate ReaderGuard, since the type system will prevent us from mutably borrowing the FlashWriter until the data returned by read() is out of scope, and we need to mutably borrow the FlashWriter before we could do any operations that would invalidate the data.

irwineffect · 2020-08-29T21:03:33Z

So, I've been playing around with different ways to deal with the locking/unlocking of the flash, and I've found things I really don't like about each solution, I'd appreciate some input.

I started out trying to wrap the Parts inside a FlashLockGuard struct that requires calling unlock() to get access the registers that require the flash hardware to be unlocked, and then calling lock() upon drop of the FlashLockGuard. The issue with this method is that with the current API, the lock() operation can fail, and I believe you don't want things that can fail inside a drop function, right?

The trouble with the do_with_unlock() method that @TheZoq2 had suggested is that if the operation() fails we will return without calling lock(). We could modify the example you provided to always lock() even if operation() fails, but if lock also fails, which error should we return, the lock failure or the operation failure? We could make a combo failure, but that feels pretty ugly.

I think my greater issue is the fact that the current lock()/unlock()API can fail. While unlocking could reasonably fail, I don't know under what circumstances the lock()operation would actually fail, I personally have not seen this occur, and I haven't noticed anything in the flash documentation that suggests this is possible. Can we not trust that writing to the lock bit in the control register will actually cause the bit to be set? Perhaps locking always succeeds, and we can ignore the possibility of failure? I'm inclined to continue punting the possible failure upwards to the user (or panic?) until this library has been more thoroughly tested and we are more confident this can't fail.

Also, I don't know if there really is a strong need to unlock/lock the flash around each write/erase operation. This does minimize the window that the flash could get corrupted if the code were to go off the rails, but why not just unlock the flash when the FlashWriter is created, keep it unlocked the entire time the FlashWriter exists, and then provide a lock() method that consumes the FlashWriter that the user can call when they are done with the flash? The only issue I foresee with this method is that the user could forgot to call lock() to turn back on the hardware lock on the flash (although is isn't the worst thing in the world). To protect against this possibility, we could implement a custom Drop for the FlashWriter, but this still has the issue that the locking operation could fail (but perhaps we should just ignore it or panic?). Also, if the user structures the program such that the FlashWriter is alive for the entire lifetime of the program, we are in the same boat as before. I think an explicit lock is better for that reason.

I'd love to get @luctius's thoughts to know the intent behind the original design, but barring that, what do you guys think?

thalesfragoso · 2020-08-30T03:23:42Z

src/flash.rs

+            // reference to this FlashWriter, and any operation that would
+            // invalidate the data returned would first require taking a mutable
+            // reference to this FlashWriter.
+            unsafe { core::slice::from_raw_parts::<'static, u8>(address, length) },


This shouldn't be 'static, it should have the same lifetime of &self. Not sure if you need to strictly annotate it in order for the compiler to be happy.

It looks like the compiler can infer the lifetime, as this compiles:

unsafe { core::slice::from_raw_parts(address, length) }

However, I decided to explicitly annotate the lifetimes around read() to make the relationship more obvious.

Hmm, but I don't think the 'a lifetime was the best idea, since it's the same lifetime as Parts like in the other case.
To be honest I think we should just let the compiler do the job and use just

pub fn read(&self, offset: u32, length: usize) -> Result<&[u8]> { //... unsafe { core::slice::from_raw_parts(address, length) } }

The lifetime elisions rules seem to be very clear in this case anyway, so it's best to leave the compiler to choose the appropriate lifetime I think, maybe add a comment. Sorry for all the nitpicking .-.

Haha don't be sorry, glad to get feedback! I didn't realize annotating the lifetime would change the behavior, I thought I was just making something that was implicit explicit.

It was just because it was the same lifetime you used at

impl<'a> FlashWriter<'a>

thalesfragoso · 2020-08-30T03:49:48Z

So, I've been playing around with different ways to deal with the locking/unlocking of the flash, and I've found things I really don't like about each solution, I'd appreciate some input.

I think we should hold on this change, better to leave for another PR, since the path isn't clear yet and the way it's now isn't that bad.

src/flash.rs

irwineffect · 2020-08-31T02:55:14Z

So, I've been playing around with different ways to deal with the locking/unlocking of the flash, and I've found things I really don't like about each solution, I'd appreciate some input.

I think we should hold on this change, better to leave for another PR, since the path isn't clear yet and the way it's now isn't that bad.

@TheZoq2, if you agree, we can leave reworking the unlock/locking logic for a future PR?

TheZoq2 · 2020-08-31T09:43:40Z

Sounds like a plan to me!

irwineffect · 2020-09-01T14:18:31Z

If everything looks good now, should I squash-rebase before we merge?

burrbull · 2020-09-01T16:41:23Z

Yes, please.

Incorporated Comments - Added FlashReaderGuard - Removed modulo operations - Added option to avoid verify operations - Cleanup Added more detailed notes for usage of unsafe in flash functions. ran "cargo fmt" to cleanup flash code Suppressed unused warnings for flash code, enum naming tweaks Added prefix '_' to registers and constants that aren't currently used, and renamed enum values based on review feedback added more descriptive comments where requested moved initialization of hword closer to where it's used Removed FlashReaderGuard, not necessary We can rely on the lifetime system to protect us here, the guard structure is unnecessary (see the unsafe comments for details). removed lifetimes from FlashWriter::read() Better to let the compiler determine the lifetime

TheZoq2

Looks great to me, just 2 things remain: a changelog entry and a tiny issue with one of the doc comments :)

TheZoq2 · 2020-09-02T19:12:05Z

src/flash.rs

+        Ok(())
+    }
+
+    /// Retrieve an slice of data from FLASH_START +offset


Nitpick: should be a slice. Also, would be nice to have the math in a code block, and a space after the +

thalesfragoso · 2020-09-03T02:20:19Z

src/flash.rs

+                let end = start + self.sector_sz.kbytes() as u32 - 1;
+                for idx in start..end {


Should this range be inclusive ? i.e. start..=end

You are absolutely right, good catch. Do you think it should be start..=end, or remove the -1 on the line above when end is calculated?

remove -1 is better

Wait, I did not see previous line.

burrbull · 2020-09-04T04:20:31Z

src/flash.rs

+                // By subtracting 1 from the sector size and masking with
+                // start_offset, we make 'start' point to the beginning of the
+                // page, and similarly 'end' at the end of the page. We do this
+                // because the entire page should have been erased, regardless
+                // of where in the page the given 'start_offset' was.
+                let start = start_offset & !(self.sector_sz.kbytes() as u32 - 1);
+                let end = start + self.sector_sz.kbytes() as u32 - 1;
+                for idx in start..end {


Suggested change

// By subtracting 1 from the sector size and masking with

// start_offset, we make 'start' point to the beginning of the

// page, and similarly 'end' at the end of the page. We do this

// because the entire page should have been erased, regardless

// of where in the page the given 'start_offset' was.

let start = start_offset & !(self.sector_sz.kbytes() as u32 - 1);

let end = start + self.sector_sz.kbytes() as u32 - 1;

for idx in start..end {

// By subtracting 1 from the sector size and masking with

// start_offset, we make 'start' point to the beginning of the

// page. We do this

// because the entire page should have been erased, regardless

// of where in the page the given 'start_offset' was.

let size = self.sector_sz.kbytes() as u32;

let start = start_offset & !(size - 1);

for idx in start..start+size {

Is this what you want to do?

I like that, I think it is more readable than the previous version.

TheZoq2 · 2020-09-05T06:38:16Z

Looks like all standing issues are resolved, thanks everyone who helped out!

irwineffect mentioned this pull request Aug 16, 2020

Flash Read/Write Support #141

Closed

irwineffect force-pushed the flash_support branch 2 times, most recently from 0a997b0 to c388e0a Compare August 21, 2020 00:39

TheZoq2 requested changes Aug 22, 2020

View reviewed changes

thalesfragoso reviewed Aug 26, 2020

View reviewed changes

src/flash.rs Outdated Show resolved Hide resolved

thalesfragoso reviewed Aug 30, 2020

View reviewed changes

ryan-summers reviewed Aug 30, 2020

View reviewed changes

src/flash.rs Show resolved Hide resolved

irwineffect requested a review from TheZoq2 September 1, 2020 02:06

irwineffect force-pushed the flash_support branch from 9d0b35b to e6cb4a8 Compare September 2, 2020 03:55

TheZoq2 requested changes Sep 2, 2020

View reviewed changes

cleaned up doc strings for flash, updated changelog

b6897cd

thalesfragoso reviewed Sep 3, 2020

View reviewed changes

burrbull reviewed Sep 4, 2020

View reviewed changes

reworked the bounds for verifying flash after erasing

536db7b

TheZoq2 merged commit 563c5d3 into stm32-rs:master Sep 5, 2020

faulesocke mentioned this pull request Apr 15, 2021

Example for writing flash #330

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flash Read/Write Support #257

Flash Read/Write Support #257

irwineffect commented Aug 16, 2020

burrbull commented Aug 16, 2020

irwineffect commented Aug 21, 2020

TheZoq2 commented Aug 22, 2020

TheZoq2 left a comment

irwineffect commented Aug 23, 2020

irwineffect commented Aug 25, 2020

thalesfragoso commented Aug 26, 2020

irwineffect commented Aug 29, 2020

irwineffect commented Aug 29, 2020

thalesfragoso Aug 30, 2020

irwineffect Aug 31, 2020

thalesfragoso Aug 31, 2020 •

edited

Loading

irwineffect Sep 1, 2020

thalesfragoso Sep 1, 2020

thalesfragoso commented Aug 30, 2020

irwineffect commented Aug 31, 2020

TheZoq2 commented Aug 31, 2020

irwineffect commented Sep 1, 2020

burrbull commented Sep 1, 2020

TheZoq2 left a comment

TheZoq2 Sep 2, 2020

thalesfragoso Sep 3, 2020

irwineffect Sep 4, 2020

burrbull Sep 4, 2020

burrbull Sep 4, 2020

burrbull Sep 4, 2020

burrbull Sep 4, 2020

irwineffect Sep 5, 2020

TheZoq2 commented Sep 5, 2020

		let end = start + self.sector_sz.kbytes() as u32 - 1;
		for idx in start..end {

Flash Read/Write Support #257

Flash Read/Write Support #257

Conversation

irwineffect commented Aug 16, 2020

burrbull commented Aug 16, 2020

irwineffect commented Aug 21, 2020

TheZoq2 commented Aug 22, 2020

TheZoq2 left a comment

Choose a reason for hiding this comment

irwineffect commented Aug 23, 2020

irwineffect commented Aug 25, 2020

thalesfragoso commented Aug 26, 2020

irwineffect commented Aug 29, 2020

irwineffect commented Aug 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalesfragoso Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalesfragoso commented Aug 30, 2020

irwineffect commented Aug 31, 2020

TheZoq2 commented Aug 31, 2020

irwineffect commented Sep 1, 2020

burrbull commented Sep 1, 2020

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

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

TheZoq2 commented Sep 5, 2020

thalesfragoso Aug 31, 2020 •

edited

Loading