Add overloaded get_erase_size API with address parameter to BlockDevice #6408

davidsaada · 2018-03-20T18:00:02Z

Description

Add an overloaded get_erase_size API with address parameter to BlockDevice and all derived classes.
This is due to the fact that in some flash components, erase size varies depending on address.

Pull request type

Feature

cmonr

LGTM!

cmonr · 2018-03-20T22:25:04Z

@AnotherButler, if you would like to take a quick look. Small doc updates.

cmonr · 2018-03-20T22:27:12Z

@geky if you could take a look at the API addition. Imo, looks straightforward.

geky

I'm for it, though unfortunately the slicing/chaining block devices need some changes.

cc @deepikabhavnani, @c1728p9

geky · 2018-03-20T22:29:46Z

features/filesystem/bd/BlockDevice.h

     *
-     *  @return         Size of a eraseable block in bytes
+     *  @return         Size of an eraseable block in bytes


Ah, that's what I get for using search/replace on english...

geky · 2018-03-20T22:50:17Z

features/filesystem/bd/ChainingBlockDevice.cpp

+{
+    return _erase_size;
+}
+


Oh, unfortunately this implementation is going to be much more complicated. We need to lookup the erase size for the block inside the list of block devices.

geky · 2018-03-20T22:51:27Z

features/filesystem/bd/SlicingBlockDevice.cpp

@@ -102,6 +102,11 @@ bd_size_t SlicingBlockDevice::get_erase_size() const
    return _bd->get_erase_size();
 }

+bd_size_t SlicingBlockDevice::get_erase_size(bd_addr_t addr) const
+{
+    return _bd->get_erase_size(addr);


This needs to have the _start offset added to addr before passing addr down

geky · 2018-03-20T22:53:25Z

features/filesystem/bd/MBRBlockDevice.cpp

@@ -276,6 +276,11 @@ bd_size_t MBRBlockDevice::get_erase_size() const
    return _bd->get_erase_size();
 }

+bd_size_t MBRBlockDevice::get_erase_size(bd_addr_t addr) const
+{
+    return _bd->get_erase_size(addr);


This also needs to add _offset to addr.

davidsaada · 2018-03-21T11:49:30Z

@geky Thanks for the comments. Pushed a fix. Please observe the changes (mainly the one in ChainingBlockDevice.cpp).

cmonr · 2018-03-21T23:07:46Z

Pinging @geky for a re-review. Also, possibly @deepikabhavnani.

Primarily wondering if ChainingBlockDevice is the only block device that needed non-trivial changes/updates.

davidsaada · 2018-03-22T12:06:46Z

@cmonr I believe it is. As:
HeapBlockDevice is the basic one, not relying on anything else.
ExhaustibleBlockDevice, ObservingBlockDevice, ReadOnlyBlockDevice and ProfilingBlockDevice only add features that don't change anything related to addresses comparing to the underlying BD.
MBRBlockDevice and SlicingBlockDevice do change things comparing to the underlying BD, but they are easy to implement (and @geky showed how).
Only non-trivial one is ChainingBlockDevice, as it's the only one dealing with multiple underlying BDs.

geky · 2018-03-22T12:14:24Z

features/filesystem/bd/ChainingBlockDevice.cpp

+    MBED_ASSERT(0);
+    return 0; // satisfy compiler
+}
+


Yeah, this looks good 👍

Nothing to block this PR: Just queries for my understanding

I see _bd_count(sizeof(bds) / sizeof(bds[0]) set during constructor, does this mean we erase all chain blocks in erase operation?

An erase can cross multiple block devices in the chain.

Here's where it's implemented:
https://github.com/davidsaada/mbed-os/blob/a6048005d7c7ee06b7956918589d381a9f41f159/features/filesystem/bd/ChainingBlockDevice.cpp#L174-L194

deepikabhavnani · 2018-03-22T15:29:48Z

This is due to the fact that in some flash components, erase size varies depending on address.

Please explain this comment, how erase size will vary based on the address

deepikabhavnani · 2018-03-22T15:30:16Z

features/filesystem/bd/BlockDevice.h

+     *  @return         Size of an erasable block in bytes
+     *  @note Must be a multiple of the program size
+     */
+    virtual bd_size_t get_erase_size(bd_addr_t addr) const


Please add param in doxygen - addr

cmonr · 2018-03-22T17:17:39Z

@davidsaada Could you squash Amanda's copy edits, and add another commit for Deepika's comments?

After that, we should be good to start CI.

davidsaada · 2018-03-22T22:16:07Z

Please explain this comment, how erase size will vary based on the address

@deepikabhavnani This is relevant in case the block device implements a flash components having different sector sizes (erase size = sector size). In this case, the API cannot return a constant erase size, as it depends on the address.

… devices

deepikabhavnani · 2018-03-22T22:43:58Z

This is relevant in case the block device implements a flash components having different sector sizes (erase size = sector size)

Single block device supporting different sector sizes? I don;t think we support that.. @geky Please clarify.

davidsaada · 2018-03-23T12:32:39Z

Single block device supporting different sector sizes? I don;t think we support that.

@deepikabhavnani Indeed, we currently don't have any block device that has variant erase sizes. However, unless we add this API, we'll never be able to implement block devices supporting flash components with variant sector sizes.

cmonr · 2018-03-23T14:44:16Z

/morph build

mbed-ci · 2018-03-23T15:18:25Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-03-23T18:45:04Z

Exporter Build : FAILURE

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

cmonr · 2018-03-23T18:48:34Z

12:38:17 FATAL: command execution failed
12:38:17 java.io.EOFException

ಠ_ಠ

/morph export-build

mbed-ci · 2018-03-23T21:49:22Z

Test : SUCCESS

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

mbed-ci · 2018-03-24T02:22:02Z

Exporter Build : SUCCESS

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

0xc0170 · 2018-03-24T09:13:05Z

@AnotherButler Can you review the latest update?

0xc0170 · 2018-03-24T09:13:21Z

@davidsaada Handbook update reference?

cmonr previously approved these changes Mar 20, 2018

View reviewed changes

cmonr requested a review from AnotherButler March 20, 2018 22:24

cmonr requested a review from geky March 20, 2018 22:26

cmonr added needs: review release-version: 5.9.0-rc1 labels Mar 20, 2018

geky reviewed Mar 20, 2018

View reviewed changes

cmonr added needs: work and removed needs: review labels Mar 20, 2018

geky added the scope: feature label Mar 20, 2018

davidsaada dismissed cmonr’s stale review via 487dc28 March 21, 2018 11:47

davidsaada force-pushed the david_erase_size_addr branch from 257740a to 487dc28 Compare March 21, 2018 11:47

0xc0170 added needs: review and removed needs: work labels Mar 21, 2018

cmonr requested a review from deepikabhavnani March 21, 2018 23:07

geky reviewed Mar 22, 2018

View reviewed changes

geky previously approved these changes Mar 22, 2018

View reviewed changes

AnotherButler dismissed geky’s stale review via 1e4a590 March 22, 2018 14:50

deepikabhavnani reviewed Mar 22, 2018

View reviewed changes

AnotherButler previously approved these changes Mar 22, 2018

View reviewed changes

davidsaada mentioned this pull request Mar 22, 2018

block device: add IAPBlockDevice, uesd user flash to store data #6422

Closed

4 tasks

Add overloaded get_erase_size API with address parameter to all block…

a604800

… devices

davidsaada dismissed AnotherButler’s stale review via a604800 March 22, 2018 22:36

davidsaada force-pushed the david_erase_size_addr branch from 58951f5 to a604800 Compare March 22, 2018 22:36

geky approved these changes Mar 23, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels Mar 23, 2018

0xc0170 added ready for merge needs: review and removed needs: CI ready for merge labels Mar 24, 2018

AnotherButler approved these changes Mar 26, 2018

View reviewed changes

theotherjimmy added ready for merge and removed needs: review labels Mar 29, 2018

theotherjimmy merged commit 64df0dd into ARMmbed:master Mar 29, 2018

cmonr removed the ready for merge label Apr 2, 2018

davidsaada deleted the david_erase_size_addr branch July 9, 2018 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add overloaded get_erase_size API with address parameter to BlockDevice #6408

Add overloaded get_erase_size API with address parameter to BlockDevice #6408

davidsaada commented Mar 20, 2018

cmonr left a comment

cmonr commented Mar 20, 2018

cmonr commented Mar 20, 2018

geky left a comment

geky Mar 20, 2018

geky Mar 20, 2018 •

edited

Loading

geky Mar 20, 2018

geky Mar 20, 2018

davidsaada commented Mar 21, 2018 •

edited

Loading

cmonr commented Mar 21, 2018

davidsaada commented Mar 22, 2018 •

edited

Loading

geky Mar 22, 2018

deepikabhavnani Mar 22, 2018

geky Mar 23, 2018

deepikabhavnani commented Mar 22, 2018

deepikabhavnani Mar 22, 2018

cmonr commented Mar 22, 2018

davidsaada commented Mar 22, 2018

deepikabhavnani commented Mar 22, 2018

davidsaada commented Mar 23, 2018

cmonr commented Mar 23, 2018

mbed-ci commented Mar 23, 2018

mbed-ci commented Mar 23, 2018

cmonr commented Mar 23, 2018 •

edited

Loading

mbed-ci commented Mar 23, 2018

mbed-ci commented Mar 24, 2018

0xc0170 commented Mar 24, 2018

0xc0170 commented Mar 24, 2018

Add overloaded get_erase_size API with address parameter to BlockDevice #6408

Add overloaded get_erase_size API with address parameter to BlockDevice #6408

Conversation

davidsaada commented Mar 20, 2018

Description

Pull request type

cmonr left a comment

Choose a reason for hiding this comment

cmonr commented Mar 20, 2018

cmonr commented Mar 20, 2018

geky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geky Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsaada commented Mar 21, 2018 • edited Loading

cmonr commented Mar 21, 2018

davidsaada commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepikabhavnani commented Mar 22, 2018

Choose a reason for hiding this comment

cmonr commented Mar 22, 2018

davidsaada commented Mar 22, 2018

deepikabhavnani commented Mar 22, 2018

davidsaada commented Mar 23, 2018

cmonr commented Mar 23, 2018

mbed-ci commented Mar 23, 2018

Triggering tests

mbed-ci commented Mar 23, 2018

cmonr commented Mar 23, 2018 • edited Loading

mbed-ci commented Mar 23, 2018

mbed-ci commented Mar 24, 2018

0xc0170 commented Mar 24, 2018

0xc0170 commented Mar 24, 2018

geky Mar 20, 2018 •

edited

Loading

davidsaada commented Mar 21, 2018 •

edited

Loading

davidsaada commented Mar 22, 2018 •

edited

Loading

cmonr commented Mar 23, 2018 •

edited

Loading