gnrc_sixlowpan_iphc: refactor sending for #8511 #9485

miri64 · 2018-07-03T14:21:34Z

Contribution description

This refactors sending/encoding part of gnrc_sixlowpan_iphc to the
more layered approach modeled in #8511. Since the reception part is
already was pretty complicated to refactor, I decided to divide send
and receive up into separate changes.

Testing

To test I used the ping6 and udp {send|server} server command tests/gnrc_udp with different packet sizes, and also with IPHC (de-)activated with ifconfig 6 -iphc on samr21-xpro. I also tested several compile configurations with the following patch (and commenting out the corresponding iphc/frag lines):

diff --git a/tests/gnrc_udp/Makefile b/tests/gnrc_udp/Makefile
index 9496288..9b594ec 100644
--- a/tests/gnrc_udp/Makefile
+++ b/tests/gnrc_udp/Makefile
@@ -8,7 +8,10 @@ BOARD_INSUFFICIENT_MEMORY := calliope-mini chronos hifive1 microbit msb-430 msb-
 
 USEMODULE += gnrc_netdev_default
 USEMODULE += auto_init_gnrc_netif
-USEMODULE += gnrc_ipv6_router_default
+USEMODULE += gnrc_ipv6_nib_6lr
+USEMODULE += gnrc_sixlowpan_router
+USEMODULE += gnrc_sixlowpan_frag
+USEMODULE += gnrc_sixlowpan_iphc
 USEMODULE += gnrc_udp
 USEMODULE += gnrc_rpl
 USEMODULE += auto_init_gnrc_rpl

(just resolves the dependency)

Issues/PRs references

Related to but independent of #9484.

Adapts IPHC reception for #8511.

miri64 · 2018-07-03T14:23:23Z

Also ping @A-Paul. I think this could be interesting regarding GHC.

cgundogan · 2018-07-25T06:33:59Z

sys/include/net/gnrc/sixlowpan/internal.h

+ */
+void gnrc_sixlowpan_multiplex_by_size(gnrc_pktsnip_t *pkt,
+                                      size_t orig_datagram_size,
+                                      gnrc_netif_t *netif,


const gnrc_netif_t *netif,

Mh... while in the current version of the code netif isn't changed, I can see a future version where netif is provided as to a _send() function which then again would require casting.

cgundogan · 2018-07-25T06:35:42Z

sys/include/net/gnrc/sixlowpan/iphc.h

 */
-bool gnrc_sixlowpan_iphc_encode(gnrc_pktsnip_t *pkt);
+void gnrc_sixlowpan_iphc_send(gnrc_pktsnip_t *pkt, void *ctx, unsigned page);


const void *ctx,

This is not what is proposed in #8511 and we also shouldn't restrict the use of ctx within the execution path (examples were ctx is changed exist enough throughout the current code-base).

cgundogan · 2018-07-25T09:11:22Z

hm, there's a weird behavior, using gnrc_networking example on the IoT-Lab testbed.
With this PR and setting -iphc via ifconfig, I hit an assert with the sending (pinging) node.
When iphc is active, it works flawlessly.
~~On master, I do not hit an assert, but the pings doesn't seem to come through with -iphc. With iphc it works flawlessly.~~

miri64 · 2018-07-25T09:21:34Z

@cgundogan can you tell me which assertion is hit, please?

cgundogan · 2018-07-25T09:25:36Z

strike that. I don't receive assertions anymore. I rebased this PR to current master to include the page change. However, with -iphc, pings don't go through. ~~Apparently, not even unfragmented packets~~

cgundogan · 2018-07-25T09:25:59Z

On master, I do not hit an assert, but the pings doesn't seem to come through with -iphc. With iphc it works flawlessly.

strike that also

cgundogan · 2018-07-25T09:29:24Z

With this PR:

1532510918.205561;m3-101;> ifconfig 7 -iphc
1532510918.206607;m3-100;> ifconfig 7 -iphc
1532510918.206866;m3-100;success: unset option
1532510918.207072;m3-101;success: unset option
m3-100;ping6 10 fe80::1711:6b10:65f6:5d02 100 100
1532510922.351252;m3-100;> ping6 10 fe80::1711:6b10:65f6:5d02 100 100
1532510923.357817;m3-100;ping timeout
1532510924.465432;m3-100;ping timeout
1532510925.573245;m3-100;ping timeout
1532510926.679893;m3-100;ping timeout
1532510927.789228;m3-100;ping timeout
1532510928.896213;m3-100;ping timeout
1532510930.004624;m3-100;ping timeout
1532510931.111466;m3-100;ping timeout
1532510932.219792;m3-100;ping timeout
1532510933.327352;m3-100;ping timeout
1532510933.329015;m3-100;--- fe80::1711:6b10:65f6:5d02 ping statistics ---
1532510933.329206;m3-100;10 packets transmitted, 0 received, 100% packet loss
ifconfig 7 iphc
1532510935.939613;m3-100;> ifconfig 7 iphc
1532510935.940000;m3-101;> ifconfig 7 iphc
1532510935.940167;m3-101;success: set option
1532510935.940426;m3-100;success: set option
m3-100;ping6 10 fe80::1711:6b10:65f6:5d02 100 100
1532510937.815347;m3-100;> ping6 10 fe80::1711:6b10:65f6:5d02 100 100
1532510937.835688;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=1 hop limit=64 time = 18.777 ms
1532510937.958329;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=2 hop limit=64 time = 20.057 ms
1532510938.078449;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=3 hop limit=64 time = 19.738 ms
1532510938.199434;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=4 hop limit=64 time = 18.151 ms
1532510938.320426;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=5 hop limit=64 time = 19.117 ms
1532510938.441446;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=6 hop limit=64 time = 19.421 ms
1532510938.561350;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=7 hop limit=64 time = 18.153 ms
1532510938.680365;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=8 hop limit=64 time = 17.176 ms
1532510938.804361;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=9 hop limit=64 time = 21.969 ms
1532510938.928356;m3-100;108 bytes from fe80::1711:6b10:65f6:5d02: id=94 seq=10 hop limit=64 time = 21.347 ms
1532510938.929147;m3-100;--- fe80::1711:6b10:65f6:5d02 ping statistics ---
1532510938.930246;m3-100;10 packets transmitted, 10 received, 0% packet loss, time 1.06113477 s
1532510938.931237;m3-100;rtt min/avg/max = 17.176/19.390/21.969 ms

miri64 · 2018-07-25T09:31:19Z

@cgundogan Ok... I'm not in a condition to debug at the moment though :-/

miri64 · 2018-07-25T09:35:44Z

@cgundogan can you maybe check if something went wrong in https://github.com/RIOT-OS/RIOT/pull/9485/files#diff-e87e12a912a7eb2cbb8a16f4ebeccabaR315 or if it even is executed, please.

cgundogan · 2018-07-25T09:59:43Z

I switched to a local setup to debug, using a samr instead of the iotlabs. I again get an assertion hit here on the sending node. datagram_size is 59 and orig_datagram_size is 58

This is what I used:

ifconfig 7 -iphc
ping6 1 fe80::7b76:645c:f47e:8e1b 10 100

@cgundogan can you maybe check if something went wrong in https://github.com/RIOT-OS/RIOT/pull/9485/files#diff-e87e12a912a7eb2cbb8a16f4ebeccabaR315 or if it even is executed, please.

it's executed and the if body is skipped to go directly to here

miri64 · 2018-07-25T10:10:03Z

@cgundogan can you see if the last commit fixes the issue (and if the sniffer still is able to recognize it). Not sure anymore if the 6Lo uncompressed dispatch is counted to the original datagram size or not in fragmentation (if not reception shouldn't work anymore now).

miri64 · 2018-07-25T15:25:38Z

@cgundogan I reverted the last commit and removed the bogus assertion instead. Of course the 6Lo-frame can be larger than the original IPv6 datagram, since you saw yourself that with deactivated compression, the uncompressed dispatch is prepended (one additional byte). This should also fix the off-by-one issue you reported to me offline.

cgundogan

Issue is fixed. Please squash. ACK!

This refactors sending/encoding part of `gnrc_sixlowpan_iphc` to the more layered approach modeled in RIOT-OS#8511. Since the reception part is already was pretty complicated to refactor, I decided to divide send and receive up into separate changes.

During the review of RIOT-OS#9485 [we found out][1] that an assertion in this function was invalid. However, the documentation on this assertion wasn't removed on that. This fixes that. [1] RIOT-OS#9485 (comment)

miri64 added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation GNRC CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 3, 2018

miri64 requested a review from cgundogan July 3, 2018 14:21

miri64 mentioned this pull request Jul 3, 2018

gnrc_sixlowpan: Refactoring #8511

Closed

6 tasks

cgundogan requested changes Jul 25, 2018

View reviewed changes

cgundogan approved these changes Jul 25, 2018

View reviewed changes

miri64 force-pushed the gnrc_sixlowpan_iphc/enh/i8511-send branch from 7e3c21f to 80322cb Compare July 25, 2018 15:37

cgundogan merged commit 7fef5e0 into RIOT-OS:master Jul 25, 2018

miri64 deleted the gnrc_sixlowpan_iphc/enh/i8511-send branch July 25, 2018 15:58

This was referenced Jul 25, 2018

gnrc_sixlowpan: remove assertion on multiple_by_size function #9636

Merged

gnrc_sixlowpan_iphc: refactor reception for #8511 #9484

Merged

miri64 mentioned this pull request Aug 3, 2018

gnrc_sixlowpan_iphc: cleanup NHC encoding #8626

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gnrc_sixlowpan_iphc: refactor sending for #8511 #9485

gnrc_sixlowpan_iphc: refactor sending for #8511 #9485

miri64 commented Jul 3, 2018 •

edited

Loading

miri64 commented Jul 3, 2018

cgundogan Jul 25, 2018

miri64 Jul 25, 2018

cgundogan Jul 25, 2018

miri64 Jul 25, 2018

cgundogan commented Jul 25, 2018 •

edited

Loading

miri64 commented Jul 25, 2018

cgundogan commented Jul 25, 2018 •

edited

Loading

cgundogan commented Jul 25, 2018

cgundogan commented Jul 25, 2018 •

edited

Loading

miri64 commented Jul 25, 2018

miri64 commented Jul 25, 2018

cgundogan commented Jul 25, 2018

miri64 commented Jul 25, 2018

miri64 commented Jul 25, 2018

cgundogan left a comment

gnrc_sixlowpan_iphc: refactor sending for #8511 #9485

gnrc_sixlowpan_iphc: refactor sending for #8511 #9485

Conversation

miri64 commented Jul 3, 2018 • edited Loading

Contribution description

Testing

Issues/PRs references

miri64 commented Jul 3, 2018

cgundogan Jul 25, 2018

Choose a reason for hiding this comment

miri64 Jul 25, 2018

Choose a reason for hiding this comment

cgundogan Jul 25, 2018

Choose a reason for hiding this comment

miri64 Jul 25, 2018

Choose a reason for hiding this comment

cgundogan commented Jul 25, 2018 • edited Loading

miri64 commented Jul 25, 2018

cgundogan commented Jul 25, 2018 • edited Loading

cgundogan commented Jul 25, 2018

cgundogan commented Jul 25, 2018 • edited Loading

miri64 commented Jul 25, 2018

miri64 commented Jul 25, 2018

cgundogan commented Jul 25, 2018

miri64 commented Jul 25, 2018

miri64 commented Jul 25, 2018

cgundogan left a comment

Choose a reason for hiding this comment

miri64 commented Jul 3, 2018 •

edited

Loading

cgundogan commented Jul 25, 2018 •

edited

Loading

cgundogan commented Jul 25, 2018 •

edited

Loading

cgundogan commented Jul 25, 2018 •

edited

Loading