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

shell_commands: gnrc_netif: only include LoRA options when LoRA PHY is present #12295

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

benpicco
Copy link
Contributor

Contribution description

If no LoRA module is used, there is no use in compiling in all the config options for LoRA PHYs.

This saves about 1k of .text.

It also slims down the switch case in _print_netopt.
Right now, adding only one more entry there will cause an error on AVR:

RIOT/sys/shell/commands/sc_gnrc_netif.c:279:(.text._print_netopt+0x32): relocation truncated to fit: R_AVR_7_PCREL against `no symbol'

This is because there are too many cases and local jumps on that platform can only be so far.

Testing procedure

Compile e.g. examples/gnrc_networking for samr21-xpro

Before:

before:
   text    data     bss     dec     hex filename
  91512     196   18724  110432   1af60 /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

After:

after:
   text    data     bss     dec     hex filename
  90440     196   18724  109360   1ab30 /home/benpicco/dev/RIOT/examples/gnrc_networking/bin/samr21-xpro/gnrc_networking.elf

Examples using the sx127x module should continue to work.

Issues/PRs references

this adapts the idea from #12171 to existing code.

In that PR, drivers will select a pseudomodule to indicate which PHY options they support

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 24, 2019
@benpicco benpicco requested review from aabadie and maribu and removed request for aabadie September 24, 2019 14:29
@miri64 miri64 requested a review from jia200x September 24, 2019 14:46
miri64
miri64 previously requested changes Sep 24, 2019
@miri64 miri64 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 Sep 24, 2019
@miri64
Copy link
Member

miri64 commented Sep 24, 2019

Re-queued build, as the PR is still under discussion.

@miri64 miri64 self-requested a review September 24, 2019 15:11
@miri64
Copy link
Member

miri64 commented Sep 24, 2019

LGTM now. Let's see, what the LoRA crowd says to this.

@miri64 miri64 dismissed their stale review September 24, 2019 15:12

Mh... thought re-requesting my review would work. Oh well, then I dismiss it formally

@miri64
Copy link
Member

miri64 commented Sep 24, 2019

(IMHO this PR can be squashed)

…s present

If no LoRA module is used, there is no use in compiling in all the config options
for LoRA PHYs.

This saves about 1k of .text
@jia200x
Copy link
Member

jia200x commented Sep 24, 2019

LGTM. I will test it now on real hardware

@jia200x
Copy link
Member

jia200x commented Sep 24, 2019

Tested on hardware (samr21-xpro and b-l072z-lrwan1). LoRa options are shown only in the b-l072z-lrwan1 (equipped with a LoRa radio). So it works

Copy link
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

ACK

@jia200x
Copy link
Member

jia200x commented Sep 24, 2019

...& GO

@jia200x jia200x merged commit dcbd12f into RIOT-OS:master Sep 24, 2019
@benpicco
Copy link
Contributor Author

Thank you both for the fast review & test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants