Get type for BlockDevice #9135

yossi2le · 2018-12-18T12:35:42Z

Description

In this PR we like to add get_type method to blockdevice classes.
The motivation for this change is described in the docs/design-documents/features/storage/BlockDevices/get_type_method.md document which is part of this PR.

Pull request type

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

kjbracey · 2018-12-18T12:49:02Z

components/storage/blockdevice/COMPONENT_SD/SDBlockDevice.cpp

@@ -248,6 +248,7 @@ using namespace mbed;

 // Only HC block size is supported. Making this a static constant reduces code size.
 const uint32_t SDBlockDevice::_block_size = BLOCK_SIZE_HC;
+const char * SDBlockDevice::_type = "SD";


This goes into RAM. const char * const would avoid that, but would still likely waste 4 bytes on a pointer.

It should be const char SDBlockDevice::_type[] = "SD";. But even then, why do that? It doesn't need to be an actual variable. The get_type() functions can just return a string literal.

10x. I have changed it to string literal as you proposed.

yossi2le · 2018-12-18T12:56:55Z

@geky , @flit , @bulislaw , @donatieng , @sg-, and @AnotherButler please review.
@kjbracey-arm, I skipped tagging you cause I see you already started the review.
Thanks

kjbracey · 2018-12-18T13:22:36Z

This achieves somethings similar to a mechanism we have in NetworkInterface - that has a series of methods that act like dynamic_casts, eg WiFiInterface *NetworkInterface::wifiInterface() which returns either WiFiInterface pointer, or NULL if it's not a WiFiInterface

So this is different to that, but I can see why - it's not an actual C++ type query, but an underlying device type query. This wouldn't be legal:

if (strcmp(bd->get_type(), "SD") == 0) {
    SDBlockDevice *sd = static_cast<SDBlockDevice *>(bd);
}

As long as that's as far as it goes, it seems reasonable, but if you ever feel you would want to get at the underlying driver type - find the underlying SDBlockDevice on a composed driver to call a method on it - then maybe the NetworkInterface "dynamic cast" version might be better than this pseudo-typeinfo. Maybe you don't want to encourage that sort of thing though - it would be stepping around the upper layer.

Or maybe the two mechanisms could coexist - a dynamic_cast-like thing that checks the actual top-level type, and a string query for the base physical(?) type.

kjbracey · 2018-12-18T13:43:22Z

components/storage/blockdevice/COMPONENT_FLASHIAP/FlashIAPBlockDevice.cpp

@@ -36,6 +36,8 @@ using namespace mbed;
 #define DEBUG_PRINTF(...)
 #endif

+#define BD_TYPE "FLASHIAP"


Others may disagree, but I'm not a fan of this model, at all. Writing:

const char * FlashIAPBlockDevice::get_type() { return BD_TYPE; }

is basically semantically meaningless. Extend that further and you can get

REG1 = REG1_VALUE; REG2 = REG2_VALUE; <repeat for 100s of lines>

which I've seen in the past and it made me wince. It basically forces people to look in two places for a single piece of information. Pointless indirection.

Isn't this better?

const char * FlashIAPBlockDevice::get_type() { return "FLASHIAP"; }

No need to have a #define for a constant if that constant is used in exactly one place - doubly so if that place is the "specification" point itself.

Alternatively you could make the constants public in the drivers' header files so people strcmp against FLASHIAP_TYPE rather than "FLASHIAP", meaning it is used in more than one place, but don't think that buys you anything.

bulislaw

I'd like to see a real situation how that will be used, it's not extremely object oriented solution and seems rather static. To explain what i'm talking about: QSPIF and SPIF for most use cases should be treated the same in this example some code will need to be extended if (strcmp(bd_type, "QSPIF") || strcmp(bd_type, "SPIF")) { I would say solution based on queried characteristics that can be inherited and overwritten when needed would be more in OOP spirit. That being said, it's up to @dannybenor to approve

bulislaw · 2018-12-18T13:56:45Z

docs/design-documents/features/storage/BlockDevices/get_type_method.md

+
+### Physical BlockDevice:
+```
+const char * HeapBlockDevice::_type = "HEAP";


Example should return string literal same as the code.

bulislaw · 2018-12-18T13:57:33Z

docs/design-documents/features/storage/BlockDevices/get_type_method.md

+| FlashIAPBlockDevice         | "FLASHIAP"         |
+| DataFlashBlockDevice        | "DATAFLASH"        |
+
+


Nit: Lots of empty lines

bulislaw · 2018-12-18T13:58:25Z

features/storage/TESTS/blockdevice/general_block_device/main.cpp

-    Case("Testing contiguous erase, write and read", test_contiguous_erase_write_read, greentea_failure_handler),
-    Case("Testing BlockDevice::get_erase_value()", test_get_erase_value, greentea_failure_handler),
-    Case("Testing program read small data sizes", test_program_read_small_data_sizes, greentea_failure_handler)
+    //Case("Testing read write random blocks", test_random_program_read_erase, greentea_failure_handler),


Why is that commented out?

sorry, my mistake. I'll fix it

bulislaw · 2018-12-18T14:08:10Z

features/storage/TESTS/blockdevice/general_block_device/main.cpp

+        return;
+    }
+    const char * bd_type = block_device->get_type();
+    TEST_ASSERT_NOT_EQUAL(0, bd_type);


Your example, in design docs, suggest NULL is legal value, here you assert on NULL it may be confusing to people.

NULL may be valid if there is no underlying bd and the blockdevice yet. However its not valid for the test and therefore assert on NULL.

yossi2le · 2018-12-18T15:04:17Z

I'd like to see a real situation how that will be used, it's not extremely object oriented solution and >seems rather static. To explain what i'm talking about: QSPIF and SPIF for most use cases should be >treated the same in this example some code will need to be extended if (strcmp(bd_type, "QSPIF") || >strcmp(bd_type, "SPIF")) { I would say solution based on queried characteristics that can be inherited >and overwritten when needed would be more in OOP spirit. That being said, it's up to @dannybenor >to approve

@bulislaw this solution is being used at PR #9136, you can take a look at it.

yossi2le · 2018-12-18T15:14:31Z

@kjbracey-arm we do expect to use this mechanism for getting the underlying physical block device and not for dynamic casting. Currently, I can't see the need for the dynamic cast mechanism on our side, but actually adding more interfaces in the long-long-term future is not blocked because of this PR.

deepikabhavnani · 2018-12-18T17:36:53Z

features/storage/blockdevice/ChainingBlockDevice.cpp

@@ -283,4 +283,9 @@ bd_size_t ChainingBlockDevice::size() const
    return _size;
 }

+const char * ChainingBlockDevice::get_type()
+{
+    return "CHAINING";


Chaining is a special case and is not covered in design doc,

Question - Do we allow different type of physical block devices to be chained together?

Chaining different physical block devices is currently allowed (there's no way to check such a thing up to this PR). However, the more interesting question is whether it should be allowed. I don't have a definite answer for it. However, if we wish to forbid it, we can change the type in this PR to the underlying one's.
@geky What do you say?

Chaining different physical block devices is currently allowed

Thanks, that answers my query

However, the more interesting question is whether it should be allowed.

Yes, but can keep it outside the scope of this PR.

For this PR, we should document the special case of chaining as note in API. Clearly stating that instead of physical block device, response return value is "chaining" string

You are right about the documentation. I will do it tomorrow morning.

Documentation for ChainingBlockDevice added

cmonr · 2018-12-20T15:30:20Z

@bulislaw @kjbracey-arm @deepikabhavnani @davidsaada @ARMmbed/mbed-os-storage @dannybenor
Any chance for a re-review sometime soon?

deepikabhavnani · 2018-12-20T17:46:19Z

#9135 (comment)

@cmonr - This is pending

kjbracey

I think I'm almost fine - just noticed another missing const though.

kjbracey · 2018-12-21T08:58:31Z

components/storage/blockdevice/COMPONENT_DATAFLASH/DataFlashBlockDevice.h

+     *
+     *  @return         A string represent the BlockDevice class type.
+     */
+    virtual const char *get_type();


Method should presumably be const. Otherwise, approved.

yossi2le · 2018-12-26T15:23:56Z

This PR probably will fail CI due to mbed-os-example-bootloader build error. I have issued a PR #94 to fix the example and solve the issue.

dannybenor · 2018-12-27T08:08:37Z

#94 seems not the right PR

yossi2le · 2018-12-27T10:21:10Z

#94 seems not the right PR

The PR is at mbed-os-example-bootloader repo.
ARMmbed/mbed-os-example-bootloader#95
Please notice that 94 has been closed and 95 was issued instead

Needs rereview

yossi2le · 2019-01-06T16:19:39Z

mbed-os-example-bootloader PR 94 has been merged instead of 95. It should be OK now. Can we proceed with this PR now?

0xc0170 · 2019-01-07T07:53:24Z

Reviewers please leave feedback

yogpan01 · 2019-01-07T08:54:57Z

@yossi2le @0xc0170 Can this be merged, we need to get list of PRs in so that we can start timely testing on Client side.
@jenia81 @evedon FYI.

0xc0170 · 2019-01-07T09:17:08Z

CI started

@yossi2le @0xc0170 Can this be merged, we need to get list of PRs in so that we can start timely testing on Client side.

It was waiting for review to complete, now in CI and shall be ready soon (note we experienced some CI node failures over the weekend , still waiting for confirmation it is fixed today)

mbed-ci · 2019-01-07T12:17:01Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

0xc0170 · 2019-01-09T13:57:15Z

Release label updated based on the agreement

screamerbg · 2019-01-28T14:03:43Z

@0xc0170 Why was this merged in a patch release when apparently is a feature and also introduces a breaking change?

dannybenor · 2019-01-29T09:31:08Z

@yossi2le can you create a PR for adding the function in https://github.com/OpenNuvoton/NuMaker-mbed-SD-driver?

0xc0170 · 2019-01-29T09:47:04Z

@0xc0170 Why was this merged in a patch release when apparently is a feature and also introduces a breaking change?

The decision was made as noted above on the product level (Release label updated based on the agreement). @ARMmbed/mbed-os-storage can provide more details

yossi2le · 2019-01-29T10:16:28Z

Hi Danny How should this one represent itself. Is it “SD” like the regular SD BlockDevice or should it be “NUSD” type? B.R. Yossi From: Daniel Benor <[email protected]> Sent: Tuesday, January 29, 2019 11:31 AM To: ARMmbed/mbed-os <[email protected]> Cc: Yossi Levy <[email protected]>; Mention <[email protected]> Subject: Re: [ARMmbed/mbed-os] Get type for BlockDevice (#9135) @yossi2le<https://github.com/yossi2le> can you create a PR for adding the function in https://github.com/OpenNuvoton/NuMaker-mbed-SD-driver? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#9135 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AiZvlYFnohXO-7uWVb0fXLQk4nLvCPdyks5vIBTpgaJpZM4ZYN2Z>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

dannybenor · 2019-01-29T10:27:20Z

NUSD From: Yossi Levy <[email protected]> Sent: Tuesday, 29 January 2019 12:17 To: ARMmbed/mbed-os <[email protected]> Cc: Daniel Benor <[email protected]>; Mention <[email protected]> Subject: Re: [ARMmbed/mbed-os] Get type for BlockDevice (#9135) Hi Danny How should this one represent itself. Is it “SD” like the regular SD BlockDevice or should it be “NUSD” type? B.R. Yossi From: Daniel Benor <[email protected]<mailto:[email protected]>> Sent: Tuesday, January 29, 2019 11:31 AM To: ARMmbed/mbed-os <[email protected]<mailto:[email protected]>> Cc: Yossi Levy <[email protected]<mailto:[email protected]>>; Mention <[email protected]<mailto:[email protected]>> Subject: Re: [ARMmbed/mbed-os] Get type for BlockDevice (#9135) @yossi2le<https://github.com/yossi2le> can you create a PR for adding the function in https://github.com/OpenNuvoton/NuMaker-mbed-SD-driver? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#9135 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AiZvlYFnohXO-7uWVb0fXLQk4nLvCPdyks5vIBTpgaJpZM4ZYN2Z>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#9135 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AH6cWBoC9LjW0qJ1cvMaCXiYDSSSorJ6ks5vIB-FgaJpZM4ZYN2Z>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

…9135 PR requirement

yossi2le changed the title ~~Get type for bd~~ Get type for BlockDevice Dec 18, 2018

yossi2le mentioned this pull request Dec 18, 2018

TDBStore and NVStore should create an error if co exist. #9136

Merged

kjbracey reviewed Dec 18, 2018

View reviewed changes

0xc0170 added the needs: review label Dec 18, 2018

0xc0170 requested a review from a team December 18, 2018 13:26

kjbracey reviewed Dec 18, 2018

View reviewed changes

bulislaw reviewed Dec 18, 2018

View reviewed changes

bulislaw requested a review from dannybenor December 18, 2018 14:15

yossi2le force-pushed the yossi_get_type_for_bd branch from c1b2323 to cec2e2f Compare December 18, 2018 15:05

deepikabhavnani reviewed Dec 18, 2018

View reviewed changes

cmonr added the release-version: 5.12.0-rc1 label Dec 19, 2018

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

kjbracey previously requested changes Dec 21, 2018

View reviewed changes

Yossi Levy added 4 commits December 23, 2018 11:03

Add get_type method to block devices.

1d71fb1

astyle fix

b8b7292

astyle fix

9590441

Documnent changes and adding const to get_type method

b17d13e

yossi2le force-pushed the yossi_get_type_for_bd branch from c88e78d to b17d13e Compare December 23, 2018 12:27

NirSonnenschein added needs: review and removed needs: work labels Dec 24, 2018

0xc0170 approved these changes Jan 4, 2019

View reviewed changes

davidsaada approved these changes Jan 7, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Jan 7, 2019

0xc0170 added ready for merge and removed needs: CI labels Jan 7, 2019

0xc0170 merged commit 0eabafa into ARMmbed:master Jan 7, 2019

0xc0170 added release-version: 5.11.2 and removed ready for merge release-version: 5.12.0-rc1 labels Jan 7, 2019

screamerbg mentioned this pull request Jan 28, 2019

The driver cannot compile with Mbed OS 5.11.2 and above. OpenNuvoton/NuMaker-mbed-SD-driver#1

Closed

screamerbg mentioned this pull request Jan 28, 2019

Prematurely introduced feature in PATCH release of Mbed OS (5.11.2) #9519

Closed

yossi2le pushed a commit to yossi2le/NuMaker-mbed-SD-driver that referenced this pull request Jan 29, 2019

Adding get_type method to NuSDBlockDevice to fulfill ARMmbed/mbed-os#…

f99570e

…9135 PR requirement

yossi2le mentioned this pull request Jan 29, 2019

Adding get_type method to NuSDBlockDevice OpenNuvoton/NuMaker-mbed-SD-driver#2

Merged

0xc0170 added the BREAKING-CHANGE label Mar 18, 2019

		\| FlashIAPBlockDevice \| "FLASHIAP" \|
		\| DataFlashBlockDevice \| "DATAFLASH" \|

Get type for BlockDevice #9135

Get type for BlockDevice #9135

Conversation

yossi2le commented Dec 18, 2018 • edited by 0xc0170 Loading

Description

Pull request type

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yossi2le commented Dec 18, 2018

kjbracey commented Dec 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bulislaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yossi2le commented Dec 18, 2018

yossi2le commented Dec 18, 2018

Choose a reason for hiding this comment

davidsaada Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

deepikabhavnani Dec 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmonr commented Dec 20, 2018

deepikabhavnani commented Dec 20, 2018 • edited Loading

kjbracey left a comment

Choose a reason for hiding this comment

kjbracey Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yossi2le commented Dec 26, 2018

dannybenor commented Dec 27, 2018

yossi2le commented Dec 27, 2018 • edited Loading

yossi2le commented Jan 6, 2019

0xc0170 commented Jan 7, 2019

yogpan01 commented Jan 7, 2019

0xc0170 commented Jan 7, 2019 • edited Loading

mbed-ci commented Jan 7, 2019

0xc0170 commented Jan 9, 2019

screamerbg commented Jan 28, 2019

dannybenor commented Jan 29, 2019

0xc0170 commented Jan 29, 2019 • edited Loading

yossi2le commented Jan 29, 2019 via email

dannybenor commented Jan 29, 2019 via email

yossi2le commented Dec 18, 2018 •

edited by 0xc0170

Loading

davidsaada Dec 19, 2018 •

edited

Loading

deepikabhavnani Dec 19, 2018 •

edited

Loading

deepikabhavnani commented Dec 20, 2018 •

edited

Loading

kjbracey Dec 21, 2018 •

edited

Loading

yossi2le commented Dec 27, 2018 •

edited

Loading

0xc0170 commented Jan 7, 2019 •

edited

Loading

0xc0170 commented Jan 29, 2019 •

edited

Loading