-
Notifications
You must be signed in to change notification settings - Fork 2k
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_ipv6_nib: use generated EUI-64 for ARO build and check #10817
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one minor nit and a potential bug
Are PR #10499 and the extension of the list by
|
With this PR I believe the distinction shouldn't be necessary anymore (but it is still would make sense). |
I think @gschorcht is currently reviewing, but when he is done feel free to rebase to address the alignment issue |
I will do it tomorrow. |
Ok, then I rebase now. |
Done |
f613022
to
e6e6a4e
Compare
👍 Works w/o PR #10499 with a border router with Ethernet connected to LAN and ESP-NOW on wireless side. Ethernet interface gets the global routing prefix from the router in LAN, ESP-NOW interface disseminates the The only one special configuration the border router required was ESP-NOW nodes worked out of the box with |
Nope |
(on)
|
Border router was compiled with following gnrc modules:
|
Thanks @gschorcht. I was planning to investigate, why the addresses never became valid on that interface. That explains it. I fixed it in #10824 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK from my side
Should we have tests for all candidates? |
We should at least test for |
Ok, using the steps described in #10524 (comment) I was able to get ping both MSB-A2 hosts from the Linux host - the other way round did not work for me (but that was expected). |
Why? |
BTW for testing this PR myself I used the following patch for the diff --git a/drivers/cc110x/cc110x.c b/drivers/cc110x/cc110x.c
index 8230f9487..80c134115 100644
--- a/drivers/cc110x/cc110x.c
+++ b/drivers/cc110x/cc110x.c
@@ -81,8 +81,7 @@ int cc110x_setup(cc110x_t *dev, const cc110x_params_t *params)
cc110x_set_channel(dev, CC110X_DEFAULT_CHANNEL);
/* set default node id */
- uint8_t addr;
- luid_get(&addr, 1);
+ uint8_t addr = 45;
cc110x_set_address(dev, addr);
LOG_INFO("cc110x: initialized with address=%u and channel=%i\n", This way everything bootstraps right from the start. |
Oh, sorry I got confused with the PRs. I though this was a cleanup PR and thus assumed everything not working before cannot be expected to work afterwards. Let me point out that I'm using an uncommon Linux distro. So the issue might be on the PC side as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is sensible, I have a minor nit below. It is not that important to address it within this PR, though, since you did not modify that function but rather moved. I cannot test right now with an 6lbr.
{ | ||
const unsigned offset = sizeof(eui64_t) - addr_len; | ||
|
||
memset(eui64->uint8, 0, sizeof(eui64->uint8)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you not use an assignment statement here?
eui64->uint8[0] = 0x00;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, further down I did see that you basically moved this function up without any modifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because eui64->uint8
is an array, so
memset(eui64->uint8, 0, sizeof(eui64->uint8)); /* sets all elements of the array to 0 */
is not the same
as
eui64->uint8[0] = 0x00; /* sets the first element of the array to 0 */
;-P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mhm, true (: never mind then
@aabadie @emmanuelsearch is this (bug fixing) PR too involved for a hotfix for the release or can it still go in? The issue it fixes won't show up in the release spec testings (however the bug fix might cause errors there when merged after improper testing ;-)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also tested with a samr and iotlab using a border router and normal 6lo router. I get a valid IPv6 address! ACK!
ah, and please squash |
3258003
to
d680aea
Compare
Squashed |
@aabadie @emmanuelsearch shall I backport? |
It's a bug, so yes please! |
Backport provided in #10906 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still no change, re-ACK.
Contribution description
This fixes #10723 by providing a new getter for the (unconverted) EUI-64 in the interface layer. This getter then is used instead of the link-layer address to set the EUI-64 in the ARO and also check it.
Testing procedure
Flash two 6Lo-capable boards with
gnrc_border_router
andgnrc_networking
. Thegnrc_networking
should get a global address which will be marked as valid. Not just IEEE 802.15.4-capable boards should be able to do this now, but also BLE-based boards and more exotic candidates likecc110x
,nrfmin
, oresp-now
.Issues/PRs references
Closes #10723.