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

Prevent sector-unaligned erase #7952

Merged
merged 2 commits into from Sep 3, 2018

Conversation

Projects
None yet
6 participants
@offirko
Contributor

offirko commented Sep 2, 2018

Description

Pull request type

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

This comment has been minimized.

Contributor

offirko commented Sep 2, 2018

@bulislaw , @donatieng , @ARMmbed/mbed-os-maintainers , @dannybenor , @davidsaada .
I'd appreciate your review of this fix for SPIFBlockDevice

@offirko

This comment has been minimized.

Contributor

offirko commented Sep 2, 2018

Please note that this fix should be entered before code freeze!
Sorry for the post-last-minute PR, due to its dependency with #7774 it wasn't possible to issue this PR earlier.

@offirko

This comment has been minimized.

Contributor

offirko commented Sep 2, 2018

SPIF_BD_ERROR_OK = 0, /*!< no error */
SPIF_BD_ERROR_DEVICE_ERROR = BD_ERROR_DEVICE_ERROR, /*!< device specific error -4001 */
SPIF_BD_ERROR_PARSING_FAILED = -4002, /* SFDP Parsing failed */
SPIF_BD_ERROR_READY_FAILED = -4003, /* Wait for Mem Ready failed */

This comment has been minimized.

@davidsaada

davidsaada Sep 2, 2018

Contributor

Rogue spaces.

This comment has been minimized.

@offirko

offirko Sep 2, 2018

Contributor

Thanks, I caught the rouge tab (-:

{
utest_printf("\nTest Unaligned Program Starts..\n");
utest_printf("\nTest Unaligned Erase Starts..\n");
SPIFBlockDevice blockD(MBED_CONF_SPIF_DRIVER_SPI_MOSI, MBED_CONF_SPIF_DRIVER_SPI_MISO, MBED_CONF_SPIF_DRIVER_SPI_CLK,

This comment has been minimized.

@davidsaada

davidsaada Sep 2, 2018

Contributor

Better not use a camel case name (blockD) here.

This comment has been minimized.

@offirko

offirko Sep 2, 2018

Contributor

will fix

return SPIF_BD_ERROR_INVALID_ERASE_PARAMS;
}
if ( ((addr % get_erase_size(addr)) != 0 ) || (((addr + in_size) % get_erase_size(addr + in_size - 1)) != 0 ) ) {

This comment has been minimized.

@davidsaada

davidsaada Sep 2, 2018

Contributor

Won't the function is_valid_erase work here?

This comment has been minimized.

@offirko

offirko Sep 2, 2018

Contributor

is_valid_erase wont work because it relies on a single erase-sector size.
This supports variable erase-sector size according to address.

This comment has been minimized.

@davidsaada

davidsaada Sep 2, 2018

Contributor

Thanks. This means that it has a bug, which should be fixed (but not in this PR of course).

This comment has been minimized.

@offirko

offirko Sep 2, 2018

Contributor

@davidsaada - I think not all Block Devices support variable sector-erase size. You're right, it will require a separate PR, probably updating BlockDevice.h base class and maybe some inheriting block devices.

@davidsaada

👍

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 3, 2018

/morph build

@0xc0170

0xc0170 approved these changes Sep 3, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Sep 3, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@adbridge adbridge merged commit 2efa4e4 into ARMmbed:master Sep 3, 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.03%)
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, 601 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10184 cycles (-112 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 needs: CI label Sep 3, 2018

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