NRF52840_DK: Fix baremetal linker error #12166

hugueskamba · 2019-12-22T23:25:36Z

Summary of changes

Fixes #11428
Compile in the inclusion of cryptocell310 only if the library is included
in the build

Impact of changes

It is now possible to build MCU_NRF52840 targets with the baremetal profile.
i.e It is now possible to build mbed-os-example-blinky-baremetal with $ mbed compile -t <TOOLCHAIN> -m NRF52840_DK

Migration actions required

Documentation

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)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2019-12-23T00:00:28Z

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

kjbracey

Why is this necessary? Why isn't the FEATURE_CRYPTOCELL310 test that's already there sufficient?

Note that this change also makes the cryptocell "opt-in", as apps will now need requires: "cryptocell".

Not saying that this change is wrong, just not quite clear about it.

Is it the case that the cryptocell stuff doesn't compile bare-metal, so doing this takes it out, and then blinky works? Would requires: "cryptocell" break it again?

bulislaw · 2019-12-23T09:05:55Z

Yeah I don't like to have double flag for hiding one thing. Is there something fundamentally stopping cryptocell working without RTOS? Answer "I don't know" is acceptable for now as we don't officially support TLS in baremetal, I would say would be ok to remove cryptocell from features in baremetal. I guess it could be done by the .lib we have in baremetal.

hugueskamba · 2019-12-23T14:37:38Z

Why is this necessary? Why isn't the FEATURE_CRYPTOCELL310 test that's already there sufficient?

Note that this change also makes the cryptocell "opt-in", as apps will now need requires: "cryptocell".

Not saying that this change is wrong, just not quite clear about it.

Is it the case that the cryptocell stuff doesn't compile bare-metal, so doing this takes it out, and then blinky works? Would requires: "cryptocell" break it again?

FEATURE_CRYPTOCELL31 is defined because the target is listed has supporting cryptocell in targets.json. What has been added is to check if the library is included in the build. A target can support a feature without using that feature as is the case with the bare metal profile. We have targets that support all kinds of features but not all the features are currently supported with the bare metal profile.
We do not modify the features that are supported by a target when using the bare metal. This PR clearly identifies the cryptocell lib and makes sure that the code that references it is included only if the library is present.

@kjbracey-arm Please see the impact of changes in the PR description. Without this PR it is not possible to build for that target using the bare metal profile.

bulislaw · 2020-01-02T10:55:57Z

As a quick fix maybe it would be better to not build cryptocell if we are in baremetal mode (without introducing new flags, but by testing the existing ones). Longer term I don't see why that shouldn't be possible.

hugueskamba · 2020-01-06T13:55:16Z

As a quick fix maybe it would be better to not build cryptocell if we are in baremetal mode (without introducing new flags, but by testing the existing ones). Longer term I don't see why that shouldn't be possible.

I am not sure there is an existing flag to use as the cryptocell code. The cryptocell code is not currently identified as a separate library.

0xc0170 · 2020-01-07T13:41:34Z

It was discussed previously here #11428 - this PR is a fix for this issue.

How do we remove tls for instance? The same could be applied to cryptocell ? Or do just need to remove this feature from the target itself and let an app to add it ?

0xc0170 · 2020-01-15T08:07:47Z

@hugueskamba Please review

hugueskamba · 2020-01-31T15:02:54Z

It was discussed previously here #11428 - this PR is a fix for this issue.

How do we remove tls for instance? The same could be applied to cryptocell ? Or do just need to remove this feature from the target itself and let an app to add it ?

From what I read in the GitHub issue, there was no proposed flag to use. Removing the feature was not seen as an option as it will also remove it for non-baremetal builds.

0xc0170 · 2020-02-10T10:54:29Z

CI started

mbed-ci · 2020-02-10T11:13:46Z

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

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

Compile in the inclusion of cryptocell310 only if the library is included in the build

Pull request has been modified.

hugueskamba · 2020-02-10T15:20:19Z

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM

jenkins-ci/mbed-os-ci_build-GCC_ARM

@0xc0170 ,
This force-push should fix it. Please re-start CI.
Thanks

0xc0170 · 2020-02-10T15:47:21Z

CI restarted

mbed-ci · 2020-02-10T17:52:15Z

Test run: SUCCESS

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

0xc0170 · 2020-02-11T12:32:16Z

This shall be now ready and can go in today.

ciarmcom requested review from a team December 23, 2019 00:00

ciarmcom added the needs: review label Dec 23, 2019

kjbracey reviewed Dec 23, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jan 3, 2020

hugueskamba force-pushed the hk-baremetal-NRF52840_DK-fix branch from ceb85fd to 89ecc81 Compare January 31, 2020 15:05

0xc0170 added needs: review and removed needs: work labels Feb 5, 2020

0xc0170 requested review from kjbracey and bulislaw February 5, 2020 14:50

bulislaw previously approved these changes Feb 10, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Feb 10, 2020

0xc0170 previously approved these changes Feb 10, 2020

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Feb 10, 2020

NRF52840_DK: Fix baremetal linker error

65fbee1

Compile in the inclusion of cryptocell310 only if the library is included in the build

hugueskamba force-pushed the hk-baremetal-NRF52840_DK-fix branch from 89ecc81 to 65fbee1 Compare February 10, 2020 15:18

0xc0170 approved these changes Feb 11, 2020

View reviewed changes

0xc0170 removed the needs: work label Feb 11, 2020

0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 14, 2020

0xc0170 merged commit a8188bf into ARMmbed:master Feb 14, 2020

hugueskamba deleted the hk-baremetal-NRF52840_DK-fix branch February 14, 2020 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

NRF52840_DK: Fix baremetal linker error #12166

NRF52840_DK: Fix baremetal linker error #12166

hugueskamba commented Dec 22, 2019 •

edited

Loading

Uh oh!

ciarmcom commented Dec 23, 2019

Uh oh!

kjbracey left a comment •

edited

Loading

Uh oh!

bulislaw commented Dec 23, 2019

Uh oh!

hugueskamba commented Dec 23, 2019 •

edited

Loading

Uh oh!

bulislaw commented Jan 2, 2020 •

edited

Loading

Uh oh!

hugueskamba commented Jan 6, 2020

Uh oh!

0xc0170 commented Jan 7, 2020

Uh oh!

0xc0170 commented Jan 15, 2020

Uh oh!

hugueskamba commented Jan 31, 2020

Uh oh!

0xc0170 commented Feb 10, 2020

Uh oh!

mbed-ci commented Feb 10, 2020

Uh oh!

hugueskamba commented Feb 10, 2020

Test run: FAILED

Uh oh!

0xc0170 commented Feb 10, 2020

Uh oh!

mbed-ci commented Feb 10, 2020

Uh oh!

0xc0170 commented Feb 11, 2020

Uh oh!

Uh oh!

NRF52840_DK: Fix baremetal linker error #12166

NRF52840_DK: Fix baremetal linker error #12166

Conversation

hugueskamba commented Dec 22, 2019 • 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 Dec 23, 2019

Uh oh!

kjbracey left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Dec 23, 2019

Uh oh!

hugueskamba commented Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bulislaw commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hugueskamba commented Jan 6, 2020

Uh oh!

0xc0170 commented Jan 7, 2020

Uh oh!

0xc0170 commented Jan 15, 2020

Uh oh!

hugueskamba commented Jan 31, 2020

Uh oh!

0xc0170 commented Feb 10, 2020

Uh oh!

mbed-ci commented Feb 10, 2020

Test run: FAILED

Uh oh!

hugueskamba commented Feb 10, 2020

Test run: FAILED

Uh oh!

0xc0170 commented Feb 10, 2020

Uh oh!

mbed-ci commented Feb 10, 2020

Test run: SUCCESS

Uh oh!

0xc0170 commented Feb 11, 2020

Uh oh!

Uh oh!

hugueskamba commented Dec 22, 2019 •

edited

Loading

kjbracey left a comment •

edited

Loading

hugueskamba commented Dec 23, 2019 •

edited

Loading

bulislaw commented Jan 2, 2020 •

edited

Loading