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

Move target implementations of NanostackRfPhy into nanostack-interface #13119

Merged
merged 2 commits into from
Jun 18, 2020

Conversation

LDong-Arm
Copy link
Contributor

@LDong-Arm LDong-Arm commented Jun 15, 2020

Summary of changes

Fixes: #13109

Prior to this commit, target implementations of NanostackRfPhy are not guarded by any mbed_lib.json - they are always visible to the build even if the interface itself is not enabled
(e.g. when using the "requires" attribute of mbed_app.json). It causes build errors.

To resolve this, this commit move target code into nanostack-interface, similar to what we do with BLE, mbedtls, etc. where the target code sits alongside the APIs.

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

@MarceloSalazar @ARMmbed/mbed-os-core


…rface library

Fixes: ARMmbed#13109

Prior to this commit, target implementations of NanostackRfPhy
are not guarded by any mbed_lib.json - they are always visible
to the build even if the interface itself is not enabled
(e.g. when using the "requires" attribute of mbed_app.json).
It causes build errors.

To resolve this, this commit move target code into
nanostack-interface, similar to what we do with BLE targets.
@mergify mergify bot added the needs: work label Jun 15, 2020
@LDong-Arm
Copy link
Contributor Author

My only concern is, this PR moves the target code into the interface library. Though as said in the PR description, this is what we already do in BLE...

Alternatively, if we really want to separate target and interface directories, we would need a new mbed_lib.json with "name": "nanostack-targets" which could be an overhead if we need to update any existing examples to match that.

@ciarmcom
Copy link
Member

@LDong-Arm, thank you for your changes.
@MarceloSalazar @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@LDong-Arm
Copy link
Contributor Author

@LDong-Arm, thank you for your changes.
@MarceloSalazar @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

I specified mbed-os-core in the description, but the script added mbed-os-ipcore instead? 🤨

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 16, 2020

It uses the predefined configuration so thus ipcore, but also core should be, will check

cc @adbridge

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 16, 2020

Please fix astyle failures

@adbridge
Copy link
Contributor

We need to look at these once we do the review of the reviewers functionality. There is a Jira ticket for this in our backlog.

0xc0170
0xc0170 previously approved these changes Jun 16, 2020
@MarceloSalazar
Copy link

@LDong-Arm thanks for the PR. I think this is good enough for now.
Ideally, we should re-structure the code and put it under a mbed_lib.json, so the Nanostack feature is only included when explicitly added in mbed_app.json. But that's something that should be done in the future.

@mergify mergify bot dismissed 0xc0170’s stale review June 16, 2020 14:30

Pull request has been modified.

@LDong-Arm
Copy link
Contributor Author

Thanks for the reviews.
@0xc0170 I copied and applied the astyle suggestion from the CI run.
@MarceloSalazar Okay, I'm keeping the current solution then.

Copy link
Contributor

@rajkan01 rajkan01 left a comment

Choose a reason for hiding this comment

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

LGTM

@LDong-Arm LDong-Arm requested a review from 0xc0170 June 17, 2020 14:04
Copy link

@MarceloSalazar MarceloSalazar left a comment

Choose a reason for hiding this comment

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

LGTM

@evedon
Copy link
Contributor

evedon commented Jun 17, 2020

@adbridge ready for CI

@adbridge
Copy link
Contributor

CI started

@mbed-ci
Copy link

mbed-ci commented Jun 17, 2020

Test run: SUCCESS

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

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 17, 2020

I accidentally started a new CI job 🤦

@mbed-ci
Copy link

mbed-ci commented Jun 17, 2020

Test run: SUCCESS

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

@0xc0170 0xc0170 added ready for merge release-type: patch Indentifies a PR as containing just a patch and removed needs: CI labels Jun 18, 2020
@0xc0170 0xc0170 merged commit 0c77249 into ARMmbed:master Jun 18, 2020
@mergify mergify bot removed the ready for merge label Jun 18, 2020
@adbridge adbridge added release-version: 6.1.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jun 24, 2020
@LDong-Arm
Copy link
Contributor Author

Ideally, we should re-structure the code and put it under a mbed_lib.json, so the Nanostack feature is only included when explicitly added in mbed_app.json. But that's something that should be done in the future.

It looks like we need to do this - in the new directory structure we will move target drivers out from core connectivity libraries.

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.

Blinky baremetal fails to compile due to config problems with Nanostack
8 participants