pkg/spiffs: add multi-partitions support by vincent-d · Pull Request #8273 · RIOT-OS/RIOT

vincent-d · 2017-12-18T12:41:41Z

Contribution description

Add the possibility of using multiple partitions (i.e. not use the whole memory for a spiffs partition).
Two new fileds added in spiffs descriptor:

base_addr: base address of the partition
part_block_count: number of sectors in the partition
If part_block_count is 0, the whole underlying MTD is used

Issues/PRs references

None

jnohlgard

I like the general idea, but I did not do any testing.
Is it possible to test this in a good way with the unit tests? I think at least on native with the MTD emulation it would be good to be able to verify this functionality somehow.

jnohlgard · 2017-12-18T15:05:00Z

sys/include/fs/spiffs_fs.h

 #endif
+#if (SPIFFS_SINGLETON == 0) || defined(DOXYGEN)
+    uint32_t base_addr;                         /**< Base address of partition */
+    uint32_t part_block_count;                  /**< Number of blocks in current partition,


would not block_count be sufficient as the name?

renamed to block_count

vincent-d · 2018-01-04T16:16:06Z

comments addressed. I added a unit test to check nothing is written outside the partition.

vincent-d · 2018-01-23T12:57:58Z

ping @gebart

vincent-d · 2018-02-27T09:26:00Z

ping

jnohlgard · 2018-02-27T15:04:55Z

pkg/spiffs/fs/spiffs_fs.c

    fs_desc->config.log_block_size = dev->page_size * dev->pages_per_sector;
    fs_desc->config.log_page_size = dev->page_size;
-    fs_desc->config.phys_addr = 0;
+    fs_desc->config.phys_addr = fs_desc->base_addr;


please add an assert that checks that the partition does not begin or end outside of the physical device limits.

added 2 assert to check:

base_addr + sector_count is inside memory boundaries

base_addr is aligned on a sector

jnohlgard

Please add a safeguard assert and fix the minor typo. Looks good, tests pass on native but fail on mulle:

spiffs_tests.tests_spiffs_partition (tests/unittests/tests-spiffs/tests-spiffs.c 379) buf[0] != 0xff

jnohlgard · 2018-02-27T15:05:28Z

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

+
+#if SPIFFS_USE_MAGIC
+    /* if SPIFFS_USE_MAGIC is used, a magic word is written at the end of the
+     * firt page of each sector */


firt -> first

jnohlgard · 2018-02-27T15:12:35Z

not sure why the tests fail on mulle though.

vincent-d · 2018-02-27T15:42:50Z

not sure why the tests fail on mulle though.

might be fixed by #8400. Since I rebased, could you give another try?

jnohlgard · 2018-02-28T08:49:28Z

Retried with the latest PR branch, a new error:

INFO # spiffs_tests.tests_spiffs_partition (tests/unittests/tests-spiffs/tests-spiffs.c 397) exp 0 was -19

Edit: -19 is -ENODEV

jnohlgard · 2018-02-28T08:52:12Z

Ah there is a missing vfs_format in the test

vincent-d · 2018-02-28T08:57:17Z

Ah there is a missing vfs_format in the test

Yep, it has been introduced in between ;) This should be fixed!

jnohlgard · 2018-02-28T09:05:07Z

I added the vfs_format and ran a test, new error:

2018-02-28 09:54:22,951 - INFO # spiffs_tests.tests_spiffs_partition (tests/unittests/tests-spiffs/tests-spiffs.c 407) buf[0] != 0xff

vincent-d · 2018-02-28T09:17:05Z

Can't reproduce on native nor on our hardware (with spi flash). On which board do you have this issue?

vincent-d · 2018-02-28T09:47:25Z

OK, I reproduced by changing the layout of my flash.
I changed the test to check the whole sectors. I guess the magic word is not always written in the same place.

jnohlgard · 2018-02-28T10:54:59Z

The errors were on mulle with the default config. I don't know if there may be a configuration issue with the mtd or spiffs?

vincent-d · 2018-03-01T10:11:41Z

@gebart could you check again with the last commit? I changed the test, because my guess with the magic word was wrong. It seems it depends on the config. I now read the whole sector to check if it has been written or not.

jnohlgard · 2018-03-01T13:46:34Z

Will test again tonight or tomorrow morning

jnohlgard

Tests pass on Mulle now, but see inline comment about memcmp

jnohlgard · 2018-03-02T09:07:31Z

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

+    res = 0;
+    for (size_t i = 0; i < _dev->page_size * _dev->pages_per_sector; i += sizeof(buf)) {
+        mtd_read(_dev, buf, (2 * _dev->page_size * _dev->pages_per_sector) + i, sizeof(buf));
+        res += memcmp(buf, buf_erased, sizeof(buf));


|= is more robust here since memcmp may return anything non-zero value, both positive and negative, so the sum may end up being 0 with some bad luck

jnohlgard · 2018-03-02T09:10:03Z

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

+    TEST_ASSERT(res != 0);
+    res = 0;
+    for (size_t i = 0; i < _dev->page_size * _dev->pages_per_sector; i += sizeof(buf)) {
+        mtd_read(_dev, buf, (2 * _dev->page_size * _dev->pages_per_sector) + i, sizeof(buf));


we need to either check the return value, or memset buf to zero between each mtd_read to avoid hiding errors

return value check added

jnohlgard · 2018-03-02T09:48:33Z

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

+    /* if SPIFFS_USE_MAGIC is used, a magic word is written in each sector */
+    uint8_t buf[4];
+    const uint8_t buf_erased[4] = {0xff, 0xff, 0xff, 0xff};
+    int read;


Change the name to nread to avoid name collision with the posix read function

jnohlgard

Tests succeed on native and on Mulle.
Please squash

Two new fileds added in spiffs descriptor: - base_addr: base address of the partition - part_block_count: number of sectors in the partition If part_block_count is 0, the whole underlying MTD is used

vincent-d · 2018-03-02T12:47:51Z

Squashed

vincent-d · 2018-03-02T13:15:43Z

All green, go!

vincent-d added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 18, 2017

vincent-d requested a review from jnohlgard December 18, 2017 12:46

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 Dec 18, 2017

jnohlgard reviewed Dec 20, 2017

View reviewed changes

smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: pkg Area: External package ports labels Jan 12, 2018

jnohlgard reviewed Feb 27, 2018

View reviewed changes

jnohlgard requested changes Feb 27, 2018

View reviewed changes

vincent-d force-pushed the pr/spiffs-multi-partitions branch from e2bd0d0 to 2e799e9 Compare February 27, 2018 15:40

jnohlgard requested changes Mar 2, 2018

View reviewed changes

jnohlgard reviewed Mar 2, 2018

View reviewed changes

jnohlgard approved these changes Mar 2, 2018

View reviewed changes

Samantha Wojtowicz and others added 2 commits March 2, 2018 13:45

pkg/spiffs: add multi-partitions support

ced65bb

Two new fileds added in spiffs descriptor: - base_addr: base address of the partition - part_block_count: number of sectors in the partition If part_block_count is 0, the whole underlying MTD is used

unittests/tests-spiffs: add partition test

647133f

vincent-d force-pushed the pr/spiffs-multi-partitions branch from d9076a4 to 647133f Compare March 2, 2018 12:47

vincent-d merged commit dcc4a5a into RIOT-OS:master Mar 2, 2018

vincent-d deleted the pr/spiffs-multi-partitions branch March 2, 2018 13:15

Conversation

vincent-d commented Dec 18, 2017

Contribution description

Issues/PRs references

Uh oh!

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

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

Uh oh!

Uh oh!

vincent-d commented Jan 23, 2018

Uh oh!

vincent-d commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

jnohlgard commented Feb 27, 2018

Uh oh!

vincent-d commented Feb 27, 2018

Uh oh!

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

Uh oh!

Uh oh!

jnohlgard commented Feb 28, 2018

Uh oh!

vincent-d commented Feb 28, 2018

Uh oh!

jnohlgard commented Feb 28, 2018

Uh oh!

vincent-d commented Feb 28, 2018

Uh oh!

vincent-d commented Feb 28, 2018

Uh oh!

jnohlgard commented Feb 28, 2018

Uh oh!

vincent-d commented Mar 1, 2018

Uh oh!

jnohlgard commented Mar 1, 2018

Uh oh!

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

jnohlgard left a comment

Choose a reason for hiding this comment

Uh oh!

vincent-d commented Mar 2, 2018

Uh oh!

vincent-d commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

vincent-d commented Jan 4, 2018 •

edited

Loading

jnohlgard commented Feb 28, 2018 •

edited

Loading