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

Removing dependency of TDBStore in SystemStorage file and moving some file location - TFM support. #9361

Merged
merged 4 commits into from
Jan 17, 2019
Merged

Removing dependency of TDBStore in SystemStorage file and moving some file location - TFM support. #9361

merged 4 commits into from
Jan 17, 2019

Conversation

yossi2le
Copy link
Contributor

@yossi2le yossi2le commented Jan 13, 2019 •
edited
Loading

Description

The TFM build encounter issues because of coupling between TDBStore and SystemStorage file.
The coupling happens because of a function which tests if TDBStore and NVStore co-exist at the same time and if so create a runtime error. However, NVStore does not exist and does not even compile for TFM, therefore this checkup is redundant for TFM support.
This PR will remove the checkup only for TFM builds by adding a macro TARGET_TFM_BYPASS_NVSTORE_CHECK.

Also to ease the build process of TFM the KVStore.h has moved to include folder so it can be included in the TFM build as a separate file. Also, both DirectAccessDevicekey.h and DirectAccessDevicekey.cpp have moved to a folder of their own from the same reasons.

Pull request type

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

Reviewers

@ciarmcom ciarmcom requested review from a team January 13, 2019 12:00
@ciarmcom
Copy link
Member

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

@yossi2le yossi2le changed the title Removing dependency of TDBStore in SystemStorage file - TFM support. Removing dependency of TDBStore in SystemStorage file and moving KVStore.h - TFM support. Jan 13, 2019
@yossi2le yossi2le changed the title Removing dependency of TDBStore in SystemStorage file and moving KVStore.h - TFM support. Removing dependency of TDBStore in SystemStorage file and moving some file location - TFM support. Jan 13, 2019
alzix
alzix approved these changes Jan 13, 2019
View reviewed changes
Copy link
Contributor

@alzix alzix left a comment

Choose a reason for hiding this comment

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

LGTM
required for #9221

@NirSonnenschein
Copy link
Contributor

stating CI

@mbed-ci
Copy link

mbed-ci commented Jan 13, 2019

Test run: SUCCESS

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

0xc0170
0xc0170 reviewed Jan 14, 2019
View reviewed changes
features/storage/kvstore/tdbstore/TDBStore.cpp Outdated
@@ -24,7 +24,10 @@
#include "mbed_error.h"
#include "mbed_wait_api.h"
#include "MbedCRC.h"
//Bypass the check of NVStore co existance if compiled for TARGET_TFM
#if !(TARGET_BYPASS_NVSTORE_CHECK)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this target macro not a config ? or rather is this true for any TFM, should there be more generic name for exclusion like this?

the commit 2acb1c5754abb9ee1817f61d1584973e78d43ce5 does not say much what is going on here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yossi2le

Copy link
Contributor

Choose a reason for hiding this comment

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

Also here BYPASS_NVSTORE_CHECK ?

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 know :( I am working on it. 1 minute and it will be fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dannybenor
Copy link

@0xc0170 This #define was added to allow the build the secure execution environment without taking in the NVStore...
This is an internal change to support the PSA team and not need to documentation.

@alzix
Copy link
Contributor

alzix commented Jan 14, 2019 •
edited
Loading

@dannybenor @yossi2le i think @0xc0170 is asking why the new macro starts with TARGET_ which typically reserved for targets and labels.

@0xc0170, initially we wanted to piggyback TARGET_TFM macro which is already in use when building TF-M, but then storage team wanted to decouple this macro from TF-M target configuration, but the prefix remained. I do agree that it is not needed and is interfering with mbed-os configuration.

To summarize, I second @0xc0170 and suggest to remove TRAGET_ prefix.

@mikisch81
Copy link
Contributor

@ARMmbed/mbed-os-maintainers Why is the tools-py2.7 test fails?

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 14, 2019

We are investigating. One of test modules was recently updated and could lead to failures. Will be fixed soon

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 14, 2019

#9371 will fix travis

mikisch81
mikisch81 approved these changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@mikisch81 mikisch81 left a comment

Choose a reason for hiding this comment

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

LGTM

@mikisch81 mikisch81 mentioned this pull request Jan 15, 2019
Merged
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 15, 2019

Test run: FAILED

Summary: 3 of 7 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 15, 2019

CI job was aborted, 5.11.2 RC candidate is in the CI. We will restart it as soon as 5.11.2 RC is ready

@cmonr
Copy link
Contributor

cmonr commented Jan 16, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 17, 2019

Test run: SUCCESS

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

@cmonr cmonr merged commit 4f95298 into ARMmbed:master Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alzix alzix alzix approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@dannybenor dannybenor dannybenor approved these changes

@mikisch81 mikisch81 mikisch81 approved these changes

@NirSonnenschein NirSonnenschein NirSonnenschein approved these changes

@davidsaada davidsaada davidsaada approved these changes

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

Successfully merging this pull request may close these issues.
10 participants
@yossi2le @ciarmcom @NirSonnenschein @mbed-ci @dannybenor @alzix @mikisch81 @0xc0170 @cmonr @davidsaada