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

Jump to bottom

sys/shell: add loramac shell command #8855

Merged
merged 5 commits into from May 29, 2019
Merged

sys/shell: add loramac shell command #8855

merged 5 commits into from May 29, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Mar 30, 2018 •
edited by fjmolinas

Contribution description

This PR moves the loramac shell command implemented in the tests/pkg_semtech-loramac application to a more central place in sys/shell.

If one imports and sets the right variable/CFLAGS, it's possible to also get them with any application importing shell_commands module. For example, with default:

USEPKG=semtech-loramac CFLAGS=-DREGION_EU868 BOARD=b-l072z-lrwan1 make -C examples/default

Note that defaultautomatically imports the sx1276 module when using the B-L072Z-LRWAN1 board (via auto_init). For other application, one would have to also pass USEMODULE=sx1276.

Issues/PRs references

This PR is based on #8639

Now based on #11583

@aabadie aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: LoRa Area: LoRa radio support State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 30, 2018
@aabadie aabadie force-pushed the pr/pkg/semtech-loramac_shell branch from 691fff7 to bbb0116 Compare March 30, 2018 09:27
@miri64 miri64 added State: waiting for other PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 5, 2018
@PeterKietzmann
Copy link
Member

#8639 got merged some time ago so I'm removing the dependency.

@PeterKietzmann PeterKietzmann removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 5, 2019
@fjmolinas
Copy link
Contributor

@aabadie Couldn't you split this in to two PR's? For me the application refactor and to make it thread safe and adding the shell should be in different PR's.

@aabadie aabadie force-pushed the pr/pkg/semtech-loramac_shell branch from bbb0116 to 567eee6 Compare April 16, 2019 11:22
@aabadie
Copy link
Contributor Author

aabadie commented Apr 16, 2019

Couldn't you split this in to two PR's? For me the application refactor and to make it thread safe and adding the shell should be in different PR's.

Well, it's just that this PR hasn't been rebased for a long time. It's done now.

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 16, 2019
fjmolinas
fjmolinas reviewed Apr 19, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment •
edited

Choose a reason for hiding this comment

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

Some minor comments, they where present in the PR before the Rebase, you can address them here or in a separate PR.

Also see travis complaints about blank line.

tests/pkg_semtech-loramac/README.md Outdated Show resolved Hide resolved
tests/pkg_semtech-loramac/README.md Outdated Show resolved Hide resolved
fjmolinas
fjmolinas requested changes Apr 19, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

Some other comments.

Also your latest commit seems like a different subject unrelated to the PR title, please change it to reflect although IMO it should be a separate PR.

sys/auto_init/auto_init.c Outdated
@@ -277,9 +277,14 @@ void auto_init(void)
#endif

#ifdef MODULE_SX127X
#ifdef MODULE_SEMTECH_LORAMAC
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO subordinate auto_init_loramac() to MODULE_SX127X is wrong, you are mixing up the mac layer and the network interface

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why this should be in GRNC_NETIF, it has nothing to do with GNRC.

@aabadie aabadie mentioned this pull request Apr 23, 2019
Merged
@aabadie
Copy link
Contributor Author

aabadie commented Apr 23, 2019

@fjmolinas regarding the application README, I prefer the separate PR: see #11426

fjmolinas
fjmolinas requested changes May 23, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

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

@aabadie I still have a couple of un-addressed comments, also could you split the PR in two the auto_init part and the shell part. Otherwise I tested that it works.

sys/auto_init/auto_init.c Outdated
@@ -277,9 +277,14 @@ void auto_init(void)
#endif

#ifdef MODULE_SX127X
#ifdef MODULE_SEMTECH_LORAMAC
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why this should be in GRNC_NETIF, it has nothing to do with GNRC.

tests/pkg_semtech-loramac/Makefile Outdated Show resolved Hide resolved
sys/auto_init/netif/auto_init_loramac.c Outdated Show resolved Hide resolved
@aabadie aabadie mentioned this pull request May 26, 2019
Merged
@aabadie aabadie force-pushed the pr/pkg/semtech-loramac_shell branch from e82549a to 45bf5cc Compare May 26, 2019 09:50
@aabadie aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label May 26, 2019
@aabadie aabadie force-pushed the pr/pkg/semtech-loramac_shell branch 3 times, most recently from 0d3c6b1 to dc4d0af Compare May 26, 2019 11:59
@fjmolinas fjmolinas removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 28, 2019
@fjmolinas
Copy link
Contributor

Tested and its working except for the fact that shell_commands got removed from examples_default. But that should be addressed on a different PR. Do you want to wait for #11541 before merging this one?

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 28, 2019
@aabadie
Copy link
Contributor Author

aabadie commented May 28, 2019

Do you want to wait for #11541 before merging this one?

nope, this is not supposed to work anymore with exemples/default. This is because auto_init_loramac is not automatically pulled in with auto_init.

@fjmolinas
Copy link
Contributor

nope,

ACK then! :)

@aabadie
Copy link
Contributor Author

aabadie commented May 28, 2019

It's all green @fjmolinas :)

@aabadie aabadie mentioned this pull request May 29, 2019
Merged
@fjmolinas
Copy link
Contributor

@aabadie Need to be rebased since #11541 was merged!

@aabadie aabadie force-pushed the pr/pkg/semtech-loramac_shell branch from dc4d0af to fd9b3c7 Compare May 29, 2019 14:55
@aabadie
Copy link
Contributor Author

aabadie commented May 29, 2019

Rebased (and still works hopefully)

@fjmolinas
Copy link
Contributor

Still works, RE-ACK and GO!

@fjmolinas fjmolinas merged commit d1f06bd into RIOT-OS:master May 29, 2019
@aabadie aabadie deleted the pr/pkg/semtech-loramac_shell branch May 30, 2019 16:40
@MrKevinWeiss MrKevinWeiss added the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

Assignees
No one assigned
Labels
Area: LoRa Area: LoRa radio support CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aabadie @PeterKietzmann @fjmolinas @miri64 @MrKevinWeiss