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.

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

NRF52: fix config #11418

Merged
merged 2 commits into from Sep 6, 2019

Conversation

@0xc0170
Copy link
Member

commented Sep 5, 2019

Description

remove lib config and use target configuration instead. To avoid duplication of symbols, etc.

Fixes #10655

It could be simplified but I noticed not all targets from NRF52 contain overrides neither have one common parent (two MCU), therefore I only moved the config already there to the target config. No other change.

Pull request type

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

Reviewers

@SeppoTakalo @evedon

Release Notes

remove lib config and use target configuration instead. To avoid duplication of symbols, etc.

Fixes #10655
@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

There are few other lib json files in targets folder. These are at least targets config but there is silabs rail library included in targets there and contain mbed_lib.json file. It makes sense to have 3rd party driver there and contain configuration (common + target overrides) - this shall not be moved to targets.json file (or if yes, how to as its MCU_ like these are). Not much relevant here just looking at other .json files in targets folder.

This should be good as it is. I'll verify I moved all data correctly, will report back.

@ARMmbed/mbed-os-tools need your opinion here.

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

All checked, one to one copy. I found one mistake that I fixed.

@evedon
evedon approved these changes Sep 5, 2019
Copy link
Contributor

left a comment

Checked manually.

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

I am testing it with blinky baremetal , fails for me with very strange python error - I suspect there is duplication of symbols somewhere.

mbed compile -m NRF52840_DK -t ARM ends with:

Traceback (most recent call last):
  File "F:\Code\mbed-os-example-blinky-baremetal\mbed-os\tools\make.py", line 78, in wrapped_build_project
    *args, **kwargs
  File "F:\Code\mbed-os-example-blinky-baremetal\mbed-os\tools\build_api.py", line 595, in build_project
    objects = toolchain.compile_sources(resources, sorted(resources.get_file_paths(FileType.INC_DIR)))
  File "F:\Code\mbed-os-example-blinky-baremetal\mbed-os\tools\toolchains\mbed_toolchain.py", line 414, in compile_sources
    return self._compile_sources(resources, inc_dirs=inc_dirs)
  File "F:\Code\mbed-os-example-blinky-baremetal\mbed-os\tools\toolchains\mbed_toolchain.py", line 491, in _compile_sources
    return self.compile_queue(queue, objects)
  File "F:\Code\mbed-os-example-blinky-baremetal\mbed-os\tools\toolchains\mbed_toolchain.py", line 559, in compile_queue

    if p._taskqueue.queue:
AttributeError: '_queue.SimpleQueue' object has no attribute 'queue'
[mbed-22440] ERROR: "c:\programs\python37-32\python.exe" returned error.
@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

Can anyone test this? It looks like I have some env issue. I tried python2 and 3 , same error ,even without this fix, just using master and requires "nordic" workaround. K64F for instance builds, so its only NRF52840_DK releated on my machine :/

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

CI started

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Test

Build agains mbed OS master

---
Building project mbed-os-example-blinky-baremetal (NRF52840_DK, GCC_ARM)
Scan: mbed-os-example-blinky-baremetal
No Linker Script found

Build against pull/11418

[Building project mbed-os-example-blinky-baremetal (NRF52840_DK, GCC_ARM)
Scan: mbed-os-example-blinky-baremetal
Compile [ 18.3%]: crypto_device_platform.c
[Fatal Error] crypto_device_platform.c@21,26: platform_alt.h: No such file or directory
[ERROR] '_queue.SimpleQueue' object has no attribute 'queue'

Now it looks like FEATURE_CRYPTOCELL310/TARGET_MCU_NRF52840/crypto_device_platform.c is not compatible with bare-metal.

:(

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

@SeppoTakalo you are receiving the same error as me above. Is this an issue on master?

[Fatal Error] crypto_device_platform.c@21,26: platform_alt.h: No such file or directory

@Patater Can you review this failure? You should be able to reproduce using this PR with latest master and command #11418 (comment)

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

From the readme:

baremetal | Mbed OS 5
| Mbed TLS | Not available | Available |

tls is not available thus cryptocell should not be neither.

@Patater

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

As a quick solution, it's probably acceptable to remove CRYPTOCELL310 from the target, requiring users to opt-in to hardware acceleration on that platform. This would need to be documented in known issues.

The best solution would be to remove CRYPTOCELL310 only for baremetal builds, if that's possible. You don't need CRYPTOCELL310 is you don't have TLS or Crypto.

@mbed-ci

This comment has been minimized.

Copy link

commented Sep 5, 2019

Test run: SUCCESS

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

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

After discussing the matter in Slack, we are proposing that this is acceptable solution:

  • Merge this PR as is
  • Document as know issue that NRF52 does not build in Baremetal configuration, until application removes FEATURE_CRYPTOCELL310

This is minimal harm for user, as it requires changes only for baremetal builds and does not affect normal builds.

@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

We shall create an issue for this where we can continue discussion about features vs baremetal as I believe this come down to it (would need a review from tools if our understanding is correct). I can create an issue tomorrow.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Sep 6, 2019
@0xc0170 0xc0170 merged commit 021c67e into ARMmbed:master Sep 6, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8699 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@0xc0170 0xc0170 removed the ready for merge label Sep 6, 2019
@0xc0170

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

@adbridge Lets make sure the issue identified here is captured in the known issues:

NRF52 targets do not build for baremetal. Workaround: remove CRYPTOCELL310 feature via target config.

@0xc0170 0xc0170 deleted the 0xc0170:fix_nrf52 branch Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.