drivers/netdev_ieee802154: cleanup NETOPT_MAX_PACKET_SIZE #9864

SemjonWilke · 2018-08-30T10:10:22Z

This PR relieves code duplications in netdev interfaces and unifies them in the shared, generic ieee802154 netdev driver.
I focused on NETOPT_MAX_PACKET_SIZE by intent, if there are more obvious duplications, I will fix those separately.

SemjonWilke · 2018-09-20T10:09:19Z

hey @bergzand are you still looking into this?

bergzand · 2018-09-20T10:24:36Z

hey @bergzand are you still looking into this?

Thanks for the ping :)

I'm missing socket_zep (cpu/native/socket_zep) in the list of changed drivers. Changes look good otherwise. Thanks for cleaning up.

bergzand · 2018-09-20T10:27:09Z

drivers/netdev_ieee802154/netdev_ieee802154.c

@@ -151,6 +151,13 @@ int netdev_ieee802154_get(netdev_ieee802154_t *dev, netopt_t opt, void *value,
            res = sizeof(l2filter_t **);
            break;
 #endif
+        case NETOPT_MAX_PACKET_SIZE:
+            assert(max_len >= sizeof(int16_t));
+            *((uint16_t *)val) = (IEEE802154_FRAME_LEN_MAX -


/home/hydrazine/dev/RIOT/drivers/netdev_ieee802154/netdev_ieee802154.c:156:27: error: 'val' undeclared (first use in this function)

I guess this should be value instead ;)

Oh oh. I fixed this right after PR, but i forgot to push it, sorry.

No worries.

In the future please use a fixup commit instead of force pushing. Using fixup commits makes it easy for the reviewer to track changes between revisions and they can be automatically squashed before merging.

You are right. This was meant to go into the PR and i just missed the push, this was allready fixed before and i just pushed it now without touching it, thats why i commented it. Sorry for the inconvenience :)

It's fine, luckily for me this PR is not that big :)

SemjonWilke · 2018-09-20T13:54:24Z

@bergzand good to sqash?

cpu/native/socket_zep/socket_zep.c

PeterKietzmann · 2018-09-25T16:23:22Z

@bergzand did you start testing? I could support a bit now

PeterKietzmann · 2018-09-25T17:08:58Z

To track some ping6 based tests that I'm gonna provide next.

A: ping6 1000 <link local IPv6 addr> 0 0 0
B: ping6 1000 <link local IPv6 addr> 150 0 0

at86rf2xx - kw2xrf
w/o PR
A: 3% loss
B: 13% loss
w/ PR
A: 4% loss
B: 15% loss
mrf24j40 - cc2538
w/o PR
A: 7% loss
B: not working
w/ PR
A: 6% loss
B: not working
at86rf233 - cc2420
w/o PR, w/ cpu/msp430: increase default stacksize for gnrc_ipv6 thread #10043
A: 1 % loss
~~B: % loss~~
w/ PR, w/ cpu/msp430: increase default stacksize for gnrc_ipv6 thread #10043
A: 1% loss
~~B: % loss~~

bergzand · 2018-09-25T17:25:13Z

@PeterKietzmann Thanks for taking over with the testing! I'm mostly curious whether all radios still use the same MAX_PACKET_SIZE when transmitting

SemjonWilke · 2018-09-26T06:55:43Z

Oh, you can proceed testing, but maybe i can get rid of some more defines. On first glance it seems that I may also delete KW2XRF_MAX_PKT_LENGTH, CC2420_PKT_MAXLEN, AT86RF2XX_MAX_PKT_LENGTH, CC2538_RF_MAX_DATA_LEN. I'll have to get into this later today.

PeterKietzmann · 2018-09-26T06:59:03Z

Then please add the WIP label next time!!! I will stop testing now. Faced other problems when putting my hands on the cc2420.

SemjonWilke · 2018-09-26T10:15:49Z

Then please add the WIP label next time!!! I will stop testing now. Faced other problems when putting my hands on the cc2420.

@PeterKietzmann Can you please specify the problems?
EDIT: Or was your problem unrelated to this PR?
This PR was not supposed to be WIP, opposing to that I expect it to work properly.

Also it was not obvious to me that the named defines are redundant because, even though they are just the same as IEEE802154_FRAME_LEN_MAX, they all are used and hence substituting them did not appeal to me in the first place.
When i thought about it earlier today, I had another view on the code and changed my mind.
If this is a concern for anyone i'd prefer to provide another cleanup PR for that so this will not interfere with anything presented here.

PeterKietzmann · 2018-09-26T11:13:28Z

Unrelated to your changes, see #10043

bergzand · 2018-10-13T18:04:46Z

@PeterKietzmann Am I correct when I conclude that this PR doesn't break/impact connectivity and packet loss? To prevent this PR from getting stuck, how about we merge this and assume that @SemjonKerner is going to provide a cleanup in the near future to remove the rest of the obsolete defines.

@SemjonKerner Does this make sense? Is this okay with you?

SemjonWilke · 2018-10-13T18:12:45Z

@bergzand I don't expect this PR to be the reason for the observed problems with packet loss, and i think the tests suggest the same. So yes, this makes sense to me.

bergzand · 2018-10-15T09:54:02Z

@SemjonKerner Murdock is pointing out a few build errors with socket_zep

SemjonWilke · 2018-10-15T11:06:03Z

@bergzand fixed. Just a missing typecast.

PeterKietzmann · 2018-10-15T13:50:17Z

Oh, you can proceed testing, but maybe i can get rid of some more defines

This let me conclude you were about to continue working on this PR. The problems I pointed out don't relate to this PR. As one can see, results w/ and w/o are "the same". I won't block this PR.

bergzand · 2018-10-15T13:52:21Z

This let me conclude you were about to continue working on this PR. The problems I pointed out don't relate to this PR. As one can see, results w/ and w/o are "the same". I won't block this PR.

Thanks, and especially thanks for the thorough test you did on this!

@SemjonKerner Please squash

bergzand · 2018-10-16T12:58:59Z

Thanks for the cleanup @SemjonKerner!

bergzand self-requested a review August 30, 2018 10:18

bergzand added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels Aug 30, 2018

bergzand requested changes Sep 20, 2018

View reviewed changes

bergzand requested changes Sep 21, 2018

View reviewed changes

cpu/native/socket_zep/socket_zep.c Outdated Show resolved Hide resolved

PeterKietzmann added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Sep 26, 2018

PeterKietzmann mentioned this pull request Sep 26, 2018

drivers: interoperability of 802.15.4 radios RIOT-OS/Tutorials#53

Closed

bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Oct 15, 2018

Semjon Kerner added 2 commits October 16, 2018 13:27

drivers/netdev_ieee802154: support NETOPT_MAX_PKT_SIZE

2728065

drivers/netdev_ieee802154: drop NETOPT_MAX_PKT_SIZE

5b0152f

bergzand approved these changes Oct 16, 2018

View reviewed changes

bergzand merged commit 9a3e9ee into RIOT-OS:master Oct 16, 2018

SemjonWilke deleted the fix_netdev_maxpacketsize branch October 16, 2018 13:37

PeterKietzmann mentioned this pull request Oct 22, 2018

drivers: interoperability of 802.15.4 radios RIOT-OS/Release-Specs#74

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/netdev_ieee802154: cleanup NETOPT_MAX_PACKET_SIZE #9864

drivers/netdev_ieee802154: cleanup NETOPT_MAX_PACKET_SIZE #9864

SemjonWilke commented Aug 30, 2018

SemjonWilke commented Sep 20, 2018

bergzand commented Sep 20, 2018

bergzand Sep 20, 2018

SemjonWilke Sep 20, 2018

bergzand Sep 20, 2018

SemjonWilke Sep 20, 2018

bergzand Sep 20, 2018

SemjonWilke commented Sep 20, 2018 •

edited

Loading

PeterKietzmann commented Sep 25, 2018

PeterKietzmann commented Sep 25, 2018 •

edited

Loading

bergzand commented Sep 25, 2018

SemjonWilke commented Sep 26, 2018

PeterKietzmann commented Sep 26, 2018

SemjonWilke commented Sep 26, 2018 •

edited

Loading

PeterKietzmann commented Sep 26, 2018

bergzand commented Oct 13, 2018

SemjonWilke commented Oct 13, 2018

bergzand commented Oct 15, 2018

SemjonWilke commented Oct 15, 2018

PeterKietzmann commented Oct 15, 2018

bergzand commented Oct 15, 2018

bergzand commented Oct 16, 2018

drivers/netdev_ieee802154: cleanup NETOPT_MAX_PACKET_SIZE #9864

drivers/netdev_ieee802154: cleanup NETOPT_MAX_PACKET_SIZE #9864

Conversation

SemjonWilke commented Aug 30, 2018

SemjonWilke commented Sep 20, 2018

bergzand commented Sep 20, 2018

bergzand Sep 20, 2018

Choose a reason for hiding this comment

SemjonWilke Sep 20, 2018

Choose a reason for hiding this comment

bergzand Sep 20, 2018

Choose a reason for hiding this comment

SemjonWilke Sep 20, 2018

Choose a reason for hiding this comment

bergzand Sep 20, 2018

Choose a reason for hiding this comment

SemjonWilke commented Sep 20, 2018 • edited Loading

PeterKietzmann commented Sep 25, 2018

PeterKietzmann commented Sep 25, 2018 • edited Loading

bergzand commented Sep 25, 2018

SemjonWilke commented Sep 26, 2018

PeterKietzmann commented Sep 26, 2018

SemjonWilke commented Sep 26, 2018 • edited Loading

PeterKietzmann commented Sep 26, 2018

bergzand commented Oct 13, 2018

SemjonWilke commented Oct 13, 2018

bergzand commented Oct 15, 2018

SemjonWilke commented Oct 15, 2018

PeterKietzmann commented Oct 15, 2018

bergzand commented Oct 15, 2018

bergzand commented Oct 16, 2018

SemjonWilke commented Sep 20, 2018 •

edited

Loading

PeterKietzmann commented Sep 25, 2018 •

edited

Loading

SemjonWilke commented Sep 26, 2018 •

edited

Loading