Skip to content

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

Merged
merged 4 commits into from
Jan 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,11 @@ bd_size_t DataFlashBlockDevice::size() const
return device_size;
}

const char *DataFlashBlockDevice::get_type() const
{
return "DATAFLASH";
}

/**
* @brief Function for reading a specific register.
* @details Used for reading either the Status Register or Manufacture and ID Register.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,12 @@ class DataFlashBlockDevice : public mbed::BlockDevice {
*/
virtual mbed::bd_size_t size() const;

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

private:
// Master side hardware
mbed::SPI _spi;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,4 +248,9 @@ bd_size_t FlashIAPBlockDevice::size() const
return _size;
}

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

#endif /* DEVICE_FLASH */
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,12 @@ class FlashIAPBlockDevice : public mbed::BlockDevice {
*/
virtual mbed::bd_size_t size() const;

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

private:
// Device configuration
mbed::FlashIAP _flash;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,11 @@ bd_size_t QSPIFBlockDevice::get_erase_size() const
return _min_common_erase_size;
}

const char *QSPIFBlockDevice::get_type() const
{
return "QSPIF";
}

// Find minimal erase size supported by the region to which the address belongs to
bd_size_t QSPIFBlockDevice::get_erase_size(bd_addr_t addr)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ class QSPIFBlockDevice : public mbed::BlockDevice {
*/
virtual mbed::bd_size_t size() const;

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

private:
// Internal functions

Expand Down Expand Up @@ -307,8 +313,6 @@ class QSPIFBlockDevice : public mbed::BlockDevice {
int _utils_iterate_next_largest_erase_type(uint8_t &bitfield, int size, int offset, int boundry);

private:
// Internal Members

// QSPI Driver Object
mbed::QSPI _qspi;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ enum ops {
#define SPIF_WEL 0x2
#define SPIF_WIP 0x1


SPIFReducedBlockDevice::SPIFReducedBlockDevice(
PinName mosi, PinName miso, PinName sclk, PinName cs, int freq)
: _spi(mosi, miso, sclk), _cs(cs), _size(0)
Expand Down Expand Up @@ -344,3 +343,9 @@ bd_size_t SPIFReducedBlockDevice::size() const
{
return _size;
}

const char *SPIFReducedBlockDevice::get_type() const
{
return "SPIFR";
}

Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,12 @@ class SPIFReducedBlockDevice : public mbed::BlockDevice {
*/
virtual mbed::bd_size_t size() const;

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

private:
// Master side hardware
mbed::SPI _spi;
Expand Down
5 changes: 5 additions & 0 deletions components/storage/blockdevice/COMPONENT_SD/SDBlockDevice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,11 @@ bd_size_t SDBlockDevice::size() const
return _block_size * _sectors;
}

const char *SDBlockDevice::get_type() const
{
return "SD";
}

void SDBlockDevice::debug(bool dbg)
{
_dbg = dbg;
Expand Down
5 changes: 5 additions & 0 deletions components/storage/blockdevice/COMPONENT_SD/SDBlockDevice.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ class SDBlockDevice : public mbed::BlockDevice {
*/
virtual int frequency(uint64_t freq);

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

private:
/* Commands : Listed below are commands supported
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,11 @@ int SPIFBlockDevice::get_erase_value() const
return 0xFF;
}

const char *SPIFBlockDevice::get_type() const
{
return "SPIF";
}

/***************************************************/
/*********** SPI Driver API Functions **************/
/***************************************************/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,12 @@ class SPIFBlockDevice : public mbed::BlockDevice {
*/
virtual mbed::bd_size_t size() const;

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

private:

// Internal functions
Expand Down
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" |
27 changes: 26 additions & 1 deletion features/storage/TESTS/blockdevice/general_block_device/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,30 @@ void test_program_read_small_data_sizes()
delete block_device;
}

void test_get_type_functionality()
{
BlockDevice *block_device = BlockDevice::get_default_instance();
if (block_device == NULL) {
TEST_SKIP_MESSAGE("No block device component is defined for this target");
return;
}
const char *bd_type = block_device->get_type();
TEST_ASSERT_NOT_EQUAL(0, bd_type);
Copy link
Member

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.

Copy link
Contributor Author

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.


#if COMPONENT_QSPIF
TEST_ASSERT_EQUAL(0, strcmp(bd_type, "QSPIF"));
#elif COMPONENT_SPIF
TEST_ASSERT_EQUAL(0, strcmp(bd_type, "SPIF"));
#elif COMPONENT_DATAFLASH
TEST_ASSERT_EQUAL(0, strcmp(bd_type, "DATAFLASH"));
#elif COMPONENT_SD
TEST_ASSERT_EQUAL(0, strcmp(bd_type, "SD"));
#elif COMPONET_FLASHIAP
TEST_ASSERT_EQUAL(0, strcmp(bd_type, "FLASHIAP"));
#endif

}

utest::v1::status_t greentea_failure_handler(const Case *const source, const failure_t reason)
{
greentea_case_failure_abort_handler(source, reason);
Expand All @@ -484,7 +508,8 @@ Case cases[] = {
Case("Testing multi threads erase program read", test_multi_threads, greentea_failure_handler),
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 program read small data sizes", test_program_read_small_data_sizes, greentea_failure_handler),
Case("Testing get type functionality", test_get_type_functionality, greentea_failure_handler)
};

Specification specification(test_setup, cases);
Expand Down
14 changes: 14 additions & 0 deletions features/storage/TESTS/blockdevice/heap_block_device/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,19 @@ void test_read_write()

}

void test_get_type_functionality()
{
uint8_t *dummy = new (std::nothrow) uint8_t[TEST_BLOCK_DEVICE_SIZE];
TEST_SKIP_UNLESS_MESSAGE(dummy, "Not enough memory for test");
delete[] dummy;

HeapBlockDevice bd(TEST_BLOCK_DEVICE_SIZE, TEST_BLOCK_SIZE);

const char *bd_type = bd.get_type();
TEST_ASSERT_NOT_EQUAL(0, bd_type);
TEST_ASSERT_EQUAL(0, strcmp(bd_type, "HEAP"));
}


// Test setup
utest::v1::status_t test_setup(const size_t number_of_cases)
Expand All @@ -164,6 +177,7 @@ utest::v1::status_t test_setup(const size_t number_of_cases)

Case cases[] = {
Case("Testing read write random blocks", test_read_write),
Case("Testing get type functionality", test_get_type_functionality)
};

Specification specification(test_setup, cases);
Expand Down
6 changes: 6 additions & 0 deletions features/storage/blockdevice/BlockDevice.h
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,12 @@ class BlockDevice {
(addr + size) % get_erase_size(addr + size - 1) == 0 &&
addr + size <= this->size());
}

/** Get the BlockDevice class type.
*
* @return A string represent the BlockDevice class type.
*/
virtual const char *get_type() const = 0;
};

} // namespace mbed
Expand Down
9 changes: 9 additions & 0 deletions features/storage/blockdevice/BufferedBlockDevice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -330,4 +330,13 @@ bd_size_t BufferedBlockDevice::size() const
return _bd_size;
}

const char *BufferedBlockDevice::get_type() const
{
if (_bd != NULL) {
return _bd->get_type();
}

return NULL;
}

} // namespace mbed
6 changes: 6 additions & 0 deletions features/storage/blockdevice/BufferedBlockDevice.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,12 @@ class BufferedBlockDevice : public BlockDevice {
*/
virtual bd_size_t size() const;

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

protected:
BlockDevice *_bd;
bd_size_t _bd_program_size;
Expand Down
5 changes: 5 additions & 0 deletions features/storage/blockdevice/ChainingBlockDevice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -283,4 +283,9 @@ bd_size_t ChainingBlockDevice::size() const
return _size;
}

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

Choose a reason for hiding this comment

The 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 physical block devices to be chained together?

Copy link
Contributor

@davidsaada davidsaada Dec 19, 2018

Choose a reason for hiding this comment

The 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.
@geky What do you say?

Copy link

@deepikabhavnani deepikabhavnani Dec 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation for ChainingBlockDevice added

}

} // namespace mbed
6 changes: 6 additions & 0 deletions features/storage/blockdevice/ChainingBlockDevice.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,12 @@ class ChainingBlockDevice : public BlockDevice {
*/
virtual bd_size_t size() const;

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

protected:
BlockDevice **_bds;
size_t _bd_count;
Expand Down
10 changes: 10 additions & 0 deletions features/storage/blockdevice/ExhaustibleBlockDevice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -194,4 +194,14 @@ bd_size_t ExhaustibleBlockDevice::size() const
return _bd->size();
}

const char *ExhaustibleBlockDevice::get_type() const
{
if (_bd != NULL) {
return _bd->get_type();
}

return NULL;
}

} // namespace mbed

Loading