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

sys/net/gnrc: Flag esp_now as 6LN #13561

Merged
merged 1 commit into from
Mar 5, 2020
Merged

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Mar 5, 2020

Contribution description

In gnrc_netif_init_6ln() the flag GNRC_NETIF_FLAGS_6LN is accidentally not set for esp_now devices. This commit fixes this.

Testing procedure

Running ifconfig in examples/gnrc_networking e.g. on the esp8266-esp-12x shows no IPv6 address with master. With this PR, the address should show up again and ping6ing should work.

Issues/PRs references

same as #12683

In gnrc_netif_init_6ln() the flag GNRC_NETIF_FLAGS_6LN is accidentally not set
for esp_now devices. This commit fixes this.
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 5, 2020
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Makes total sense.

@miri64 miri64 merged commit 73e41f6 into RIOT-OS:master Mar 5, 2020
@benpicco benpicco deleted the esp_now-6ln-fix branch March 5, 2020 14:49
@gschorcht
Copy link
Contributor

I'm completely confused now.

The output of the long, long discussion in PR #10817 was that ESP-NOW nodes are no "real" 6Lo nodes since they don't have a EUI64 which is required for ARO in 6Lo-ND. Even though, PR #10817 provided a possibility to use the modified EUI64 instead, PR #10499 introduced the distinction between 6LN nodes and 6LO nodes.

According to this distinction only 6LN nodes should get the GNRC_NETIF_FLAGS_6LN.

/**
 * @brief   This interface represents a 6Lo node (6LN) according to RFC 6775
 *
 * @see [RFC 6775, section 2](https://tools.ietf.org/html/rfc6775#section-2)
 */
#define GNRC_NETIF_FLAGS_6LN                       (0x00001000U)

Neither CC110x nor esp_now are 6LN interfaces according to this definition.

That's why I never proposed the change in this PR but thought we have to to set the GNRC_IPV6_NIB_CONF_SLAAC instead 😟

@benpicco
Copy link
Contributor Author

benpicco commented Mar 5, 2020

I don't know enough about the standard or the previous discussion to draw any such conclusions from it, but intuitively it makes total sense that those wireless technologies should perform neighborhood discovery (and remember what Linus said about standards… 😉)
Without having read the standard: neither cc110x nor esp_wifi are standard protocols, so of course the IETF standard would not mention those.

As for separating 6LO and 6ND it makes logical sense to me, even though afaik currently in RIOT everytime we do 6LO we also want 6ND.
Maybe for something like 6LoCAN you want 6LO but without neighbor discovery?

@miri64
Copy link
Member

miri64 commented Mar 5, 2020

I'm completely confused now.

The output of the long, long discussion in PR #10817 was that ESP-NOW nodes are no "real" 6Lo nodes since they don't have a EUI64 which is required for ARO in 6Lo-ND. Even though, PR #10817 provided a possibility to use the modified EUI64 instead, PR #10499 introduced the distinction between 6LN nodes and 6LO nodes.

According to this distinction only 6LN nodes should get the GNRC_NETIF_FLAGS_6LN.

/**
 * @brief   This interface represents a 6Lo node (6LN) according to RFC 6775
 *
 * @see [RFC 6775, section 2](https://tools.ietf.org/html/rfc6775#section-2)
 */
#define GNRC_NETIF_FLAGS_6LN                       (0x00001000U)

Neither CC110x nor esp_now are 6LN interfaces according to this definition.

That's why I never proposed the change in this PR but thought we have to to set the GNRC_IPV6_NIB_CONF_SLAAC instead worried

Ok sorry, I forgot about that discussion completely 😞. What you say makes sense. Both of them should due to the hardware address restrictions rather use classic neighbor discovery, or use the converted EUI64 (like IPv6-over-BLE does) in the ARO.

@miri64
Copy link
Member

miri64 commented Mar 5, 2020

I don't know enough about the standard

There no standard for both IPv6 over cc110x or esp-now ;-) We just use 6LoWPAN because it is convenient to re-use existing standards for radios with similar restrictions IEEE 802.15.4 has.

@miri64
Copy link
Member

miri64 commented Mar 5, 2020

That said: only because an IPv6-over-X implementation uses 6LoWPAN to make IPv6 over a restricted medium possible (i.e. use its fragmentation and header compression features) does not mean they have to use the neighbor discovery optimizations in RFC 6775 (which are more about power conservation than anything else). If the problem is the missing dissemination of compression context, than we should disable the automatic configuration for those interfaces, i.e. check if the given interface represents a 6LN.

@miri64
Copy link
Member

miri64 commented Mar 5, 2020

than we should disable the automatic configuration for those interfaces, i.e. check if the given interface represents a 6LN.

Done in #13564 and as part of #13485 (43b2be1 atm)

@gschorcht
Copy link
Contributor

Ok sorry, I forgot about that discussion completely.

Yeah, me too 😟 Even worse, since 6LoWPAN is not part of my daily work, I also forgot many details about the mechanisms in 6LoWPAN and their implementation in RIOT.

But if I remember correctly, 6Lo-ND simply did not work with ESP-NOW because of the missing EUI64. However, with a small hack using the IID as EUI64 it worked for me (as far as I remember).

With the eui48_to_eui64 function, introduced with PR #10817, we theoretically have now an EUI64. Does this mean we could use 6Lo-ND? Or did I miss something? Is that what you meant by:

or use the converted EUI64 (like IPv6-over-BLE does) in the ARO.

For me, it sounds like an option. Or would it require to implement something more?

@benpicco
Copy link
Contributor Author

benpicco commented Mar 5, 2020

Huh? Why would the source of the EUI-64 be relevant as to whether the device can use 6Lo-ND?

Most radio drivers just call luid_get() to generate a EUI-64 (even if a 'real' EUI-64 is provided by the board)

@miri64
Copy link
Member

miri64 commented Mar 5, 2020

With the eui48_to_eui64 function, introduced with PR #10817, we theoretically have now an EUI64. Does this mean we could use 6Lo-ND? Or did I miss something? Is that what you meant by:

or use the converted EUI64 (like IPv6-over-BLE does) in the ARO.

For me, it sounds like an option. Or would it require to implement something more?

Yepp, using eui48_to_eui64 at the right place (need to search that myself ^^") would fix that.

@miri64
Copy link
Member

miri64 commented Mar 5, 2020

Huh? Why would the source of the EUI-64 be relevant as to whether the device can use 6Lo-ND?

Because 6Lo-ND's address registration feature (which is used for duplicate address detection) uses the EUI-64 to uniquely identify the nodes. See

Most radio drivers just call luid_get() to generate a EUI-64 (even if a 'real' EUI-64 is provided by the board)

Don't mix up the IEEE 802.15.4 EUI-64 (which is a real hardware address) and what typically is understood as an EUI-64 (a 64-bit identifier generated from the real hardware address by the specfied algorithm; this is why your Laptop has an ff:fe in the middle of its auto-generated IPv6 addresses' IID).

@gschorcht
Copy link
Contributor

gschorcht commented Mar 5, 2020

@miri64 While looking in sys/net/gnrc/netif/gnrc_netif.c for the difference between NETDEV_TYPE_BLE and NETDEV_TYPE_ESP_NEW is, I found:

case NETDEV_TYPE_ESP_NOW:
assert(netif->ipv6.mtu <= ETHERNET_DATA_LEN);
Shouldn't it be IPV6_MIN_MTU?

@miri64
Copy link
Member

miri64 commented Mar 5, 2020

Shouldn't it be IPV6_MIN_MTU?

IPV6_MIN_MTU is lesser than ETHERNET_DATA_LEN, so the assertion is true either way. Regarding correctness lets go through case by case: For Ethernet the MTU has to be 1500 (== ETHERNET_DATA_LEN). BLE does not use 6LoWPAN fragmentation but BLE's own fragmentation. RFC 7668 only recommends to use 1280 byte (IPV6_MIN_MTU), but says larger MTUs are possible (but we should probably cap it at ETHERNET_DATA_LEN to prevent fragmentation at the border router... if that justifies an assertion 🤷‍♀️). Regarding ESP now: while RFC 4944 limits the MTU to 1280, this number is basically artificial (but its lower limit justified by RFC 8200), as 6LoWPAN fragmentation supports datagram length up to 2^11-1. I am not sure, if I check the MTU internally again, but in theory longer MTUs are possible. Also, one should not forget this paragraph from RFC 8200 (which did not exist in its predecessor RFC 2460:

A node must be able to accept a fragmented packet that, after
reassembly, is as large as 1500 octets. A node is permitted to
accept fragmented packets that reassemble to more than 1500 octets.
An upper-layer protocol or application that depends on IPv6
fragmentation to send packets larger than the MTU of a path should
not send packets larger than 1500 octets unless it has assurance that
the destination is capable of reassembling packets of that larger
size.

So for the proprietary link-layers, its probably smart to pick 1500 as MTU, to avoid the need for IPv6 fragmentation on top.

@gschorcht
Copy link
Contributor

All right, thank you for the detailed explanation 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer 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.

4 participants