storage: Add block device API #3671

geky · 2017-01-31T19:39:15Z

Update/rewrite of #3449 after significant discussion.

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

Additional classes:

BlockDevice
HeapBlockDevice
SDBlockDevice
ChainingBlockDevice
SlicingBlockDevice

Also adds a small suite of tests and integration into the FATFileSystem.

supersedes: #3449
cc @simonqhughes, @c1728p9, @theotherjimmy

simonqhughes · 2017-02-01T10:18:55Z

features/filesystem/sd/SDBlockDevice.h

+    virtual void debug(bool dbg);
+
+protected:
+    int _cmd(int cmd, int arg);


Please could you:

Add detailed doxygen comments for the protected member functions and attributes in this header file including @param lines? Where appropriate, comments can be simplified by referencing the appropriate section in Simplified_Physical_Layer_Spec.pdf (e.g. for commands).

Add brief comment explaining the 3 versions of initialize_card() are for the different card types.

Not a blocker for accepting PR but would like to have before 5.4.

Can add more documentation, although should these actually be private? not sure why these were protected.

simonqhughes · 2017-02-01T10:29:59Z

features/filesystem/bd/BlockDevice.h

+     *  @param buffer   Buffer to write blocks to
+     *  @param addr     Address of block to begin reading from
+     *  @param size     Size to read in bytes, must be a multiple of read block size
+     *  @return         0 on success, negative error code on failure


Developers normally expect read()/write() to return number of bytes successfully read/written on success (i.e. number > 0), and when this is smaller than the requested size then code would check why e.g. by inspecting errno. The reasons for it being different should be documented. During prototyping please look at failure modes and error handling to see if returning bytes read/written would be beneficial. If not, then OK.

Not a blocker for accepting PR but would like to have before 5.4.

Added a little comment.

The only case in which read/write/erase/program return fewer than the number of bytes requested of the operation is when an error occurs. However, in that condition, an error code is returned.

Adding size to the return value adds extra work to the implementations and extra checks to the users that have no benefit from what I can tell.

simonqhughes · 2017-02-01T10:41:40Z

features/filesystem/bd/HeapBlockDevice.h

+    virtual bd_size_t size();
+
+protected:
+    bd_size_t _read_size;


Please doxygen comment protected member attributes in header to explain function. Not a blocker for accepting PR but would like to have before 5.4.

Understood, though these should be private (will update)

simonqhughes · 2017-02-01T10:46:15Z

features/TESTS/filesystem/util_block_device/main.cpp

@@ -0,0 +1,171 @@
+#include "mbed.h"


Great to see tests :)

Please could you:

add standard copyright header.

add comment block to each test briefly explaining what they do.

Ah! good catch, will update

simonqhughes · 2017-02-01T10:48:22Z

features/TESTS/filesystem/util_block_device/main.cpp

@@ -0,0 +1,171 @@
+#include "mbed.h"


Great to see tests :)

Please could you:

add standard copyright header.

add comment block to each test briefly explaining what they do.

Suggestion: its beneficial to generate some trace message on an assert so there is some diagnostic information in the CI if/when the assert fails.

Not a blocker for accepting PR but would like to have before 5.4.

I've been using TEST_ASSERT_EQUAL since it will log the values it compared on failure, giving the user the return code of the function and line of the assert. I can also use TEST_ASSERT_EQUAL_MESSAGE to give context, I'll see where I can adopt this.

simonqhughes

The current CI's don't run the filesystem tests because the infrastructure is not enabled with SDcards in boards. When I ran the fopen and basic filesystem test locally on K64F with sdcards, there have been test regressions. Details follow.

This is what I did to run the tests:

I cloned your repo and checked out the block-device branch.
I edited fopen.cpp and basic.cpp to insert the following #define FSFAT_SDCARD_INSTALLED and #define DEVICE_SPI at the place indicated:

/* DEVICE_SPI
 *  This symbol is defined in targets.json if the target has a SPI interface, which is required for SDCard support.
 * FSFAT_SDCARD_INSTALLTED
 *  For testing purposes, an SDCard must be installed on the target for the test cases in this file to succeed.
 *  If the target has an SD card installed then uncomment the #define FSFAT_SDCARD_INSTALLED directive for the target.
 *
 * Notes
 *   The following 2 lines should be instated to perform the tests in this file:
 *      #define FSFAT_SDCARD_INSTALLED
 *      #define DEVICE_SPI
 */
// >>>>>>> insert the defines here <<<<<<<<<<<
#define FSFAT_SDCARD_INSTALLED
#define DEVICE_SPI
#if defined(DEVICE_SPI) && defined(FSFAT_SDCARD_INSTALLED)

I built
I ran the tests with:

(mx1_venv1) simhug01@E107851:/d/datastore/public/jobs/yr2017/2273/sdh_dev_mx1/geky_pr_3671$ mbedgt -VS --test-spec=ex_app2_fopen_spec.json 2>&1 | tee run_tests_master_gcc_geky_ex_app2_fopen_20170201_1432.txt
(mx1_venv1) simhug01@E107851:/d/datastore/public/jobs/yr2017/2273/sdh_dev_mx1/geky_pr_3671$ mbedgt -VS --test-spec=ex_app1_fat_basic_spec.json 2>&1 | tee run_tests_master_gcc_geky_ex_app2_basic_20170201_1432.txt

The test run files are attached below for reference.

run_tests_master_gcc_geky_ex_app2_fopen_20170201_1432.txt
run_tests_master_gcc_geky_ex_app2_basic_20170201_1432.txt

Please could you:

verify you see the same issue with the tests.
Please do the rework to get the tests working again.

I built armmbed/feature-storage branch this morning to check that all the test work. basic works without problem. fopen test 20 (a mkdir test) fails now, which I'll investigate and fix.

We need to get the CI's K64F test boards fitted with SDCards so this testing can be done by default and regressions don't creep in. @bridadan, please could you help getting the sdcards inserted into the boards? Please let us know many SDcards we need to purchase and we'll ask Richard to add it to the current target equipment requisition.

0xc0170 · 2017-02-01T13:40:13Z

features/filesystem/fat/FATFileSystem.cpp

 #include "ffconf.h"
 #include "mbed_debug.h"

 #include "FATFileSystem.h"
 #include "FATFileHandle.h"
 #include "FATDirHandle.h"
 #include "critical.h"
-#include "ff.h"
+//#include "ff.h"


should this inclusion be removed?

0xc0170 · 2017-02-01T14:07:35Z

features/TESTS/filesystem/util_block_device/main.cpp

+}
+
+void test_chaining() {
+    HeapBlockDevice bd1(8*BLOCK_SIZE, BLOCK_SIZE);


The constant 8 is all over the file, shall it be const uint8_t blocks_number = 8; for instance? I would also use a regular variable rather than a macro for BLOCK_SIZE if possible

Can update.

Although currently other tests in mbed use macros to allow overriding from the command line. I would rather keep this pr consistent with the current master.

If we want to change this it may be better as a separate pr that changes all tests at once.

to allow overriding from the command line

Forgot about this, sounds fine. If we want to fix it, might be good to have macros to set constants, to be visible during debugging. but thats for separate thread.

0xc0170 · 2017-02-01T14:10:41Z

features/filesystem/bd/ChainingBlockDevice.cpp

+        if (i == 0 || (read >= _read_size && is_aligned(read, _read_size))) {
+            _read_size = read;
+        } else if (_read_size > read && is_aligned(_read_size, read)) {
+            _read_size = _read_size;


is this self-assignment? A reason? I can see the same pattern below.

Just a style thing.

The purpose of this if statement is to choose the new value of _read_size. I can invert the second predicate as a condition for returning the error if you would like.

0xc0170 · 2017-02-01T14:28:36Z

features/filesystem/bd/ChainingBlockDevice.cpp

+    return 0;
+}
+
+bd_size_t ChainingBlockDevice::read_size()


get_ prefix for getters?

So should size become get_size?

Related, should File::len become File::get_len?

I don't have a preference, just want to clarify (and be consistent)

Lets verify but I assumed that we do get_/set_ or read_/write_ or start_/finish_ .

These are the only existing function I could find:

CallChain::size

FileHandle::flen

The size function is the odd one because of the STL. Maybe keep size but use get_ for read/write/erase_size? Thoughts?

be consistent

+1

The size function is the odd one because of the STL. Maybe keep size but use get_ for read/write/erase_size?

Sounds good to me, however I would use get_ everywhere for above +1 reason, no strong preference.

lets ask for opinions, thats not many in our code base yet. The HAL functions use prefixes.

cc @pan- @c1728p9 @sg- @simonqhughes

0xc0170 · 2017-02-01T14:31:12Z

features/filesystem/bd/ChainingBlockDevice.h

+     *  @param bds      Array of block devices to chain with sequential block addresses
+     *  @note All block devices must have the same block size
+     */
+    template <size_t SIZE>


is SIZE intentionally capital letters, I assume this is intended because there might be a macro SIZE somewhere? Usually it is Size

There's not a reason, I just wasn't sure if we had a standard style for template parameters. I can update to Size

If it's not a macro, should not be all uppercase.

0xc0170 · 2017-02-01T14:37:26Z

features/filesystem/sd/SDBlockDevice.h

+    uint32_t _sd_sectors();
+    uint32_t _sectors;
+
+    void set_init_sck(uint32_t sck) { _init_sck = sck; }


shall not a oneliner, same for 133

Will update, may just move inside source file, these were all just taken from the mbed 2 source

0xc0170 · 2017-02-01T14:38:04Z

features/filesystem/sd/SDBlockDevice.cpp

+
+bd_error_t SDBlockDevice::initialise_card()
+{
+	_dbg = SD_DBG;


tabs ? misaligned

0xc0170 · 2017-02-01T14:41:47Z

features/filesystem/sd/SDBlockDevice.cpp

+
+bd_error_t SDBlockDevice::erase(bd_addr_t addr, bd_size_t size)
+{
+    return 0;


Why is this method not implemented? (just I dont see a comment in the method docs neither here). Is there a reason this is empty or yet not implemented

You make a good point, I'll add a comment why this just returns zero.

As for the reason:

/** Erase blocks on a block device * * The state of an erased block is undefined until it has been programmed * * @param addr Address of block to begin erasing * @param size Size to erase in bytes, must be a multiple of erase block size * @return 0 on success, negative error code on failure */ virtual bd_error_t erase(bd_addr_t addr, bd_size_t size) = 0;

The SD card doesn't have a concept of "erasing" a block, so erase is a noop. This is intentionally allowed by the api (state of erased block is undefined).

As for why the SD class can't just implement write: There's no good way in c++ to have mutually recursive abstract functions, that is, you can't give an implementor the option of implementing write or program/erase. Since we can't give the implementor a choice, I went with having the more power functions abstract, since we want to encourage implementors to expose the most functionality.

0xc0170 · 2017-02-01T17:07:42Z

features/filesystem/sd/SDBlockDevice.h

+     *  @return         Size of a eraseable block in bytes
+     *  @note Must be a multiple of the program size
+     */
+    virtual bd_size_t erase_size();


I comment here but its probably for BlockDevice and others as well. Should erase_size depend on the address.

Use case - variable blocks that depend on the address ranges.
My use case is a flash sector and that one depends on the address range. For instance there's a device that on addresses 0-0x10000 contains sector sizes 0x1000 but on addresses 0x10000 - 0x20000 sector size is 0x500 (half of it). Therefore to get this erase size, a user needs to know what address is the block at.

Or any ideas how to resolve this variable blocks within a device?

This is sort of an open question and for external storage devices (the target of the block device api for now) we decided this was a low priority.

That being said, I was thinking about how the block device api would work on these devices. You could simply have a seperate block device for each region. With the chaining/slicing utility block devices you could combind these regions as needed:

FlashBlockDevice region1(/*start*/ 0x00000, /*stop*/ 0x10000, /*sector size*/ 0x1000); FlashBlockDevice region2(/*start*/ 0x10000, /*stop*/ 0x20000, /*sector size*/ 0x500); BlockDevice *bds[] = {&region1, &region2}; ChainingBlockDevice memory(bds); // sector size of 0x1000 inferred from arguments

Also, If you did want to add address-based erase_sizes to the api, this could be added later as an overload without breaking compatibility.

I think I'll overload this function with one argument address for this use case. Then erase_size returns 0, as it's would not be implemented.

Sounds like a good idea.

Maybe default erase_size should return the maximum erase size, which would retain compatibility with algorithms assuming a single erase size.

mazimkhan · 2017-02-02T09:54:40Z

@Patater
In here http://e108747.cambridge.arm.com:8080/job/mbed-os/job/mbed-os-pr-uvisor-test-pipeline/1456/execution/node/139/log/
is it normal
20:45:07 [1485981878.22][CONN][RXD] RTX error code: 0x00000001, task ID: 0x20002C78
after receiving this print we don't see anymore prints coming from the target

geky · 2017-02-02T17:33:07Z

I tested locally and fixed the regressions in the fopen/basic filesystem tests. Passes locally (except for mkdir issue).

Also updated based on review. Let me know if there's any other issues.

simonqhughes · 2017-02-03T10:03:39Z

Waiting for the uvisor team to fix the cam CI failure (possibly stack problem on EFM32?).

mazimkhan · 2017-02-03T10:42:22Z

@simonqhughes just clarify: This not a CI infrastructure failure. It is a test failure in uvisor-tests and @Patater is working on it.

0xc0170 · 2017-02-03T10:57:48Z

features/filesystem/sd/SDBlockDevice.h

+
+    SPI _spi;
+    DigitalOut _cs;
+    int cdv;


This private member should be _cdv as the rest, and what does it stand for? Might be longer to have more clear meaning?

That's a good question, this is what I get for inheriting the SD code, looking into it more it seems to be the block size? I'll update.

0xc0170 · 2017-02-03T11:00:36Z

features/filesystem/sd/SDBlockDevice.h

+    SPI _spi;
+    DigitalOut _cs;
+    int cdv;
+    int _is_initialized;


shouldnt this be bool?

0xc0170 · 2017-02-03T11:03:26Z

features/filesystem/sd/SDBlockDevice.h

+    bd_error_t initialise_card_v1();
+    bd_error_t initialise_card_v2();
+
+    int _read(uint8_t * buffer, uint32_t length);


should private methods start with _ ? we have it for private variables. Looking at other private methods in this patch, the rest is without _. same as 2 lines above.

I think so? Though I don't have a strong opinion.

I'll change for consistency.

0xc0170 · 2017-02-03T11:05:41Z

features/filesystem/bd/SlicingBlockDevice.h

@@ -0,0 +1,151 @@
+/* mbed Microcontroller Library
+ * Copyright (c) 2006-2012 ARM Limited


year in this patch for new files should be 2017

** Shakes fist at time gods **

Port of MemBlockDevice from mbed 2

- ChainingBlockDevice - SlicingBlockDevice

Patater · 2017-02-03T17:15:01Z

@geky

uVisor CI is failing due to a stack overflow in our test harness. A fix to uvisor-tests has been merged, but a required PR to mbed OS is in progress: #3692

Running the uVisor tests locally with the contents of this PR, all uVisor tests pass.

Thanks for helping to improve the uVisor CI.

@simonqhughes @mazimkhan

geky · 2017-02-03T18:05:32Z

Thanks @Patater, unfortunately we can't merge this pr until all checks are green to avoid trivializing the CI results. To get the little checkmark, do we need #3692 to get merged through to the feature-storage branch?

Patater · 2017-02-03T18:14:08Z

@geky

Yes, you'll need #3692 in feature-storage. It be good to wait to backport #3692 to feature-storage until #3692 has passed muster and landed in mbed OS master, less additional churn is introduced.

I know it's policy to require green CI for the mbed OS master branch. Is it also a written policy to require green CI for all PRs to mbed OS feature-storage branch? For instance, a feature branch could be pretty far out of date with mbed OS master, and there may not be time to rebase on the latest mbed OS. This was the case with the feature-storage branch not working with mbed-client previously. I do like that there is pressure to keep feature branches up to date with mbed OS master branch by requiring CI to pass even on feature branches, but that comes at the loss of flexibility of the feature branch owners: feature branch owners can no longer choose when to go through the effort of rebasing on the latest mbed OS master branch, as the CI will force them to do it now.

simonqhughes · 2017-02-04T09:30:28Z

@Patater feature-storage branch was rebased with master 4 days ago: #3667

We'll start merging feature-storage to master from 10 Feb; there is no intention to rebase again due to the additional work. Please could you revert the changes causing the CAM uvisor CI to fail? The current problem is having too big a delay on this PR.

@geky
I've tested filesystem test git://github.com/geky/mbed.git block-device branch and everything is OK. This is ready to merge.

AlessandroA · 2017-02-04T11:21:25Z

retest uvisor

geky added needs: CI needs: review scope: feature labels Jan 31, 2017

geky requested a review from simonqhughes January 31, 2017 19:39

geky force-pushed the block-device branch from 6457303 to b389861 Compare January 31, 2017 20:01

This was referenced Jan 31, 2017

storage: Add block device API #3449

Closed

storage: Add util classes for composing block devices #3569

Closed

geky force-pushed the block-device branch 2 times, most recently from c847d5b to c5de533 Compare January 31, 2017 20:28

geky mentioned this pull request Jan 31, 2017

storage: Add the MX25RBlockDevice #3580

Closed

geky force-pushed the block-device branch from c5de533 to 3440cd7 Compare January 31, 2017 22:29

geky mentioned this pull request Jan 31, 2017

storage: Add the I2C EEPROM block device #3564

Closed

simonqhughes reviewed Feb 1, 2017

View reviewed changes

simonqhughes suggested changes Feb 1, 2017

View reviewed changes

0xc0170 reviewed Feb 1, 2017

View reviewed changes

simonqhughes force-pushed the block-device branch from fc1f318 to 3440cd7 Compare February 1, 2017 14:20

0xc0170 reviewed Feb 1, 2017

View reviewed changes

geky force-pushed the block-device branch from 3440cd7 to 939fe65 Compare February 1, 2017 20:32

geky force-pushed the block-device branch 3 times, most recently from 39dd84a to 7468276 Compare February 2, 2017 17:32

geky force-pushed the block-device branch from 7468276 to f0593dd Compare February 2, 2017 20:29

0xc0170 reviewed Feb 3, 2017

View reviewed changes

geky force-pushed the block-device branch from f0593dd to b623db7 Compare February 3, 2017 16:09

geky added 7 commits February 3, 2017 10:13

bd: Added prototype block device api

95919ea

bd: Adopted the block device api in the HeapBlockDevice

1464471

Port of MemBlockDevice from mbed 2

bd: Adopted the block device api in the SDBlockDevice

5a6294d

bd: Added utility block device classes

ca27f6c

- ChainingBlockDevice - SlicingBlockDevice

bd: Adopted the block storage api in the FATFileSystem

fa33829

Fixed mbed 2 build_travis with block device

7563407

Updated filesystem tests to use FATFileSystem + SDBlockDevice

86f0468

geky force-pushed the block-device branch from b623db7 to 86f0468 Compare February 3, 2017 16:19

simonqhughes approved these changes Feb 4, 2017

View reviewed changes

simonqhughes merged commit 69ac772 into ARMmbed:feature-storage Feb 4, 2017

cmonr removed needs: CI needs: review labels Apr 3, 2018

		@@ -0,0 +1,151 @@
		/* mbed Microcontroller Library
		* Copyright (c) 2006-2012 ARM Limited

storage: Add block device API #3671

storage: Add block device API #3671

Uh oh!

Conversation

geky commented Jan 31, 2017

Uh oh!

simonqhughes Feb 1, 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!

simonqhughes Feb 1, 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!

simonqhughes Feb 1, 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!

Choose a reason for hiding this comment

Uh oh!

geky Feb 1, 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!

simonqhughes Feb 1, 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!

simonqhughes left a comment • 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!

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!

0xc0170 Feb 1, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geky Feb 1, 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!

0xc0170 Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

simonqhughes Feb 1, 2017 •

edited

Loading

simonqhughes Feb 1, 2017 •

edited

Loading

simonqhughes Feb 1, 2017 •

edited

Loading

geky Feb 1, 2017 •

edited

Loading

simonqhughes Feb 1, 2017 •

edited

Loading

simonqhughes left a comment •

edited

Loading

0xc0170 Feb 1, 2017 •

edited

Loading

geky Feb 1, 2017 •

edited

Loading

0xc0170 Feb 1, 2017 •

edited

Loading

0xc0170 Feb 1, 2017 •

edited

Loading

geky Feb 1, 2017 •

edited

Loading