storage: Add block device API #3449

geky · 2016-12-15T17:29:50Z

Add prototype c++ block device api to the feature_block_device branch. Provides a simple interface class that can be extended to support different block devices.

Adopted the block device api in the SDBlockDevice and MemBlockDevice classes taken from mbed 2. Also added integration into the FATFileSystem class from mbed 2.

Sorry this is coming in so late, the topic was delayed by spawned discussions and put on hold for the release. I expect this may require changes based on @simonqhughes's requirements, just did not want to lose the work.

cc @simonqhughes, @c1728p9, @theotherjimmy

geky · 2016-12-19T23:57:35Z

Note: This is order dependent on #3468. Directory structure will be updated to match when #3468 is merged.

c1728p9

Might want to keep SDFileSystem around since user are use to using it. It should be straight forward to implement using the existing SDBlockDevice and FATFileSystem.

theotherjimmy · 2017-01-05T16:29:02Z

features/storage/bd/SDBlockDevice.cpp

@@ -157,10 +166,13 @@ int SDFileSystem::initialise_card() {
    }
    _spi.unlock();

+    int i = _cmd(0, 0);
+    printf("%x\n", i);


Is this printf supposed to be here? maybe debug_if?

Good catch, I was just verifying progress with mbed test so I didn't notice any serial output. will update

sg-

Looking good! The long typedef error names pushing parameters to a second line is kind of annoying. Especially as the application examples seem to use the native types rather than the classes type.

sg- · 2017-01-09T22:55:19Z

features/storage/bd/MemBlockDevice.cpp

+bd_error_t MemBlockDevice::init()
+{
+    if (!_blocks) {
+        _blocks = (uint8_t**)malloc(_count*sizeof(uint8_t*));


Didn't we decide a while back that out of memory was a programming terminating error? Guessing the intent is to use init for partitioning or multiple instances so new maybe a better choice here?

Can update to new[], this failure is unlikely relative to the allocation of the actual blocks of memory though it may still fail as a result of the user requesting too large a block of memory.

sg- · 2017-01-09T22:59:05Z

features/storage/bd/MemBlockDevice.h

+#include "mbed.h"
+
+
+/** Lazily allocated heap-backed block device


Heap is usually in RAM so maybe name RAMBlockDevice or HeapBlockDevice to remove ambiguity.

Was thinking about the name, MemBlockDevice comes from mbed 2 class. HeapBlockDevice seems the least ambiguous since you could have a RAMBlockDevice that is mapped to a region of RAM.

sg- · 2017-01-09T23:04:36Z

features/storage/bd/SDBlockDevice.cpp

 #include "mbed_debug.h"

 #define SD_COMMAND_TIMEOUT 5000

-#define SD_DBG             0
+#define SD_DBG             1


is this better enabled or disabled by default? I'd hate people not to see the debug member and change this to turn it off and fork the library

sg- · 2017-01-09T23:08:57Z

features/storage/fat/FATFileSystem.h

+    virtual int sync();
+
+    /**
+     * Formats a logical drive, FDISK artitioning rule, 512 bytes per cluster


geky · 2017-01-10T00:48:59Z

Sorry about the awful stateof debugging hooks in the pr. They should all be removed now and the directory structure should be updated to match the structure of the feature-branch.

geky · 2017-01-10T01:19:38Z

Everything should be updated and good to go besides tests/review.

@c1728p9, Do we still need to keep around the SDFileSystem? From offline discussion it sounded like a no since it was not included in the mbed 2 library. Let me know if you want me to include it with deprecations.

@sg- the long typedef names are a bit ugly to look at, but they are the most clear. We adopted similar names in the nsapi (example) which have worked well for identifying which functions don't return simple error codes. Users don't need to use the long names, and can stick to simple types.

Do you think there is better formatting (all one line? indention to line up parens?).

c1728p9 · 2017-01-10T15:15:49Z

After our discussion no need to keep SDFileSystem in the OS.

One possibly way you could simplify naming is to remove the name prefix from the enum and values and instead have it scoped to the class, similar to I2C and Serial.

geky · 2017-01-10T17:31:53Z

Not so sure about moving the type names into class scope. The namespacing works fine for inlined functions, but explodes in any external functions:

BlockDevice::count_or_error_t HeapBlockDevice::write(const void *b,
    BlockDevice::block_t block, BlockDevice::count_t count)
{
    blablabla;
}

The types declared in Serial/I2C see little use for this reason. A separate namespace would be more practical, but I'm worried about aggressive namespacing since we would loose the grepability and compatiblity with C. I'm not really sure the 3-character savings helps as much as just reformatting the functions anyways (should be updated to reflect this).

theotherjimmy · 2017-01-10T17:32:19Z

👍 for getting the testing in with the feature.

theotherjimmy · 2017-01-10T17:34:06Z

Certainly the 3 character prefix is hard to beat with a namespace.

Adopted in the SDBlockDevice and MemBlockDevice taken from mbed 2

Enables persistant storage between de/reinitialization cycles

Relatively unmodified, except for the addition of support for interchangeable block devices

@sg-

per @sg- on mbed-os#3449

options: MemBlockDevice - Same as mbed 2 class, but ambiguous RAMBlockDevice - May be ambiguous with BD on unmanaged RAM HeapBlockDevice - Seems the most reasonable name

adbridge · 2017-01-12T15:30:12Z

/morph test-nightly

mbed-bot · 2017-01-12T21:17:39Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1388

Build failed!

geky · 2017-01-31T20:02:55Z

Continued over here #3671

geky added the scope: feature label Dec 15, 2016

geky requested review from theotherjimmy, c1728p9 and simonqhughes December 15, 2016 17:29

theotherjimmy mentioned this pull request Dec 19, 2016

STORAGE: FAT32/SDCARD Filesystem support with basic test (basic.cpp) #3468

Merged

geky changed the base branch from feature_block_device to feature-storage December 19, 2016 23:57

geky changed the title ~~feature_block_device: Add block device API~~ feature-storage: Add block device API Dec 19, 2016

geky changed the title ~~feature-storage: Add block device API~~ storage: Add block device API Dec 19, 2016

c1728p9 approved these changes Jan 4, 2017

View reviewed changes

theotherjimmy reviewed Jan 5, 2017

View reviewed changes

geky added the needs: work label Jan 5, 2017

sg- reviewed Jan 9, 2017

View reviewed changes

geky force-pushed the block-device-2 branch 2 times, most recently from 38efe43 to 043028b Compare January 10, 2017 00:35

geky force-pushed the block-device-2 branch from 9825ac8 to 1a40f15 Compare January 10, 2017 01:07

geky added needs: CI needs: review and removed needs: work labels Jan 10, 2017

geky force-pushed the block-device-2 branch from ae13827 to 63deb38 Compare January 10, 2017 17:30

geky added 4 commits January 10, 2017 15:18

Added prototype block device

5834bcb

Adopted in the SDBlockDevice and MemBlockDevice taken from mbed 2

Moved deallocation of mem block device to destructor

9032ffe

Enables persistant storage between de/reinitialization cycles

Added the FATFileSystem from mbed 2

7633015

Relatively unmodified, except for the addition of support for interchangeable block devices

Moved MemBlockDevice allocation of block pointers to new

47e5154

per @sg- on mbed-os#3449

Renamed MemBlockDevice -> HeapBlockDevice

7321620

options: MemBlockDevice - Same as mbed 2 class, but ambiguous RAMBlockDevice - May be ambiguous with BD on unmanaged RAM HeapBlockDevice - Seems the most reasonable name

geky force-pushed the block-device-2 branch from 63deb38 to 16cd6c1 Compare January 10, 2017 21:48

This was referenced Jan 10, 2017

storage: Add the I2C EEPROM block device #3564

Closed

storage: Add util classes for composing block devices #3569

Closed

geky mentioned this pull request Jan 12, 2017

storage: Add the MX25RBlockDevice #3580

Closed

geky force-pushed the block-device-2 branch 2 times, most recently from 1474b75 to 5219a72 Compare January 13, 2017 21:27

0xc0170 added needs: work and removed needs: CI labels Jan 16, 2017

geky force-pushed the block-device-2 branch from 5219a72 to 8f84e62 Compare January 17, 2017 18:45

Fixed mbed 2 build_travis support of block device

f17c7dc

geky force-pushed the block-device-2 branch 2 times, most recently from 0964775 to f17c7dc Compare January 17, 2017 20:39

Adopted block device in filesystem tests

4ed1f19

geky force-pushed the block-device-2 branch from 9647280 to 4ed1f19 Compare January 19, 2017 09:31

geky mentioned this pull request Jan 31, 2017

storage: Add block device API #3671

Merged

geky closed this Jan 31, 2017

		#include "mbed.h"


		/** Lazily allocated heap-backed block device

storage: Add block device API #3449

storage: Add block device API #3449

Uh oh!

Conversation

geky commented Dec 15, 2016

Uh oh!

geky commented Dec 19, 2016

Uh oh!

c1728p9 left a comment

Choose a reason for hiding this comment

Uh oh!

theotherjimmy Jan 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sg- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky commented Jan 10, 2017

Uh oh!

geky commented Jan 10, 2017

Uh oh!

c1728p9 commented Jan 10, 2017

Uh oh!

geky commented Jan 10, 2017

Uh oh!

theotherjimmy commented Jan 10, 2017

Uh oh!

theotherjimmy commented Jan 10, 2017

Uh oh!

adbridge commented Jan 12, 2017

Uh oh!

mbed-bot commented Jan 12, 2017

Output

Uh oh!

geky commented Jan 31, 2017

Uh oh!

Uh oh!

theotherjimmy Jan 5, 2017 •

edited

Loading