Restructure storage blockdevice directory #13273

rajkan01 · 2020-07-10T14:33:04Z

Summary of changes

Restructured storage/blockdevice as per the new directory structure proposal:

storage
├── blockdevice
│   ├── include
│   │   └── blockdevice
│   │          └── [...]
│   ├── source
│   │   └── [...]
│   ├── tests
│        ├── TESTS
│        ├── UNITTESTS

Note:

UNITTESTS/stubs sources left untouched as it is used across different libraries.

Impact of changes

None.

Migration actions required

None.

Documentation

To Be Updated.

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Manual testing: (Build for K64F target with ARMC6 and GCC_ARM toolchain)

mbed-os-example-blockdevice
Greentea tests: Full and bare metal profiles
Unit tests build and run

Reviewers

@bulislaw @0xc0170 @ARMmbed/mbed-os-core

ciarmcom · 2020-07-10T15:00:32Z

@rajkan01, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

LDong-Arm · 2020-07-13T10:22:56Z

UNITTESTS/CMakeLists.txt

@@ -175,10 +176,11 @@ foreach(testfile ${unittest-file-list})
  include("${testfile}")

  get_filename_component(TEST_SUITE_DIR ${testfile} DIRECTORY)
+  message( STATUS ${PROJECT_SOURCE_DIR} )


Is it a debug message?

Sorry, I missed this one. I will update

LDong-Arm · 2020-07-13T10:23:23Z

.astyleignore

@@ -27,3 +27,4 @@
 ^TESTS/mbed_hal/trng/pithy
 ^tools
 ^UNITTESTS
+^storage/blockdevice/tests/UNITTESTS


I wonder if it's possible to match all UNITTESTS in mbed-os

LDong-Arm

LGTM

evedon

Code change is fine.
(You could squash the last commit with the commit that introduced the issue)

Pull request has been modified.

rajkan01 · 2020-07-15T17:07:28Z

Code change is fine.
(You could squash the last commit with the commit that introduced the issue)

@evedon I squashed last commit as suggested, please re-review

evedon · 2020-07-15T17:40:52Z

I would also suggest to simplify the path for greentea tests by removing one blockdevice instance. So instead of storage/blockdevice/tests/TESTS/blockdevice/flashsim_block_device/main.cpp, you could have storage/blockdevice/tests/TESTS/flashsim_block_device/main.cpp

rajkan01 · 2020-07-15T17:55:30Z

I would also suggest to simplify the path for greentea tests by removing one blockdevice instance. So instead of storage/blockdevice/tests/TESTS/blockdevice/flashsim_block_device/main.cpp, you could have storage/blockdevice/tests/TESTS/flashsim_block_device/main.cpp

Current Mbed OS build tools to build the greentea test looks for an XYZ directory (It can be any name) under TESTS (Magic name) to consider as test suite and then it will build all the test under XYZ directory
for example:
TESTS/block device/flashsimlock_device => build
TESTS/flashsim_block_device => not recognized by build tools

[Warning] Test with name 'storage-blockdevice-tests-tests-flashsim_block_device' was not found in the available tests

evedon · 2020-07-16T10:48:57Z

I would also suggest to simplify the path for greentea tests by removing one blockdevice instance. So instead of storage/blockdevice/tests/TESTS/blockdevice/flashsim_block_device/main.cpp, you could have storage/blockdevice/tests/TESTS/flashsim_block_device/main.cpp

Current Mbed OS build tools to build the greentea test looks for an XYZ directory (It can be any name) under TESTS (Magic name) to consider as test suite and then it will build all the test under XYZ directory
for example:
TESTS/block device/flashsimlock_device => build
TESTS/flashsim_block_device => not recognized by build tools

[Warning] Test with name 'storage-blockdevice-tests-tests-flashsim_block_device' was not found in the available tests

Alas, let's leave this for another day then...

LDong-Arm

Just have one question regarding the COMPONENT_* "sub" libraries to discuss.

LDong-Arm · 2020-07-16T15:20:36Z

storage/kvstore/conf/kv_config.cpp

 #if COMPONENT_FLASHIAP
-#include "components/storage/blockdevice/COMPONENT_FLASHIAP/FlashIAPBlockDevice.h"
+#include "storage/blockdevice/COMPONENT_FLASHIAP/FlashIAPBlockDevice.h"
 #endif

 #if COMPONENT_QSPIF
-#include "components/storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h"
+#include "storage/blockdevice/COMPONENT_QSPIF/QSPIFBlockDevice.h"
 #endif


Just realised we haven't refactored the include paths for COMPONENT_*/*BlockDevice.h - they are public APIs but don't follow the "include" proposal in this PR.

Shall we use the same path name **/include/blockdevice/** inside each of them, so we would have a consistent

#include "blockdevice/FlashIAPBlockDevice.h" #include "blockdevice/QSPIFBlockDevice.h" ...

@rajkan01 @evedon ?

Initial blockdevice directory restructure proposal, just to move the source COMPONENT_xxx directly into storage/blockdevice and no refactoring. if we would like to changes on COMPONENT_XXX then we will do it incremental changes

I agree that we should do in a subsequent PR.

LDong-Arm

In the proposal we didn't explicit document some of the sub libraries under blockdevice, but anything that's public API requires refactoring.

- Update the header reference and source reference in cmake - Update astyleignore to ignore blockdevice UNITTESTS - Removed the redundant debug log

Pull request has been modified.

rajkan01 · 2020-07-17T11:03:08Z

@adbridge Rebased this PR to resolve the merge conflicts, Could you start the CI

adbridge · 2020-07-17T11:17:00Z

@evedon @LDong-Arm are you happy with this PR? If so please approve so we can get it into CI

evedon · 2020-07-17T11:31:24Z

@evedon @LDong-Arm are you happy with this PR? If so please approve so we can get it into CI

@adbridge I am pretty sure this PR was approved by both of us but the rebase has dismissed the approvals.

adbridge · 2020-07-17T13:00:01Z

CI started

mbed-ci · 2020-07-17T14:04:21Z

Test run: SUCCESS

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

mergify · 2020-07-17T14:56:49Z

This PR does not contain release version label after merging.

ciarmcom requested review from a team July 10, 2020 15:00

ciarmcom added the needs: review label Jul 10, 2020

mergify bot added needs: work and removed needs: review labels Jul 10, 2020

rajkan01 force-pushed the storage_rb_dir_restructure branch from a794899 to c1df660 Compare July 10, 2020 18:13

rajkan01 changed the title ~~Storage rb dir restructure~~ Refactor storage blockdevice directory Jul 10, 2020

LDong-Arm reviewed Jul 13, 2020

View reviewed changes

LDong-Arm approved these changes Jul 13, 2020

View reviewed changes

rajkan01 changed the title ~~Refactor storage blockdevice directory~~ Restructure storage blockdevice directory Jul 13, 2020

rajkan01 mentioned this pull request Jul 15, 2020

Restructure storage filesystem directory #13296

Closed

evedon previously approved these changes Jul 15, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: work labels Jul 15, 2020

rajkan01 force-pushed the storage_rb_dir_restructure branch from fc3a412 to 2d98fd8 Compare July 15, 2020 16:51

rajkan01 mentioned this pull request Jul 15, 2020

Restructure storage filesystem directory #13300

Merged

rajkan01 closed this Jul 15, 2020

rajkan01 reopened this Jul 15, 2020

evedon previously approved these changes Jul 16, 2020

View reviewed changes

LDong-Arm reviewed Jul 16, 2020

View reviewed changes

LDong-Arm suggested changes Jul 16, 2020

View reviewed changes

mergify bot added the needs: work label Jul 17, 2020

rajkan01 added 5 commits July 17, 2020 03:10

Refactor storage/blockdevice directory

036831e

Update the header file reference

e92efbd

Move storgae greentea test

9aec101

Move storgae unittests

d47689b

Unittests and Astyle:

3bcb4eb

- Update the header reference and source reference in cmake - Update astyleignore to ignore blockdevice UNITTESTS - Removed the redundant debug log

rajkan01 force-pushed the storage_rb_dir_restructure branch from 2d98fd8 to 3bcb4eb Compare July 17, 2020 10:26

adbridge added needs: review and removed needs: work labels Jul 17, 2020

evedon approved these changes Jul 17, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jul 17, 2020

LDong-Arm approved these changes Jul 17, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Jul 17, 2020

adbridge merged commit ca2f980 into ARMmbed:master Jul 17, 2020

mergify bot removed the ready for merge label Jul 17, 2020

mergify bot added the release version missing When PR does not contain release version, bot should label it and we fix it afterwards label Jul 17, 2020

adbridge added release-type: patch Indentifies a PR as containing just a patch and removed release version missing When PR does not contain release version, bot should label it and we fix it afterwards labels Jul 17, 2020

mbedmain added release-version: 6.2.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 16, 2020

Restructure storage blockdevice directory #13273

Restructure storage blockdevice directory #13273

Uh oh!

Conversation

rajkan01 commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Jul 10, 2020

Uh oh!

LDong-Arm Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

rajkan01 Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm Jul 13, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

evedon left a comment

Choose a reason for hiding this comment

Uh oh!

rajkan01 commented Jul 15, 2020

Uh oh!

evedon commented Jul 15, 2020

Uh oh!

rajkan01 commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evedon commented Jul 16, 2020

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

LDong-Arm Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

rajkan01 Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

evedon Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

LDong-Arm left a comment

Choose a reason for hiding this comment

Uh oh!

rajkan01 commented Jul 17, 2020

Uh oh!

adbridge commented Jul 17, 2020

Uh oh!

evedon commented Jul 17, 2020

Uh oh!

adbridge commented Jul 17, 2020

Uh oh!

mbed-ci commented Jul 17, 2020

Test run: SUCCESS

Uh oh!

mergify bot commented Jul 17, 2020

Uh oh!

Uh oh!

rajkan01 commented Jul 10, 2020 •

edited

Loading

rajkan01 commented Jul 15, 2020 •

edited

Loading