Skip to content

Refactoring \lorawan --> moving it inside \connectivity. #13410

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

Merged
merged 7 commits into from
Aug 14, 2020

Conversation

ashok-rao
Copy link
Contributor

@ashok-rao ashok-rao commented Aug 10, 2020

Summary of changes

Mbed OS will soon be changing directory structure to the below:

connectivity
├── netsocket
├── lorawan
│   ├── mbed_lib.json
│   ├── lorastack
│   ├── source
│   ├── include
│   ├── tests
├── nanostack
├── cellular
├── ...

This PR is a part of a wider \connectivity refactoring and implements the above new directory structure for \lorawan.

Todo:

- Move \components\lora ---> \connectivity\lorawan\drivers
Done as part of commit 7e2d1ef8

Impact of changes

-None-

Migration actions required

-None-

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

Build successes:

  • K64F::GCC_ARM::MBED-BUILD
  • K64F::GCC_ARM::MBED-OS-CONNECTIVITY-LORAWAN-TESTS-TESTS-LORAWAN-LORARADIO

Reviewers


@LDong-Arm @gpsimenos @rajkan01

@mergify mergify bot added the do not merge label Aug 10, 2020
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Aug 10, 2020
@ciarmcom
Copy link
Member

@ashok-rao, thank you for your changes.
@rajkan01 @gpsimenos @LDong-Arm @ARMmbed/mbed-os-maintainers please review.

@LDong-Arm
Copy link
Contributor

Also, lorawan_types.h could be moved into include/lorawan instead of just include

@ashok-rao ashok-rao changed the title WIP: Refactoring \lorawan --> moving it inside \connectivity. Refactoring \lorawan --> moving it inside \connectivity. Aug 11, 2020
@LDong-Arm
Copy link
Contributor

Here comes a question:
The lorawan drivers in mbed-os repo are mirrored from the standalone repo https://github.com/ARMmbed/mbed-semtech-lora-rf-drivers. Our example mbed-os-example-lorawan uses drivers from the standalone repo instead of mbed-os. Probably we should align this - either remove the drivers from mbed-os, or update the examples to use those from mbed-os? @kivaisan @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-connectivity?

Anyway, it's beyond the scope of this PR which looks good to me apart from my inline remark. @ashok-rao

@ashok-rao
Copy link
Contributor Author

Reveiw comments addressed, thanks @LDong-Arm 👍

Copy link
Contributor

@LDong-Arm LDong-Arm left a comment

Choose a reason for hiding this comment

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

LGTM

@kivaisan
Copy link
Contributor

kivaisan commented Aug 12, 2020

Here comes a question:
The lorawan drivers in mbed-os repo are mirrored from the standalone repo https://github.com/ARMmbed/mbed-semtech-lora-rf-drivers. Our example mbed-os-example-lorawan uses drivers from the standalone repo instead of mbed-os. Probably we should align this - either remove the drivers from mbed-os, or update the examples to use those from mbed-os? @kivaisan @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-connectivity?

The problem is with mbed-os 5.15 -branch. As it doesn't have drivers yet, so if someone wants to use lora example with that, standalone repository must be used for drivers. At the time drivers were requested to be included in mbed-os, 6.0 was not out yet.

So as example now used mbed-os 6, it should be now modified to use drivers from mbed-os instead of standalone repository.

@kivaisan
Copy link
Contributor

Here comes a question:
The lorawan drivers in mbed-os repo are mirrored from the standalone repo https://github.com/ARMmbed/mbed-semtech-lora-rf-drivers. Our example mbed-os-example-lorawan uses drivers from the standalone repo instead of mbed-os. Probably we should align this - either remove the drivers from mbed-os, or update the examples to use those from mbed-os? @kivaisan @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-connectivity?

The problem is with mbed-os 5.15 -branch. As it doesn't have drivers yet, so if someone wants to use lora example with that, standalone repository must be used for drivers. At the time drivers were requested to be included in mbed-os, 6.0 was not out yet.

So as example now used mbed-os 6, it should be now modified to use drivers from mbed-os instead of standalone repository.

I modified example to use drivers from mbed-os instead of separate repository: ARMmbed/mbed-os-example-lorawan@38536b2

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 12, 2020

Thanks @kivaisan , this means this is unblocked now (the example will work with 6.x and 5.15.x)

0xc0170
0xc0170 previously approved these changes Aug 12, 2020
@mergify
Copy link

mergify bot commented Aug 12, 2020

This PR cannot be merged due to conflicts. Please rebase to resolve them.

@mergify mergify bot removed the needs: CI label Aug 12, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Aug 12, 2020

One of the latest PRs (mbed tls restructure was it?) made a conflict in unittests, please resolve

Ashok Rao added 4 commits August 12, 2020 11:06
```
connectivity
├── netsocket
├── lorawan
│   ├── mbed_lib.json // nanostack-interface's mbed_lib.json
│   ├── lorastack
│   ├── tests
├── nanostack
├── cellular
├── ...

```

This PR is a part of a wider \connectivity refactoring and implements the above new directory structure for \lorawan.
Ashok Rao added 2 commits August 12, 2020 11:18
    1. Moving lora drivers from \components\lora to \connectivity\drivers\lora
    2. Incorporating review comments.
@ashok-rao
Copy link
Contributor Author

Rebased and force pushed..

@mergify mergify bot dismissed 0xc0170’s stale review August 12, 2020 10:20

Pull request has been modified.

Copy link
Contributor

@LDong-Arm LDong-Arm left a comment

Choose a reason for hiding this comment

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

Still LGTM after the rebase

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 13, 2020

CI restarted

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 13, 2020

Unittest failed right away

@mbed-ci
Copy link

mbed-ci commented Aug 13, 2020

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_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️

@rajkan01
Copy link
Contributor

@0xc0170 I fixed the UNITTESTS failure, could you re-start the CI

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 14, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Aug 14, 2020

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_build-ARM ✔️
jenkins-ci/mbed-os-ci_build-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️
jenkins-ci/mbed-os-ci_dynamic-memory-usage ✔️
jenkins-ci/mbed-os-ci_cloud-client-pytest ✔️

@rajkan01
Copy link
Contributor

@0xc0170 CI passed for this PR, please merge

@0xc0170 0xc0170 merged commit 327fe53 into ARMmbed:master Aug 14, 2020
@mergify mergify bot removed the ready for merge label Aug 14, 2020
@mbedmain mbedmain added release-version: 6.2.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 16, 2020
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.

8 participants