drivers: add mtd wrapper for periph_flashpage #8774

vincent-d · 2018-03-13T15:57:36Z

Contribution description

This adds a basic mtd wrapper around perip_flashpage interface. periph_flashpage_raw is needed to write.

Issues/PRs references

Based on #8773

aabadie · 2018-03-13T16:03:40Z

drivers/Makefile.dep

  USEMODULE += mtd
+endif
+
+ifneq (,$(filter mtd_sdcard,$(USEMODULE)))


Could all mtd_ sub modules be nested in the generic block mtd_% ? This would avoid confusion I think.

kYc0o

Just a comment on this, I need more info about the purpose of this PR.

kYc0o · 2018-03-14T23:16:59Z

drivers/include/mtd_flashpage.h

+    .sector_count = FLASHPAGE_NUMOF, \
+    .pages_per_sector = _pages_per_sector,\
+    .page_size = FLASHPAGE_SIZE / _pages_per_sector,\
+}


I'm not really sure if this is a good idea... What's the purpose of having this wrapper? The biggest problem I see is that you'd hardly know where you can write without precise knowledge about the non-filled flash space left when flashing a firmware... The main purpose of the flashpage driver is to write new program data, or extend the current one (new elf files, scripts of any kind, etc.), and, if I understand correctly, this driver aims to write user data...

The goal is to be able to use a file system on internal flash.

It's then completely up to the developer to be sure of what's he's doing and it might be dangerous, of course.

I see the point, but I have the issue of erasing. How do you cope with the small erases case? You need to always backup the whole block in RAM before adding something, then erase the whole block and write it from the buffer...

I don't understand your point here. This is the case with any flash, either internal or not.
A file system designed to run on a flash will not try to erase a few bytes, but always the entire sector. What is different here?

So what you're saying is that whenever you do a write using the MTD, you rely on the file system to do it correctly? In some devices the flash page size is huge (2KB for stm32f103re), that's why I'm concerned about using it in that fine-grained way.

Most SPI flashes have a sector size of 4KB, or even 64KB. And a typical "page" size (i.e. maximum number of bytes you can write with a single instruction) is 256B on these flashes.

Then yes, we rely on the file system to manage erases and write cycles.

So, in overall (a bit out of scope now) using ANY file system over a flash-based MTD will take at least as much RAM as the smallest erasable block, is it? I just checked and for the N25Q128 series (the one attached to the iotlab-m3) has 4KB as eraseable sub-sector.

If you take the assumption that a sector will be modified, this will be true. But a good filesystem won't need to modify a byte in the middle of a sector. See littles or spiffs, they don't need a buffer of the size of a sector.

kYc0o

I'm ACKing this but I'm still not super convinced about its usefulness. I'd say it's a very experimental feature and since it doesn't break anything it doesn't hurt to have it as an option. If someone else cares about it please express it. Otherwise @vincent-d you can merge it before the feature freeze.

kYc0o · 2018-04-11T13:18:43Z

Please squash so we have Murdock happy.

aabadie

I like this feature because it allows to use a lot more available space for storing data.
I also agree with @kYc0o that this space is normally meant for program data but most of the time a RIOT based firmware is less than 200kB (and I'm large). Fro example, with a flash of 1MB on a L476, there's quite some space left.

Otherwise I found a minor indentation issue and have a comment regarding the fact by default the MTD can potentially takes the whole flash.

aabadie · 2018-04-13T09:42:00Z

tests/unittests/tests-mtd_flashpage/tests-mtd_flashpage.h

+#endif
+
+/**
+    * @brief   The entry point of this test suite.


Indentation is off in this doxygen block

aabadie · 2018-04-13T09:43:28Z

drivers/include/mtd_flashpage.h

+
+#define MTD_FLASHPAGE_INIT_VAL(_pages_per_sector) { \
+    .driver = &mtd_flashpage_driver, \
+    .sector_count = FLASHPAGE_NUMOF, \


So by default, IIUC the MTD can take the whole flash. Can we make this configurable at build time with a more reasonable default value (half of the flash) ?

@vincent-d, do you think it would make sense to limit the default size used to half of the flash size ?

I took a new look to this and there are 2 options:

we leave it as-is and rely on the configuration of the filesystem for the offset in the mtd (i.e. the mtd allows access to the whole internal flash and the filesystem is configured correctly to not write outside its area)

we change the mtd_flashpage to let the filesystem access only a part of the internal flash, but we need to have a specific mtd_flashpage_t descriptor to store the offset

If we only change sector_count the filesystem will be able to access the number of sector passed through this field, but it will always start from address 0 (the first sector)

I'm not sure to understand everything clearly.

If the first option is kept, we'll have to trust the filesystem implementation and most of them come from external packages. My question in this case is how do you configure this external filesystem ? (I checked littlefs and I'm not sure to understand how to do that)

If the second option is kept, could the offset be automatically determined from the number of sectors defined (using offset = flashsize - number_of_sector) ?

he filesystem will be able to access the number of sector passed through this field, but it will always start from address 0

Is this an issue ?

cladmi · 2018-06-14T14:48:41Z

I was pointed here as I was writing a PR to limit the ROM used for a firmware: #9351 It might be of your interest too.

aabadie

@vincent-d, I had another look at this one. I'm still not convinced that letting filesystems managing the whole flash via the mtd is a good idea. But maybe I'm missing obvious things.

I have other minor comments.

tests/unittests/tests-mtd_flashpage/tests-mtd_flashpage.c

drivers/mtd_flashpage/mtd_flashpage.c

aabadie · 2018-06-15T08:52:45Z

drivers/include/mtd_flashpage.h

+
+#define MTD_FLASHPAGE_INIT_VAL(_pages_per_sector) { \
+    .driver = &mtd_flashpage_driver, \
+    .sector_count = FLASHPAGE_NUMOF, \


I'm not sure to understand everything clearly.

If the first option is kept, we'll have to trust the filesystem implementation and most of them come from external packages. My question in this case is how do you configure this external filesystem ? (I checked littlefs and I'm not sure to understand how to do that)

If the second option is kept, could the offset be automatically determined from the number of sectors defined (using offset = flashsize - number_of_sector) ?

he filesystem will be able to access the number of sector passed through this field, but it will always start from address 0

Is this an issue ?

basilfx · 2018-08-20T07:21:56Z

@vincent-d What's the status of this PR? Would love to have this in.

kYc0o · 2018-08-20T12:50:07Z

Now that I think twice, I see that this PR worth the "risk" of letting the user use the internal flash at will.

I'd ACK it if @aabadie 's comments are addressed.

kYc0o · 2018-08-27T17:04:44Z

@aabadie ping.

aabadie · 2018-08-27T17:48:22Z

@aabadie ping.

From what I see, I still have unaddressed comments. So it should be ping @vincent-d :)

vincent-d · 2018-09-05T11:55:30Z

@aabadie @kYc0o comments should be addressed

kYc0o · 2018-09-11T15:37:20Z

Can we go on this one?

kYc0o · 2018-09-19T12:03:00Z

@aabadie ping.

aabadie

ACK, please squash

vincent-d · 2018-09-20T15:48:02Z

squashed

aabadie · 2018-09-20T15:54:48Z

Static tests are already reporting issues with ~~a missing doxygen group~~ an undocumented macro.

stale · 2019-08-10T03:07:57Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

vincent-d · 2019-08-21T08:53:15Z

@kYc0o your changes look OK. Maybe someone else should take a look and test (I don't have hardware supporting this at the moment).

kYc0o · 2019-08-21T09:58:18Z

Maybe @fjmolinas would like to test? The failures and differences are mostly related to stm32l152 devices, and other cortex-m series uC which I also don't have now.

aabadie · 2019-08-21T10:07:36Z

I can test tomorrow on nucleo-l1 and some other boards.

aabadie

Unfortunately, I found an important problem with this PR: it adds periph_flashpage dependency to the unittests and thus disable them for native and other boards that don't provide support for this feature.
@vincent-d can you move the test in its own application, tests/mtd_flashpage or something equivalent ? (also having a basic python script with it will be a big plus for automatic testing ;) )

Appart from that the unittest are passing on nucleo-l152re and nucleo-l476rg.

aabadie · 2019-08-22T13:11:52Z

It would also be great to rebase this PR on top of latest master as it contains other changes for unittests and nucleo-l1.

vincent-d · 2019-09-10T09:40:04Z

I guess this one only needs squashing now, @aabadie ?

aabadie

I re-tested this PR on several boards providing flashpage_raw: arduino-zero, nrf52dk and iotlab-m3. The new test application works like a charm.
Code changes are good, so ACK

@kYc0o, are you also ok to ACK ?

@vincent-d, please squash!

This is a MTD wrapper for flashpage internal flash

kYc0o · 2019-09-11T11:52:36Z

@aabadie did you test on a stm32l152?

aabadie · 2019-09-11T13:57:14Z

did you test on a stm21l152?

Not the very last version (I admit I forgot...), but I tested it before (see #8774 (review)) the test was cut out from the unittest and it was working.

kYc0o · 2019-09-11T14:12:35Z

Cold you try again nonetheless? I really have doubts about the differences on those platforms.

kYc0o

ACK. I tested on a lobaro-lorabox which also have a stm32l1 microcontroller. It's enough for me as a test.

benpicco · 2023-02-09T13:36:40Z

drivers/include/mtd_flashpage.h

+    .pages_per_sector = _pages_per_sector,\
+    .page_size = FLASHPAGE_SIZE / _pages_per_sector,\


What is the reason to create 'virtual' sectors here?
Usually page is the max transfer unit and sector the min erase unit. For internal flash they tend to be the same. So why artificially make the erase granularity worse (and complicate the code)?

vincent-d added State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers Area: fs Area: File systems labels Mar 13, 2018

vincent-d requested review from aabadie, jnohlgard and kYc0o March 13, 2018 15:57

aabadie reviewed Mar 13, 2018

View reviewed changes

vincent-d force-pushed the pr/flashpage_mtd branch 2 times, most recently from 3f61679 to f119787 Compare March 14, 2018 18:21

vincent-d added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 14, 2018

kYc0o reviewed Mar 14, 2018

View reviewed changes

kYc0o approved these changes Apr 11, 2018

View reviewed changes

aabadie requested changes Apr 13, 2018

View reviewed changes

vincent-d force-pushed the pr/flashpage_mtd branch from d11e7d0 to 206f69d Compare June 14, 2018 08:21

aabadie requested changes Jun 15, 2018

View reviewed changes

aabadie approved these changes Sep 19, 2018

View reviewed changes

vincent-d force-pushed the pr/flashpage_mtd branch from db2070d to abf9739 Compare September 20, 2018 15:47

vincent-d force-pushed the pr/flashpage_mtd branch from abf9739 to e3778dc Compare September 21, 2018 13:56

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 21, 2019

aabadie requested changes Aug 22, 2019

View reviewed changes

vincent-d force-pushed the pr/flashpage_mtd branch from f119427 to fac7730 Compare September 6, 2019 07:33

aabadie approved these changes Sep 10, 2019

View reviewed changes

Vincent Dupont added 3 commits September 10, 2019 13:26

drivers: add mtd_flashpage driver

f3d2f53

This is a MTD wrapper for flashpage internal flash

unittests: add mtd_flashpage unittests

a05f7c7

drivers/mtd: improve mtd API doc

7c8a3ea

vincent-d force-pushed the pr/flashpage_mtd branch from 123e5ef to 7c8a3ea Compare September 10, 2019 11:26

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 10, 2019

benpicco requested a review from kYc0o September 10, 2019 14:28

kYc0o approved these changes Sep 11, 2019

View reviewed changes

kYc0o merged commit 092f8e8 into RIOT-OS:master Sep 11, 2019

kb2ma added this to the Release 2019.10 milestone Sep 16, 2019

benpicco reviewed Feb 9, 2023

View reviewed changes

		.pages_per_sector = _pages_per_sector,\
		.page_size = FLASHPAGE_SIZE / _pages_per_sector,\

drivers: add mtd wrapper for periph_flashpage #8774

drivers: add mtd wrapper for periph_flashpage #8774

Uh oh!

Conversation

vincent-d commented Mar 13, 2018

Contribution description

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o 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!

kYc0o Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Apr 11, 2018

Uh oh!

aabadie 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!

vincent-d Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cladmi commented Jun 14, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basilfx commented Aug 20, 2018

Uh oh!

kYc0o commented Aug 20, 2018

Uh oh!

kYc0o commented Aug 27, 2018

Uh oh!

aabadie commented Aug 27, 2018

Uh oh!

vincent-d commented Sep 5, 2018

Uh oh!

kYc0o commented Sep 11, 2018

Uh oh!

kYc0o commented Sep 19, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Sep 20, 2018

kYc0o Apr 9, 2018 •

edited

Loading

vincent-d Jun 13, 2018 •

edited

Loading

aabadie commented Sep 20, 2018 •

edited

Loading

kYc0o commented Sep 11, 2019 •

edited

Loading

benpicco Feb 9, 2023 •

edited

Loading