Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bare metal green tea test for storage component #11825

Merged
merged 2 commits into from
Nov 12, 2019
Merged

Bare metal green tea test for storage component #11825

merged 2 commits into from
Nov 12, 2019

Conversation

rajkan01
Copy link
Contributor

@rajkan01 rajkan01 commented Nov 6, 2019 •
edited
Loading

Description (required)

Skip partially or completely Bare metal green tea test for storage-related components which is based on RTOS and will not run in bare-metal.

Summary of change (What the change is for and why)
  1. Storage components like kvstore, nvstore, block devices, device key use the RTOS API partially in their test cases or complete test cases.
  2. Added the MBED_CONF_RTOS_PRESENT guard to skip those components test cases either completely or partially.
  • Note: These changes are done as identified that storage component uses RTOS API and not a very detailed analysis of the component.

3.Tested the green tea test on below list of targets with bare metal config and added the logs in IOTCORE-1397[https://jira.arm.com/browse/IOTCORE-1397] Jira ticket

  • K64F
  • K66F
  • Nucleo F429ZI
  • Nucleo F411RE
  • Nucleo L073RZ
  • Nucleo F207ZG
  1. A main green tea test framework and mbed-os test case changes for bare metal is available in PR Bare metal greentea support #11721
Documentation (Details of any document updates required)

Pull request type (required)

[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 (required)

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

Reviewers (optional)

@SeppoTakalo @jamesbeyond @evedon

Release Notes (required for feature/major PRs)

Summary of changes
Impact of changes
Migration actions required

@rajkan01
Skip Bare metal green tea test for storage related components or the …
8ecce14 
…components test cases which is based on RTOS
@ciarmcom ciarmcom requested review from evedon, jamesbeyond, SeppoTakalo and a team November 6, 2019 14:00
@ciarmcom
Copy link
Member

ciarmcom commented Nov 6, 2019

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

jamesbeyond
jamesbeyond approved these changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@jamesbeyond jamesbeyond left a comment

Choose a reason for hiding this comment

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

LGTM

@VeijoPesonen VeijoPesonen mentioned this pull request Nov 7, 2019
Merged
VeijoPesonen
VeijoPesonen suggested changes Nov 7, 2019
View reviewed changes
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

Please see my previous comment.

SeppoTakalo
SeppoTakalo suggested changes Nov 7, 2019
View reviewed changes
components/storage/blockdevice/COMPONENT_SD/SDBlockDevice.cpp Outdated
@@ -872,7 +872,9 @@ uint32_t SDBlockDevice::_go_idle_state()
if (R1_IDLE_STATE == response) {
break;
}
#if defined(MBED_CONF_RTOS_PRESENT)
Copy link
Contributor

Choose a reason for hiding this comment

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

ThisThread::sleep_for() is not requiring RTOS, so no need to guard.

features/device_key/TESTS/device_key/functionality/main.cpp Outdated
@@ -28,10 +32,6 @@
using namespace utest::v1;
using namespace mbed;

#if !DEVICEKEY_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

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

DEVICEKEY_ENABLED macro is coming from DeviceKey.h so this test should not be before the #include

features/storage/TESTS/kvstore/static_tests/main.cpp Outdated
@@ -67,6 +69,7 @@ static void parse_default_kv()
//init the blockdevice
static void kvstore_init()
{
#if defined(MBED_CONF_RTOS_PRESENT)
Copy link
Contributor

Choose a reason for hiding this comment

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

If init is skipped, then why is not the whole .cpp skipped on the compilation phase? Rest of the test cases are not working anyway.

features/storage/nvstore/TESTS/nvstore/functionality/main.cpp Outdated
@@ -27,10 +31,6 @@
#include <stdio.h>
#include <algorithm>

#if !NVSTORE_ENABLED
#error [NOT_SUPPORTED] NVSTORE needs to be enabled for this test
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I don't see the point of moving the #if to before #include as nvstore.h might disable that flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This macro comes from "features/storage/nvstore/mbed_lib.json".i think this is fine to keep at the beginning

Copy link
Contributor

Choose a reason for hiding this comment

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

Not totally true. It can be disabled, if FLASH is not enabled:
https://github.com/ARMmbed/mbed-os/blob/master/features/storage/nvstore/source/nvstore.h#L23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I noticed, I will move this after nvstore.h file

@rajkan01 rajkan01 force-pushed the feature-baremetal-greentea-storage branch from c560947 to d8e2dd5 Compare November 8, 2019 17:50
@SeppoTakalo
Copy link
Contributor

Looks OK.

VeijoPesonen
VeijoPesonen approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

Looks good to me

@rajkan01
Copy link
Contributor Author

rajkan01 commented Nov 11, 2019 •
edited
Loading

@adbridge @0xc0170 Please start the CI

@rajkan01
Copy link
Contributor Author

@adbridge Please start the CI

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 11, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Nov 11, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit dd753ab into ARMmbed:master Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evedon evedon evedon left review comments

@jamesbeyond jamesbeyond jamesbeyond approved these changes

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-version: 5.15.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@rajkan01 @ciarmcom @SeppoTakalo @0xc0170 @mbed-ci @evedon @VeijoPesonen @jamesbeyond @adbridge