-
Notifications
You must be signed in to change notification settings - Fork 3k
Get type for BlockDevice #9135
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
Get type for BlockDevice #9135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
# Get type method addon to block devices class | ||
|
||
### Revision history | ||
| Revision | Date | Authors | Mbed OS version | Comments | | ||
|---------- |-----------------|-------------------------------------------------------------|------------------------|------------------| | ||
| 1.0 | 04/12/2018 | Yossi Levy ([@yossi2le](https://github.com/yossi2le/)) | 5.11+ | Initial revision | | ||
|
||
## Introduction | ||
|
||
Most storage solutions use block devices, whether it is Filesystems, KVStore or any other solution | ||
most of them will have some BlockDevice class underneath. However, sometimes a storage type or an application needs to know the physical | ||
BlockDevice type in order to work smoothly or the BlockDevice type in order to decide what is | ||
the best storage configuration to use. | ||
To address this an add-on method of getting type is proposed for BlockDevice interface class. | ||
|
||
## The Motivation | ||
|
||
Below there is a list of some examples to explain the motivation and the need for the adding of get_type to BlockDevice interface. | ||
|
||
examples: | ||
- TDBStore needs to know if there are flash characteristics for the block device and if there aren�t it should use | ||
FlashSimBlockDevice to simulate a flash BlockDevice. | ||
- TDBStore should not co-exists with NVStore, but this is true only if TDBStore is running on internal memory. Therefore if TDBStore running on | ||
internal memory and NVStore is also there an error should be raised. | ||
- When creating a file system you would prefer working with FAT on top of SD while LITTLEFS on top of any flash block device. | ||
Those preference in favor of better performance. | ||
|
||
To summarize the above, it may be very useful when using block device to know the type of the instance and especially, but not only, | ||
when using get_default_instace. Sometimes applications and tests would like to behave differently depending on the instance that has been created | ||
or provided to them. | ||
|
||
In fact it might be worth to consider adding the get_type to any interface with get_default_instance at mbed-os. | ||
|
||
## Dive into details | ||
we should add the following method to BlockDevice interface class. | ||
|
||
```virtual const char * get_type() const = 0;``` | ||
|
||
then every physical BlockDevice class which implements the interface should also implement this method and return a string | ||
representing its type. Furthermore, a nonphysical BlockDevice like SlicingBlockDevice should return the underlying physical | ||
BlockDevice type. | ||
|
||
### Physical BlockDevice: | ||
``` | ||
const char * HeapBlockDevice::get_type() const | ||
{ | ||
return "HEAP"; | ||
} | ||
``` | ||
|
||
### Logical BlockDevice: | ||
``` | ||
const char * SlicingBlockDevice::get_type() const | ||
{ | ||
if (_bd != NULL) { | ||
return _bd->get_type(); | ||
} | ||
|
||
return NULL; | ||
} | ||
``` | ||
|
||
### Open issue | ||
The ChainingBlockDevice which chains different type of physical block devices into one block device is unable | ||
to return the underneath physical as it contains two or more types. Therefore it will return CHAINING as its | ||
identity and its left for the user to decide how the application will treat this information. | ||
|
||
|
||
The below table describes physical BlockDevice and its tyep names | ||
|
||
|
||
| BlockDevice class | String description | | ||
|-----------------------------|--------------------| | ||
| HeapBlockDevice | "HEAP" | | ||
| SPIFBlockDevice | "SPIF" | | ||
| SPIFReducedBlockDevice | "SPIFR" | | ||
| QSPIFBlockDevice | "QSPIF" | | ||
| SDBlockDevice | "SD" | | ||
| FlashIAPBlockDevice | "FLASHIAP" | | ||
| DataFlashBlockDevice | "DATAFLASH" | | ||
| ChainingBlockDevice | "CHAINING" | |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -283,4 +283,9 @@ bd_size_t ChainingBlockDevice::size() const | |
return _size; | ||
} | ||
|
||
const char *ChainingBlockDevice::get_type() const | ||
{ | ||
return "CHAINING"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chaining is a special case and is not covered in design doc, Question - Do we allow different type of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks, that answers my query
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right about the documentation. I will do it tomorrow morning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documentation for ChainingBlockDevice added |
||
} | ||
|
||
} // namespace mbed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your example, in design docs, suggest NULL is legal value, here you assert on NULL it may be confusing to people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.