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

examples/gnrc_minimal: minimalize gnrc_minimal example #12561

Merged
merged 3 commits into from
Oct 25, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 24, 2019

Contribution description

Inspired by 9103e5b I looked and checked if the gnrc_minimal example also has LOG_LEVEL=LOG_NONE set. And in fact it hasn't. So I activated it. While I was minimizing around, I noticed that DEVELHELP was activated for this minimal example. This change goes way back to a change by @kYc0o in abf6c3e (#6406)... Apparently, I didn't look into the details of this example for a while now... IMHO the logic for this should be the other way around, due to the minimalized nature of that example: DEVELHELP=0, but the comment should provide help what to do to gain its advantages.

Testing procedure

Compile examples/gnrc_minimal. It should now be significantly smaller than without these changes.

Issues/PRs references

None

@miri64 miri64 added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up 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: examples Area: Example Applications labels Oct 24, 2019
@miri64 miri64 added this to the Release 2020.01 milestone Oct 24, 2019
examples/gnrc_minimal/Makefile Show resolved Hide resolved
@miri64
Copy link
Member Author

miri64 commented Oct 24, 2019

I hope the comment makes sense now ^^"

@miri64 miri64 force-pushed the examples/enh/minimalize-gnrc_minimal branch from 8e24db6 to d4dd89b Compare October 24, 2019 19:18
@benpicco
Copy link
Contributor

I agree with the changes in this PR, but I just tried gnrc_minimal on the microduino-corerf (ATmega128RFA1), the other node is running gnrc_networking.
I did ping6 ff02::1:

  • stock configuration: no response.
  • changed USEMODULE += gnrc_ipv6 to USEMODULE += gnrc_ipv6_router_default: no response
  • removed CFLAGS += -DGNRC_PKTBUF_SIZE=512 -DGNRC_NETIF…: no response
  • changed USEMODULE += gnrc_ipv6 to USEMODULE += gnrc_ipv6_router_default and removed CFLAGS += -DGNRC_PKTBUF_SIZE=512 -DGNRC_NETIF…:
2019-10-25 00:46:17,715 # 12 bytes from fe80::863d:27a0:3c22:843e: icmp_seq=0 ttl=64 rssi=-71 dBm time=8.095 ms
2019-10-25 00:46:18,715 # 12 bytes from fe80::863d:27a0:3c22:843e: icmp_seq=1 ttl=64 rssi=-70 dBm time=8.095 ms
2019-10-25 00:46:19,714 # 12 bytes from fe80::863d:27a0:3c22:843e: icmp_seq=2 ttl=64 rssi=-70 dBm time=7.776 ms
2019-10-25 00:46:19,715 # 
2019-10-25 00:46:19,717 # --- ff02::1 PING statistics ---
2019-10-25 00:46:19,722 # 3 packets transmitted, 3 packets received, 0% packet loss
2019-10-25 00:46:19,726 # round-trip min/avg/max = 7.776/7.988/8.095 ms

@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

So none of that has to do with the proposed changes, but the application is broken in itself?

@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

What was your network interface?

@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

Ah... 6LoWPAN IPHC and fragmentation are not compiled in (on purpose). That is why you can't ping the gnrc_minimal application. Try ifconfig <if> -iphc on the gnrc_networking node.

@benpicco
Copy link
Contributor

Ah that is it!
After ifconfig <if> -iphc the ping works.

So while you are minimizing this example, please maximize the documentation so it doesn't seem to be broken for the unsuspecting user.

@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

@benpicco
Copy link
Contributor

Oh it's already right there! 😅
Sorry for the noise.

My only complaints unrelated to this PR are

  • it's not intuitive that header compression is related to the gnrc_ipv6_router_default module
  • could the nib contain a flag whether a neighbour supports header compression or not?

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Disabling debug options makes sense for a example in minimalism.

@bergzand bergzand dismissed their stale review October 25, 2019 12:07

review issues are resolved

@benpicco
Copy link
Contributor

Please squash!

@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

My only complaints unrelated to this PR are

  • it's not intuitive that header compression is related to the gnrc_ipv6_router_default module

Indeed it isn't and it is only incidental. gnrc_ipv6_router_default (or its small, non-forwarding sibling gnrc_ipv6_default) pull in all dependencies to have a fully working IPv6 protocol (up to the respective RFCs). For radios that need it, this also pulls in gnrc_sixlowpan_router_default (or gnrc_sixlowpan_default respectively), which in turn does the same for 6LoWPAN (so also pulls in IPHC if the IPv6-over-X RFC demands it). Doc on that exists in the GNRC documentation.

  • could the nib contain a flag whether a neighbour supports header compression or not?

It could, but how would that be propagated is not specified (I think it is just assumed IPHC is in place for every IPv6-over-IEEE802.15.4 capable node). The 6CO option (not supported by RIOT yet) only indicates if GHC is supported so far.

Having `DEVELHELP` activated for a *minimal* example seems
counter-productive.
@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

Squashed

@miri64 miri64 force-pushed the examples/enh/minimalize-gnrc_minimal branch from d4dd89b to 7859225 Compare October 25, 2019 14:31
@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

  • could the nib contain a flag whether a neighbour supports header compression or not?

It could, but how would that be propagated is not specified (I think it is just assumed IPHC is in place for every IPv6-over-IEEE802.15.4 capable node). The 6CO option (not supported by RIOT yet) only indicates if GHC is supported so far.

PS: this also would add complexity to the node which we are trying to avoid ;-).

@miri64 miri64 merged commit c1ea3c2 into RIOT-OS:master Oct 25, 2019
@miri64 miri64 deleted the examples/enh/minimalize-gnrc_minimal branch October 25, 2019 15:33
@miri64
Copy link
Member Author

miri64 commented Oct 25, 2019

Let's see how it fairs during the nightlies :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: examples Area: Example Applications 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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants