search

ARMmbed / mbed-littlefs

[experimental] Mbed OS wrapper for LittleFS v2.0 (alpha)
https://github.com/ARMmbed/littlefs
76 stars 34 forks source link

upgrade to v2.1.4 #14

Closed pilotak closed 4 years ago

pilotak commented 5 years ago

Updates outdated alpha version to v2.1.4

pilotak commented 4 years ago

for some reason test fails with LFS2_READ_SIZE=64 and LFS2_CACHE_SIZE=64 all other are good, i noticed that in main repo there is no 64 byte test. So this is related to the source not to this PR. I have tested it physically on the board and it works fine.

Could somebody please review? maybe @geky or @0xc0170 ?

0xc0170 commented 4 years ago

@ARMmbed/mbed-os-storage Could you help please?

VeijoPesonen commented 4 years ago

I tried to run the tests against Mbed OS master(d3078a39b1). As you can see there were two failing test cases. As a next step I'll take a look on your changes.

Fetching

mbed-os/features/storage/filesystem$ git clone git@github.com:ARMmbed/mbed-littlefs.git littlefsv2
cd littlefsv2
mbed-os/features/storage/filesystem/littlefsv2$ git fetch origin pull/14/head:littlefsv2 && git checkout littlefsv2

Compilation

Patch required

diff --git i/tools/test_configs/QSPIFBlockDeviceAndHeapBlockDevice.json w/tools/test_configs/QSPIFBlockDeviceAndHeapBlockDevice.json
index 91dbefc5fa..58f3949d69 100644
--- i/tools/test_configs/QSPIFBlockDeviceAndHeapBlockDevice.json
+++ w/tools/test_configs/QSPIFBlockDeviceAndHeapBlockDevice.json
@@ -13,7 +13,7 @@
         "test-filesystem": {
             "help": "Used filesystem",
             "macro_name": "MBED_TEST_FILESYSTEM",
-            "value": "LittleFileSystem"
+            "value": "LittleFileSystem2"
         }
     }
 }

Compilation and testing

mbed test -m NRF52840_DK -n features-storage-filesystem-littlefsv2-* --app-config tools/test_configs/QSPIFBlockDeviceAndHeapBlockDevice.json

Test results

All results

mbedgt: test suite report:
| target              | platform_name | test suite                                                                             | result | elapsed_time (sec) | copy_method |
|---------------------|---------------|----------------------------------------------------------------------------------------|--------|--------------------|-------------|
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem-dirs                           | OK     | 55.56              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem-files                          | OK     | 38.39              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem-interspersed                   | OK     | 24.31              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem-seek                           | OK     | 71.6               | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_integration-format             | OK     | 28.82              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-resilience            | OK     | 51.7               | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-resilience_functional | ERROR  | 36.24              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-wear_leveling         | FAIL   | 84.48              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-dirs                  | OK     | 58.58              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-files                 | OK     | 40.09              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-interspersed          | OK     | 23.5               | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-seek                  | OK     | 82.22              | default     |
mbedgt: test suite results: 10 OK / 1 ERROR / 1 FAIL

Failures

| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-resilience_functional | ERROR  | 36.24              | default     |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-wear_leveling         | FAIL   | 84.48              | default     |
VeijoPesonen commented 4 years ago

To be certain that the test case failures were not somehow specific for the forementioned NRF52840_DK board (with QSPIF) I tried with K82F which is having a SPIF module. The issues remain.

mbedgt: test suite report:
| target       | platform_name | test suite                                                                             | result | elapsed_time (sec) | copy_method |
|--------------|---------------|----------------------------------------------------------------------------------------|--------|--------------------|-------------|
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem-dirs                           | OK     | 50.36              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem-files                          | OK     | 32.8               | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem-interspersed                   | OK     | 26.26              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem-seek                           | OK     | 71.85              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_integration-format             | OK     | 31.97              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-resilience            | OK     | 31.62              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-resilience_functional | ERROR  | 14.68              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_recovery-wear_leveling         | FAIL   | 53.96              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-dirs                  | OK     | 52.24              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-files                 | OK     | 34.17              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-interspersed          | OK     | 15.44              | default     |
| K82F-GCC_ARM | K82F          | features-storage-filesystem-littlefsv2-tests-filesystem_retarget-seek                  | OK     | 75.33              | default     |
mbedgt: test suite results: 10 OK / 1 ERROR / 1 FAIL
geky commented 4 years ago

Thanks for keeping this alive @pilotak, at this point I think we should bring this in if possible. Sorry for not getting the update script working in a timely manner.

I looked into the Travis tests, since those are fairly easy to run locally. There were two failures:

  1. test_alloc fails with a false-positive. This isn't too surprising, test_alloc is a flimsy test that relies on the filesystem running out of blocks at an exact time. Changing the read/prog/cache sizes can change cause this to break.

    I would just change the if statement in test_alloc.sh to skip the problematic tests for now:

    - if [[ ! $MAKEFLAGS =~ "LFS_BLOCK_CYCLES" ]]
+ if [[ ! $MAKEFLAGS =~ "LFS_BLOCK_CYCLES" && ! $MAKEFLAGS =~ "LFS_READ_SIZE" ]]

  2. test_relocations fails with a true-positive. This is the bug found in https://github.com/ARMmbed/littlefs/issues/343. Unfortunately it isn't going to be fixed until https://github.com/ARMmbed/littlefs/pull/372. Though we may be able to cherry-pick https://github.com/ARMmbed/littlefs/commit/f4b6a6b3284d8b721ba3db0bba93ee45fde7ead9 onto this branch.

The Mbed OS tests get a bit harder. These failures come from relocation errors. These are my fault since I did not run the Mbed OS tests on the relocation changes.

The relocation issues were the main motivation for https://github.com/ARMmbed/littlefs/pull/372, which required some rather invasive fixes. So I'm not sure these can be fixed without waiting for https://github.com/ARMmbed/littlefs/pull/372.

geky commented 4 years ago

It looks like these were all caused by the change to wear-leveling in v2.1. So I think all versions v2.1.0-v2.1.4 will show these failures.

geky commented 4 years ago

I wonder if it may be worth bringing in the CRC fix as a separate patch while waiting for v2.1.5?

pilotak commented 4 years ago

Thanks @geky for being onboard. I just like the code to be up to date, it would be shame if others can use v2 and mbedOS will use the old one. And because next major release in rising it's just in time.

pilotak commented 4 years ago

@VeijoPesonen i know you requested changes but what do I need to do? i only brought the update over to this repo including the test. It would be nice if somebody steps in and makes all the things required.

VeijoPesonen commented 4 years ago

I wonder if it may be worth bringing in the CRC fix as a separate patch while waiting for v2.1.5?

@geky, @pilotak, @SeppoTakalo fine by me.

pilotak commented 4 years ago

that we be perfect

geky commented 4 years ago

Hi @pilotak, with the release of v2.2, I've gone ahead and created a new PR based off this: https://github.com/ARMmbed/mbed-littlefs/pull/16

I should have cherry-picked all of your changes onto the v2.2 PR, let me know if you notice anything missing.

I noticed the addition of MBED_LFS2_READ_SIZE and MBED_LFS2_PROG_SIZE. I think they are a good idea but if we add the configs to mbed_lib.json we should also add the configs to LittleFileSystem2's constructor (in case multiple filesystem are instantiated in the system). Feel free to create another PR for that.

Unless you notice any problem, I'm going to go ahead and merge https://github.com/ARMmbed/mbed-littlefs/pull/16 in once it passes CI.

Thanks for creating this PR!

pilotak commented 4 years ago

@geky thanks for the update. I noticed that you have not updated the mbed_lib.json the debug there is obsolete and generated macros wil no nothing, it should be updated from mine. I realise that PROG and READ size is not perfect there, but it was there for V2-aplha so a kept it there

pilotak commented 4 years ago

actually whole file will generate macros which will not work, they have a MBED prefix which is not used by the lib

geky commented 4 years ago

Ah! Correct me if I'm wrong, but aren't the MBED_* macros used here? https://github.com/ARMmbed/mbed-littlefs/blob/70aa179fa7cfbf247e774ffd16175bacdbdc1e20/LittleFileSystem2.h#L60-L64

This way the config options slot into the C++ constructor and can be overwritten per class.

I noticed the READ/PROG config options were missing from here.

pilotak commented 4 years ago

@geky sorry i didn't spot that, my fault.

As you say LFS2_READ_SIZE and LFS2_PROG_SIZE are still not defined. Can you make a PR or do you want me to do it?

geky commented 4 years ago

Ah, if you think READ_SIZE/PROG_SIZE is valuable a PR would be helpful. I'd be happy to bring it in.

I'm not sure it's necessary though, since the LittleFileSystem class can get the read_size/prog_size from the BlockDevice class. This isn't the same in the C API since a callback for read_size/prog_size would have been overkill: https://github.com/ARMmbed/mbed-littlefs/blob/70aa179fa7cfbf247e774ffd16175bacdbdc1e20/LittleFileSystem2.cpp#L183-L184

One of the changes v1->v2 was to separate the limitations of the hardware (read_size/prog_size) from the cache allocation (cache_size). So now the read_size/prog_size serve a less important role for configuring littlefs.

pilotak commented 4 years ago

Thats fine, I'm only making sure it has everything set so it can go straight into OS so if you think it's not necessary i'm ok.

Now that everything is set can you please bring it to the OS so it's there ready for OS6 release?

geky commented 4 years ago

That part is out of my hands.

@VeijoPesonen, what are the next steps for this? v2.2 (master now) would be a great target for Mbed OS integration.

geky commented 4 years ago

@pilotak also thanks for all the help with bringing v2.x in here!

VeijoPesonen commented 4 years ago

PR ARMmbed/mbed-os#12783 to bring LittleFSv2 to Mbed OS created. Thanks @pilotak for your work.