Implement flash read/erase/program #239
Conversation
astro
force-pushed
the
flash
branch
5 times, most recently
from
b5532de
to
45d804d
Compare
astro
changed the title
There was a problem hiding this comment.
Thanks for the PR, nice work. I left a few comments, but something that I didn't quite understand from the manual is this:
It is not allowed to program data to the Flash memory that would cross the 128-bit row
boundary. In such a case, the write operation is not performed and a program alignment
error flag (PGAERR) is set in the FLASH_SR register.
This seems to bound the programming operation to a maximum of 64 16 bytes (and all inside the same 128-bit aligned "row"), I see that you are checking for this error, but I think this is also important to document.
And also, missing changelog entry.
astro
force-pushed
the
flash
branch
2 times, most recently
from
a34e0de
to
72ae805
Compare
|
I have now taken care of the 128-bit row alignment, and added a changelog entry. |
|
Really well-structured, nice work! This makes me want to migrate your work to the Were you working on read/write protection? If not, I'll gladly take that up. I just "finished" a basic implementation for the stm32f1xx-hal, I imagine the F4 is very similar. Hope to see this merged soon! |
| /// aligned to 128-bit rows | ||
| pub fn program<I>(&mut self, mut offset: usize, mut bytes: I) -> Result<(), Error> | ||
| where | ||
| I: Iterator<Item = u8>, |
There was a problem hiding this comment.
I think this may be more easily used as IntoIterator<Item = &'a u8>.
That or just change bytes to a slice?
There was a problem hiding this comment.
Isn't Iterator more flexible than IntoInterator ?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Oh, just realized that the suggestion was with Item = &'a u8
There was a problem hiding this comment.
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!).
|
|
||
| impl FlashExt for FLASH { | ||
| fn address(&self) -> usize { | ||
| 0x0800_0000 |
There was a problem hiding this comment.
Could this be a static fn instead? Same question applies to len().
This could allow for FlashExt::read() to also be static.
There was a problem hiding this comment.
read needs to borrow self, otherwise the user would be able to have a & and &mut to the same memory.
There was a problem hiding this comment.
Sorry, not &mut, but they would be able to mutate the memory while it has a live immutable reference to it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You are right -- I was thinking of my own implementation for read(), not the one included here 🤦♂️ . Apologies.
|
@astro Thanks for the PR. I like the ideas proposed by @TheRealBluesun, what are your opinions on that? Also the unsafe warnings are a bit curious; I haven't checked whether they're actually valid or caused by differences in the patched SVD files. |
|
@therealprof Is there an update on this? |
@thalesfragoso needs to withdraw the concern, otherwise bors will not let me merge it. |
|
What status of this? |
|
I have no authority here, but I've been using a forked version with these changes for a while with no problems. I'd love to see this merged so I can switch back to the main (non-forked) source. Can I do anything to help with this? Do we need someone to create a new pull request with the same changes that won't be stuck? |
|
The code looks good enough for me. |
| I: Iterator<Item = u8>, | ||
| { | ||
| let ptr = self.flash.address() as *mut u8; | ||
| let mut bytes_written = 1; |
There was a problem hiding this comment.
What is the use of bytes_written ?
|
We probably should change the |
No description provided.