Skip to content

gnrc_ipv6_nib: add exception in address resolution for SLIP#8041

Merged
miri64 merged 1 commit intoRIOT-OS:gnrc_netif2_integration/masterfrom
miri64:gnrc_ipv6_nib/fix/slip-addr-res-exception
Nov 15, 2017
Merged

gnrc_ipv6_nib: add exception in address resolution for SLIP#8041
miri64 merged 1 commit intoRIOT-OS:gnrc_netif2_integration/masterfrom
miri64:gnrc_ipv6_nib/fix/slip-addr-res-exception

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Nov 15, 2017

Linux doesn't answer to NS over SLIP and since there are no link-layer addresses in SLIP, address resolution doesn't make much sense anyway, this PR circumvents it for SLIP interfaces. If the interface is provided to the address resolution (which it is for link-local or if a route is configured, which is the case for a border router where SLIP is usually used) the returned neighbor cache entry is just set to that interface with link-layer address 0.

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) GNRC Area: network Area: Networking labels Nov 15, 2017
@miri64 miri64 requested a review from bergzand November 15, 2017 13:33
This was referenced Nov 15, 2017
Merged
Merged
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 15, 2017

Ignoring any instabilities, ping works! I suspect the issue to be somewhere in the uart code of the nucleo. A value of 0xff sometimes lead the frame confusing the tunslip6 application. On the nucleo-side of the uart it is also easily possible to break connectivity, again suspected to happen in the UART code.

To get things working:

  1. disconnect the nucleo-TX, host side RX wire.
  2. start tunslip6 application (using aaaa::1/64 in this example)
  3. add aaaa::2/64 on the nucleo side to the interface
  4. start a ping6 on the host to aaaa::2
  5. while keeping the ping6 running, connect the TX wire again.

This way the tunslip application doesn't receive the 0xff byte and the ping6 on the host side should start reporting responses (around 30ms at 115200 baud). pinging from the node to the host also works, with responses around 20ms.

TL;DR: it unstable, but it works, and this PR is not to blame for the instability.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

@bergzand can you open an issue regarding your suspicions? I'm not really that much into the periph code of any platform and this way we can notify people about this, that do actually know about that code ;-).

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

(fixed some wording in the comment in my latest commit)

@miri64 miri64 force-pushed the gnrc_ipv6_nib/fix/slip-addr-res-exception branch from a3cf564 to cb21d6f Compare November 15, 2017 16:59
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

And rebased to make testing easier.

@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 Nov 15, 2017
bergzand
bergzand requested changes Nov 15, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Minor grammar nitpicking, code looks good.

sys/net/gnrc/network_layer/ipv6/nib/nib.c Outdated
bool res = false;
if ((netif != NULL) && (netif->device_type == NETDEV_TYPE_SLIP)) {
/* XXX: Linux doesn't do neighbor discovery for SLIP so no use sending
* NS and since SLIP doesn't has link-layer addresses anyway we can just
Copy link
Copy Markdown
Member

@bergzand bergzand Nov 15, 2017

Choose a reason for hiding this comment

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

has -> have, and a comma after "anyway" (I think).

Copy link
Copy Markdown
Member Author

@miri64 miri64 Nov 15, 2017

Choose a reason for hiding this comment

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

Fixed

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 15, 2017

Ack, please squash

@miri64 miri64 force-pushed the gnrc_ipv6_nib/fix/slip-addr-res-exception branch from 21fb017 to 9054910 Compare November 15, 2017 19:03
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

Squashed

@bergzand can you open an issue regarding your suspicions? I'm not really that much into the periph code of any platform and this way we can notify people about this, that do actually know about that code ;-).

Please remember to do that.

@bergzand bergzand mentioned this pull request Nov 15, 2017
Closed
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 15, 2017

Please remember to do that.

Thanks for the reminder, #8045 :)

bergzand
bergzand approved these changes Nov 15, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Ack

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

Errors on Murdock are all related to #7414. @bergzand would you like to have the honor of hitting merge ;-)?

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 15, 2017

I'm afraid I don't have the permissions to do that 😢
permission_denied

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

Weird... can you do to master? Might be that I misconfigured something for gnrc_netif2_integration/master.

@miri64 miri64 merged commit 099cdcf into RIOT-OS:gnrc_netif2_integration/master Nov 15, 2017
@miri64 miri64 deleted the gnrc_ipv6_nib/fix/slip-addr-res-exception branch November 15, 2017 19:42
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 15, 2017

Yeah, master works (#7977). I don't really mind for this case, but it might explain why I never merged anything on this branch :)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

Now you should be able to :-)

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 15, 2017

(as should any non-admin maintainer)

@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 15, 2017

🎉

@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergzand bergzand bergzand approved these changes

Assignees

@bergzand bergzand

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

Milestone

Release 2018.01

Development

Successfully merging this pull request may close these issues.

3 participants

@miri64 @bergzand @aabadie