Navigation Menu

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

CMake: Fix mbed-802.15.4-rf driver #14546

Merged

Conversation

rwalton-arm
Copy link
Contributor

@rwalton-arm rwalton-arm commented Apr 14, 2021

Summary of changes

mbed-nanostack should depend on mbed-802.15.4-rf to avoid a linker error
with an undefined reference to NanostackRfPhy::get_default_instance().
The error occurs when device_has: 802_15_4_PHY is defined and the
consumer depends on mbed-nanostack in their CMakeLists.txt. Previously
we linked mbed-nanostack to mbed-802.15.4-rf, so mbed-802.15.4-rf's
usage requirements weren't forwarded to consumers who depended on
mbed-nanostack.

With the previous configuration, the consumer would have to depend on
mbed-802.15.4-rf directly to avoid an issue. This seems like a layering
violation: it appears that mbed-nanostack is "the API" and
mbed-802.15.4-rf is one of several possible implementations which are
selected based on configuration macros.

This commit changes the flow of dependencies so that mbed-nanostack ends
up with the correct symbol definitions.

Impact of changes

Migration actions required

Documentation

None


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 ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Apr 14, 2021
@ciarmcom ciarmcom requested a review from a team April 14, 2021 11:30
@ciarmcom
Copy link
Member

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

mbed-nanostack should depend on mbed-802.15.4-rf to avoid a linker error
with an undefined reference to `NanostackRfPhy::get_default_instance()`.
The error occurs when `device_has: 802_15_4_PHY` is defined and the
consumer depends on mbed-nanostack in their CMakeLists.txt. Previously
we linked mbed-nanostack to mbed-802.15.4-rf, so mbed-802.15.4-rf's
usage requirements weren't forwarded to consumers who depended on
mbed-nanostack.

With the previous configuration, the consumer would have to depend on
mbed-802.15.4-rf directly to avoid an issue. This seems like a layering
violation: it appears that mbed-nanostack is "the API" and
mbed-802.15.4-rf is one of several possible implementations which are
selected based on configuration macros.

This commit changes the flow of dependencies so that mbed-nanostack ends
up with the correct symbol definitions.
@rwalton-arm rwalton-arm force-pushed the dev/rwalton-arm/fix-connectivity-driver branch from c4f9897 to c7c2580 Compare April 14, 2021 11:32
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 14, 2021

We should link a driver to nanostack but aren't we still missing something here? As it is, atmel is a default driver always present. If a user uses another there would be duplicated drivers in the tree (I checked, all folders under this directory define the same functionality). I assume atmel should be conditional as well - depends on 802_15_5_RF label set?

I dont know how other drivers (mcr for instance) were selected with cli1 as they are not "components" neither using any label in directory name ? I might be missing something.

@rwalton-arm
Copy link
Contributor Author

rwalton-arm commented Apr 14, 2021

We should link a driver to nanostack but aren't we still missing something here? As it is, atmel is a default driver always present. If a user uses another there would be duplicated drivers in the tree (I checked, all folders under this directory define the same functionality). I assume atmel should be conditional as well - depends on 802_15_5_RF label set?

I dont know how other drivers (mcr for instance) were selected with cli1 as they are not "components" neither using any label in directory name ? I might be missing something.

device_has: 802_15_4_PHY also becomes the macro DEVICE_802_15_4_PHY. Some of these "labels" are also used as macros within mbed-os. In MeshInterfaceNanostack.cpp and LoWPANNDInterface.cpp we protect the implementations of get_default_instance and get_target_default_instance with an ifdef DEVICE_802_15_4_PHY. So it's not being used as a directory label in this case, as far as I can tell.

@mergify mergify bot added needs: CI and removed needs: review labels Apr 14, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 14, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 14, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cmake-example-test

@mergify mergify bot added needs: work and removed needs: CI labels Apr 14, 2021
@Patater
Copy link
Contributor

Patater commented Apr 15, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 15, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_cmake-example-test ✔️

@0xc0170 0xc0170 merged commit 076cf35 into ARMmbed:master Apr 15, 2021
@mergify mergify bot removed the ready for merge label Apr 15, 2021
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants