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: centralize device-type-specific functions #10524

Merged

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 30, 2018

Contribution description

This moves all remaining switch (netif->device_type)-like functions to gnrc_netif_device_type.c and also applies the warning mechanisms introduced in #10513. The hope is that if a new device type is implemented the implementor will stumble over these functions basically immediately.

Testing procedure

Pinging a global address with gnrc_networking via a gnrc_border_router should still work for

After pinging the global address of the other node, the neighbor cache (nib neigh) of the border router (or advertising router - so the one were you didn't type ifconfig <if> -rtr_adv) should contain the other node's global address with the correct link-layer address

Issues/PRs references

Depends on #10513 (merged)

gnrc_netif/gnrc_sixlowpan_iphc BLE capability

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Nov 30, 2018
@miri64 miri64 requested a review from haukepetersen November 30, 2018 06:51
@miri64 miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 30, 2018
@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from 2593e04 to cc0bc9c Compare November 30, 2018 08:04
@miri64 miri64 added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Nov 30, 2018
@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from cc0bc9c to 84093cf Compare December 3, 2018 14:43
@miri64
Copy link
Member Author

miri64 commented Dec 3, 2018

Rebased and adapted for current #10513.

#if defined(MODULE_CC110X)
case NETDEV_TYPE_CC110X:
(void)opt;
return sizeof(uint8_t);
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 added this (in an extra commit), as it wasn't implemented yet.

@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 6, 2018
@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from 4849569 to ef0429e Compare December 6, 2018 15:42
@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

Rebased to current master. No longer dependent on another PR.

@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from ef0429e to 43110bd Compare December 6, 2018 18:53
@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

Rebased again and squashed this time

@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 Dec 6, 2018
@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from 43110bd to 6c8f432 Compare December 6, 2018 20:15
@miri64
Copy link
Member Author

miri64 commented Dec 17, 2018

@maribu @gschorcht can you test this again?

@miri64
Copy link
Member Author

miri64 commented Dec 17, 2018

Maybe @aabadie can test for nordic softdevice and nrfmin (and all the rest :-))

@gschorcht
Copy link
Contributor

@miri64 Tested.

After configuring 2001:db8::1 as global unicast address on the node with enabled RTR_ADV, the other node with disabled RTR_ADV gets the global unicast address 2001:db8::32ae:a4ff:fe18:7a3d. nib neigh on this gives:

> nib neigh
fe80::32ae:a4ff:fe41:60f9 dev #9 lladdr 30:AE:A4:41:60:F9 router REACHABLE
2001:db8::1 dev #9 lladdr 30:AE:A4:41:60:F9  REACHABLE

After pinging any global unicast address, nib neigh on the node with enabled RTR_ADV gives:

> nib neigh
fe80::32ae:a4ff:fe18:7a3d dev #9 lladdr 30:AE:A4:18:7A:3D router DELAY
2001:db8::32ae:a4ff:fe18:7a3d dev #9 lladdr 30:AE:A4:18:7A:3D  REACHABLE

I guess that is what you expected.

@miri64
Copy link
Member Author

miri64 commented Dec 17, 2018

Yepp :-)

@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from 6438b3b to 21261a8 Compare January 3, 2019 11:37
@miri64
Copy link
Member Author

miri64 commented Jan 3, 2019

Rebased and adapted for current master.

@miri64
Copy link
Member Author

miri64 commented Jan 3, 2019

ESP-now needs retesting, since I needed to adopt for #10581.

@maribu
Copy link
Member

maribu commented Jan 7, 2019

Btw: Adding

if ! lsmod | grep tun > /dev/null ; then
    sudo modprobe tun
fi

To the script run on make term would make the developers life easier ;-)

@maribu
Copy link
Member

maribu commented Jan 7, 2019

OK, it does not work on the MSB-A2

This is what I did in example/gnrc_border_router:

make BOARD=msba2 PORT=/dev/ttyUSB0 flash
sudo modprobe tun
make BOARD=msba2 PORT=/dev/ttyUSB0 term

and in examples/gnrc_networking (while the border router was still running in a separate shell):

make BOARD=msba2 PORT=/dev/ttyUSB1 flash
make BOARD=msba2 PORT=/dev/ttyUSB1 term

But the MSB-A2 all get the same HW and IPv6 address. I changed the address on the board running examples/gnrc_netowrking after boot using:

ifconfig 7 del fe80::ff:fe00:22
ifconfig 7 add fe80::ff:fe00:42
ifconfig 7 set addr 42

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

@maribu what does not work on MSB-A2? Your comment doesn't really provide any information on that :-/

@miri64
Copy link
Member Author

miri64 commented Jan 7, 2019

Btw: Adding

if ! lsmod | grep tun > /dev/null ; then
    sudo modprobe tun
fi

To the script run on make term would make the developers life easier ;-)

I believe you are the first one to report such a problem. I (with both Ubuntu and Arch) never had to load that module (nor add it to /etc/modules or something like that).

@maribu
Copy link
Member

maribu commented Jan 8, 2019

If I understood correctly, the boarder router should forward packets to the Internet. I did ping6 <IPv6_ADDRESS> and I only got a reply when I used the link lokal address of the border router, but not when pinging a global address. The output of nib neigh was empty at any time.

@gschorcht
Copy link
Contributor

gschorcht commented Jan 8, 2019

@maribu I guess your border router is connected to your LAN. Pinging global like 2a00:1450:4013:c07::5e from dig AAAA www.google.de requires that the way back is defined and you use a routable prefix for your 6LoWPAN. This would be the case, for example, when you use a subnet of the prefix that was assigned to your LAN from your ISP. In that case you could tell your router that a certain subnet is forwared to the border router LAN interface. Another test could be to define the route back to your border router on a machine in your LAN which you want to use as destination for your ping.

@maribu
Copy link
Member

maribu commented Jan 14, 2019

I just tested it with the current master and was unable to ping IPv6 addresses within my LAN. @miri64: Have you tried make term in the border router with the MSB-A2 lately? Could also be that the script is not compatible with Alpine Linux (even after modprobe tun). I'll give it a try with another Linux Distro as soon as I have time to check whether this is indeed an compatibility issue with Alpine.

@miri64
Copy link
Member Author

miri64 commented Jan 15, 2019

@maribu I tried on current master and there were two things I noticed when executing make term with the gnrc_border_router application on msba2:

  1. it executes
    sudo sh /home/mlenders/Repositories/RIOT-OS/RIOT/dist/tools/ethos/start_network.sh /dev/ttyUSB0 tap0 2001:db8::/64 -tg -p "/dev/ttyUSB0"
    The last three parameters are wrong and don't belong there.
  2. When executing
    sudo sh /home/mlenders/Repositories/RIOT-OS/RIOT/dist/tools/ethos/start_network.sh /dev/ttyUSB0 tap0 2001:db8::/64
    I still need to toggle the reset pins by hand (I did it by starting pyterm -tg -p /dev/ttyUSB0 and closing it immediately after). After that I can use the terminal of the border router (test with help)

After that I flashed and configured a second node (I removed lladdr fe80::ff:fe00:22 and set the hwaddr to 23 and added fe80::ff:fe00:23 and 2001:db8::ff:fe00:23). I then added a route to the other node on the BR (nib route add 7 2001:db8::/64 fe80::ff:fe00:23) (instead I could also just compile the border router with USEMODULE=gnrc_rpl).

Interestingly enough I am able to ping 2001:db8::ff:fe00:23 from the Linux host, but not the other way around (I suspect because the global address never becomes valid which hints to a not working neighbor discovery / address registration).

I confirmed that this is also works for this branch the same way. Since this the bug I reported above also is on current master, but the addresses and MTU are still configured correctly in this branch I'd say it works as expected (address registration doesn't seem to work but also not in master, so I investigate).

@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from c066069 to 68bf1f5 Compare January 15, 2019 10:54
@miri64
Copy link
Member Author

miri64 commented Jan 15, 2019

I confirmed that this is also works for this branch the same way. Since this the bug I reported above also is on current master, but the addresses and MTU are still configured correctly in this branch I'd say it works as expected (address registration doesn't seem to work but also not in master, so I investigate).

Same problem as described in #10723.

@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

Addressed #10589 (comment)

* @brief Get the default link-layer address option for the given
* gnrc_netif_t::device_type of a network interface
*/
netopt_t gnrc_netif_get_l2addr_opt(gnrc_netif_t *netif);
Copy link
Member

@cgundogan cgundogan Jan 16, 2019

Choose a reason for hiding this comment

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

Aren't @return and @param missing here, or did you left them out intentionally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I also fixed some weird behavior (and the constness of parameter).

Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

Tested successfully with ethernet and 15.4. Cannot test the softdevice at the moment. Nevertheless, IMO the code change shouldn't have an affect on the softdevice operation. ACK

@cgundogan cgundogan 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: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jan 16, 2019
@cgundogan
Copy link
Member

addendum: please squash!

The function to infer the link-layer address length from the length of
a S/TLLAO is very dependent on the IPv6 over X specification and thus
should be grouped with the other IP over X functions.
@miri64
Copy link
Member Author

miri64 commented Jan 16, 2019

Squashed.

@miri64 miri64 force-pushed the gnrc_netif/enh/centralize-dev-type-functions branch from 311b278 to 55b9757 Compare January 16, 2019 14:53
@cgundogan cgundogan merged commit b24a8fb into RIOT-OS:master Jan 16, 2019
@miri64 miri64 deleted the gnrc_netif/enh/centralize-dev-type-functions branch January 16, 2019 15:45
@aabadie aabadie added this to the Release 2019.01 milestone Jan 21, 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 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: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation 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.

5 participants