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

Fixing a macro to detect when RTOS was in use for the NRF52840_DK #4021

Merged
merged 2 commits into from Mar 29, 2017

Conversation

Projects
None yet
7 participants
@bridadan
Contributor

bridadan commented Mar 24, 2017

Description

The recently added NRF52840_DK broke the mbed 2 nightly. This is because the check for the presence of the RTOS was using the wrong macro. I have fixed it in this PR.

Status

READY

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

NO

Todos

  • morph test
  • export build
  • mbed 2 bot

Steps to test or reproduce

Currently on master if you run the following command:

$ python tools/build_release.py -t GCC_ARM -p NRF52840_DK -v

You will get the following error:

[Error] app_timer_rtx.c@47,0:  #5: cannot open source input file "cmsis_os.h": No such file or directory

This branch should fix the issue.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 24, 2017

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 24, 2017

Just tried the uvision exporter locally and there's still an issue with this platform. It's because there are two nrf_soc.h files that are being included:

  1. https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NORDIC/TARGET_NRF5_SDK13/sdk/drivers_nrf/nrf_soc_nosd/nrf_soc.h
  2. https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_NORDIC/TARGET_NRF5_SDK13/TARGET_MCU_NRF52840/sdk/softdevice/s140/headers/nrf_soc.h

I tried my hand at resolving it and I wasn't sure which file to rename. I think we probably want to rename the one in the softdevice dirtectory but I'm not sure. Any tips @anangl @nvlsianpu @pan-? This will also affect all the files that include it and I'm not sure which of the files that include nrf_soc.h will need the updated file.

@nvlsianpu

This comment has been minimized.

Contributor

nvlsianpu commented Mar 24, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 24, 2017

@nvlsianpu You can't remove that directory, as features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5_SDK13/source/btle/btle.cpp depends on targets/TARGET_NORDIC/TARGET_NRF5_SDK13/TARGET_MCU_NRF52840/sdk/softdevice/s140/headers/ble_types.h Whoops, wrong dir.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 24, 2017

@nvlsianpu It compiles fine with targets/TARGET_NORDIC/TARGET_NRF5_SDK13/sdk/drivers_nrf/nrf_soc_nosd/ removed.

Remove the nrf_soc_nosd directory
This removes the duplicate header files from the build. We were getting
lucky on most invocations of `mbed compile` in that these headers were
searched for after some others, but not when exporting to uvision.
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 24, 2017

/morph export-build

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 24, 2017

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 24, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph export-build

Output

mbed Build Number: 142

All exports and builds passed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 24, 2017

[Build 1317]
SUCCESS: Building succeeded and tests were run! Be sure to check the test results

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 24, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 24, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1740

All builds and test passed!

@bridadan bridadan removed the needs: CI label Mar 24, 2017

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 24, 2017

Just looking for reviews from @c1728p9 and @pan-, but then this should be ok to come in. And the sooner the better, this fixes issues on master.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 26, 2017

@bridadan thanks for spotting this and fixing

@pan-

pan- approved these changes Mar 27, 2017

I'm happy with the changes.
@bridadan thanks.

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 29, 2017

@0xc0170 This PR is blocking a whole bunch of other PRs from being tested, so if we can prioritize this getting merged that'd be great 😄

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 29, 2017

@0xc0170 This PR is blocking a whole bunch of other PRs from being tested, so if we can prioritize this getting merged that'd be great 😄

Yes, aware of this. I'll merge it now

@0xc0170 0xc0170 merged commit 16bf07e into ARMmbed:master Mar 29, 2017

6 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-export-build Job has completed
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/mbedci Merged build finished. No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment