Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

at86rf2xx: fix receive before send detection #12728

Merged
merged 2 commits into from
Feb 7, 2020

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented Nov 16, 2019

Contribution description

The at86rf2xx radio handled a transfer complete condition with the radio in the BUSY_TX_ARET state as a finished transmission. This condition and state also occurs when a reception occurs just before switching to transmitting. This would cause a condition where first a TX_COMPLETE was
signalled and second a RX_COMPLETE was signalled. The network stack would then read the transmitted frame as a received frame.

The patch fixes the errornous RX callback by only submitting the TX_COMPLETE condition when there are at least 2 frames pending (at86rf2xx::pending_tx).

This should not impact packet loss with the radio. The frame received is also discarded without this patch.

The change set is not that big, it just appears big due to splitting out the tx complete handling from the _isr() call. Edit: splitted the tx complete refactor to a separate commit to ease reviewing.

Testing procedure

The test as supplied with #11256 must pass. examples/gnrc_networking should work as usual on a samr21-xpro or on the iotlab-m3,

Issues/PRs references

Fixes the issue described in #11256

@bergzand bergzand added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking Area: drivers Area: Device drivers labels Nov 16, 2019
@bergzand bergzand force-pushed the pr/at86rf2xx/fix_recv_before_send branch from ecd9334 to ce56dc8 Compare November 16, 2019 21:29
@jia200x
Copy link
Member

jia200x commented Nov 18, 2019

This is also fixed in #12069 , but I'm fine to go with this approach since #12069 will make more sense with the netif rework. I will give it a deeper look

@bergzand
Copy link
Member Author

I will give it a deeper look

Thanks!

@bergzand
Copy link
Member Author

bergzand commented Jan 6, 2020

ping @jia200x

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 5, 2020
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

The code looks good and I found no regressions.
#11256 is running fine - so let's go forward with this!

@benpicco
Copy link
Contributor

benpicco commented Feb 5, 2020

Murdock/clang are not happy about excessive parenthesis - you can squash directly.

The at86rf2xx radio handled a transfer complete condition with the radio
in the BUSY_TX_ARET state as a finished transmission. This condition and
state also occurs when a reception occurs just before switching to
transmitting. This would cause a condition where first a TX_COMPLETE was
signalled and second a RX_COMPLETE was signalled. The network stack
would then read the transmitted frame as a received frame.

The patch fixes the errornous RX callback by only submitting the
TX_COMPLETE condition when there are at least 2 frames pending
(at86rf2xx::pending_tx).
@bergzand bergzand force-pushed the pr/at86rf2xx/fix_recv_before_send branch from c4ca33c to 42d5460 Compare February 7, 2020 12:08
@benpicco benpicco merged commit 16ab89b into RIOT-OS:master Feb 7, 2020
@bergzand bergzand deleted the pr/at86rf2xx/fix_recv_before_send branch February 7, 2020 12:55
@bergzand
Copy link
Member Author

bergzand commented Feb 7, 2020

Thanks @benpicco!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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.

4 participants