Skip to content

at86rf2xx: Re-enable ack requests by default#10250

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/at86rf2xx/reenable_ack_req
Oct 25, 2018
Merged

at86rf2xx: Re-enable ack requests by default#10250
miri64 merged 1 commit intoRIOT-OS:masterfrom
bergzand:pr/at86rf2xx/reenable_ack_req

Conversation

@bergzand
Copy link
Copy Markdown
Member

Contribution description

Properly splitting the flags between netdev_ieee802154 and the at86rf2xx
driver had as side effect that the ACK REQ flag was no longer set by
default. This commit reverts the default settings by enabling it again
on startup. The current code calls the generic netdev setter to set the
flag.

Testing procedure

Test if ack requests are set on ieee802.15.4 frames for the at86rf2xx driver. Setting up gnrc_networking and pinging a unicast (link local) address and sniffing the frames is probably the easiest way to test this.

Issues/PRs references

#9957

@bergzand bergzand added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers labels Oct 25, 2018
Properly splitting the flags between netdev_ieee802154 and the at86rf2xx
driver had as side effect that the ACK REQ flag was no longer set by
default. This commit reverts the default settings by enabling it again
on startup. The current code calls the generic netdev setter to set the
flag.
@bergzand bergzand force-pushed the pr/at86rf2xx/reenable_ack_req branch from b63e954 to c3da55b Compare October 25, 2018 10:17
@miri64 miri64 added this to the Release 2018.10 milestone Oct 25, 2018
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Oct 25, 2018
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Tested on samr21-xpro and iotlab-m3 and confirmed that

a) the receiving node sends ACKs
b) fragmented ping6 yields better results than on master.


static const netopt_enable_t enable = NETOPT_ENABLE;
netdev_ieee802154_set(&dev->netdev, NETOPT_ACK_REQ,
&enable, sizeof(enable));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add a comment on top what this does and why it is required (and if this can be reverted (?) in the future).

@miri64 miri64 added the Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines label Oct 25, 2018
@miri64 miri64 merged commit bd06f7e into RIOT-OS:master Oct 25, 2018
@bergzand
Copy link
Copy Markdown
Member Author

Thanks for the quick review! Do you still want me to add a comment in a new PR? :)

@bergzand bergzand deleted the pr/at86rf2xx/reenable_ack_req branch October 25, 2018 12:10
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 25, 2018

If you want an extra PR for that ^^" totally forgot my own remark when I hit the friendly green button. 😄

@miri64 miri64 added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Oct 25, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Oct 25, 2018

(it was non-blocking anyway, so no big deal)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants