FLC Rework #81
FLC Rework #81
Conversation
MelonShooter
left a comment
MelonShooter
left a comment
There was a problem hiding this comment.
Approved - I would like to see those test outputs on UART and semihosting, and potential verification that this doesn't mess with other pieces of flash memory
suaviloquence
left a comment
suaviloquence
left a comment
There was a problem hiding this comment.
Also, did we audit the actual logic in the big write and read functions that splits it into atomic read/write128s?
| /// - Lock write protection | ||
| /// - Flush ICC | ||
| /// - Enable icc0 | ||
| fn write_guard<F: Fn()>(&self, sys_clk: &SystemClock, operation: F) -> Result<(), FlashErr> { |
There was a problem hiding this comment.
| fn write_guard<F: Fn()>(&self, sys_clk: &SystemClock, operation: F) -> Result<(), FlashErr> { | |
| fn write_guard<F: FnOnce()>(&self, sys_clk: &SystemClock, operation: F) -> Result<(), FlashErr> { |
| /// [`flash_write_full_partially_outbound_end`]. | ||
| pub fn run_flc_tests( | ||
| stdout: &mut hio::HostStream, | ||
| pub fn run_flc_tests<T: Write>( |
There was a problem hiding this comment.
We might want to add a test that does two writes in a row, irrespective of output, to prevent this issue from showing up again.
| /// Behavior is undefined if any of the following conditions are violated: | ||
| /// * `address` must be in a valid flash page | ||
| pub unsafe fn page_erase(&self, address: u32, sys_clk: &SystemClock) -> Result<(), FlashErr> { | ||
| self.check_address_bounds(address..address)?; |
There was a problem hiding this comment.
Doesn't this have size 0?
There was a problem hiding this comment.
Well the address is the address of the page, check_address_bounds does a contains check on the given range. Essentially this is just checking that address is within the bounds of flash.
Unless I am mistaken about what range syntax is doing here.
There was a problem hiding this comment.
max78000-hal/hal/src/peripherals/flash_controller.rs
Lines 71 to 73 in f0072de
You're totally right here - we are just treating a range x..y as a tuple (x, y)
(although i think the checking code is subtly wrong - it's treating the range as inclusive when it should be a <= x < b) 😭
There was a problem hiding this comment.
Specifically this is a false positive where end = FLASH_START + FLASH_SIZE
|
contained in #83 |
Main Changes: