gnrc: integrate gnrc_netif2 #7424

miri64 · 2017-07-27T14:02:06Z

Does not link ATM, since I did not bother to port the old NC and NDP (will provide a port for #7014 in a separate PR though [#7456]) and those modules now complain that they don't find the old interfaces functions. Since this change is unintrusive (and I do not provide special new applications for this), this should be okay.

~~Depends on #7410 and #7409 and all their dependencies.~~

This PR is part of the network layer remodelling effort:

… HEAD

miri64 · 2017-08-21T11:33:54Z

Rebased and adapted to current master and current dependencies

…IOT-OS#7456

…OS#7424

…#7424

miri64 · 2017-10-06T16:06:34Z

Rebased to current master and dependencies

The function `_update_nce_ar_state()` was introduced during the review of \RIOT-OS#7424, but it's return value never used, causing faulty behavior.

miri64 · 2017-10-27T15:13:42Z

I changed the base branch now to the freshly created gnrc_netif2_integration/master branch. I will remove all the ifdefs later.

miri64 · 2017-10-28T14:14:33Z

I will remove all the ifdefs later.

Seems like I already did that ^^" Ready for review.

cgundogan · 2017-10-30T15:28:37Z

Makefile.dep

@@ -319,38 +313,6 @@ ifneq (,$(filter gnrc_ipv6_nc,$(USEMODULE)))
  USEMODULE += ipv6_addr
 endif

-ifneq (,$(filter gnrc_ipv6_nib_6lbr,$(USEMODULE)))


why do you remove the nib dependency information?

Probably rebase error.

miri64 · 2017-11-01T14:15:46Z

Mhhhhh problem with all the non-unintrusive porting now is: Murdock fails to build (actually only to link), since old NDP isn't ported (and I don't plan to). This is now merged into a seperate branch. Do we accept the temporary broken state?

miri64 · 2017-11-01T16:19:50Z

Rebased to current gnrc_netif2_integration/master.

bergzand

First part of the PR revieweded (Up to (and including) sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c).

So far only three minor things. Until now everything looks like obvious old-new code replacements.

bergzand · 2017-11-07T09:24:06Z

examples/gnrc_minimal/main.c

-                ipv6_addr_to_str(ipv6_addr, &entry->addrs[i].addr, IPV6_ADDR_MAX_STR_LEN);
-                printf("My address is %s\n", ipv6_addr);
-            }
+    /* get interface and print their addresses */


Small grammatical error between "interface" (singular) and "their" (plural)

"their" can be singular, but I guess this does not apply for things 😛

bergzand · 2017-11-07T09:27:16Z

examples/gnrc_minimal/main.c

+        int res = gnrc_netapi_get(netif->pid, NETOPT_IPV6_ADDR, 0, ipv6_addrs,
+                                  sizeof(ipv6_addrs));
+
+        for (int i = 0; i < (res / sizeof(ipv6_addr_t)); i++) {


I'm getting a signed vs unsigned comparison error here (gcc-5.4.0)

bergzand · 2017-11-07T09:29:35Z

examples/gnrc_networking/Makefile

@@ -22,7 +22,7 @@ USEMODULE += gnrc_ipv6_router_default
 USEMODULE += gnrc_udp
 # Add a routing protocol
 USEMODULE += gnrc_rpl
-USEMODULE += auto_init_gnrc_rpl
+# USEMODULE += auto_init_gnrc_rpl


I assume this depends on the RPL port to gnrc_ipv6_nib?

Will remove.

miri64 · 2017-11-07T12:56:47Z

@bergzand addressed your comments

bergzand

Second half of my review.

bergzand · 2017-11-07T16:58:49Z

sys/net/gnrc/routing/rpl/gnrc_rpl_dodag.c

@@ -129,8 +129,7 @@ gnrc_rpl_instance_t *gnrc_rpl_instance_get(uint8_t instance_id)
    return NULL;
 }

-bool gnrc_rpl_dodag_init(gnrc_rpl_instance_t *instance, ipv6_addr_t *dodag_id, kernel_pid_t iface,
-                         gnrc_ipv6_netif_addr_t *netif_addr)
+bool gnrc_rpl_dodag_init(gnrc_rpl_instance_t *instance, ipv6_addr_t *dodag_id, kernel_pid_t iface)
 {
    /* TODO: check if netif_addr belongs to iface */


This todo looks obsolete now that the netif_addr argument is removed

I would like to keep it in for the person that is porting RPL (@cgundogan) so he can decide how they get the interface address.

Ack, fine with me :)

bergzand · 2017-11-07T17:03:53Z

sys/shell/commands/sc_fib.c

@@ -208,10 +206,11 @@ int _fib_route_handler(int argc, char **argv)
    /* e.g. fibroute add <destination> via <next hop> lifetime <lifetime> */
    if ((argc == 7) && (strcmp("add", argv[1]) == 0) && (strcmp("via", argv[3]) == 0)
            && (strcmp("lifetime", argv[5]) == 0)) {
-        kernel_pid_t ifs[GNRC_NETIF_NUMOF];
-        size_t ifnum = gnrc_netif_get(ifs);
+                size_t ifnum = gnrc_netif2_numof();


Indentation looks off here

bergzand · 2017-11-07T17:06:14Z

sys/shell/commands/sc_gnrc_6ctx.c

@@ -72,6 +69,7 @@ static void _adv_ctx(void)
            gnrc_ndp_internal_send_rtr_adv(ifs[i], NULL, NULL, false);
        }
    }
+#endif


This looks like a stray #endif

bergzand · 2017-11-07T17:13:24Z

Just to confirm, I'm getting compilation errors (not only linking errors) with the gnrc_networking example. All due to missing gnrc_ipv6_nib_iface functions:

/home/hydrazine/dev/RIOT/sys/net/gnrc/netif2/gnrc_netif2.c: In function ‘gnrc_netif2_set_from_netdev’:
/home/hydrazine/dev/RIOT/sys/net/gnrc/netif2/gnrc_netif2.c:281:21: error: implicit declaration of function ‘gnrc_ipv6_nib_iface_cease_rtr_adv’ [-Werror=implicit-function-declaration]
                     gnrc_ipv6_nib_iface_cease_rtr_adv(netif);
                     ^
/home/hydrazine/dev/RIOT/sys/net/gnrc/netif2/gnrc_netif2.c:290:17: error: implicit declaration of function ‘gnrc_ipv6_nib_iface_start_rtr_adv’ [-Werror=implicit-function-declaration]
                 gnrc_ipv6_nib_iface_start_rtr_adv(netif);
                 ^
/home/hydrazine/dev/RIOT/sys/net/gnrc/netif2/gnrc_netif2.c: In function ‘_gnrc_netif2_thread’:
/home/hydrazine/dev/RIOT/sys/net/gnrc/netif2/gnrc_netif2.c:1137:5: error: implicit declaration of function ‘gnrc_ipv6_nib_init_iface’ [-Werror=implicit-function-declaration]
     gnrc_ipv6_nib_init_iface(netif);
     ^
In file included from /home/hydrazine/dev/RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-6ln.h:27:0,
                 from /home/hydrazine/dev/RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-6ln.c:18:
/home/hydrazine/dev/RIOT/sys/net/gnrc/network_layer/ipv6/nib/_nib-arsm.h:50:38: error: unknown type name ‘gnrc_ipv6_netif_t’
 void _snd_ns(const ipv6_addr_t *tgt, gnrc_ipv6_netif_t *netif,
                                      ^

They are gone when testing with #7456, so I don't really mind as long as it is known.
gnrc_minimal compiles but fails to link as expected.

miri64 · 2017-11-07T17:28:35Z

They are gone when testing with #7456, so I don't really mind as long as it is known.

It is known :-)

miri64 · 2017-11-07T17:32:10Z

Addressed comments

bergzand · 2017-11-07T17:58:07Z

Thanks for the changes and clarifications. Looks good to me now

Not link-able, since NDP and NC are missing (intentionally)

miri64 · 2017-11-07T18:09:07Z

Squashed

miri64 · 2017-11-07T18:09:52Z

@smlng can we dismiss your review?

smlng · 2017-11-07T18:47:40Z

@miri64 general question (and suggestion if you want): netif2 will replace netif, right? As you now PRed against a feature branch it would IMHO make perfect sense to use netif for the naming already. Otherwise this whole feature branch doesn't make sense at all, the idea would be get the full replacement working and tested within this branch and merge the feature at once, when everything is fine.

miri64 · 2017-11-07T18:49:20Z

@miri64 general question (and suggestion if you want): netif2 will replace netif, right? As you now PRed against a feature branch it would IMHO make perfect sense to use netif for the naming already. Otherwise this whole feature branch doesn't make sense at all, the idea would be get the full replacement working and tested within this branch and merge the feature at once, when everything is fine.

I will do that, but not in this PR. In fact that's intended to be the very last PR against #7925 before reintegrating it into master ;)

changes addressed, but haven't had time for 2 second review round

miri64 · 2017-11-07T18:50:35Z

See also https://github.com/RIOT-OS/RIOT/projects/6#card-5336168 regarding that

miri64 · 2017-11-07T18:51:48Z

Merged intentionally with broken Murdock state

miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. GNRC Area: network Area: Networking labels Jul 27, 2017

miri64 assigned haukepetersen Jul 27, 2017

miri64 requested a review from haukepetersen July 27, 2017 14:02

miri64 force-pushed the gnrc_netif2/feat/gnrc-integration branch from ede4a9c to 2522259 Compare July 31, 2017 17:19

miri64 added a commit to miri64/RIOT that referenced this pull request Jul 31, 2017

Merge branch 'gnrc_netif2/feat/sc' (RIOT-OS#7404) into RIOT-OS#7424

99f5c7a

miri64 added a commit to miri64/RIOT that referenced this pull request Jul 31, 2017

Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…

d4058f2

…#7424

miri64 added a commit to miri64/RIOT that referenced this pull request Jul 31, 2017

Merge branch 'gnrc_netif2/feat/ieee802154' (RIOT-OS#7409) into RIOT-…

b3c7168

…OS#7424

miri64 force-pushed the gnrc_netif2/feat/gnrc-integration branch from f45f37b to 61e9ef0 Compare August 1, 2017 14:14

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 1, 2017

Merge branch 'gnrc_netif2/feat/sc' (RIOT-OS#7404) into RIOT-OS#7424

084d3d3

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 1, 2017

Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…

70ed025

…#7424

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 1, 2017

Merge branch 'gnrc_netif2/feat/ieee802154' (RIOT-OS#7409) into RIOT-…

fc561bd

…OS#7424

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 1, 2017

Merge branch 'gnrc_netif2/feat/gnrc-integration' (RIOT-OS#7424) into…

809741e

… HEAD

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 7, 2017

Merge branch 'gnrc_netif2/feat/gnrc-integration' (RIOT-OS#7424) into…

1a74ce9

… HEAD

This was referenced Aug 7, 2017

gnrc_ipv6_nib: port to gnrc_netif2 #7456

Merged

shell_commands: port ifconfig netstats neighbor for gnrc_netif2 #7463

Closed

miri64 force-pushed the gnrc_netif2/feat/gnrc-integration branch from 50ccef3 to 9f1a304 Compare August 21, 2017 11:32

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017

Merge branch 'gnrc_netif2/feat/sc' (RIOT-OS#7404) into RIOT-OS#7424

afc23eb

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017

Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS#…

231d4dd

…7424

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017

Merge branch 'gnrc_netif2/feat/ieee802154' (RIOT-OS#7409) into RIOT-O…

613aa2d

…S#7424

miri64 force-pushed the gnrc_netif2/feat/gnrc-integration branch from 9f1a304 to 65ae715 Compare August 21, 2017 11:39

miri64 added a commit to miri64/RIOT that referenced this pull request Aug 21, 2017

Merge branch 'gnrc_netif2/feat/gnrc-integration' (RIOT-OS#7424) into R…

5b4edb3

…IOT-OS#7456

miri64 mentioned this pull request Sep 6, 2017

gnrc_netif_ieee802154: drop duplicate broadcast packets (optionally) #7577

Merged

miri64 force-pushed the gnrc_netif2/feat/gnrc-integration branch from c02ef0c to eba909a Compare October 6, 2017 16:06

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 6, 2017

Merge branch 'gnrc_netif2/feat/sc' (RIOT-OS#7404) into RIOT-OS#7424

d2dfe6b

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 6, 2017

Merge branch 'gnrc_netif2/feat/ieee802154' (RIOT-OS#7409) into RIOT-…

198b937

…OS#7424

miri64 added a commit to miri64/RIOT that referenced this pull request Oct 6, 2017

Merge branch 'gnrc_netif2/feat/ethernet' (RIOT-OS#7410) into RIOT-OS…

4ced871

…#7424

miri64 changed the base branch from master to gnrc_netif2_integration/master October 27, 2017 15:12

cgundogan reviewed Oct 30, 2017

View reviewed changes

miri64 changed the title ~~gnrc: integrate gnrc_netif2 unintrusively~~ gnrc: integrate gnrc_netif2 Nov 1, 2017

miri64 mentioned this pull request Nov 1, 2017

gnrc_netif2: gnrc_ipv6_nib: full integration into GNRC #7925

Merged

28 tasks

miri64 force-pushed the gnrc_netif2/feat/gnrc-integration branch from 8f4a5dd to f4b9474 Compare November 1, 2017 16:19

bergzand requested changes Nov 7, 2017

View reviewed changes

bergzand approved these changes Nov 7, 2017

View reviewed changes

gnrc: integrate gnrc_netif2

861035f

Not link-able, since NDP and NC are missing (intentionally)

miri64 force-pushed the gnrc_netif2/feat/gnrc-integration branch from 5bf981b to 861035f Compare November 7, 2017 18:09

miri64 merged commit c202885 into RIOT-OS:gnrc_netif2_integration/master Nov 7, 2017

miri64 deleted the gnrc_netif2/feat/gnrc-integration branch November 7, 2017 18:51

miri64 mentioned this pull request Nov 8, 2017

examples: tests: port applications to gnrc_netif2 #7966

Merged

gnrc: integrate gnrc_netif2 #7424

gnrc: integrate gnrc_netif2 #7424

Uh oh!

Conversation

miri64 commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 21, 2017

Uh oh!

miri64 commented Oct 6, 2017

Uh oh!

miri64 commented Oct 27, 2017

Uh oh!

miri64 commented Oct 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 1, 2017

Uh oh!

miri64 commented Nov 1, 2017

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

bergzand left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergzand commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

bergzand commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

smlng commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

miri64 commented Nov 7, 2017

Uh oh!

Uh oh!

miri64 commented Jul 27, 2017 •

edited

Loading