sam0 flashpage_write: correct translation from RIOT pages to CPU pages writing by fedepell · Pull Request #10069 · RIOT-OS/RIOT

fedepell · 2018-09-28T09:31:44Z

Contribution description

Solve erroneous writing on flash with sam0 boards because of wrong assumption of page sizing. Discovered on a saml21-xpro board.
A "RIOT page" is 256 bytes but a physical page on sam0 is 64, therefore the write has to be translated into four 64 writes.

Testing procedure

The automated test tests/periph_flashpage can be used to verify the correctness of the PR.

Issues/PRs references

Fixes: #10068

jcarrano

@fedepell

I have a SAML21 Xplained here. I will test this if you provide a "hands-free" (i.e. automated) test app. Also:

Fix codacy issue.
Fix formatting.

fedepell · 2018-09-28T10:25:41Z

@jcarrano : formatting should be fixed, sorry for that. Codacy should also be better now, an earlier casting is needed since no arithmetic can be done with void*.

As for test: it is not immediate but I did if using the OTA firmware update at: https://github.com/kYc0o/RIOT/tree/wip/rebase/ota_work_branch
I don't know if there is (or could be added?) an automated test for this (in theory it is simple, but is potentially destructive as writes directly a flash page).

The number 4 actually is hardcoded also in the header file mentioned, but yes maybe it could be made a define and used where needed:

/* one SAM0 row contains 4 SAM0 pages -> 4x the amount of RIOT flashpages */
#define FLASHPAGE_NUMOF            (FLASH_NB_OF_PAGES / 4)

jcarrano · 2018-09-28T10:31:17Z

I don't know if there is (or could be added?) an automated test for this (in theory it is simple, but is potentially destructive as writes directly a flash page).

Destructive in the sense that it wears flash? In that case flashing the test binary is just as destructive.
If you are worried about overwriting a part of the program, you can fix that by declaring a const array of page size, aligned to a page boundary and that should reserve a space that you can safely overwrite.

dylad · 2018-09-28T12:15:11Z

@jcarrano
What about tests/periph_flashpage ?
This test is intend to use this driver and play with the flash memory.

jcarrano · 2018-09-28T13:26:57Z

This test is intend to use this driver and play with the flash memory.

I don't want to write stuff in a terminal. I want to push for PRs that test themselves and contribute to enhance the RIOT test suite. That means automated tests. I do not object if somebody wants to test it manually, though.

dylad · 2018-09-28T13:30:14Z

I want to push for PRs that test themselves and contribute to enhance the RIOT test suite.

I understand your PoV but this is out of the scope of this PR.

aabadie · 2018-09-28T13:30:53Z

I want to push for PRs that test themselves and contribute to enhance the RIOT test suite. That means automated tests

I see this as a good candidate for its own PR => just add an autotest shell command that checks the different features of flashpage, call it from a Python script and parse the output.
But I also think this is out of the scope of this PR.

fedepell · 2018-09-28T15:41:31Z

Hi,
Regarding a manual test of this PR the already present test tests/periph_flashpage that @dylad mentioned indeed shows openly the problem on the board:

-) Without patch:

ROM flash read write test
Please refer to the README.md for further information

Flash start addr:       0x00000000
Page size:              256
Number of pages:        1024
> 
> dump_local
Local page buffer:
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
> write 100
error: verification for page 100 failed
> read 100
Read flash page 100 into local page buffer
Local page buffer:
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
>

You see the write fails explicitly and just the last part of flash is written while the rest is 0xff which means just erased.

-) With patch applied:


> dump_local
Local page buffer:
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
> write 100
wrote local page buffer to flash page 100 at addr 0x6400
> read 100
Read flash page 100 into local page buffer
Local page buffer:
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
>

So seems to work (of course I just tested on my saml21-xpro).

Regarding automated test: I totally agree there should be some. I will try to study a bit myself the test part of RIOT (I'm rather new, need some time :) ) and see if I can contribute something in that direction!

Thanks for comments and insights!

fedepell · 2018-09-30T06:20:54Z

@aabadie :

I see this as a good candidate for its own PR => just add an autotest shell command that checks the different features of flashpage, call it from a Python script and parse the output.
But I also think this is out of the scope of this PR.

I've looked around the source tree and didn't see any other similar example already in place. If there is one could you point me at it?
It sounds quite simple to do and I'd like to add it (in the periph_flash there is already a "test" which could be just called, as it does indeed a erase/write/read+compare cycle, or add an autotest with more stuff as well) but if there is something "standard"-ish I would like to follow that.
Thanks!

aabadie · 2018-09-30T09:14:22Z

@fedepell, have a look at the shell test application. There are shell commands defined (for example start_test) and a python script that call each of them and check their output using the pexpect module.
With pexpect, you can use regexp to finely parse the output.

The python script is run using the test target:

make flash test

So the idea would to call the test shell command and parse its output from the python script.

fedepell · 2018-10-02T21:10:40Z

@aabadie : thanks for the pointer! Please give a look at #10106 and let me know if it sounds good (or at least a good start :) ! Thanks!

aabadie · 2018-10-04T06:35:46Z

There is an automated test now: #10106 is merged. Please rebase and adapt the testing procedure of this PR.

fedepell · 2018-10-04T07:24:40Z

@aabadie : I rebased to master and corrected the testing procedure, hope it is fine.

Regarding @jcarrano request about removing that "4": I would give that a look in depth asap, but should we tackle it as a separate PR since it's something that was there already before (to make that change more traceable and not mixed into this one)?

fedepell · 2018-10-06T04:29:19Z

I tried to clean up a bit the PR this morning, hope it's getting better.

@jcarrano : I have removed the magic 4 and put a define instead and improved the comments (the old comment was actually stating the opposite). I also corrected a bit the code formatting.

About the fact that this "4" could be in principle any number: at the moment the code makes quite an assumption that this number is 4. To make this number easily changeable we should take into account that at least:
-) page erasing erases a row, so 4 at a time. In a generic case we should take this into account. If a multiple of 4 would be easy to implement (just loop over erasing pages), if not a multiple could be very complicated to implement (as partial pages cannot be erased, we would need to save parts of page)
-) data should be anyway contiguous. So for example 4 pages and 256 length, 8 and 512 and so on. If not then also higher level functions (such as flashpage_addr()) should be changed to take this into account (but we would be wasting flash).

I did a manual test on 8/512 (with two erases) and then it works but this is not a generic solution and I'm not sure it makes sense to implement one. 4 is quite hardware optimized (given that 4 pages on SAM0 are a row) and I don't see any performance improvement in going on higher multiples (given the size of flash we have, could make sense if we had very big flash memories).

Thanks!
F.

fedepell · 2018-10-16T04:26:08Z

@aabadie , @jcarrano , @dylad :
Is there anything else I may add/correct to this PR to get it merged? Was looking forward for it upstream as it is a showstopper for remote firmware upgrade for this arch. Many thanks!

dylad · 2018-10-28T13:43:21Z

I ran some tests on arduino-zero like board. automated tests work well.
@jcarrano @aabadie Do you have any more comments ?

fedepell · 2018-12-20T19:47:06Z

@dylad : sorry for the delay, didn't have the board with me in daytime today :)

So all clear now! The test_last_raw does the write with flashpage_write_raw but does not erase the page first, therefore the write de facto fails (but there is no reporting back about this, since you can only know it failed if you re-read and compare). If you first erase the page then everything works fine.

This explains why test_last works: in that case flashpage_write is called which first erases the page, then writes with flashpage_write_raw (in the new loop that was modified by this PR).

So the code of this PR should be fine I believe, what should be fixed is the test (put an erase before the call to flash_write_raw). Question: should I do this in this PR (with a separate commit) or a separate one or what do you suggest is the best?

Here is the output showing what I wrote above:

> main(): This is RIOT! (Version: 2018.10-RC1-622-ga0054-sam0_flashwrite)
ROM flash read write test

Please refer to the README.md for main(): This is RIOT! (Version: 2018.10-RC1-622-ga0054-sam0_flashwrite)
ROM flash read write test

Please refer to the README.md for further information

Flash start addr:       0x00000000
Page size:              256
Number of pages:        1024
> test_last
wrote local page buffer to last flash page
> read 1022
Read flash page 1022 into local page buffer
Local page buffer:
0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 
  a    b    c    d    e    f    g    h    i    j    k    l    m    n    o    p  
0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 
  q    r    s    t    u    v    w    x    y    z    a    b    c    d    e    f  
0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 
  g    h    i    j    k    l    m    n    o    p    q    r    s    t    u    v  
0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 
  w    x    y    z    a    b    c    d    e    f    g    h    i    j    k    l  
0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 
  m    n    o    p    q    r    s    t    u    v    w    x    y    z    a    b  
0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 
  c    d    e    f    g    h    i    j    k    l    m    n    o    p    q    r  
0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 
  s    t    u    v    w    x    y    z    a    b    c    d    e    f    g    h  
0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 
  i    j    k    l    m    n    o    p    q    r    s    t    u    v    w    x  
0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 
  y    z    a    b    c    d    e    f    g    h    i    j    k    l    m    n  
0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 
  o    p    q    r    s    t    u    v    w    x    y    z    a    b    c    d  
0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 
  e    f    g    h    i    j    k    l    m    n    o    p    q    r    s    t  
0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 
  u    v    w    x    y    z    a    b    c    d    e    f    g    h    i    j  
0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 
  k    l    m    n    o    p    q    r    s    t    u    v    w    x    y    z  
0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 
  a    b    c    d    e    f    g    h    i    j    k    l    m    n    o    p  
0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 
  q    r    s    t    u    v    w    x    y    z    a    b    c    d    e    f  
0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 
  g    h    i    j    k    l    m    n    o    p    q    r    s    t    u    v  
> test_last_raw
wrote raw short buffer to last flash page
> read 1022
Read flash page 1022 into local page buffer
Local page buffer:
0x60 0x60 0x63 0x64 0x21 0x22 0x23 0x20 0x20 0x22 0x22 0x20 0x64 0x62 0x65 0x70 
  `    `    c    d    !    "    #              "    "         d    b    e    p  
0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 
  q    r    s    t    u    v    w    x    y    z    a    b    c    d    e    f  
0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 
  g    h    i    j    k    l    m    n    o    p    q    r    s    t    u    v  
0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 
  w    x    y    z    a    b    c    d    e    f    g    h    i    j    k    l  
0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 
  m    n    o    p    q    r    s    t    u    v    w    x    y    z    a    b  
0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 
  c    d    e    f    g    h    i    j    k    l    m    n    o    p    q    r  
0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 
  s    t    u    v    w    x    y    z    a    b    c    d    e    f    g    h  
0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 
  i    j    k    l    m    n    o    p    q    r    s    t    u    v    w    x  
0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 
  y    z    a    b    c    d    e    f    g    h    i    j    k    l    m    n  
0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 
  o    p    q    r    s    t    u    v    w    x    y    z    a    b    c    d  
0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 
  e    f    g    h    i    j    k    l    m    n    o    p    q    r    s    t  
0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 
  u    v    w    x    y    z    a    b    c    d    e    f    g    h    i    j  
0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 
  k    l    m    n    o    p    q    r    s    t    u    v    w    x    y    z  
0x61 0x62 0x63 0x64 0x65 0x66 0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 
  a    b    c    d    e    f    g    h    i    j    k    l    m    n    o    p  
0x71 0x72 0x73 0x74 0x75 0x76 0x77 0x78 0x79 0x7a 0x61 0x62 0x63 0x64 0x65 0x66 
  q    r    s    t    u    v    w    x    y    z    a    b    c    d    e    f  
0x67 0x68 0x69 0x6a 0x6b 0x6c 0x6d 0x6e 0x6f 0x70 0x71 0x72 0x73 0x74 0x75 0x76 
  g    h    i    j    k    l    m    n    o    p    q    r    s    t    u    v  
> erase 1022
successfully erased page 1022 (addr 0x3fe00)
> test_last_raw
wrote raw short buffer to last flash page
> read 1022
Read flash page 1022 into local page buffer
Local page buffer:
0x74 0x65 0x73 0x74 0x31 0x32 0x33 0x34 0x34 0x33 0x32 0x31 0x74 0x73 0x65 0x74 
  t    e    s    t    1    2    3    4    4    3    2    1    t    s    e    t  
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ?? 
0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 
  ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??   ??

Thanks!

dylad · 2018-12-20T20:16:57Z

@fedepell I'm fine with another commit here since this is not a 'real' bug. Thanks for re-testing. I'll retry tomorrow on hardware.

fedepell · 2018-12-20T20:21:07Z

@dylad : added commit with the modification to the test_last_raw command. Hope everything will be fine now! :)

dylad · 2018-12-21T09:52:01Z

@fedepell your fix works like a charm ! However I still get a kernel panic if I try to use thewrite 1023 command. I think it would be better to fix it now otherwise you will get weird behaviour later while playing with bootloader.

dylad · 2018-12-21T10:09:25Z

RIOT/cpu/sam0_common/periph/flashpage.c

Lines 70 to 72 in 7196120

    
           /* ensure the length doesn't exceed the actual flash size */ 
        
           assert(((unsigned)target_addr + len) < 
        
                  (CPU_FLASH_BASE + (FLASHPAGE_SIZE * FLASHPAGE_NUMOF)));

I think the < condition should be <= otherwise we cannot write the very last byte of flash.

fedepell · 2018-12-21T10:50:47Z

@dylad : thanks for testing!

You are absolutely correct, the assert needs to be >= to be able to write until exactly the last byte! I corrected, tested it succesfully and added a commit.

Let me know if we should squash or not (would in principle say to keep the 3 commits separated as they deal with different issues, so it is easier to track in the future, but let me know!)

dylad · 2018-12-21T10:56:29Z

@fedepell Keep it that way, it's perfect. I'll re-test again.

fedepell · 2018-12-21T10:59:02Z

(sorry, force pushed since I made the comment shorter for Murdock ;) )

dylad · 2018-12-21T11:40:13Z

Could you squash the last commit so we keep 3 commits please ?

fedepell · 2018-12-21T12:13:16Z

Could you squash the last commit so we keep 3 commits please ?

Done!

dylad

All comments have been addressed. Successfully tested on SAML21-XPRO.
Murdock is happy -> Here we go !

dylad · 2018-12-21T13:17:43Z

@fedepell Thanks for your good work !

fedepell · 2018-12-21T13:29:00Z

Great! Thanks to you @dylad and everyone that partecipated! :)

jcarrano previously requested changes Sep 28, 2018

View reviewed changes

Comment thread cpu/sam0_common/periph/flashpage.c Outdated

fedepell force-pushed the sam0_flashwrite branch 2 times, most recently from 8824e06 to d77c280 Compare September 28, 2018 10:14

RIOT-OS deleted a comment Sep 28, 2018

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers and removed Area: cpu Area: CPU/MCU ports labels Sep 30, 2018

fedepell mentioned this pull request Oct 2, 2018

tests/periph_flashpage: add automated test for flash peripheral #10106

Merged

fedepell force-pushed the sam0_flashwrite branch from c000a45 to 4824caf Compare October 4, 2018 07:19

fedepell force-pushed the sam0_flashwrite branch from bd042b1 to 6778004 Compare October 6, 2018 04:02

RIOT-OS deleted a comment Oct 6, 2018

fedepell force-pushed the sam0_flashwrite branch 2 times, most recently from 0c7f036 to 4e5e811 Compare October 6, 2018 04:16

tests/periph_flashpage: test_last_raw must erase page before write

50f2078

fedepell force-pushed the sam0_flashwrite branch from 6e645b1 to 9082675 Compare December 21, 2018 10:58

dylad reviewed Dec 21, 2018

View reviewed changes

Comment thread cpu/sam0_common/periph/flashpage.c Outdated

dylad reviewed Dec 21, 2018

View reviewed changes

Comment thread cpu/sam0_common/include/cpu_conf.h Outdated

sam0 flashpage_write: correct assert for last byte of flash + style

b8b8ffd

fedepell force-pushed the sam0_flashwrite branch from 803dced to b8b8ffd Compare December 21, 2018 12:12

dylad approved these changes Dec 21, 2018

View reviewed changes

dylad merged commit 764c7d2 into RIOT-OS:master Dec 21, 2018

fedepell mentioned this pull request Jan 16, 2019

tests/periph_flashpage: improve flashpage tests #10777

Merged

PeterKietzmann mentioned this pull request Feb 21, 2019

sam0 flashpage_write issue #7667

Closed

Uh oh!

Conversation

fedepell commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fedepell commented Sep 28, 2018

Uh oh!

jcarrano commented Sep 28, 2018

Uh oh!

dylad commented Sep 28, 2018

Uh oh!

jcarrano commented Sep 28, 2018

Uh oh!

dylad commented Sep 28, 2018

Uh oh!

aabadie commented Sep 28, 2018

Uh oh!

fedepell commented Sep 28, 2018

Uh oh!

fedepell commented Sep 30, 2018

Uh oh!

aabadie commented Sep 30, 2018

Uh oh!

fedepell commented Oct 2, 2018

Uh oh!

aabadie commented Oct 4, 2018

Uh oh!

fedepell commented Oct 4, 2018

Uh oh!

fedepell commented Oct 6, 2018

Uh oh!

fedepell commented Oct 16, 2018

Uh oh!

dylad commented Oct 28, 2018

Uh oh!

fedepell commented Dec 20, 2018

Uh oh!

dylad commented Dec 20, 2018

Uh oh!

fedepell commented Dec 20, 2018

Uh oh!

dylad commented Dec 21, 2018

Uh oh!

dylad commented Dec 21, 2018

Uh oh!

fedepell commented Dec 21, 2018

Uh oh!

dylad commented Dec 21, 2018

Uh oh!

fedepell commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dylad commented Dec 21, 2018

Uh oh!

fedepell commented Dec 21, 2018

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

dylad commented Dec 21, 2018

Uh oh!

fedepell commented Dec 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fedepell commented Sep 28, 2018 •

edited

Loading

fedepell commented Dec 21, 2018 •

edited

Loading