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

Use MbedCRC for LittleFS #7824

Merged
merged 3 commits into from Sep 5, 2018

Conversation

Projects
None yet
7 participants
@deepikabhavnani
Contributor

deepikabhavnani commented Aug 17, 2018

Description

916dec9 : CRC used in LittleFS is Reversed ANSI, hence new polynomial added. Reversed polynomials perform shift in reverse direction of standard polynomial, and we do not have option to notify reverse shift to hardware. Hence this option is available in software only.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

Dependent on #7793

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:lfs_crc branch from 916dec9 to 5d5a732 Aug 17, 2018

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:lfs_crc branch from 5d5a732 to 80d6f64 Aug 21, 2018

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 21, 2018

Preceding PR merged, hence rebased

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:lfs_crc branch from 80d6f64 to 4986673 Aug 21, 2018

@deepikabhavnani deepikabhavnani requested a review from geky Aug 21, 2018

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:lfs_crc branch from 4986673 to 6aab43c Aug 21, 2018

// Only compile if user does not provide custom config
#ifndef LFS_CONFIG

#ifdef __MBED__

This comment has been minimized.

@geky

geky Aug 21, 2018

Member

nit: Could this be moved to the header file? And just make the crc function declared LittleFileSystem.cpp named "lfs_crc"?

@geky

This comment has been minimized.

Member

geky commented Aug 21, 2018

Looks good 👍 Though need to double check that the resulting disk image is portable.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 22, 2018

@deepikabhavnani Is this ready for review or still needs work?

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 22, 2018

@0xc0170 - Its ready for review

@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 23, 2018

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Aug 23, 2018



// Software CRC implementation with small lookup table
#ifndef __MBED__

This comment has been minimized.

@0xc0170

0xc0170 Aug 23, 2018

Member

shouldn't we rather provide LFS_CONFIG than changing directly littlefs (another update might replace this?)

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 23, 2018

Contributor

@0xc0170 - Yes we can but I assumed LFS_CONFIG is apart from mbed-os changes.

This comment has been minimized.

@geky

geky Aug 23, 2018

Member

LFS_CONFIG was added after we first integrated LittleFS into Mbed OS, so that's why lfs_util.c/h have been modified directly.

git subtrees handle this surprisingly gracefully, so moving to LFS_CONFIG has been low priority.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 23, 2018

Contributor

As part of this PR, shall we update "lfs_util.c/h" or add separate files?

This comment has been minimized.

@geky

geky Aug 23, 2018

Member

Up to you, adding separate files can be a separate PR

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Aug 23, 2018

Contributor

Up to you, adding separate files can be a separate PR

Will stay with this for now. Thanks

@geky

This comment has been minimized.

Member

geky commented Aug 23, 2018

This has a risk of effecting LittleFS structures on disk. @cmonr Could I request we hold off on this until 5.10?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 24, 2018

This has a risk of effecting LittleFS structures on disk. @cmonr Could I request we hold off on this until 5.10?

This is why I like to tag PRs as soon as possible :)

@deepikabhavnani

This comment has been minimized.

Contributor

deepikabhavnani commented Aug 24, 2018

Code freeze for 5.10 is before 5.9.7 and this PR is on top of other 3 CRC PR’s. Please tag all 4 to same release

@cmonr

This comment has been minimized.

Contributor

cmonr commented Aug 29, 2018

@geky @0xc0170 Good with this PR?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 30, 2018

@geky please finalize the review

@deepikabhavnani deepikabhavnani force-pushed the deepikabhavnani:lfs_crc branch from 9078aab to ce8731a Aug 30, 2018

@geky

This comment has been minimized.

Member

geky commented Aug 30, 2018

Sorry for delay, I wanted to verify that this did not break portability. It looks good to me 👍

@deepikabhavnani, thanks for putting this together

@cmonr cmonr removed the needs: work label Sep 3, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 3, 2018

Build : SUCCESS

Build number : 2990
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7824/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 3, 2018

2 test failures related to the filesystem, please review

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Sep 3, 2018

@mbed-ci

This comment has been minimized.

CRC: Removed assertion on NULL buffer
When length is zero, the buffer is not accessed. The CRC functions are used
inside several other layers where a 0-length buffer may have meanings in
different contexts and being able to pass 0-length NULL buffers to CRC as a
noop simplifies the code.
@adbridge

This comment has been minimized.

Contributor

adbridge commented Sep 3, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 3, 2018

Build : SUCCESS

Build number : 2997
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7824/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 4, 2018

Marking this for RC2.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 4, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Sep 5, 2018

/morph test

@cmonr cmonr added needs: CI and removed needs: work labels Sep 5, 2018

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 5, 2018

@cmonr cmonr merged commit f940443 into ARMmbed:master Sep 5, 2018

14 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed , RTOS ROM(+0.0%) RAM(+0.0%)
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/cloud_client_smoke_test Test job: successful
Details
travis-ci/astyle Passed, 597 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10448 cycles (+438 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@0xc0170 0xc0170 removed the ready for merge label Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment