pkg: add SPIFFS support #5625

vincent-d · 2016-07-12T15:11:02Z

This PR adds support for SPIFFS as a package based on @gebart recent VFS work.

This is still WIP and all functions are not implemented right now.
Some unit tests may be used to test it.

This is based on ~~#5616~~ and uses ~~#5624~~ for the low level.

jnohlgard · 2016-07-13T12:18:14Z

The patch for the const buffer in spiffs_write should be upstreamed, if possible. I don't see a reason for why a source buffer in a write situation should be writable.

vincent-d · 2016-07-13T12:24:34Z

@gebart I agree, I'll open a PR on the upstream project, but I wanted to have something working.

vincent-d · 2016-07-13T13:00:40Z

opendir/readdir/closedir support added. No support for mkdir/rmdir as SPIFFS does not support directory, but has a flat structure and I'm not 100% sure about the behavior of readdir.

Still TODO: find a better way to use spiffs_config.h. Some part of this file should be set in a common part instead of in the board.

jnohlgard · 2016-07-13T14:06:39Z

Still TODO: find a better way to use spiffs_config.h. Some part of this file should be set in a common part instead of in the board.

How about since all defines in spiffs_config.h are already surrounded by #ifndef, we add #include "spiffs_board_config.h" to the top and let the board specifics come first, then fall back to the defaults?

jnohlgard · 2016-07-13T14:16:21Z

just realized that the above idea will cause problems with missing files. We could just add #include "board.h" at the top instead

jnohlgard · 2016-07-13T15:05:44Z

for reference: pellepl/spiffs#94

vincent-d · 2016-07-13T20:51:44Z

Added rename support and rebased on top of #5616

vincent-d · 2016-07-14T12:48:25Z

OTAkeys#1 merged and rebased on top of #5616

jnohlgard · 2016-07-14T13:54:25Z

tests/unittests/tests-spiffs/tests-spiffs.c

+    vfs_dirent_t entry;
+    res = vfs_readdir(&dirp, &entry);
+    TEST_ASSERT(res == 1);
+    TEST_ASSERT_EQUAL_STRING("/test0.txt", &(entry.d_name[0]));


The order of these files may not be the same order as the order in which they were created. It is also possible that there are other files left over from previous runs or failed tests in the same run which interfere with this check. The test should only verify that all the created files are reported back by readdir, the order should be irrelevant.

jnohlgard · 2016-07-16T08:36:01Z

There's a problem with the unit test: Running it after a failed run may have left files around in the SPIFFS file system, so there should be some clean up at the beginning of each test to ensure that no leftover files are interfering with the outcome.

vincent-d · 2016-07-19T14:27:23Z

Unit tests improved.
Rebased with last mtd commits.

vincent-d · 2016-07-19T15:16:06Z

rebased on top of #5616

vincent-d · 2017-03-30T10:16:57Z

Done, thanks @kaspar030

vincent-d · 2017-03-30T11:06:33Z

Unittests seem OK now

jnohlgard · 2017-03-31T08:50:20Z

The unit tests take ages to run on Mulle with a real SPI NOR flash memory. Will do some more investigation.

jnohlgard · 2017-03-31T08:51:12Z

@vincent-d what's the run time length on your platform?
Mulle took several minutes to run the entire spiffs unit tests, most of it was spent in the write2 test.

vincent-d · 2017-03-31T08:57:07Z

@gebart the write2 test was initially there to ensure erase works correctly, so we need to fill the whole memory before the first erase actually happen, that's why it takes ages... Maybe we should just remove that test

jnohlgard · 2017-03-31T11:26:33Z

@vincent-d I think it makes sense as a test, but it slows down the execution of the unit tests too much. Maybe only enable it on native for now?

I think it would be great if we had a separate benchmark test for spiffs, to compare read/write speeds between commits and between platforms. The write2 test could be a part of that kind of application as well.

jnohlgard

A few comments on the unit tests.
The write2 test needs some comment describing what it does.

jnohlgard · 2017-03-31T08:53:40Z

tests/unittests/tests-spiffs/tests-spiffs.c

+    vfs_umount(&_test_spiffs_mount);
+}
+
+static void test_spiffs_mount_umount(void)


this should be renamed to match the other tests. tests_spiffs_mount_umount (pluralis "tests")

jnohlgard · 2017-03-31T11:41:13Z

tests/unittests/tests-spiffs/tests-spiffs.c

+
+static void test_spiffs_teardown(void)
+{
+    vfs_mount(&_test_spiffs_mount);


There's a lot of mounting and unmounting going on, what do you think about mounting in the setup phase, saving the result to a file-global (i.e. static global) variable and just checking the variable inside the mount_unmount test?

The unmount would need to be tested inside the mount_unmount test though

We could mount in the test_spiffs_setup, unmount in the test_spiffs_teardown and change the mount_unmount test to a unmount_mount. What do you think ?

Sure, that would work I guess. I forgot, do we handle unmounting an already unmounted fs in VFS?

I guess either the fs implementation or removing the mountpoint from the list will fail, but it will fail silently.

jnohlgard · 2017-03-31T11:41:45Z

tests/unittests/tests-spiffs/tests-spiffs.c

+    vfs_unlink("/test-spiffs/test0.txt");
+    vfs_unlink("/test-spiffs/test1.txt");
+    vfs_unlink("/test-spiffs/a/test2.txt");
+    vfs_umount(&_test_spiffs_mount);


... and a flag variable to tell whether we need to unmount or not.

jnohlgard · 2017-03-31T11:43:57Z

tests/unittests/tests-spiffs/tests-spiffs.c

+    TEST_ASSERT(mp >= 0);
+
+    int res;
+    for (int j = 0; j < 5; j++) {


This needs a comment explaining what we want to test.
I don't really see how the magic numbers in this test ensure that we fill the memory and need to erase sectors..

jnohlgard · 2017-03-31T11:44:19Z

tests/unittests/tests-spiffs/tests-spiffs.c

+            TEST_ASSERT_EQUAL_INT(sizeof(buf), res);
+        }
+
+        res = vfs_lseek(fd, 0, SEEK_SET);


is this line necessary?

vincent-d · 2017-03-31T12:12:30Z

For the write2 test, I used the magic numbers with my memory. But I already reduced them to avoid the timeout on native, which does not really make sense, I agree.
I propose we just drop this test for now to not block that PR and we prepare a separate test app which could be used for benchmarking.

vincent-d · 2017-03-31T13:02:47Z

Fix unittests

write2 removed
comments addressed

jnohlgard

ACK, OK to squash

This implements basic features with the VFS interface

vincent-d · 2017-03-31T15:20:08Z

Squashed

jnohlgard · 2017-03-31T16:00:08Z

Did another test run on Mulle and native, both work fine.

jnohlgard · 2017-03-31T18:29:57Z

Murdock is fine, go!

jnohlgard · 2017-03-31T18:31:45Z

Thanks, @vincent-d! Finally RIOT has a file system out of the box! 🎉

vincent-d · 2017-03-31T19:03:02Z

Great! Thanks @gebart

vincent-d mentioned this pull request Jul 13, 2016

vfs: file system abstraction #5616

Merged

55 tasks

jnohlgard added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jul 13, 2016

jnohlgard added this to the Release 2016.10 milestone Jul 13, 2016

vincent-d force-pushed the pr/spiffs branch from 4d431db to 7d7abdf Compare July 13, 2016 12:55

vincent-d force-pushed the pr/spiffs branch from 7d7abdf to e0c8be4 Compare July 13, 2016 20:48

vincent-d force-pushed the pr/spiffs branch 2 times, most recently from 2d53838 to 9535de5 Compare July 14, 2016 12:45

jnohlgard mentioned this pull request Jul 14, 2016

State of low-level flash storage interfaces #5398

Closed

jnohlgard reviewed Jul 14, 2016
View reviewed changes

vincent-d force-pushed the pr/spiffs branch 2 times, most recently from 66e0fe4 to 2a85197 Compare July 19, 2016 14:26

vincent-d force-pushed the pr/spiffs branch from 2a85197 to 8ba9f86 Compare July 19, 2016 15:14

jnohlgard mentioned this pull request Jul 20, 2016

WIP mtd_spi_nor: MTD interface driver for SPI NOR flash chips #5668

Closed

vincent-d force-pushed the pr/spiffs branch 2 times, most recently from 80eae7b to 65dc155 Compare July 28, 2016 08:46

vincent-d force-pushed the pr/spiffs branch from 65dc155 to a71bb8b Compare August 10, 2016 13:38

jnohlgard requested changes Mar 31, 2017

View reviewed changes

kaspar030 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 Mar 31, 2017

jnohlgard approved these changes Mar 31, 2017

View reviewed changes

Vincent Dupont added 3 commits March 31, 2017 17:17

spiffs: add SPIFFS as a package

f0bd96b

This implements basic features with the VFS interface

unittests: add spiffs unittests

b52c9a7

native: add spiffs options in board.h

4cb3232

vincent-d force-pushed the pr/spiffs branch from b9cb035 to 4cb3232 Compare March 31, 2017 15:17

jnohlgard merged commit f4dd0a3 into RIOT-OS:master Mar 31, 2017

vincent-d deleted the pr/spiffs branch April 3, 2017 08:03

jnohlgard added the Area: fs Area: File systems label Feb 19, 2018

pkg: add SPIFFS support #5625

pkg: add SPIFFS support #5625

Conversation

vincent-d commented Jul 12, 2016 • edited by jnohlgard Loading

jnohlgard commented Jul 13, 2016

vincent-d commented Jul 13, 2016

vincent-d commented Jul 13, 2016

jnohlgard commented Jul 13, 2016

jnohlgard commented Jul 13, 2016

jnohlgard commented Jul 13, 2016

vincent-d commented Jul 13, 2016

vincent-d commented Jul 14, 2016

Choose a reason for hiding this comment

jnohlgard commented Jul 16, 2016

vincent-d commented Jul 19, 2016

vincent-d commented Jul 19, 2016

vincent-d commented Mar 30, 2017

vincent-d commented Mar 30, 2017

jnohlgard commented Mar 31, 2017

jnohlgard commented Mar 31, 2017

vincent-d commented Mar 31, 2017

jnohlgard commented Mar 31, 2017

jnohlgard left a 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

vincent-d Mar 31, 2017 • 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

vincent-d commented Mar 31, 2017

vincent-d commented Mar 31, 2017

jnohlgard left a comment

Choose a reason for hiding this comment

vincent-d commented Mar 31, 2017

jnohlgard commented Mar 31, 2017

jnohlgard commented Mar 31, 2017

jnohlgard commented Mar 31, 2017

vincent-d commented Mar 31, 2017

vincent-d commented Jul 12, 2016 •

edited by jnohlgard

Loading

vincent-d Mar 31, 2017 •

edited

Loading