at86rf2xx: Don't use netdev_ieee802154_t for link layer address #10401

bergzand · 2018-11-15T13:43:58Z

Contribution description

This PR modifies the behaviour of the at86rf2xx driver to not propagate the link layer address to the netdev_ieee802154_t struct. This helps the goal of separating the netdev and the ieee802154_t layer by another bit. The end goal is to remove the short and long address from the netdev_ieee802154_t struct and match the behaviour of the ethernet drivers.

The performance impact of this change should be negligible, the NETOPT_ADDRESS(_LONG) call is only used on initialization and by the ifconfig command.

Testing procedure

Test whether:

ifconfig still shows the correct link layer addresses. These should not have changed between this PR and master.
ping still works. This to verify that the link layer address is also properly configured in the radio, if the ping echo/reply fails, the address filter of the radio might be configured in the wrong endianness.

Issues/PRs references

First part in a sequence to remove the link layer address from the netdev_ieee802154_t struct

This change modifies the at86rf2xx driver to read the address directly from the device instead of returning the netdev_ieee802154_t member

This adds the link layer address retrieval to the netdev::get function. The consequence is that the address is directly read from the device and no longer from the netdev_ieee802154_t layer when requesting the address.

miri64

I'm not sure about the pulling in of the byteorder dependency. I don't mind it, but other voices might disagree.

miri64 · 2018-11-15T13:52:17Z

drivers/at86rf2xx/at86rf2xx_getset.c

-    dev->netdev.short_addr[0] = (uint8_t)(addr);
-    dev->netdev.short_addr[1] = (uint8_t)(addr >> 8);
+    /* addr is in network byte order */
+    network_uint16_t ap;


What does ap stand for? in the original code here I could imagine it meaning "address pointer", but this is not a pointer anymore... :-/

Oops only wanted to comment

bergzand · 2018-11-15T14:01:51Z

I'm not sure about the pulling in of the byteorder dependency. I don't mind it, but other voices might disagree.

It can do without, the motivation for adding it here was to clarify the code a bit, at least to have it clear that the addresses here are already in big endian form.

miri64 · 2018-11-15T14:02:22Z

Question: How are netdev_ieee802154_t::short_addr and netdev_ieee802154_t::long_addr set now? I wasn't able to find anything on that in the code base.

miri64 · 2018-11-15T14:03:07Z

It can do without, the motivation for adding it here was to clarify the code a bit, at least to have it clear that the addresses here are already in big endian form.

Maybe put the introduction into a separate commit. That way we can easier test the size impact and also revert it if others don't like it.

bergzand · 2018-11-15T14:07:03Z

Question: How a netdev_ieee802154_t::short_addr and netdev_ieee802154_t::long_addr set now? I wasn't able to find anything on that in the code base.

Do you mean after merging this PR or in current master?

In current master they are set in the set_addr functions of the drivers and with the netdev_driver::set call. The at86rf2xx sets the return value to -ENOTSUP to propagate the value to the netdev_ieee802154::set` function.

WIth this PR it is no longer set, but also no longer read. Instead it is directly read from the device driver. This way the behaviour matches the behaviour for ethernet drivers.

miri64 · 2018-11-15T14:17:30Z

WIth this PR it is no longer set, but also no longer read. Instead it is directly read from the device driver. This way the behaviour matches the behaviour for ethernet drivers.

Aren't they read/set by the netdev_ieee802154 "layer"? Or is this removed now somehow?

bergzand · 2018-11-15T14:38:22Z

Aren't they read/set by the netdev_ieee802154 "layer"? Or is this removed now somehow?

I can't remove them yet since other drivers are not yet adapted. But the idea is to remove the short_address and long_address from the netdev_ieee802154_t struct. This PR only modifies the at86rf2xx driver so that the addresses are no longer set and read from the netdev_ieee802154 layer.

miri64 · 2018-11-15T14:41:20Z

Thanks, that and #7736 (comment) clarify the trajectory of this for me.

bergzand · 2018-11-15T19:28:43Z

I'm not sure about the pulling in of the byteorder dependency. I don't mind it, but other voices might disagree.

Just remembered, byteorder.h is already used in drivers/at86rf2xx/at86rf2xx.c to convert the link layer address to big endian.

bergzand · 2018-11-16T16:40:31Z

I want to do a similar PR for the radio channel, I can imagine that waiting for that PR and testing them both at the same time can save some time :)

miri64 · 2018-12-04T11:08:23Z

We should wait merging this until #10537 is merged ;-)

miri64 · 2018-12-12T09:11:59Z

I believe tests/driver_at86rf2xx needs to be adapted for this change

miri64 · 2018-12-14T00:30:11Z

#10537 got merged into master. Please rebase.

miri64 · 2019-02-26T18:49:20Z

Ping @bergzand?

stale · 2019-08-30T19:18:05Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

stale · 2020-03-02T20:30:24Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

miri64 · 2020-06-24T13:57:55Z

I guess this needs to be re-thought with #13943 around the corner. Let's close it as archived?

bergzand added 3 commits November 15, 2018 14:34

at86rf2xx: read address from device

c436530

This change modifies the at86rf2xx driver to read the address directly from the device instead of returning the netdev_ieee802154_t member

at86rf2xx: Add NETOPT_ADDRESS{,_LONG} to getters

cb052ac

This adds the link layer address retrieval to the netdev::get function. The consequence is that the address is directly read from the device and no longer from the netdev_ieee802154_t layer when requesting the address.

at86rf2xx: Don't propagate address to 802154 layer

c986b46

bergzand added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers labels Nov 15, 2018

bergzand requested review from haukepetersen, miri64 and kaspar030 November 15, 2018 13:43

miri64 previously requested changes Nov 15, 2018

View reviewed changes

bergzand mentioned this pull request Nov 15, 2018

mrf24j40: Don't use netdev_ieee802154_t for link layer address #10402

Merged

This was referenced Nov 17, 2018

cc2538_rf: Don't use netdev_ieee802154_t for link layer address #10425

Merged

cc2538_rf: Don't use netdev_ieee802154_t for channel #10426

Merged

bergzand added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 4, 2018

miri64 mentioned this pull request Dec 6, 2018

netdev_ieee802154: fix IID getter #10537

Merged

miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Feb 26, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 30, 2019

miri64 removed the State: stale State: The issue / PR has no activity for >185 days label Aug 30, 2019

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2020

miri64 removed the State: stale State: The issue / PR has no activity for >185 days label Mar 3, 2020

miri64 closed this Jun 24, 2020

miri64 added the State: archived State: The PR has been archived for possible future re-adaptation label Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

at86rf2xx: Don't use netdev_ieee802154_t for link layer address #10401

at86rf2xx: Don't use netdev_ieee802154_t for link layer address #10401

bergzand commented Nov 15, 2018 •

edited

Loading

miri64 left a comment

miri64 Nov 15, 2018

bergzand commented Nov 15, 2018

miri64 commented Nov 15, 2018 •

edited

Loading

miri64 commented Nov 15, 2018

bergzand commented Nov 15, 2018

miri64 commented Nov 15, 2018

bergzand commented Nov 15, 2018

miri64 commented Nov 15, 2018

bergzand commented Nov 15, 2018

bergzand commented Nov 16, 2018

miri64 commented Dec 4, 2018

miri64 commented Dec 12, 2018

miri64 commented Dec 14, 2018

miri64 commented Feb 26, 2019

stale bot commented Aug 30, 2019

stale bot commented Mar 2, 2020

miri64 commented Jun 24, 2020

at86rf2xx: Don't use netdev_ieee802154_t for link layer address #10401

at86rf2xx: Don't use netdev_ieee802154_t for link layer address #10401

Conversation

bergzand commented Nov 15, 2018 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

miri64 left a comment

Choose a reason for hiding this comment

miri64 Nov 15, 2018

Choose a reason for hiding this comment

bergzand commented Nov 15, 2018

miri64 commented Nov 15, 2018 • edited Loading

miri64 commented Nov 15, 2018

bergzand commented Nov 15, 2018

miri64 commented Nov 15, 2018

bergzand commented Nov 15, 2018

miri64 commented Nov 15, 2018

bergzand commented Nov 15, 2018

bergzand commented Nov 16, 2018

miri64 commented Dec 4, 2018

miri64 commented Dec 12, 2018

miri64 commented Dec 14, 2018

miri64 commented Feb 26, 2019

stale bot commented Aug 30, 2019

stale bot commented Mar 2, 2020

miri64 commented Jun 24, 2020

bergzand commented Nov 15, 2018 •

edited

Loading

miri64 commented Nov 15, 2018 •

edited

Loading