drivers/periph_common/flashpage: add offset and length to API by fabian18 · Pull Request #22248 · RIOT-OS/RIOT

fabian18 · 2026-05-06T13:55:36Z

Contribution description

Adds offset and length parameters to flashpage_read() and flashpage_verify() API.

Testing procedure

BOARD=same54-xpro make -C tests/periph/flashpage flash term

Type '/exit' to exit.
help
2026-05-06 15:59:08,235 # help
2026-05-06 15:59:08,237 # Command              Description
2026-05-06 15:59:08,241 # ---------------------------------------
2026-05-06 15:59:08,245 # info                 Show information about pages
2026-05-06 15:59:08,250 # dump                 Dump the selected page to STDOUT
2026-05-06 15:59:08,255 # dump_local           Dump the local page buffer to STDOUT
2026-05-06 15:59:08,263 # read                 Copy the given page to the local page buffer and dump to STDOUT
2026-05-06 15:59:08,268 # write                Write the local page buffer to the given page
2026-05-06 15:59:08,274 # write_raw            Write raw bytes (max 64B) to the given address
2026-05-06 15:59:08,279 # erase                Erase the given page buffer
2026-05-06 15:59:08,284 # edit                 Write bytes to the local page buffer
2026-05-06 15:59:08,288 # test                 Write and verify test pattern
2026-05-06 15:59:08,295 # test_last_pagewise   Write and verify test pattern on last page available
2026-05-06 15:59:08,301 # test_reserved_pagewise Write and verify short write on reserved page
2026-05-06 15:59:08,307 # test_last_raw        Write and verify raw short write on last page available
2026-05-06 15:59:08,313 # dump_config_page     Dump the content of the MCU configuration page
2026-05-06 15:59:08,319 # test_config_page     Test writing config page. (!DANGER ZONE!)
> test
2026-05-06 15:59:18,311 # test
2026-05-06 15:59:18,313 # usage: test <page>
> test 22
2026-05-06 15:59:24,975 # test 22
2026-05-06 15:59:25,031 # wrote local page buffer to flash page 22 at addr 0x0002c000
> test_last_pagewise
2026-05-06 15:59:40,386 # test_last_pagewise
2026-05-06 15:59:40,456 # wrote local page buffer to last flash page
> test_reserved_pagewise
2026-05-06 15:59:54,033 # test_reserved_pagewise
2026-05-06 15:59:54,035 # Reserved page num: 4 
2026-05-06 15:59:54,041 # Since the last firmware update this test has been run 2 times 
2026-05-06 15:59:54,109 # wrote local page buffer to reserved flash page
2026-05-06 15:59:54,109 # 
2026-05-06 15:59:54,120 # When running on a bootloader, as an extra check, try restarting the board and check whether this application still comes up.
> test_last_raw
2026-05-06 16:00:13,445 # test_last_raw
2026-05-06 16:00:13,491 # wrote raw short buffer to last flash page

Issues/PRs references

Declaration of AI-Tools / LLMs usage:

AI-Tools / LLMs that were used are:

none

riot-ci · 2026-05-06T14:38:58Z

Murdock results

✔️ PASSED

42f817e tests/periph/flashpage: test page offset read

Success	Failures	Total	Runtime
11108	0	11108	12m:34s

Artifacts

Documentation preview

crasbe

Sorry for highjacking your PR for unrelated style fixes 😅

crasbe · 2026-05-06T20:47:03Z

+void flashpage_read(unsigned page, void *data, size_t offset, size_t len)
 {
    assert(page < FLASHPAGE_NUMOF);
+    assert(offset + len <= flashpage_size(page) && offset + len > offset);


Isn't offset + len > offset just a fancy way of writing len > 0 or len != 0, considering size_t is an unsigned integer under the hood?

I don't try to be fancy :)
This should catch overflow.

Well yes, but the only case where the condition wouldn't be met, is when len is zero.

If I am not mistaken, than for example (10 + SIZE_MAX) overflows to 9. 9 is less than page size (8K for same54-xpro), but the second check 9 > 10 catches the overflow.

crasbe · 2026-05-06T20:47:29Z

 * @return              FLASHPAGE_OK if data in the RWWEE page is identical to @p data
 * @return              FLASHPAGE_NOMATCH if data and RWWEE page content diverge


Suggested change

* @return FLASHPAGE_OK if data in the RWWEE page is identical to @p data

* @return FLASHPAGE_NOMATCH if data and RWWEE page content diverge

* @retval FLASHPAGE_OK if data in the RWWEE page is identical to @p data

* @retval FLASHPAGE_NOMATCH if data and RWWEE page content diverge

Unrelated to your changes, but while we're here 👀

crasbe · 2026-05-06T20:48:34Z

Suggested change

od_hex_dump(addr, len, LINE_LEN);

Unrelated static-test warning fix.

crasbe · 2026-05-06T20:50:06Z

Suggested change

/* Make each page slightly different by changing starting char for easier comparison by eye */

fill += (page % ('z' - 'a'));

Unrelated comment style fix.

crasbe · 2026-05-06T20:51:05Z

Now that we have offset and length, wouldn't it make sense to also test that specifically?
The test still reads a whole page 🤔

crasbe · 2026-05-06T20:51:33Z

 * @return              FLASHPAGE_OK if data in the page is identical to @p data
 * @return              FLASHPAGE_NOMATCH if data and page content diverge


Suggested change

* @return FLASHPAGE_OK if data in the page is identical to @p data

* @return FLASHPAGE_NOMATCH if data and page content diverge

* @retval FLASHPAGE_OK if data in the page is identical to @p data

* @retval FLASHPAGE_NOMATCH if data and page content diverge

crasbe · 2026-05-06T20:52:19Z

Suggested change

* @retval FLASHPAGE_OK on success

* @retval FLASHPAGE_NOMATCH if data and page content diverge

crasbe · 2026-05-06T20:52:34Z

Suggested change

* @retval FLASHPAGE_OK on success

* @retval FLASHPAGE_NOMATCH if data and RWWEE page content diverge

crasbe · 2026-05-06T20:54:09Z

 * @param[out] data     memory to write the page to, MUST be FLASHPAGE_SIZE
 *                      byte
+ * @param[in] offset    offset in the page to start reading from
+ * @param[in] len       length of the data to be read.


Suggested change

* @param[in] len length of the data to be read.

* @param[in] len length of the data to be read (must be bigger than 0).

If my assumption in the previous comment is correct, this would be good to add IMO.

kfessel

not sure why the functions keep their name if they are no longer page bound

kfessel · 2026-05-07T15:11:01Z

+ * @pre     @p offset + @p len MUST be less than or equal to FLASHPAGE_SIZE.
 */
-void flashpage_read(unsigned page, void *data);
+void flashpage_read(unsigned page, void *data, size_t offset, size_t len);


why keep the name if it no longer reads the entire flash page?

The name is not fundamentally wrong. Neither does flashpage_write necessarily write a full page.
It depends on whether we want to offer 2 APIs to get around API change and people not having to adapt to that.
Or have 1 API which is more compact. I don't like to read large headers and figure out which function to use.
We have some functions which are called _read_byte, when there is a general read, which I find is a bit overengineered. The nightmare of that coap_. So on personal preference I simply chose to extend the existing API, instead of introducing a new one.

flashpage_write seems to be to the lower level API point - it does not even take a page as input

if write and read would share a naming scheme the original read should have been named flashpage_read_page (see flashpage_write_page for comparison) but in that case this function should be void flashpage_read(void *target_addr, const void *data, size_t len);

similar for _read_byte functions these are often the result of a lower level that provides byte based access and than a higher level is put on top that basically loops for how many bytes should be read.

This is what I would call overengineering.

uint8_t eeprom_read_byte(uint32_t pos) { uint8_t byte; eeprom_read(pos, &byte, 1); return byte; }

But _read_byte functions are not the main topic here. There is still nothing fundamentally wrong.
What do you want?

Having flashpage_read_page() to read a full page requires people to adapt and having this PR's API change also requires people to adapt.

Keeping flashpage_read() as is, and introducing flashpage_offset_read() requires noone to adapt, but for me counts as API overengineering

flashpage_offset_read would be my preference to cause less api break downstream

kfessel · 2026-05-07T15:12:20Z

+ * @retval              FLASHPAGE_NOMATCH if data and page content diverge
 */
-int flashpage_verify(unsigned page, const void *data);
+int flashpage_verify(unsigned page, const void *data, size_t offset, size_t len);


why keep the name if it no longer ensures the entire flash page matches

kfessel · 2026-05-07T15:16:07Z

+void flashpage_read(unsigned page, void *data, size_t offset, size_t len);

 /**
 * @brief   Verify the given page against the given data


Suggested change

* @brief Verify the given page against the given data

* @brief Verify the given page or half a page or two pages or ... against the given data

drivers/periph_common/flashpage: add offset and length to API

8ac689c

fabian18 requested review from MichelRottleuthner, MrKevinWeiss, aabadie and leandrolanzieri as code owners May 6, 2026 13:55

github-actions Bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers labels May 6, 2026

crasbe reviewed May 6, 2026

View reviewed changes

fabian18 added 3 commits May 7, 2026 10:22

drivers/periph_common/flashpage: use retval in Doxygen

9361dfb

tests/periph/flashpage: style fix

3508f79

tests/periph/flashpage: test page offset read

42f817e

kfessel reviewed May 7, 2026

View reviewed changes

		* @return FLASHPAGE_OK if data in the RWWEE page is identical to @p data
		* @return FLASHPAGE_NOMATCH if data and RWWEE page content diverge


	/* Make each page slightly different by changing starting char for easier comparison by eye */
	fill += (page % ('z' - 'a'));

		* @return FLASHPAGE_OK if data in the page is identical to @p data
		* @return FLASHPAGE_NOMATCH if data and page content diverge


	* @retval FLASHPAGE_OK on success
	* @retval FLASHPAGE_NOMATCH if data and page content diverge

	* @param[in] len length of the data to be read.
	* @param[in] len length of the data to be read (must be bigger than 0).

	* @brief Verify the given page against the given data
	* @brief Verify the given page or half a page or two pages or ... against the given data

Conversation

fabian18 commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Declaration of AI-Tools / LLMs usage:

Uh oh!

riot-ci commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

crasbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfessel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fabian18 commented May 6, 2026 •

edited

Loading

riot-ci commented May 6, 2026 •

edited

Loading