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

Implement FlashSimBlockDevice - flash simulated block device over RAM #6559

Merged
merged 1 commit into from May 18, 2018

Conversation

Projects
None yet
10 participants
@davidsaada
Contributor

davidsaada commented Apr 6, 2018

Description

Implement a simulated flash block device over RAM. This will allow us to test features that use flash block devices (like StorageLite) on targets that don't have a real flash block device (practically all targets).

Pull request type

[ ] Fix
[ ] Refactor
[ ] New target
[X ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested review from geky and deepikabhavnani Apr 6, 2018

@geky

This comment has been minimized.

Member

geky commented Apr 6, 2018

Ah! This looks great.

Though do you think it would be possible to make this a block device adapter? That way you could use it on top of any block device without a proper erase:

// Flash on heap
HeapBlockDevice heap(512, 512*1024);
FlashSimBlockDevice sim(&heap);

// Flash on I2C EEPROM
I2CEEBlockDevice i2c(D11, D12);
FlashSimBlockDevice sim(&i2c);

Thoughts? Sorry if this is more effort to implement.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Apr 6, 2018

Though do you think it would be possible to make this a block device adapter? That way you could use it on top of any block device without a proper erase.
...
Thoughts? Sorry if this is more effort to implement.

No big deal. This is indeed a cleaner solution.

@davidsaada davidsaada force-pushed the davidsaada:david_flash_sim_bd branch from 0aa6320 to 0dfae76 Apr 6, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Apr 6, 2018

Updated as a BD adaptor.

@davidsaada davidsaada force-pushed the davidsaada:david_flash_sim_bd branch from 0dfae76 to 16391e8 Apr 6, 2018

#include <algorithm>
#include <stdlib.h>
static const uint8_t blank = 0xFF;

This comment has been minimized.

@geky

geky Apr 6, 2018

Member

Do you think we should make this configurable in the constructor? Since this variable is the point of the class.

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

Good question. Would argue that this value is device specific, and that a user of either this class or a real flash block device would need to read this value via the get_erase_value API, hence wouldn't be interested in the predefined value.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 6, 2018

Contributor

In that case I suggest we should add an API to block device class to read this value from block device

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

There is such an API - get_erase_value, and this class implements it (just returns blank).

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 6, 2018

Contributor

Sorry missed that. If it is device specific and not application/user specific we can have a macro and target device based on the RAM in hardware can define this macro in targets.json macro section. But I am not sure if any RAM device will have erase state as 0.

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

You are right. If the underlying BD's erase value is 0xFF, this will not work well (as we will allow programming of unerased blocks). Will add a parameter to the constructor (with a default value of 0xFF).

return ret;
}
_blank_buf_size = align_up(min_blank_buf_size, _bd->get_read_size());
_blank_buf_size = align_up(_blank_buf_size, _bd->get_program_size());

This comment has been minimized.

@geky

geky Apr 6, 2018

Member

non-blocker: Program size must be a multiple of the read size for the BlockDevice API, so you probably don't need both of these align-ups.

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

Maybe not blocking, but also not very nice. Will fix.

@geky

Looks good to me 👍 Thanks for putting this together

bd_size_t curr_size = size;
while (curr_size) {
bd_size_t read_size = std::min(_blank_buf_size, curr_size);

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 6, 2018

Contributor

Query: What should be the value of _blank_buf_size in case read size of block device is 1 byte and program size is 512 bytes. ?

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 6, 2018

Contributor

From code it looks like blank_buf size and buffer parameters are to find out if the buffer is blank to be programmed then program it else erase (which is re-program with 0x0/0xFF) first.
If my understanding is correct then you don;t have to consider read size at all, it should be atleast program size big always.

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

Right. Also raised by @geky. Will fix.

#include <stdlib.h>
static const uint8_t blank = 0xFF;
static const bd_size_t min_blank_buf_size = 32;

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 6, 2018

Contributor

I think even min_blank_buf_size should be configurable

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

Can you explain? Was trying to define a value that should be small enough, so it wouldn't effect heap usage. Do you think it should be smaller? A larger value can make the blank programming/checking a bit more efficient, but this seems negligible to me.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 6, 2018

Contributor

It depends on the use case of this class. If you have it as variable, we keep it configurable for application to have more flexibility.

Ideally for me it should be equivalent to program size and not separate parameter. (As in case of real flash only if entire programmable block is empty than we can say it is free and not by checking few bytes of block)

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

Ah, I think you missed the purpose of this buffer. If you look at the code (both in erase and program APIs) you'll see that the entire prog_size is checked. This buffer is just used as an intermediate one for this purpose, read/programmed in a few iterations until prog_size is reached.

This comment has been minimized.

@deepikabhavnani

deepikabhavnani Apr 6, 2018

Contributor

Yeah I am kind of confused with the usage of this buffer, instead of having it equivalent to program size it is less to save on heap space, and since it is RAM the iterations will be fast. But in the test example program size (8) is less then blank buffer size (32), shouldn't we cap it to program size.
If you would have used defines, and static allocation I would not have made this argument, but we are using dynamic allocation here.

This comment has been minimized.

@davidsaada

davidsaada Apr 6, 2018

Contributor

Capping it to program size will make it extremely inefficient if program size is 1 (saw flash devices with such parameters), meaning that both program and erase functions will read/write chunks of 1. Hence the minimum of 32, which should work even if program size is 1.

@deepikabhavnani

Looks good to me 👍

@davidsaada davidsaada dismissed stale reviews from deepikabhavnani and geky via 4a169b6 Apr 6, 2018

@davidsaada davidsaada force-pushed the davidsaada:david_flash_sim_bd branch from 16391e8 to 4a169b6 Apr 6, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented Apr 6, 2018

Pushed a fix with the following:

  • Removed redundant alignment to read_size
  • Added erase value as a parameter to the constructor

@davidsaada davidsaada force-pushed the davidsaada:david_flash_sim_bd branch 2 times, most recently from b8609c1 to cb992de Apr 6, 2018

@0xc0170 0xc0170 requested a review from SenRamakri Apr 9, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 9, 2018

Looks good to me 👍

Can you please use approve/request changes rather than comment (helps to see the actual status in reviewers) ?

@geky

Looks great 👍
This will be a nice block device wrapper to have

@0xc0170 0xc0170 added needs: review and removed needs: review labels Apr 10, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 10, 2018

waiting for @SenRamakri approval, meanwhile

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 10, 2018

Build : SUCCESS

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

Triggering tests

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

@0xc0170 0xc0170 requested a review from AnotherButler Apr 10, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 10, 2018

@AnotherButler Please review documentation

@mbed-ci

This comment has been minimized.

mbed-ci commented May 6, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added needs: review and removed needs: work labels May 7, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 7, 2018

Waiting for approvals

@davidsaada Docs will be added here as a reference (handbook PR) ?

@davidsaada davidsaada force-pushed the davidsaada:david_flash_sim_bd branch from 38cea35 to c3e3999 May 8, 2018

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented May 8, 2018

Pushed astyle related fixes

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented May 9, 2018

Doc PR here.

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 15, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 15, 2018

Requesting re-review from reviewers. Should be quick.

@0xc0170 @geky @AnotherButler @deepikabhavnani @SenRamakri

@mbed-ci

This comment has been minimized.

mbed-ci commented May 15, 2018

Build : SUCCESS

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

Triggering tests

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

@geky

geky approved these changes May 15, 2018

Looks great to me 👍

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@davidsaada

This comment has been minimized.

Contributor

davidsaada commented May 16, 2018

@cmonr @0xc0170 Looks like the failure is CI related (no space left on disk on couple of the failed devices). Would appreciate a morph test restart.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 16, 2018

That is correct, we noticed the failures today morning. We will restart all jobs as soon as the devices are back running (just restart does not help in this case).

_blank_buf_size = align_up(min_blank_buf_size, _bd->get_program_size());
if (!_blank_buf) {
_blank_buf = new uint8_t[_blank_buf_size];
MBED_ASSERT(_blank_buf);

This comment has been minimized.

@deepikabhavnani

deepikabhavnani May 16, 2018

Contributor

Non blocking and can be fixed in future - New will assert if memory allocation fails, additional check is not needed. Do you intent to use malloc instead here?

This comment has been minimized.

@davidsaada

davidsaada May 16, 2018

Contributor

Thanks, will remove the redundant check. And no - will keep using new[].

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 16, 2018

/morph test

1 similar comment
@cmonr

This comment has been minimized.

Contributor

cmonr commented May 17, 2018

/morph test

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 4b72158 into ARMmbed:master May 18, 2018

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 845 warnings
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9637 cycles (+78 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the needs: review label May 18, 2018

@davidsaada davidsaada deleted the davidsaada:david_flash_sim_bd branch Jul 9, 2018

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