gnrc: add l2_address into gnrc_netdev2_t #5942

zhuoshuguo · 2016-10-13T17:42:56Z

This is based on #5941, add l2_address into gnrc_netdev2_t.

miri64 · 2016-10-14T06:23:19Z

sys/include/net/gnrc/gnrc_mac/gnrc_mac.h

+
+/* Max link layer address length in bytes */
+#ifndef GNRC_MAC_MAX_L2_ADDR_LEN
+#define GNRC_MAC_MAX_L2_ADDR_LEN           (8U)


This again is a value we already have defined. I know that file might not be the most ideal location for that, so we might want to move it, but a new define is not really beneficial.

Thanks for telling me this. By the way, if I want to provide both two L2_ADDR_LEN values options (long as 8 bytes and short as 2 bytes) for the "l2_addr_t" data structure below to use in MAC, should I just define another SHORT_ADDR_LEN here? Thanks.

Or, what do you think is a better way to do it? :-)

That is very IEEE 802.15.4-specific and those address lengths are already defined in the respective header. This value (and the one in net/gnrc/netif/hdr.h) is the maximum length an address can have in the L2s we support (which is also why this value should be somewhere more central, since it might change if other L2 with longer addresses will be supported in the future).

Thanks! Got it.

miri64 · 2016-10-20T13:22:26Z

sys/include/net/gnrc/gnrc_mac/gnrc_mac_types.h

+    gnrc_netdev2_t* gnrc_netdev2;
+
+    /* Own address */
+    l2_addr_t l2_addr;


With zhuoshuguo#2 you can pull this into gnrc_netdev2_t, too. However, I would suggest to not add an extra type for that. Just add the fields l2_addr (as a byte-array) and l2_addr_len (as a byte) into the #ifdef MODULE_GNRC_MAC'd section. Since l2_addr_len will be out of alignment I prefer it to be last in that type however.

zhuoshuguo · 2016-10-28T13:28:18Z

sys/include/net/gnrc/netdev2.h

+    /**
+     * @brief device's l2 address
+     */
+    uint8_t  l2_addr[GNRC_NETIF_HDR_L2ADDR_MAX_LEN];


@miri64 , updated according to your last comment in this PR, moved l2_addr_t into gnrc_netdev2_t without creating a new type. Is this OK for you? :-)

Mhhh now seeing it like this it make sense to add a new macro above and use it here:

#include "net/ieee802154.h" /* ... */ #define GNRC_NETDEV2_MAX_L2ADDR_LEN (IEEE802154_LONG_ADDRESS_LEN)

miri64 · 2016-11-07T09:13:02Z

Needs rebase.

zhuoshuguo · 2016-11-07T09:24:06Z

Got it, do it right now~

zhuoshuguo · 2016-11-07T09:35:01Z

@miri64 rebased and squashed.

zhuoshuguo · 2016-11-07T10:20:19Z

Can this PR be also merged now? @miri64

miri64 · 2016-11-07T12:14:39Z

sys/include/net/gnrc/mac/types.h

@@ -29,9 +29,9 @@
 #include <xtimer.h>
 #include <net/netdev2.h>
 #include <net/gnrc/netdev2.h>
+#include <net/gnrc/netif/hdr.h>


I think this include is not needed anymore here

miri64 · 2016-11-07T12:14:46Z

sys/include/net/gnrc/mac/types.h

 #include <net/gnrc.h>

-


Why remove the line?

By mistake..

miri64 · 2016-11-07T12:17:45Z

sys/include/net/gnrc/netdev2.h

+    /**
+     * @brief device's l2 address
+     */
+    uint8_t  l2_addr[GNRC_NETIF_HDR_L2ADDR_MAX_LEN];


Mhhh now seeing it like this it make sense to add a new macro above and use it here:

#include "net/ieee802154.h" /* ... */ #define GNRC_NETDEV2_MAX_L2ADDR_LEN (IEEE802154_LONG_ADDRESS_LEN)

miri64 · 2016-11-07T12:19:08Z

Apart from the code changes there are still a few things I'm wondering:

How is this set?
What happens if the address (or the address length) is changed at user level?
Who is responsible for making this changes?

zhuoshuguo · 2016-11-07T13:05:49Z

@miri64

How is this set?

By something like below:

/* Get own address from netdev */
netdev2.l2_addr.len = dev->driver->get(dev, NETOPT_ADDRESS_LONG, &netdev2.l2_addr.addr, sizeof(netdev2.l2_addr.addr));

What happens if the address (or the address length) is changed at user level?

Yes, it will be unwanted that the address is changed at other levels.

Who is responsible for making this changes?

I think it is the MAC layer (L2) only.

I introduce this, since, in some special MAC layer cases (e.g., destination address of a packet is not put in netif_headher, especially for some broadcast packets), address comparison between the receiver's (device's) own address and the destination address extracted from the received packtet, is quite frequently executed. I simply don't want to execute dev->driver->get() each time when address comparison is needed.

What do you think? Should we not introduce this l2_address into netdev2, and use function instead?
If not, I will (close this PR and) come back to use dev->driver->get() each time when I need the device's own address.

Or, another solution maybe, still, put this l2_addres into a gnrc_mac_internal_states structure (contained in gnrc_netdev2) which specifies its usage range.

miri64 · 2016-11-07T13:46:08Z

What happens if the address (or the address length) is changed at user level?

Yes, it will be unwanted that the address is changed at other levels.

That is not ideal.

Who is responsible for making this changes?

I think it is the MAC layer (L2) only.

Then the MAC layer could theoretically change the l2addr, when ever one calls NETAPI_SET with the options that do change the address, right?

I introduce this, since, in some special MAC layer cases (e.g., destination address of a packet is not put in netif_headher, especially for some broadcast packets), address comparison between the receiver's (device's) own address and the destination address extracted from the received packtet, is quite frequently executed. I simply don't want to execute dev->driver->get() each time when address comparison is needed.

I was expecting something like this, that is why I did not ask "Why?" ;-)

What do you think? Should we not introduce this l2_address into netdev2, and use function instead?
If not, I will (close this PR and) come back to use dev->driver->get() each time when I need the device's own address.

No, I was thinking about something similar for the future, but I think NETAPI_SET should react to user behaviour.

zhuoshuguo · 2016-11-07T14:00:50Z

@miri64

Then the MAC layer could theoretically change the l2addr, when ever one calls NETAPI_SET with the options that do change the address, right?

Yes, I think so.

No, I was thinking about something similar for the future, but I think NETAPI_SET should react to user behaviour.

I think maybe I didn't get your point here, sorry... Do you mean, we should update the netdev2.l2_address when NETAPI_SET is called?

miri64 · 2016-11-07T14:03:39Z

I think maybe I didn't get your point here, sorry... Do you mean, we should update the netdev2.l2_address when NETAPI_SET is called?

yes.

zhuoshuguo · 2016-11-07T14:07:03Z

@miri64 , OK, so we still keep the l2_address in gnrc_netdev2_t?

miri64 · 2016-11-07T14:19:04Z

Yes.

zhuoshuguo · 2016-11-07T14:48:00Z

Updated.

zhuoshuguo · 2016-11-14T15:42:11Z

@miri64 Ping~ Apart from the "l2_address updating issue" , is there any other stuff keep this PR from getting merged? :-)

zhuoshuguo · 2016-11-14T15:56:04Z

By the way, do you think I sould specify (#if-NETDEV-IS-154-RADIO) the usage range of this l2_address? Or, maybe, I should not limit the address length to IEEE802154_LONG_ADDRESS_LEN, since netdev2 is not limited to 15.4 standard only?

zhuoshuguo · 2016-11-27T15:47:53Z

sys/include/net/gnrc/netdev2.h

+    /**
+     * @brief device's l2 address
+     */
+    uint8_t  l2_addr[IEEE802154_LONG_ADDRESS_LEN];


Just realized that, maybe what I want (for 15.4 device) in this PR is already here..

But, didn't understand that, for example, in netdev2_ieee802154_set, why we don't call dev->netdev.driver->set in netdev2_ieee802154_set-address to update the address at lower level at the same time?

@zhuoshuguo because an IEEE 802.15.4 device driver is supposed to call netdev2_ieee802154_set in its set method to reduce code duplication. What you propose would be a loop ;-).

@miri64 Thanks a lot!~ Just checked the function-calling-graph of gnrc_netdev2->dev->driver->get(). It is clear to me now that gnrc_netdev2->dev->driver->get() is doing the same thing of what I wanted to introduce in this PR (I previously thought that dev->driver->get() will go deep to read the registers of the radio which may cost too much overhead when I just want to do address comparison).

So, do you think I should close this PR? ;-) (simply continue to use gnrc_netdev2->dev->driver->get())

BTW, is there a way/clue to get/reach netdev2_ieee802154_t.long_addr and netdev2_ieee802154_t.short_addr ?

So, do you think I should close this PR? ;-) (simply continue to use gnrc_netdev2->dev->driver->get())

Maybe a good idea to store the address regardless, just to save some time and code.

BTW, is there a way/clue to get/reach netdev2_ieee802154_t.long_addr and netdev2_ieee802154_t.short_addr ?

Use dev->driver->get() with NETOPT_ADDRESS_LONG or NETOPT_ADDRESS. respectively

@miri64

Maybe a good idea to store the address regardless, just to save some time and code.

Really sorry for my English, but I am not sure I got the idea, so do you mean we still keep this PR?
which introduces uint8_t l2_addr[IEEE802154_LONG_ADDRESS_LEN] into gnrc_netdev2?

@miri64 Then, is this PR be OK for ACKed now?

zhuoshuguo · 2016-11-29T16:42:58Z

Ping~ ;-)

miri64

@miri64 Then, is this PR be OK for ACKed now?

Yes

miri64 self-assigned this Oct 14, 2016

miri64 added Area: network Area: Networking GNRC labels Oct 14, 2016

miri64 requested changes Oct 14, 2016

View reviewed changes

zhuoshuguo force-pushed the add_gnrc_mac_definition branch from 32f8139 to c86ad24 Compare October 14, 2016 09:20

This was referenced Oct 14, 2016

gnrc_mac: add timeout module. #5949

Merged

gnrc: Add priority packet queue module to gnrc #5950

Merged

miri64 requested changes Oct 20, 2016

View reviewed changes

zhuoshuguo force-pushed the add_gnrc_mac_definition branch 2 times, most recently from d8e8ffe to 4c81989 Compare October 28, 2016 13:24

zhuoshuguo commented Oct 28, 2016

View reviewed changes

zhuoshuguo changed the title ~~gnrc: add gnrc_mac definition~~ gnrc: add l2-address into gnrc_netdev2_t Oct 28, 2016

zhuoshuguo changed the title ~~gnrc: add l2-address into gnrc_netdev2_t~~ gnrc: add l2_address into gnrc_netdev2_t Oct 28, 2016

zhuoshuguo force-pushed the add_gnrc_mac_definition branch from 4c81989 to 36e7a0c Compare October 28, 2016 14:47

zhuoshuguo force-pushed the add_gnrc_mac_definition branch from 36e7a0c to ff353b6 Compare November 4, 2016 16:00

gnrc_mac: add l2 address into netdev2.

1a44c0f

zhuoshuguo force-pushed the add_gnrc_mac_definition branch from ff353b6 to 1a44c0f Compare November 7, 2016 09:34

miri64 reviewed Nov 7, 2016

View reviewed changes

miri64 requested changes Nov 7, 2016

View reviewed changes

gnrc_mac: fix includes.

70369d6

zhuoshuguo commented Nov 27, 2016

View reviewed changes

miri64 approved these changes Nov 29, 2016

View reviewed changes

miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Nov 29, 2016

miri64 merged commit 4bf9d59 into RIOT-OS:master Nov 30, 2016

gnrc: add l2_address into gnrc_netdev2_t #5942

gnrc: add l2_address into gnrc_netdev2_t #5942

Conversation

zhuoshuguo commented Oct 13, 2016 • edited

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

zhuoshuguo Oct 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Nov 7, 2016

zhuoshuguo commented Nov 7, 2016

zhuoshuguo commented Nov 7, 2016

zhuoshuguo commented Nov 7, 2016

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

miri64 commented Nov 7, 2016

zhuoshuguo commented Nov 7, 2016 • edited

miri64 commented Nov 7, 2016

zhuoshuguo commented Nov 7, 2016

miri64 commented Nov 7, 2016

zhuoshuguo commented Nov 7, 2016 • edited

miri64 commented Nov 7, 2016

zhuoshuguo commented Nov 7, 2016

zhuoshuguo commented Nov 14, 2016

zhuoshuguo commented Nov 14, 2016

zhuoshuguo Nov 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhuoshuguo Nov 27, 2016 • edited

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

zhuoshuguo commented Nov 29, 2016

miri64 left a comment

Choose a reason for hiding this comment

zhuoshuguo commented Oct 13, 2016 •

edited

zhuoshuguo Oct 28, 2016 •

edited

zhuoshuguo commented Nov 7, 2016 •

edited

zhuoshuguo commented Nov 7, 2016 •

edited

zhuoshuguo Nov 27, 2016 •

edited

zhuoshuguo Nov 27, 2016 •

edited