Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

shell: fix multicast pings #4005

Merged
merged 2 commits into from
Sep 30, 2015

Conversation

OlegHahm
Copy link
Member

This PR introduces two things in order to fix pinging of multicast addresses (or more general: to be able to handle additional ICMPv6 echo replies):

  • adds a message queue to the main thread for the gnrc_networking example
  • checks for pending additional packets delivered to the ping shell handler after unregistering

@OlegHahm OlegHahm added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Sep 30, 2015
@OlegHahm OlegHahm added this to the Release 2015.09 milestone Sep 30, 2015
@miri64
Copy link
Member

miri64 commented Sep 30, 2015

You are aware, that this is not correct, since the packets you receive before are also duplicates

@OlegHahm
Copy link
Member Author

Yes, but there's no way to distinguish without a lot of added memory/complexity.

@OlegHahm
Copy link
Member Author

I'm wondering if we shouldn't make it even mandatory for a thread registering at gnrc to have a message queue. What do you think?

@OlegHahm
Copy link
Member Author

Added the queue also for the border router example, added a comment to explain the necessity, reduced the queue size, and made the error message for superfluous pongs more accurate.

@miri64
Copy link
Member

miri64 commented Sep 30, 2015

I'm wondering if we shouldn't make it even mandatory for a thread registering at gnrc to have a message queue. What do you think?

I remember that I tried to include an assert() for that sometime in the past, but someone (I really can't remember) was against it.

@OlegHahm
Copy link
Member Author

Let's discuss #4010 after the release.

@OlegHahm
Copy link
Member Author

I just figured out that only the combination of this PR and #4006 will fix the issue completely.

@miri64
Copy link
Member

miri64 commented Sep 30, 2015

Kind off was expecting something like that. How big is the packet loss with #4006?

@OlegHahm
Copy link
Member Author

On native more or less 0.

@OlegHahm
Copy link
Member Author

However the packetbuffer is not empty.

@OlegHahm
Copy link
Member Author

The fragments in the packetbuffer look weird to me:

packet buffer: first byte: 0x8088fc0, last byte: 0x808a7c0 (size: 6144)
~ unused: 0x8088fc0 (next: 0x80893f0, size: 1056) ~
================ chunk   0 (size:   16) ================
000000 86 dd 00 00 08 00 00 00 e0 10 4d cf 00 00 00 00
~ unused: 0x80893f0 (next: 0x8089490, size:  152) ~
================ chunk   1 (size:    8) ================
000000 08 00 00 00 08 00 00 00
~ unused: 0x8089490 (next: (nil), size: 4912) ~

@OlegHahm
Copy link
Member Author

However, I'm not able to create a fragmented packet buffer on iotlabs.

@OlegHahm
Copy link
Member Author

I think the changes in this PR are sane anyway. If you don't disagree, how about getting this merged quickly?

@OlegHahm OlegHahm added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 30, 2015
@OlegHahm
Copy link
Member Author

Tests for Specs 02 to 06 succeed with #4006 and this PR.

@miri64
Copy link
Member

miri64 commented Sep 30, 2015

True the packet buffer looks really weird, since it looks like abandoned data... Otherwise, this change is a step in the right direction so ACK (is there a release version for it)?

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 30, 2015
@OlegHahm
Copy link
Member Author

Yes, see #4009.

Since it is likely that the main thread will send netapi IPC calls that expects a reply. These replies may come faster than the thread can handle them, causing the layers below to stuck.
@OlegHahm OlegHahm force-pushed the shell_ping_multicast_fixes branch from ce93858 to b538c74 Compare September 30, 2015 21:21
@OlegHahm OlegHahm removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 30, 2015
@OlegHahm
Copy link
Member Author

squashed

OlegHahm added a commit that referenced this pull request Sep 30, 2015
@OlegHahm OlegHahm merged commit 5376dfd into RIOT-OS:master Sep 30, 2015
@OlegHahm OlegHahm deleted the shell_ping_multicast_fixes branch September 30, 2015 21:55
# for free to join this conversation on GitHub. Already have an account? # 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: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants