Changes to Flash.rs #83

amcelroy · 2023-09-14T19:21:38Z

FlashWriter for certain chips requires the PAGE_SIZE_KB to be passed in. Some chips are single bank with 4096kB page size and some chips are dual bank with 2048kB page size. It seems this should be an option easily configured by the user.
Can unlock the Options register (at your own risk)
Changed Flash write to write 2 32-bit words as per the manual.
Can write arbitrary array lengths to flash. If data is < 8 bytes (2 32-bit words), the data is padded with 0xFF.
page_erase was fixed. Writing to the pnb() register was wiping the per() bit. Changed register access from write -> modify
Verify now checks the 2 32-bit words.

- FlashWriter for certain chips requires the PAGE_SIZE_KB to be passed in. Some chips are single bank with 4096kB page size and some chips are dual bank with 2048kB page size. It seems this should be an option easily configured by the user. - Can unlock the Options register (at your own risk) - Changed Flash write to write 2 32-bit words as per the manual. - Can write arbitrary array lengths to flash. If data is < 8 bytes (2 32-bit words), the data is padded with 0xFF. - page_erase was fixed. Writing to the pnb() register was wiping the per() bit. Changed register access from write -> modify - Verify now checks the 2 32-bit words.

systec-ms

I have tested the implementation with my stm32g491re and it works fine.
The only issue I had is attributable to a problem in the PAC.

The stm32g491re is a category 4 device with 512 Kbytes flash and a page size of 2048.
Consequently, 256 pages must addressable but according to the svd-file/PAC the page number (pnb) bits of the FLASH_CR is 7 bit width.
The 7 bit width is correct for category 3 and category 2 devices, but not for category 4 devices where 8 bits would be proper (see RM0440 Rev 7 P. 180 (Embedded Flash memory (FLASH) for category 4 devices) FLASH_CR register).

This shouldn't be an issue of your implementation, and should be resolved when this is fixed in svd2rust.

Btw. thanks and in my opinion it would be nice to have your example simplified and linked/included in the docs.

src/flash.rs

amcelroy

Thank you for the review!

src/flash.rs

- Linter fixes - Added code comment that clarifies the need to write double words - Added an example called flash_with_rtic.rs to show flash memory use case.

systec-ms · 2023-11-15T07:41:08Z

Thanks, for the quick response ;).

I have added a note from my colleague.
Additionally, my colleague is not a huge fan of the possible endless while loops (he would like a timeout) but this isn't an issue of this specific pull request and also part of all STM32-HALs as far as I can tell.

systec-ms · 2023-11-15T07:44:17Z

src/flash.rs

+            // 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];
+                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;


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.

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.

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.

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.

- FlashWriter.write used to automatically pad incoming data such that is was divisible by 8, a requirement of the STM32 flash hardware. A colleague suggested this could cause confusion during subsequent writes. FlashWriter.write now forces the user to acknowledge this with the `force_data_padding` flag. If true, the data will be padded automatically to be divisible by 8. If false and the data is not divisible by 8, an error is thrown. - Added new error ArrayMustBeDivisibleBy8 - Modified the flash example based on the above updates

examples/flash_with_rtic.rs

no111u3 · 2023-11-28T15:44:34Z

examples/flash_with_rtic.rs

+            match i {
+                0 => {
+                    // This test should fail, as the data needs to be divisible by 8 and force padding is false
+                    let result = flash_writer.write(0x1FC00 + i * FLASH_SPACING, &one_byte, false);


Use constant instead magic numbers

no111u3 · 2023-11-28T15:45:34Z

src/flash.rs

+
+            // 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];


should be 255u8?

also, why this buffer length use?

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.

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

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

- Added logging and const test address location to flash_with_rtic.rs - Changed type formatting on a buffer array in flash.rs - Removed //TODO and #[allow(dead_code)] from flash register structure Parts.

amcelroy mentioned this pull request Sep 14, 2023

Flash memory read / write issues? #81

Closed

systec-ms reviewed Nov 14, 2023

View reviewed changes

src/flash.rs Outdated Show resolved Hide resolved

src/flash.rs Show resolved Hide resolved

amcelroy commented Nov 14, 2023

View reviewed changes

src/flash.rs Outdated Show resolved Hide resolved

src/flash.rs Show resolved Hide resolved

Added changes systec-ms

9b31810

- Linter fixes - Added code comment that clarifies the need to write double words - Added an example called flash_with_rtic.rs to show flash memory use case.

systec-ms reviewed Nov 15, 2023

View reviewed changes

amcelroy added 3 commits November 16, 2023 08:23

Merge branch 'stm32-rs:main' into flash_patches

883ca94

Merge branch 'stm32-rs:main' into flash_patches

fcc39d3

no111u3 requested a review from systec-ms November 28, 2023 15:43

no111u3 reviewed Nov 28, 2023

View reviewed changes

Post review changes

f5fdae3

- Added logging and const test address location to flash_with_rtic.rs - Changed type formatting on a buffer array in flash.rs - Removed //TODO and #[allow(dead_code)] from flash register structure Parts.

no111u3 approved these changes Nov 29, 2023

View reviewed changes

no111u3 merged commit 035f767 into stm32-rs:main Nov 29, 2023
9 checks passed

amcelroy deleted the flash_patches branch November 29, 2023 14:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to Flash.rs #83

Changes to Flash.rs #83

amcelroy commented Sep 14, 2023

systec-ms left a comment •

edited

Loading

amcelroy left a comment

systec-ms commented Nov 15, 2023 •

edited

Loading

systec-ms Nov 15, 2023

amcelroy Nov 15, 2023

amcelroy Nov 16, 2023

systec-ms Nov 21, 2023

no111u3 Nov 28, 2023

no111u3 Nov 28, 2023

no111u3 Nov 28, 2023

amcelroy Nov 28, 2023

no111u3 Nov 28, 2023

amcelroy Nov 28, 2023

Changes to Flash.rs #83

Changes to Flash.rs #83

Conversation

amcelroy commented Sep 14, 2023

systec-ms left a comment • edited Loading

Choose a reason for hiding this comment

amcelroy left a comment

Choose a reason for hiding this comment

systec-ms commented Nov 15, 2023 • 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

systec-ms left a comment •

edited

Loading

systec-ms commented Nov 15, 2023 •

edited

Loading