periph/flashpage: extend API #16972

Ollrogge · 2021-10-11T16:27:55Z

Contribution description

This PR extends the flashpage API with two functions that return information about the first and last free flashpage.
This is supposed to make it easier store data in flash without accidentally corrupting e.g. firmware or bootloaders.

I am aware that as of now this only adds the function implementations for Cortex-M CPUs.

I am opening this PR to start a discussion if this approach is correct and a reasonable addition to the API. If it is, I will also add function implementations for other CPUs implementing the flashpage APl.

benpicco

Please also extend tests/periph_flashpage so we can see / verify the output of those functions.

cpu/nrf5x_common/periph/flashpage.c

cpu/cortexm_common/periph/flashpage.c

Ollrogge · 2021-10-13T20:28:31Z

@benpicco Added the symbols to riscv linker script and lpc23xx. I know that msp430_common also implements the flashpage interface but I haven't found a good solution how to add the sections without touching the vendor linker scripts.

benpicco · 2021-10-14T19:57:34Z

Looks like you missed esp8266/32

Ollrogge · 2021-10-15T08:22:04Z

Looks like you missed esp8266/32

I don't think esp8266/32 implements the flashpage driver ?

benpicco

Ah right the files in drivers/periph_common all get compiled always - so you'll need

drivers/periph_common/flashpage.c

fjmolinas · 2021-10-18T07:59:37Z

Looks like you missed esp8266/32

I think its msp430 that's missing...

fjmolinas · 2021-10-25T11:29:14Z

@MrKevinWeiss do you have remote access to the frdm-k22f? Could you paste output of:

test_last_raw in master and

info in this PR?

riot-hil-bot · 2021-10-25T11:52:17Z

HiL Test Results

PASS	FAIL	SKIP
0	1	0

❌ frdm-k22f (1 fail test)

PASS	FAIL	SKIP
0	1	0

TEST	RESULT
tests/periph_flashpage	❌ test fail

MrKevinWeiss · 2021-10-25T11:52:38Z

master

> info
2021-10-25 12:48:59,557 # info
2021-10-25 12:48:59,563 # Flash start addr:	0x00000000
2021-10-25 12:48:59,566 # Page size:		4096
2021-10-25 12:48:59,570 # Number of pages:	128
> test_last_raw
2021-10-25 12:49:06,536 # test_last_raw
2021-10-25 12:49:06,547 # error verifying the content of last page
>

this PR

info
2021-10-25 12:52:29,782 # info
2021-10-25 12:52:29,788 # Flash start addr:		0x00000000
2021-10-25 12:52:29,792 # Page size:			4096
2021-10-25 12:52:29,796 # Number of pages:		128
2021-10-25 12:52:29,803 # Number of first free page: 	4 
2021-10-25 12:52:29,809 # Number of last free page: 	127 
test_last_raw
2021-10-25 12:52:31,430 # test_last_raw
2021-10-25 12:52:31,439 # error verifying the content of last page

fjmolinas · 2021-10-25T11:55:02Z

master

> info
2021-10-25 12:48:59,557 # info
2021-10-25 12:48:59,563 # Flash start addr:	0x00000000
2021-10-25 12:48:59,566 # Page size:		4096
2021-10-25 12:48:59,570 # Number of pages:	128
> test_last_raw
2021-10-25 12:49:06,536 # test_last_raw
2021-10-25 12:49:06,547 # error verifying the content of last page
>

this PR

info
2021-10-25 12:52:29,782 # info
2021-10-25 12:52:29,788 # Flash start addr:		0x00000000
2021-10-25 12:52:29,792 # Page size:			4096
2021-10-25 12:52:29,796 # Number of pages:		128
2021-10-25 12:52:29,803 # Number of first free page: 	4 
2021-10-25 12:52:29,809 # Number of last free page: 	127 
test_last_raw
2021-10-25 12:52:31,430 # test_last_raw
2021-10-25 12:52:31,439 # error verifying the content of last page

Ok so test does not pass in master either

Ollrogge · 2021-10-25T12:41:15Z

For the remote:

Please refer to the README.md for further information

Flash start addr:		0x00200000
Page size:			2048
Number of pages:		255
Number of first free page: 	8 
Number of last free page: 	255

Shouldn't the number of the last free page be 254?

Yes it should. I think I could fix the remote-revb error by adjusting the rom definition in cc2538.ld to account for the cca region:

_cca_length = 44;

MEMORY
{
    rom (rx)    : ORIGIN = _rom_start_addr + _rom_offset, LENGTH = _fw_rom_length - _cca_length
    cca           : ORIGIN = 0x0027ffd4, LENGTH = _cca_length
    sram0       : ORIGIN = 0x20000000, LENGTH = 16K /* Lost in PM2 and PM3 */
    sram1       : ORIGIN = 0x20004000, LENGTH = 16K
    ram (w!rx)  : ORIGIN = _ram_start_addr, LENGTH = _ram_length
}

Ollrogge · 2021-10-26T08:58:30Z

@fjmolinas can you test if it works on openmote-b now ?

fjmolinas

Testing on openmote-b

tests/periph_flashpage

Connect to serial port /dev/riot/tty-openmote-b
Welcome to pyterm!
Type '/exit' to exit.

> 

> test_last_raw

> test_last_raw
wrote raw short buffer to last flash page
> help
help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write                Write the local page buffer to the given page
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test                 Write and verify test pattern
test_last_pagewise   Write and verify test pattern on last page available
test_last_pagewise
test_last_raw        Write and verify raw short write on last page available
> test_last_pagewise
wrote local page buffer to last flash page
> help
help
Command              Description
---------------------------------------
info                 Show information about pages
dump                 Dump the selected page to STDOUT
dump_local           Dump the local page buffer to STDOUT
read                 Copy the given page to the local page buffer and dump to STDOUT
write                Write the local page buffer to the given page
write_raw            Write (ASCII, max 64B) data to the given address
erase                Erase the given page buffer
edit                 Write bytes to the local page buffer
test                 Write and verify test pattern
test_last_pagewise   Write and verify test pattern on last page available
test_last_raw        Write and verify raw short write on last page available
>

examples/suit_update

Running from slot 0
> ifconfig
Iface  4  HWaddr: EE:6B:EE:D1:62:F7 
          L2-PDU:1500  MTU:1500  HL:64  RTR  
          Source address length: 6
          Link type: wired
          inet6 addr: fe80::ec6b:eeff:fed1:62f7  scope: link  VAL
pinging node...
PING fe80::ec6b:eeff:fed1:62f7%riot0(fe80::ec6b:eeff:fed1:62f7%riot0) 56 data bytes

--- fe80::ec6b:eeff:fed1:62f7%riot0 ping statistics ---
1 packets transmitted, 1 received, 0% packet loss, time 0ms
rtt min/avg/max/mdev = 57.773/57.773/57.773/0.000 ms
pinging node succeeded.
TEST PASSED

tests/riotboot

> getslotaddr 0
getslotaddr 0
Slot 0 address=0x00201400
> dumpaddrs
dumpaddrs
slot 0: metadata: 0x201000 image: 0x00201400
slot 1: metadata: 0x240000 image: 0x00240400
> 
TEST PASSED

One thing is that I notice is that when using riotboot 2 pages are subtracted from ROM_LEN to not write over the CCA region, this probably means that an extra 44 bytes are lost from the last page. But I think the extra complexity in the build system to sabe thos extra 44bytes is out of scope here.

fjmolinas · 2021-10-26T09:25:18Z

@MrKevinWeiss can you open an issue for frdm-kw22f and the failing test? That way this one can go in

fjmolinas

ACK, failing tests as reported by @MrKevinWeiss are also failing in master, see #17057.

drivers/include/periph/flashpage.h

PeterKietzmann · 2021-11-11T13:22:24Z

Was it intended to add the new files (flashpage.c) to the doxygen group drivers_periph_pm? If so, why?

Ollrogge · 2021-11-15T07:23:03Z

Was it intended to add the new files (flashpage.c) to the doxygen group drivers_periph_pm? If so, why?

That is a stupid mistake on my side because I copied the header from another file. It should be drivers_periph_flashpage not drivers_periph_pm. Should I open a follow up for this ?

PeterKietzmann · 2021-11-15T07:50:22Z

Of course :)

Ollrogge · 2021-12-01T08:49:36Z

In a follow up PR based on the functionality introduced in this PR, concerns about the necessity of the functionality were raised. I think the mentioned arguments are correct and reduce the necessity of the flashpage_*_free functions.

What are you opinions on this @benpicco @fjmolinas ?

Since commit 3a11b1f (RIOT-OS#16972) building RIOT applications with `BOARD=hifive1` causes the following linker error to be emitted on my system: /opt/rv32imc/lib/gcc/riscv32-unknown-elf/10.2.0/../../../../riscv32-unknown-elf/bin/ld:riscv_base.ld:220: warning: memory region `rom' not declared This is due to the fact that the RISC-V linker script doesn't have a rom memory region. While many other ARM-based boards have a rom memory region defined in the linker script, the corresponding region name in the RISC-V linker script is flash and rom is not declared as a memory region hence the warning. I think this was accidentally overlooked in 3a11b1f. It is fixed in this commit by replacing the rom region with the flash region. The linker script identifiers (e.g. _srom and _erom) are not renamed.

Ollrogge requested review from aabadie, bergzand, haukepetersen, kaspar030 and MrKevinWeiss as code owners October 11, 2021 16:27

github-actions bot added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Oct 11, 2021

benpicco reviewed Oct 11, 2021

View reviewed changes

cpu/nrf5x_common/periph/flashpage.c Outdated Show resolved Hide resolved

cpu/nrf5x_common/periph/flashpage.c Outdated Show resolved Hide resolved

Ollrogge requested review from fjmolinas, leandrolanzieri, MichelRottleuthner and smlng as code owners October 12, 2021 07:33

github-actions bot added the Area: tests Area: tests and testing framework label Oct 12, 2021

benpicco reviewed Oct 12, 2021

View reviewed changes

cpu/cortexm_common/periph/flashpage.c Show resolved Hide resolved

Ollrogge force-pushed the flashpage_pr branch from 209939e to 7309c9c Compare October 13, 2021 20:26

github-actions bot added the Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms label Oct 13, 2021

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 14, 2021

benpicco reviewed Oct 15, 2021

View reviewed changes

drivers/periph_common/flashpage.c Outdated Show resolved Hide resolved

drivers/periph_common/flashpage.c Outdated Show resolved Hide resolved

Ollrogge force-pushed the flashpage_pr branch from 7309c9c to 42e4e78 Compare October 15, 2021 11:26

Ollrogge force-pushed the flashpage_pr branch from 42e4e78 to 0babf55 Compare October 18, 2021 21:51

Ollrogge requested a review from gschorcht as a code owner October 18, 2021 21:51

github-actions bot added Area: build system Area: Build system Platform: MSP Platform: This PR/issue effects MSP-based platforms labels Oct 18, 2021

Ollrogge force-pushed the flashpage_pr branch from 0babf55 to 1f28e34 Compare October 19, 2021 07:41

github-actions bot added the Platform: native Platform: This PR/issue effects the native platform label Oct 19, 2021

Ollrogge added 5 commits October 25, 2021 15:03

cpu/msp430_common: extend flashpage API

741943f

cpu/riscv_common: extend flashpage API

3a11b1f

cpu/lpc23xx: extend flashpage API

4fa25af

cpu/native: extend flashpage API

cdd9f30

cpu/cc2538: adjust rom size to account for cca region

be254d4

Ollrogge force-pushed the flashpage_pr branch from aeb4404 to be254d4 Compare October 25, 2021 13:14

fjmolinas reviewed Oct 26, 2021

View reviewed changes

MrKevinWeiss mentioned this pull request Oct 26, 2021

frdm-k22f failing tests/periph_flashpage #17057

Open

fjmolinas approved these changes Oct 26, 2021

View reviewed changes

fjmolinas merged commit 7f33448 into RIOT-OS:master Oct 26, 2021

benpicco reviewed Oct 26, 2021

View reviewed changes

drivers/include/periph/flashpage.h Show resolved Hide resolved

This was referenced Oct 26, 2021

periph/flashpage.h: add comment about riotboot impact on flashpage la… #17061

Merged

Fido2 follow up #17096

Merged

Ollrogge mentioned this pull request Nov 15, 2021

periph/flashpage: fix wrong doxygen group #17202

Merged

fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021

nmeum mentioned this pull request Jan 27, 2022

cpu/riscv_common: fix undeclared memory region linker error #17581

Merged

Ollrogge mentioned this pull request Mar 24, 2022

periph/flashpage: deprecate *_free functions #17860

Merged

Ollrogge mentioned this pull request May 12, 2022

periph/flashpage: remove deprecated flashpage_*_free functions #18093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

periph/flashpage: extend API #16972

periph/flashpage: extend API #16972

Ollrogge commented Oct 11, 2021

benpicco left a comment

Ollrogge commented Oct 13, 2021

benpicco commented Oct 14, 2021

Ollrogge commented Oct 15, 2021

benpicco left a comment

fjmolinas commented Oct 18, 2021

fjmolinas commented Oct 25, 2021

riot-hil-bot commented Oct 25, 2021

MrKevinWeiss commented Oct 25, 2021

fjmolinas commented Oct 25, 2021

Ollrogge commented Oct 25, 2021 •

edited

Loading

Ollrogge commented Oct 26, 2021

fjmolinas left a comment •

edited

Loading

fjmolinas commented Oct 26, 2021

fjmolinas left a comment

PeterKietzmann commented Nov 11, 2021

Ollrogge commented Nov 15, 2021

PeterKietzmann commented Nov 15, 2021

Ollrogge commented Dec 1, 2021

periph/flashpage: extend API #16972

periph/flashpage: extend API #16972

Conversation

Ollrogge commented Oct 11, 2021

Contribution description

benpicco left a comment

Choose a reason for hiding this comment

Ollrogge commented Oct 13, 2021

benpicco commented Oct 14, 2021

Ollrogge commented Oct 15, 2021

benpicco left a comment

Choose a reason for hiding this comment

fjmolinas commented Oct 18, 2021

fjmolinas commented Oct 25, 2021

riot-hil-bot commented Oct 25, 2021

MrKevinWeiss commented Oct 25, 2021

fjmolinas commented Oct 25, 2021

Ollrogge commented Oct 25, 2021 • edited Loading

Ollrogge commented Oct 26, 2021

fjmolinas left a comment • edited Loading

Choose a reason for hiding this comment

fjmolinas commented Oct 26, 2021

fjmolinas left a comment

Choose a reason for hiding this comment

PeterKietzmann commented Nov 11, 2021

Ollrogge commented Nov 15, 2021

PeterKietzmann commented Nov 15, 2021

Ollrogge commented Dec 1, 2021

Ollrogge commented Oct 25, 2021 •

edited

Loading

fjmolinas left a comment •

edited

Loading