Skip to content

Add block device test for small data sizes #8740

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 24, 2018
Merged

Add block device test for small data sizes #8740

merged 1 commit into from
Nov 24, 2018

Conversation

theamirocohen
Copy link
Contributor

@theamirocohen theamirocohen commented Nov 14, 2018 •
edited
Loading

Description

Added a new test + astyle the test file
Depends on PR #8703

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

@theamirocohen
Copy link
Contributor Author

@dannybenor @davidsaada @offirko
please review..
Thanks

offirko
offirko reviewed Nov 14, 2018
View reviewed changes
features/storage/TESTS/blockdevice/general_block_device/main.cpp Outdated
// Determine starting address
bd_addr_t start_address = 0;

for (int i = 1; i < 7; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 'i < 8' if you want the test to run 7 times, to include the entire write_buffer "1234567"

@adbridge
Copy link
Contributor

@theamirocohen Please do not remove parts of the description template! Please fix

offirko
offirko reviewed Nov 14, 2018
View reviewed changes
features/storage/TESTS/blockdevice/general_block_device/main.cpp

err = block_device->program((const void *)write_buffer, start_address, i);
TEST_ASSERT_EQUAL(0, err);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should call 'sync' with BufferedBlockDevice to make sure the data was flushed to the underlying bd.
Even with 'sync' I think next read will read from the 'cache' and not from the underlying bd - at least for the first iteration.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@offirko Cache is invalidate after flushing (sync included), so it will always read from the bd itself.
But I agree with you regarding the need to call sync.

@jeromecoutant
Copy link
Collaborator

Hi
Which targets have been tested ?
Thx

davidsaada
davidsaada suggested changes Nov 14, 2018
View reviewed changes
Copy link
Contributor

@davidsaada davidsaada left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got nothing to add on top of @offirko's comments. Just please note that this PR should depend on #8703 (should be in master pretty soon I think). So please test this with that PR as well.

@theamirocohen
Copy link
Contributor Author

Hi
Which targets have been tested ?
Thx

K82F & K64F

@theamirocohen
Copy link
Contributor Author

Hi @cmonr,
PR #8703 was merged

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 19, 2018

@theamirocohen Can you rebase instead for merging master here? Commit 3ca93c9

0xc0170
0xc0170 requested changes Nov 19, 2018
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebase instead of merge, and should be ready for CI

@theamirocohen theamirocohen force-pushed the block_device_general_tests branch from 785d9c5 to b114a12 Compare November 19, 2018 12:45
@theamirocohen theamirocohen force-pushed the block_device_general_tests branch from b114a12 to cf84615 Compare November 19, 2018 12:49
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 20, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Nov 20, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Nov 20, 2018

Exporter Build : FAILURE

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

@mbed-ci
Copy link

mbed-ci commented Nov 20, 2018

Test : FAILURE

Build number : 3462
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8740/3462

@theamirocohen
Copy link
Contributor Author

@0xc0170 it looks like the failures are CI problems.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 21, 2018

Correct, we fixed some and restarting some jobs. This might be a good candidate to be included in rollup PR

@0xc0170 0xc0170 mentioned this pull request Nov 22, 2018
Merged
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 22, 2018

Info: This PR has been re-bundled into a new rollup PR (#8838 ).

No further work is needed here, as once that PR is merged, this PR will also be closed and marked as merged.
If any more commits are made in this PR, this PR will remain open and have to go through CI on its own.

@0xc0170 0xc0170 merged commit cf84615 into ARMmbed:master Nov 24, 2018
@0xc0170 0xc0170 removed the needs: CI label Nov 24, 2018
@cmonr cmonr removed the rollup PR label Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@offirko offirko offirko left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

@davidsaada davidsaada davidsaada approved these changes

@dannybenor dannybenor Awaiting requested review from dannybenor

Assignees
No one assigned
Labels
release-version: 5.11.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@theamirocohen @adbridge @jeromecoutant @0xc0170 @mbed-ci @davidsaada @offirko @cmonr