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_netif: introduce distinction if an interface supports 6Lo or if it performs ND according to RFC 6775 #10499

Merged
merged 6 commits into from
Oct 21, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 28, 2018

Contribution description

This splits the semantic of gnrc_netif_is_6ln() into two check functions:

  1. Is the interface a 6Lo interface (i.e. does it require 6Lo to communicate with IPv6 over it)
  2. Is the interface performing 6Lo-ND as specified in RFC 6775.

It also fixes some logical inconclusions (check if interface is 6Lo when 6Lo is not compiled in wtf?) for the compile-time optimizations of those functions.

Testing procedure

It should be checked:

  • does it still compile as an IPv6 non-routing host (e.g. by using examples/gcoap on native)
  • does it still compile as an IPv6 routing host (e.g. by using examples/gnrc_networking on native)
  • does it still compile in (non-routing) 6LN config (e.g. by using examples/gcoap on samr21-xpro)
  • does it still compile in 6LR config (e.g. by using examples/gcoap on samr21-xpro)
  • are there significant size differences between the master versions of those builds and the version in this PR (due to the change of the compile-time optimization)
  • does it still work as expected:
    • a non-routing, non-6LN host should start sending multicast RS, if SLAAC is activated a NS with source :: should be sent, when a packet is sent a NS for address resolution should be sent
    • a routing host, non-6LN should start sending multicast RA and except for the sending of NS should behave exactaly as a non-routing host
    • a non-routing 6LN should start sending multicast RS, packets to link-local addresses should be resolved by not sending an NS first but by reverting the IID of the address back to the L2 address.
    • a 6LR should do the same and start answering RS with a unicast RA as soon as it receives a RA from an upstream router
    • a 6LBR should answer RS with a unicast RA.

Issues/PRs references

None

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Nov 28, 2018
@miri64 miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Dec 17, 2018
@miri64 miri64 force-pushed the gnrc_netif/enh/split-6lo-6ln branch from 6ecd8de to 998663b Compare December 27, 2018 20:48
@miri64
Copy link
Member Author

miri64 commented Dec 27, 2018

Rebased to current master.

@miri64
Copy link
Member Author

miri64 commented Jan 4, 2019

Rebased and adapted to current master.

@gschorcht
Copy link
Contributor

gschorcht commented Jan 6, 2019

With the changes, address assignments neither work for boarder routers nor for other nodes (routers and nodes). If I'm right, non-6Lo-ND nodes should use classic DAD instead of 6Lo-ND. That is, they are sending an NS with their own address instead of the router address. It also doesn't work with GNRC_IPV6_NIB_CONF_SLAAC = 1 and GNRC_IPV6_NIB_CONF_MULTIHOP_P6C = 0.

The problem seems to be that gnrc_netif_is_6ln(netif) returns true here

#if GNRC_IPV6_NIB_CONF_SLAAC
else if (!gnrc_netif_is_6ln(netif)) {
/* cast to remove const qualifier (will still be used NIB internally as
* const) */
msg_t msg = { .type = GNRC_IPV6_NIB_DAD,
.content = { .ptr = &netif->ipv6.addrs[idx] } };
msg_send(&msg, gnrc_ipv6_pid);
}
#endif
also for non-6Lo-ND nodes.

gnrc_netif_is_6ln(netif) returns just the GNRC_NETIF_FLAGS_6LN flag which is set for each interface of a node that is compiled with module gnrc_ipv6_nib_6ln

#if GNRC_IPV6_NIB_CONF_6LN
netif->flags |= GNRC_NETIF_FLAGS_6LN;
netif->ipv6.rs_sent = 0;
#endif /* GNRC_IPV6_NIB_CONF_6LN */
Each node that uses 6Lo (module ieee802154[cc110x, cc2420, ...], nrfmin, esp_now) is compiled with module gnrc_sixlowpan_default and uses module gnrc_ipv6_nib_6ln due to the dependencies.

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

I'll have a look if this is true for IEEE 802.15.4 also.

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

Ah sorry, I overlooked your last paragraph.

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

@gschorcht Thanks for the analysis. d8ced2e should fix that.

@gschorcht
Copy link
Contributor

I will try.

/* intentionally falls through */
}
case NETDEV_TYPE_BLE:
case NETDEV_TYPE_NRFMIN:
Copy link
Member Author

Choose a reason for hiding this comment

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

@gschorcht note that I intentionally left out NETDEV_TYPE_ESP_NOW here for now. After reading your comments in #10524: should I add it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping? Hope this didn't get lost.

Copy link
Member Author

Choose a reason for hiding this comment

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

(though I moved the function now)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still rolling through the changes and discussions. I'm a bit slower than you 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still rolling through the changes and discussions. I'm a bit slower than you

In some cases this could be better (see all my "oops"s and "argh"s ;-D)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok for the moment. Once #10723 is solved, there should be be no reason why ESP-NOW nodes shouldn't talk 6Lo-ND. It worked in the tests with the workaround as expected.

There might be another reason why ESP-NOW nodes are different, the RSSI. It is not possible to extract the RSSI for a single message. RSSI is only measured during the scan-for-peers phase every 10 s. On the other hand, I did not find any place in the source code, where RSSI or LQI are used for routing or something like that.;

Copy link
Member Author

Choose a reason for hiding this comment

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

There might be another reason why ESP-NOW nodes are different, the RSSI. It is not possible to extract the RSSI for a single message. RSSI is only measured during the scan-for-peers phase every 10 s. On the other hand, I did not find any place in the source code, where RSSI or LQI are used for routing or something like that.;

Yupp, neither RSSI nor LQI are of interest for neighbor discovery, just for routing protocols (but AFAIK we don't use it as a metric for RPL at the moment).

@gschorcht
Copy link
Contributor

@miri64 I changed it locally and have already tested it. Address assignment with global routing prefix from RAs works now, at least for nodes that are able to receive the RAs from the border router. Following remarks:

Maybe it would be possible to define a pseudomodule for multihop prefix or 6Lo-ND and to tune the dependencies a bit that the following setting are done automatically if the nodes are non 6Lo-ND nodes.

 CFLAGS="-DGNRC_IPV6_NIB_CONF_MULTIHOP_P6C=0 -DGNRC_IPV6_NIB_CONF_SLAAC=1" make ...

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

  • Shouldn't they send RAs to distribute the prefix over the whole mesh, or is 6Lo-ND and multihop prefix capability a must for it?

If they are a 6LN (so a 6Lo node according to RFC 6775 -- the RFC defining 6Lo-ND) they must use 6Lo-ND of course. This is what this PR is for to distinct: 6LNs (i.e. nodes that use 6Lo-ND) vs 6Lo-operating nodes (i.e. nodes that use the 6Lo adaptation layer).

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

Maybe it would be possible to define a pseudomodule for multihop prefix or 6Lo-ND and to tune the dependencies a bit that the following setting are done automatically if the nodes are non 6Lo-ND nodes.

If you are a non-6Lo-ND node why would you pull in 6Lo-ND dependencies anyway? Or are you referring to gnrc_sixlowpan_default?

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

(I moved the new 6LN-flag initialization to gnrc_netif_device_type.c so I don't have to add it later in #10524 ;-))

@gschorcht
Copy link
Contributor

If they are a 6LN (so a 6Lo node according to RFC 6775 -- the RFC defining 6Lo-ND) they must use 6Lo-ND of course. This is what this PR is for to distinct: 6LNs (i.e. nodes that use 6Lo-ND) vs 6Lo-operating nodes (i.e. nodes that use the 6Lo adaptation layer).

That's clear. I was just wondering whether it is possible to propagate a prefix over the whole mesh, if the nodes are only 6LO (6Lo-operation nodes 😉). More precisely, do 6LO router nodes react on a RS with a RA event though their GNRC_NETIF_FLAGS_IPV6_RTR_ADV flag is not set for the interface?

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

That's clear. I was just wondering whether it is possible to propagate a prefix over the whole mesh, if the nodes are only 6LO (6Lo-operation nodes wink). More precisely, do 6LO router nodes react on a RS with a RA event though their GNRC_NETIF_FLAGS_IPV6_RTR_ADV flag is not set for the interface?

Not in classic NDP. But I would suggest to use RPL for prefix dissemination then.

@gschorcht
Copy link
Contributor

But I would suggest to use RPL for prefix dissemination then.

If it does the same, then everything is fine.

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

But I would suggest to use RPL for prefix dissemination then.

If it does the same, then everything is fine.

Yep, it also disseminated prefixes (but in addition provides routes as it is a routing protocol).

@miri64 miri64 force-pushed the gnrc_netif/enh/split-6lo-6ln branch from 5f53e73 to c93b302 Compare October 20, 2019 13:40
@miri64
Copy link
Member Author

miri64 commented Oct 20, 2019

Rebased and added the msb-430 boards to the Makefile.ci

@miri64
Copy link
Member Author

miri64 commented Oct 20, 2019

Ok, just retested if my claim in #12512 is true

When using two interfaces with one being a 6LoWPAN interface, the node should not crash, pinging might however not work due to the bug fixed in #10499.

sadly this isn't the case, as the Ethernet interface's link-local address still never becomes valid. Will make a mental note about that to investigate ASAP.

@miri64
Copy link
Member Author

miri64 commented Oct 20, 2019

Ah... it's because SLAAC is not compiled as I didn't compile it as a border router and thus this config path is chosen :-/

#ifdef MODULE_GNRC_IPV6_NIB_6LR
#ifndef GNRC_IPV6_NIB_CONF_6LR
#define GNRC_IPV6_NIB_CONF_6LR (1)
#endif
#ifndef GNRC_IPV6_NIB_CONF_SLAAC
#define GNRC_IPV6_NIB_CONF_SLAAC (0)
#endif
#endif

However, it's pretty mean that the address is assigned but never set to valid, without any DAD being run. I'll make a separate PR to fix that.

@miri64
Copy link
Member Author

miri64 commented Oct 20, 2019

[…] I'll make a separate PR to fix that.

See #12513

@benpicco benpicco added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 20, 2019
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.

The code looks reasonable.
I only did basic testing with two 802.15.4 nodes where everything behaved as before, but I trust your and @gschorcht's testing for the rest of the cases.

@miri64
Copy link
Member Author

miri64 commented Oct 20, 2019

I only did basic testing with two 802.15.4 nodes where everything behaved as before, but I trust your and @gschorcht's testing for the rest of the cases.

Mhhhh in both cases the testing was done almost a year ago (I'm not even sure what of the listed tests above I was able to do myself)... Let me repeat at least some of them before we merge.

@gschorcht
Copy link
Contributor

Sorry that I'm jumping in so late. The winter term started last week and there were a lot of other open action items and discussions.

How could I help with testing? I reviewed the code some time ago #10499 (review). It has been a long time since we discussed this distinction and I do not remember all the details.

ESP's esp-now network interfaces are 6Lo but not 6Lo-ND interfaces. esp-now nodes as well as border router with an esp-now interface work with current master. RPL is used for prefix dissemination.

As I remember, even if esp-now are 6Lo nodes, they can't become or emulate a 6Lo-ND node because they have no official EUI-64 that could be used for ARO. The use of a generated one was not possible. What should change with this PR that would have to be tested?

@miri64
Copy link
Member Author

miri64 commented Oct 21, 2019

As I remember, even if esp-now are 6Lo nodes, they can't become or emulate a 6Lo-ND node because they have no official EUI-64 that could be used for ARO. The use of a generated one was not possible. What should change with this PR that would have to be tested?

As far as I remember that is what it was, yes. The easiest way to test this case if without SLAAC compiled in CFLAGS=-DGNRC_IPV6_NIB_CONF_SLAAC=0 and #12522 merged, the esp-now interface should not receive an auto-configured address, even when CFLAGS=-DGNRC_IPV6_NIB_CONF_6LN=1.

@miri64
Copy link
Member Author

miri64 commented Oct 21, 2019

For the regression testing I transformed the list in the Testing Procedures into a tick-list.

@miri64
Copy link
Member Author

miri64 commented Oct 21, 2019

(the compile tests were done by Murdock already ;-))

@miri64
Copy link
Member Author

miri64 commented Oct 21, 2019

Re-tested all the regression tests: no regression found.

@miri64 miri64 merged commit 05bcdba into RIOT-OS:master Oct 21, 2019
@miri64 miri64 deleted the gnrc_netif/enh/split-6lo-6ln branch October 21, 2019 09:47
@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Oct 21, 2019
@miri64 miri64 restored the gnrc_netif/enh/split-6lo-6ln branch October 31, 2019 18:50
@miri64 miri64 deleted the gnrc_netif/enh/split-6lo-6ln branch October 31, 2019 18:52
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019
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 Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines 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.

6 participants