-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upgrade to v2.1.4 #14
Conversation
for some reason test fails with |
@ARMmbed/mbed-os-storage Could you help please? |
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
CompilationPatch required
Compilation and testing
Test resultsAll results
Failures
|
@@ -24,8 +24,9 @@ namespace mbed { | |||
|
|||
extern "C" uint32_t lfs2_crc(uint32_t crc, const void *buffer, size_t size) | |||
{ | |||
uint32_t initial_xor = crc; | |||
MbedCRC<POLY_32BIT_REV_ANSI, 32> ct(initial_xor, 0x0, true, false); | |||
uint32_t initial_xor = lfs2_rbit(crc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geky, @kjbracey-arm This part is like black magic for me so I trust you also take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which comes from this PR from @kjbracey-arm ARMmbed/mbed-os#11957
Given that it's the same as what's on mbed-os, I think it should be fine. The CRC didn't change v1->v2.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following test case failures need to be addressed.
| 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 |
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:
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 littlefs-project/littlefs#372, which required some rather invasive fixes. So I'm not sure these can be fixed without waiting for littlefs-project/littlefs#372. |
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. |
I wonder if it may be worth bringing in the CRC fix as a separate patch while waiting for v2.1.5? |
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. |
@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. |
@geky, @pilotak, @SeppoTakalo fine by me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say this PR can be merged to the master branch of this repository. Once 2.1.5 is available - or we apply the forementioned patch - that can be brought in as a separate PR and then we can merge it to Mbed OS. Maybe after that exercise we should get rid of this repository and push changes directly to Mbed OS?
that we be perfect |
Hi @pilotak, with the release of v2.2, I've gone ahead and created a new PR based off this: 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 #16 in once it passes CI. Thanks for creating this PR! |
@geky thanks for the update. I noticed that you have not updated the |
actually whole file will generate macros which will not work, they have a MBED prefix which is not used by the lib |
Ah! Correct me if I'm wrong, but aren't the MBED_* macros used here? mbed-littlefs/LittleFileSystem2.h Lines 60 to 64 in 70aa179
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. |
@geky sorry i didn't spot that, my fault. As you say |
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: mbed-littlefs/LittleFileSystem2.cpp Lines 183 to 184 in 70aa179
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. |
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? |
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. |
@pilotak also thanks for all the help with bringing v2.x in here! |
PR ARMmbed/mbed-os#12783 to bring LittleFSv2 to Mbed OS created. Thanks @pilotak for your work. |
Updates outdated alpha version to v2.1.4