network: make auto_init_ng_netif less board-dependant #2901

kaspar030 · 2015-05-01T11:26:54Z

PR for #2900, IMHO improves #2891.

~~This is work in progress, let's please discuss the concept.~~

I've adapted ng_at86rf2xx and xbee ~~but only the iot-lab_M3 node.~~
Couldn't test xbee for lack of device.

miri64 · 2015-05-01T11:58:54Z

boards/iot-lab_M3/include/ng_at86rf2xx_params.h

+/**
+ * @name AT86RF231 configuration
+ */
+    static const  at86rf2xx_params_t at86rf2xx_params[] =


indentation

miri64 · 2015-05-01T12:01:41Z

Can we find a solution that has less code duplication AND does not need any arrays stored in memory?

kaspar030 · 2015-05-01T12:20:45Z

@authmillenon Where does this store arrays in memory?

Only the device structs are stored in memory, but they have to.

miri64 · 2015-05-01T12:21:44Z

boards/iot-lab_M3/include/ng_at86rf2xx_params.h

+            AT86RF231_SLEEP,
+            AT86RF231_SPI_CLK,
+        },
+    };


@authmillenon Where does this store arrays in memory?

here

This get's optimized out.

miri64 · 2015-05-01T12:22:46Z

Only the device structs are stored in memory, but they have to.

Yes but not for the whole run-time as global constants IMHO.

kaspar030 · 2015-05-01T12:28:14Z

Only the device structs are stored in memory, but they have to.

Yes but not for the whole run-time as global constants IMHO.

They are driver-specific state variables. If they get stored in memory, that's how the driver has been implemented.

miri64 · 2015-05-01T12:29:28Z

Have look at this example:

#include <stdio.h>

#define ARRAY_SIZE  (2)

struct test {
    int one;
    int two;
};

const struct test t_array[] = {
    { 1, 2 },
    { 2, 3 },
};

void output(const struct test *t, size_t t_size) {
    for (int i = 0; i < t_size; i++) {
        printf("one: %d; two: %d\n", t[i].one, t[i].two);
    }
}

int main(int argc, char **argv) {
    output(t_array, ARRAY_SIZE);
}

   text    data     bss     dec     hex filename
   1406     560       8    1974     7b6 main

vs.

#include <stdio.h>

#define ARRAY_SIZE  (2)

struct test {
    int one;
    int two;
};

#define ARRAY (struct test[]){ \
    { 1, 2 }, \
    { 2, 3 }, \
}

void output(const struct test *t, size_t t_size) {
    for (int i = 0; i < t_size; i++) {
        printf("one: %d; two: %d\n", t[i].one, t[i].two);
    }
}

int main(int argc, char **argv) {
    output(ARRAY, ARRAY_SIZE);
}

   text    data     bss     dec     hex filename
   1394     560       8    1962     7aa main

miri64 · 2015-05-01T12:30:40Z

Interesting… I'm a little bit surprised, that the text section is shrinking... not the bss section oO

Compiler was clang btw

miri64 · 2015-05-01T12:33:02Z

However, the current device init-functions do not allow for that currently. But since you introduce this structs anyways, we can save parameters and just define them with your param struct.

miri64 · 2015-05-01T12:37:23Z

You are however propably right about the optimization when I look at the bss section ;-)

kaspar030 · 2015-05-01T12:39:08Z

@authmillenon gcc compiles like this: (main.c is the const array example, main2.c the second with #define ARRAY):

[kaspar@localhost hello-world]$ gcc -std=c99 main.c -Os -o main
[kaspar@localhost hello-world]$ gcc -std=c99 main2.c -Os -o main2
[kaspar@localhost hello-world]$ size main
   text    data     bss     dec     hex filename
   1360     560       8    1928     788 main
[kaspar@localhost hello-world]$ size main2
   text    data     bss     dec     hex filename
   1366     560       8    1934     78e main2
[kaspar@localhost hello-world]$

miri64 · 2015-05-01T12:39:21Z

sys/auto_init/auto_init_ng_netif.c

+static void start_nomac(char *stack, int stack_size, ng_netdev_t *device, const char* name) {
+    /* starting NOMAC */
+    DEBUG("Start NOMAC layer for dev %s\n", name);
+    ng_nomac_init(stack, stack_size, MAC_PRIO, name,


What if other MAC layers than nomac want to be used?

Uff. All the configurability, all the time.

Either #define AT86RF2XX_MAC NOMAC or another field in *_params, then a switch(...) over it. The first would be global for all devices of one type, the second would allow for two devices with different MAC layers.

Let's cross that bridge as soon as we have a) a second MAC layer, b) a use case.

Basically we need a mapping from MAC to device somewhere. I think it's safe to neglect the case of multiple radios of the same type with different MAC protocols for now.

I think it must be possible for a board, to specify the MAC layer that should be used, e.g. BOARD A, driver A, MAC A - BOARD B, driver A, MAC B... That is something, that we will see in the foreseeable future...

Even for the same board this is required.

kaspar030 · 2015-05-01T12:40:32Z

However, the current device init-functions do not allow for that currently. But since you introduce this structs anyways, we can save parameters and just define them with your param struct.

well, that could be a next step.

kaspar030 · 2015-05-01T12:40:49Z

So you approve the concept? @haukepetersen what do you think?

miri64 · 2015-05-01T12:41:59Z

So you approve the concept?

Only problem laying still around is #2901 (comment)

kaspar030 · 2015-05-01T12:47:02Z

Only problem laying still around is #2901 (comment)

Easy to solve as soon as there's a second option for the MAC layer.

miri64 · 2015-05-01T12:48:15Z

I'd prefer to see your solution now :-).

kaspar030 · 2015-05-01T12:49:04Z

It's already in there and works with all available MAC layers. :-)

miri64 · 2015-05-01T12:53:09Z

No seriously? How do you propose to work with multiple MAC layers? Let's say the user wants at86rf2xx[0] to use nomac, at86rf2xx[1] ieee802154e, and xbee[0] nomac, too.

kaspar030 · 2015-05-01T12:57:40Z

in e.g., xbee_params.h:

static const netlayer_nr_typt_t xbee_mac_layers[XBEE_NUM] = { MAC_LAYER_NOMAC, MAC_LAYER_802154 }

in auto_init_ng_netif.c:

maclayer_start_functions[xbee_mac_layers[i]](xbee_dev);

kaspar030 · 2015-05-01T13:17:11Z

But anyhow, I'd rather go for selecting a sane standard MAC layer and not use auto_init for the special cases. Choice is good but not if it complicates 99% of the use cases.

OlegHahm · 2015-05-02T14:39:33Z

sys/auto_init/auto_init_ng_netif.c

+#include "net/ng_nomac.h"
+#include "net/ng_netbase.h"
+
+#define ENABLE_DEBUG 1


kaspar030 · 2015-05-12T16:28:59Z

Should I squash this?

kaspar030 · 2015-05-12T16:30:11Z

@OlegHahm did you retry the xbee test?

OlegHahm · 2015-05-12T16:39:15Z

I just did. Builds without problem on my system. Pleas squash.

kaspar030 · 2015-05-12T16:44:35Z

squashed

kaspar030 · 2015-05-15T10:51:34Z

@haukepetersen ping

OlegHahm · 2015-05-17T11:00:23Z

Please rebase and add USEMODULE += fix_auto_init_netif to tests/driver_kw2xrf/Makefile.

ACK then. Let's get this merged.

jonas-rem · 2015-05-17T17:26:39Z

I already prepared a new PR to adapt kw2xrf to this auto-init poperly. Will open when this is merged.

OlegHahm · 2015-05-17T17:31:09Z

Cool! 👍

…interfaces

kaspar030 · 2015-05-17T19:03:52Z

add USEMODULE += fix_auto_init_netif to tests/driver_kw2xrf/Makefile.

I fixed the problem differently: the auto_init_* functions where called whenever a radio module was selected, but the auto_init_ng_module wasn't linked in. Fixed. Pls have a quick look, then I'll squash.

OlegHahm · 2015-05-17T19:13:15Z

Had a quick look. I'm fine. Go squashing.

…ally selected

kaspar030 · 2015-05-17T19:24:25Z

The last commit doesn't squash easily, so I just fixed the commit message.

jonas-rem · 2015-05-18T08:39:36Z

@kaspar030 @OlegHahm I think Travis had an unrelated error. Could someone kick Travis?

kaspar030 · 2015-05-18T10:28:54Z

So this was ACKed? travis is happy...

OlegHahm · 2015-05-18T10:55:15Z

I say ACK and as far as I understood @haukepetersen, he doesn't NACK.

kaspar030 · 2015-05-18T11:09:50Z

allright, then GO.

network: make auto_init_ng_netif less board-dependant

OlegHahm · 2015-05-18T11:14:19Z

🍻 🎈 🎊

kaspar030 assigned haukepetersen May 1, 2015

miri64 reviewed May 1, 2015
View reviewed changes

OlegHahm reviewed May 2, 2015
View reviewed changes

sys/auto_init/auto_init_ng_netif.c

#include "net/ng_nomac.h"

#include "net/ng_netbase.h"

#define ENABLE_DEBUG 1

Copy link

Member

OlegHahm May 2, 2015

Choose a reason for hiding this comment

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

(0)

kaspar030 force-pushed the fix_auto_init_netif branch from f1bbc67 to 481a5b8 Compare May 12, 2015 16:44

kaspar030 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label May 12, 2015

kaspar030 mentioned this pull request May 15, 2015

boards/pba-d-01-kw2x: add network interface auto init #2983

Merged

kaspar030 added 3 commits May 17, 2015 21:02

sys: auto_init: introduce new auto initialization scheme for network …

e655137

…interfaces

drivers: adapt ng_at86rf2xx to new auto_init_ng_netif scheme

3c00ff0

drivers: xbee: adapt to new auto_init_ng_netif scheme

a89b382

kaspar030 force-pushed the fix_auto_init_netif branch from 481a5b8 to fa4bdd9 Compare May 17, 2015 19:02

sys: auto_init: only auto init network interfaces when module is actu…

8e1370b

…ally selected

kaspar030 force-pushed the fix_auto_init_netif branch from fa4bdd9 to 8e1370b Compare May 17, 2015 19:23

kaspar030 added a commit that referenced this pull request May 18, 2015

Merge pull request #2901 from kaspar030/fix_auto_init_netif

895b629

network: make auto_init_ng_netif less board-dependant

kaspar030 merged commit 895b629 into RIOT-OS:master May 18, 2015

kaspar030 deleted the fix_auto_init_netif branch May 18, 2015 11:09

OlegHahm mentioned this pull request May 18, 2015

netif auto-initialization suboptimal #2900

Closed

jonas-rem mentioned this pull request May 21, 2015

Clarify and unify naming of thread-related constants (stacksize and priority) #2881

Merged

network: make auto_init_ng_netif less board-dependant #2901

network: make auto_init_ng_netif less board-dependant #2901

Conversation

kaspar030 commented May 1, 2015

Choose a reason for hiding this comment

miri64 commented May 1, 2015

kaspar030 commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented May 1, 2015

kaspar030 commented May 1, 2015

miri64 commented May 1, 2015

miri64 commented May 1, 2015

miri64 commented May 1, 2015

miri64 commented May 1, 2015

kaspar030 commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaspar030 commented May 1, 2015

kaspar030 commented May 1, 2015

miri64 commented May 1, 2015

kaspar030 commented May 1, 2015

miri64 commented May 1, 2015

kaspar030 commented May 1, 2015

miri64 commented May 1, 2015

kaspar030 commented May 1, 2015

kaspar030 commented May 1, 2015

Choose a reason for hiding this comment

kaspar030 commented May 12, 2015

kaspar030 commented May 12, 2015

OlegHahm commented May 12, 2015

kaspar030 commented May 12, 2015

kaspar030 commented May 15, 2015

OlegHahm commented May 17, 2015

jonas-rem commented May 17, 2015

OlegHahm commented May 17, 2015

kaspar030 commented May 17, 2015

OlegHahm commented May 17, 2015

kaspar030 commented May 17, 2015

jonas-rem commented May 18, 2015

kaspar030 commented May 18, 2015

OlegHahm commented May 18, 2015

kaspar030 commented May 18, 2015

OlegHahm commented May 18, 2015