sys/auto_init: allow delayed initialisation of SAUL & gnrc_netif

[benpicco]

Allow to disable auto_init_gnrc_netif, but still retain the capability
of initialising network interfaces manually at a later time.

Contribution description

Testing procedure

The default behavior should remain unchanged.
You can try out the delayed manual initialization of network devices with

this patch

--- a/examples/gnrc_networking/Makefile
+++ b/examples/gnrc_networking/Makefile
@@ -10,7 +10,7 @@ RIOTBASE ?= $(CURDIR)/../..
 # Include packages that pull up and auto-init the link layer.
 # NOTE: 6LoWPAN will be included if IEEE802.15.4 devices are present
 USEMODULE += gnrc_netdev_default
-USEMODULE += auto_init_gnrc_netif
+USEMODULE += gnrc_netif_init_devs
 # Activate ICMPv6 error messages
 USEMODULE += gnrc_icmpv6_error
 # Specify the mandatory networking modules for IPv6 and UDP
diff --git a/examples/gnrc_networking/main.c b/examples/gnrc_networking/main.c
index 6301f4291d..88fe54d600 100644
--- a/examples/gnrc_networking/main.c
+++ b/examples/gnrc_networking/main.c
@@ -22,14 +22,25 @@
 
 #include "shell.h"
 #include "msg.h"
+#include "net/gnrc/netif.h"
 
 #define MAIN_QUEUE_SIZE     (8)
 static msg_t _main_msg_queue[MAIN_QUEUE_SIZE];
 
 extern int udp_cmd(int argc, char **argv);
 
+static int start_netif(int argc, char **argv) {
+    (void) argc;
+    (void) argv;
+
+    gnrc_netif_init_devs();
+
+    return 0;
+}
+
 static const shell_command_t shell_commands[] = {
     { "udp", "send data over UDP and listen on UDP ports", udp_cmd },
+    { "start", "Bring up network interfaces", start_netif },
     { NULL, NULL, NULL }
 };

Issues/PRs references

alternative to #13488

[miri64]

First ground-level review.

[miri64]

Neither saul nor gnrc_netif use the SUBMODULE feature of our build system. I'm not sure it can be used for either, so I think you need to put them in sub directories for this to be able to build.

[benpicco]

Neither saul nor gnrc_netif use the SUBMODULE feature of our build system. I'm not sure it can be used for either, so I think you need to put them in sub directories for this to be able to build.

Huh?
With the patch in the PR description I can build examples/gnrc_networking and manually bring up the network interfaces with the start command in the shell.

[miri64]

Ah, maybe because you now effectively made them PSEUDOMODULES, but that means, they are always included in the build even if they are thrown out later (in the optimization step).

[benpicco]

but that means, they are always included in the build even if they are thrown out later (in the optimization step).

Is that a problem?
It's only one (mostly empty) function that is being compiled.

[miri64]

Is that a problem?

Modules you don't have to include but are just compiled under certain side conditions sound like a problem, yeah. Maybe its not really inconvenient now, but can very well fall on our feet some day ;-).

It's only one (mostly empty) function that is being compiled.

If you want to have it as a mostly empty function as part of the main module then describe it as such in the build system. If not, then make it a sub-module (maybe as a pseudo-module).

[benpicco]

I'm not sure what you mean (and how the possible solution would look like).
With this PR, there are two modules now where there previously was one:

• gnrc_netif_init a real module that can be enabled independently of
• auto_init_gnrc_netif a pseudo-module that will enable and call gnrc_netif_init automatically.

If neither of the two modules is used, neither of the two modules is build.
If gnrc_netif_init is selected, the files in sys/auto_init/netif/ are compiled.
If auto_init_gnrc_netif is selected, gnrc_netif_init_devs() is called in auto_init() and gnrc_netif_init will be selected.

[miri64]

May I push into your branch? I think it's quicker if I just do that instead of trying to explain...

[benpicco]

Sure, go ahead.

[miri64]

Done.

[miri64]

I moved the auto_init_* files to the new module as well, as they are now more related to the devs_init mechanism than the auto-init mechanism (which just calls the devs_init mechanism). We could rename those functions and files as well to keep everything neat, but I think for the scope of what this PR was originally trying to do this is then somewhat getting out of hand.

Furthermore, IMHO the PR should contain in the end two commits: one for the auto_init_gnrc_netif rework and one for the auto_init_saul rework. They are similar changes, but handle two distinct modules.

[benpicco]

Yes that works for me 😄
Should I squash?

I am now a co-author of this PR.

[miri64]

Should I squash?

Yes, please.

[fjmolinas]

All green here!

[fjmolinas]

ACK. Tested that functions where still compiled in. And tested the delyed init mechanism:

2020-03-05 17:02:05,324 #  help
2020-03-05 17:02:05,326 # Command              Description
2020-03-05 17:02:05,330 # ---------------------------------------
2020-03-05 17:02:05,335 # udp                  send data over UDP and listen on UDP ports
2020-03-05 17:02:05,340 # start                Bring up network interfaces
2020-03-05 17:02:05,343 # reboot               Reboot the node
2020-03-05 17:02:05,348 # ps                   Prints information about running threads.
2020-03-05 17:02:05,352 # ping6                Ping via ICMPv6
2020-03-05 17:02:05,355 # random_init          initializes the PRNG
2020-03-05 17:02:05,360 # random_get           returns 32 bit of pseudo randomness
2020-03-05 17:02:05,365 # nib                  Configure neighbor information base
2020-03-05 17:02:05,369 # ifconfig             Configure network interfaces
2020-03-05 17:02:05,376 # rpl                  rpl configuration tool ('rpl help' for more information)
2020-03-05 17:02:05,381 # 6ctx                 6LoWPAN context configuration tool
> ifconfig
2020-03-05 17:02:06,589 #  ifconfig
> start
2020-03-05 17:02:09,213 #  start
> ifconfig
2020-03-05 17:02:10,918 #  ifconfig
2020-03-05 17:02:10,922 # Iface  7  HWaddr: 13:36  Channel: 26  NID: 0x23
2020-03-05 17:02:10,926 #           Long HWaddr: D3:C1:6D:4E:AB:6A:13:36 
2020-03-05 17:02:10,930 #            TX-Power: 0dBm  State: IDLE 
2020-03-05 17:02:10,933 #           AUTOACK  ACK_REQ  AUTOCCA 
2020-03-05 17:02:10,938 #           L2-PDU:102 MTU:1280  HL:64  RTR  6LO  IPHC  
2020-03-05 17:02:10,941 #           Source address length: 8
2020-03-05 17:02:10,944 #           Link type: wireless
2020-03-05 17:02:10,949 #           inet6 addr: fe80::d1c1:6d4e:ab6a:1336  scope: link  VAL
2020-03-05 17:02:10,952 #           inet6 group: ff02::2
2020-03-05 17:02:10,955 #           inet6 group: ff02::1
2020-03-05 17:02:10,958 #           inet6 group: ff02::1:ff6a:1336
2020-03-05 17:02:10,959 #           
2020-03-05 17:02:10,962 #           Statistics for Layer 2
2020-03-05 17:02:10,965 #             RX packets 0  bytes 0
2020-03-05 17:02:10,969 #             TX packets 2 (Multicast: 2)  bytes 86
2020-03-05 17:02:10,973 #             TX succeeded 2 errors 0
2020-03-05 17:02:10,975 #           Statistics for IPv6
2020-03-05 17:02:10,978 #             RX packets 0  bytes 0
2020-03-05 17:02:10,983 #             TX packets 2 (Multicast: 2)  bytes 128
2020-03-05 17:02:10,986 #             TX succeeded 2 errors 0
2020-03-05 17:02:10,986 # 

[jia200x]

Maybe the next step is to generalize the "netif up" command (already used in gnrc_lorawan for setting up the connection).

It would be possible to call e.g gnrc_netif_up(netif) to e.g init the associated device, try to join a network, etc.

[miri64]

It would be possible to call e.g gnrc_netif_up(netif) to e.g init the associated device, try to join a network, etc.

In general, I agree but shouldn't this rather be modeled as a NETOPT_?

[jia200x]

In general, I agree but shouldn't this rather be modeled as a NETOPT_?

It would be indeed possible, but IMO it's too primitive and low level that I have the impression it would be a perfect candidate for it's own mechanism (same as the "send()").
If we assume (for some time) that gnrc_netif depends on netdev, we don't even need function pointers.

[jia200x]

(for handling connections we could use the NETOPT_LINK netopt, but I still think it would be better to have a dedicated function to initialize the device)

[miri64]

It would be indeed possible, but IMO it's too primitive and low level that I have the impression it would be a perfect candidate for it's own mechanism (same as the "send()").

I don't understand how this is too primitive. NETOPT_ usually is a nice way to hide a lot of functionality under a single function.

(for handling connections we could use the NETOPT_LINK netopt, but I still think it would be better to have a dedicated function to initialize the device)

If so, then that function should call gnrc_netif_set(), so the netif's thread is the sole controller of that function.

[benpicco]

Maybe the next step is to generalize the "netif up" command (already used in gnrc_lorawan for setting up the connection).

It would be possible to call e.g gnrc_netif_up(netif) to e.g init the associated device, try to join a network, etc.

That raises the question: What state should a device be in after .init() has been called?
Right it seems like it's IDLE (RX), but if we go for a two-stage approach that might as well be changed to have the device in a SLEEP state after init, but ready to accept commands.

[miri64]

That raises the question: What state should a device be in after .init() has been called?
Right it seems like it's IDLE (RX), but if we go for a two-stage approach that might as well be changed to have the device in a SLEEP state after init, but ready to accept commands.

What speaks against that? This PR is still valuable IMHO, as a separate auto_init configuration could ensure the ip link up at startup.

[jia200x]

I don't understand how this is too primitive. NETOPT_ usually is a nice way to hide a lot of functionality under a single function.

But for now we only have one way to initialize the interface (first lines of the gnrc_netif thread). Why don't we just add that code in a function instead of adding a NETOPT_ per interface?

That raises the question: What state should a device be in after .init() has been called?
Right it seems like it's IDLE (RX), but if we go for a two-stage approach that might as well be changed to have the device in a SLEEP state after init, but ready to accept commands.

I tend to say SLEEP after _init would be the best. Starting with RX_ON already collide with most MAC layers (LoRaWAN, variants of IEEEE802.15.4, etc)

[miri64]

Why don't we just add that code in a function instead of adding a NETOPT_ per interface?

up is a different action that init. And why per interface? Adding this to the gnrc_netif_set_from_netdev() function should suffice.

[jia200x]

The flow could look like auto_init_gnrc_netif -> gnrc_netif_up (or NETOPT_ variant) -> dev->driver->init. That way the gnrc_netif module is up but devices can be initialized on demand.

[jia200x]

up is a different action that init. And why per interface? Adding this to the gnrc_netif_set_from_netdev() function should suffice.

Ok, it falls to semantics. But the idea is the same :)

Adding this to the gnrc_netif_set_from_netdev() function should suffice.

True

[miri64]

The flow could look like auto_init_gnrc_netif -> gnrc_netif_up (or NETOPT_ variant) -> dev->driver->init. That way the gnrc_netif module is up but devices can be initialized on demand.

And that way you break potentially your interface because none of these functions were called from the operating context/thread (which the network device might expect ;-)).

[jia200x]

And that way you break potentially your interface because none of these functions were called from the operating context/thread (which the network device might expect ;-)).

There might be some benefits of leveraging this though IMO :). As discussed some time ago, we could potentially get rid of the gnrc_netif thread.

[benpicco]

The flow could look like auto_init_gnrc_netif -> gnrc_netif_up (or NETOPT_ variant) -> dev->driver->init. That way the gnrc_netif module is up but devices can be initialized on demand.

Shouldn't it be the other way round?
First you have to initialize the device, then you can bring up the interface (RX_ON )

[miri64]

And that way you break potentially your interface because none of these functions were called from the operating context/thread (which the network device might expect ;-)).

There might be some benefits of leveraging this though IMO :). As discussed some time ago, we could potentially get rid of the gnrc_netif thread.

We should first make it possible to wake an interface up, then redesign ;-).

[jia200x]

Shouldn't it be the other way round?
First you have to initialize the device, then you can bring up the interface (RX_ON )

Yes, of course. I was referring to the process (basically the "up" mechanism calls netdev->driver->init).

[benpicco]

Yes, of course. I was referring to the process (basically the "up" mechanism calls netdev->driver->init).

But you might want to initialize the radio even if you don't want to bring up the interface.
E.g. when powered up, the at86rf215 radio will be in TRXOFF state where it consumes 3mA.
So ideally you'd want to initialize it just so you can put it to DEEP_SLEEP where that drops to 30nA.
(But only at power-on reset. If you woke from deep sleep you probably already have put the interface to deep sleep to before going to sleep, so it will stay in that state if you don't touch it)

[jia200x]

But you might want to initialize the radio even if you don't want to bring up the interface.
E.g. when powered up, the at86rf215 radio will be in TRXOFF state where it consumes 3mA.
So ideally you'd want to initialize it just so you can put it to DEEP_SLEEP where that drops to 30nA.
(But only at power-on reset. If you woke from deep sleep you probably already have put the interface to deep sleep to before going to sleep, so it will stay in that state if you don't touch it)

I see what you mean.
IMO we should differentiate the "init" procedure to the "start". We don't have init functions in the drivers (only "_setup" to pass the params array). The current dev->driver->init function acts as a real "init" and a "start" function.

Why don't we add a proper "init" function to the raw driver (e.g at86rf2xx_init" and only leave the start up code in dev->driver->init?

[jia200x]

This was pointed out here #12469 (comment)