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

gnrc_sixlowpan: enable NHC by default #5044

Merged
merged 1 commit into from
Apr 19, 2016

Conversation

cgundogan
Copy link
Member

@cgundogan cgundogan commented Mar 11, 2016

Now that #4935 is merged, we can enable NHC again by default.

edit: Depends on: #4544, #5029, #5049

@cgundogan cgundogan added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer GNRC labels Mar 11, 2016
@cgundogan
Copy link
Member Author

@gebart or should we still wait because of the other issue?

@miri64
Copy link
Member

miri64 commented Mar 11, 2016

I would wait for #4544 and #5029 before we do that.

@jnohlgard
Copy link
Member

The problem I am seeing is illustrated in this packet dump from a router running on top of #4544 + #4935 + #5029
https://www.dropbox.com/s/pgzjf0vq0fuyaku/lowpan.pcap?dl=0

(cloudshark just responds with "Code 0" when I try to upload now so you'll have to download it and open in wireshark)

The setup is:
CoAP server -> wireless -> RPL router -> wireless -> RPL border router -> ethos -> PC

The fragmented packet at number 141+142 is correct. The RPL router then tries to retransmit the packet but forgets the actual payload so it just sends an empty UDP packet to the border router

LL addrs:
CoAP: 0b:92:56:92:ff:79:f0:9b
middle: 12:16:4e:54:8a:f1:40:12
BR: 12:16:4e:54:8b:ab:40:12

Both BR and middle router are running on the same base RIOT checkout with the above PRs applied. BR is running https://github.com/gebart/RIOT-applications/tree/wip/6lbr/6lbr and middle is running microcoap_server customized to set the channel, page and NID for the transceiver and start RPL automatically after boot..
CoAP is running Contiki er-rest-example if it matters..

@jnohlgard
Copy link
Member

The setup is kinda convoluted so I didn't know how to present a clear and concise issue report, which is why I didn't post anything before.

@jnohlgard
Copy link
Member

Further down in the same packet dump is the setup:

CoAP -> wireless -> RPL border router -> ethos -> PC

Same firmware, only restarted the nodes to remap the RPL DODAG so that CoAP has a direct link to the BR.

See packet 244+245. It comes out correct on the PC side and Firefox Copper shows the correct data.

@jnohlgard jnohlgard added this to the Release 2016.04 milestone Mar 11, 2016
@cgundogan
Copy link
Member Author

does it break all the time (reproduceable) or just randomly?
(in your first setup)

@jnohlgard
Copy link
Member

Consistently the same results in my first setup.

If you look at the dump I do get some correct fragmented packets through, but that is a large payload ping packet (ICMPv6), so the problem lies somewhere in the UDP or IPHC UDP NHC codepaths

@cgundogan
Copy link
Member Author

so the problem lies somewhere in the UDP or IPHC UDP NHC codepaths

just to be sure, did you also test the same setup without enabiling udp NHC?

@jnohlgard
Copy link
Member

The first setup is what caused the hard fault described in #5029 in the first place. The BR crashed because the received payload was missing. It did not crash in setup number 2.

@jnohlgard
Copy link
Member

@cgundogan didn't test without NHC because Contiki doesn't have a configuration switch for it. I could re-flash the CoAP node with the microcoap_server example and retry later.

@cgundogan
Copy link
Member Author

would be nice, but it's most probably the NHC that breaks here.

@jnohlgard
Copy link
Member

I think I found a problem. The payload is never marked from the rest of the IPv6 packet when doing routing:
The reassembly buffer builds an IPv6 packet (not 6lowpan) and passes it on to the IPv6 thread _receive. The packet has the gnrc snip structure: IPv6->netif->NULL.
IPv6 _receive then parses the IPv6 header and finds out that the destination is another host and strips the netif header. The packet now has the structure IPv6->NULL (a single monolithic snip).
The packet is passed to the IPv6 _send function.
The _send function expects the payload to be found at pkt->next, which in this case is NULL.

@jnohlgard
Copy link
Member

The issue I am seeing is the same one as in #4544, except the pkt->type is set to SIXLOWPAN instead of IPV6, which is why #4544 does not fix it in my case. Working on a fix

@miri64
Copy link
Member

miri64 commented Mar 12, 2016

That is weird. IPv6 should mark the header in https://github.com/RIOT-OS/RIOT/blob/master/sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c#L756 so why doesn't it?

@jnohlgard
Copy link
Member

@authmillenon Traced a bit more and it shows that IPV6 is marking its header but the remaining payload is still marked 6lowpan

@jnohlgard
Copy link
Member

I think the best solution would be to mark the UDP header properly

@miri64
Copy link
Member

miri64 commented Mar 12, 2016

Arrggs, no I would vote for setting it to UNDEF as GNRC expects.

@miri64
Copy link
Member

miri64 commented Mar 12, 2016

#5049

@jnohlgard jnohlgard added the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 12, 2016
@jnohlgard
Copy link
Member

I'm using a combination of:
#5044 (this PR) + #5029 + #5049 + #4544
and everything seems to work now with fragmented CoAP packets over a two hop network with an ethos BR, the same node configuration as the first setup described in my comment above #5044 (comment)

I published my temporary test branch at https://github.com/gebart/RIOT/tree/wip/routing-fixes in case someone wants to try it.

@miri64
Copy link
Member

miri64 commented Mar 12, 2016

Will try after the weekend, but anybody else is welcome to try before me :-)

@jnohlgard jnohlgard removed the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label Mar 12, 2016
@miri64
Copy link
Member

miri64 commented Mar 16, 2016

Now only #4544 is missing to merge this PR. @gebart can you test that PR again, please?

@miri64 miri64 modified the milestones: Release 2016.07, Release 2016.04 Mar 31, 2016
@miri64
Copy link
Member

miri64 commented Mar 31, 2016

Since there is nothing going on in #4544 I say we postpone this one for next time.

@OlegHahm
Copy link
Member

OlegHahm commented Apr 1, 2016

But we still have two weeks to fix that!?

@miri64
Copy link
Member

miri64 commented Apr 19, 2016

#5232 is working for the 6LBR so I would like to take this into the release again.

@miri64 miri64 modified the milestones: Release 2016.04, Release 2016.07 Apr 19, 2016
@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 State: waiting for other PR State: The PR requires another PR to be merged first labels Apr 19, 2016
@miri64
Copy link
Member

miri64 commented Apr 19, 2016

Doesn't need to be rebased to master to compile, but needs #5232 to work. So ACK & go if Murdock is happy and #5232 was merged.

@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 19, 2016
@miri64
Copy link
Member

miri64 commented Apr 19, 2016

#5232 was merged, murdock is happy. So go.

@miri64 miri64 merged commit 7f2f833 into RIOT-OS:master Apr 19, 2016
@cgundogan cgundogan deleted the pr/gnrc/enable_iphc_nhc branch April 19, 2016 12:42
@miri64 miri64 removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Apr 19, 2016
@kaspar030
Copy link
Contributor

This seems to have broken master.

@kYc0o
Copy link
Contributor

kYc0o commented Apr 20, 2016

Nice catch @kaspar030!

@miri64
Copy link
Member

miri64 commented Apr 20, 2016

Fix provided in #5360.

@cgundogan cgundogan mentioned this pull request Apr 21, 2016
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.

None yet

6 participants