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: do not try to send when device is busy #11264

Closed
wants to merge 3 commits into from

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Mar 25, 2019

Contribution description

Otherwise, a race condition might be triggered where data that was just
received (which is the reason the device might be busy) is overridden
by the sent data (see #11256).

Testing procedure

Run the tests in #11256 together with #11263 [the test doesn't include gnrc_netif so merging #11263 would be pointless] (I did not do this myself yet, this is why I marked it as WIP).

Issues/PRs references

This prevents the race condition show-cased in #11256.
Together with #11263 the sent packets are tried to be sent later instead of just being dropped.

Depends on #11263

Basis for #11068

Route to 6Lo minimal fragment forwarding

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: drivers Area: Device drivers labels Mar 25, 2019
@miri64
Copy link
Member Author

miri64 commented Jul 12, 2019

Rebased and added #11263 as a dependency when gnrc_netif is also included.

@miri64
Copy link
Member Author

miri64 commented Aug 30, 2019

Rebased to current master and dependencies.

@miri64
Copy link
Member Author

miri64 commented Sep 2, 2019

Maybe this will be fixed less hacky with #12069. Until this is confirmed, I keep this open.

@miri64
Copy link
Member Author

miri64 commented Sep 25, 2019

Rebased to current #11263

miri64 added a commit to 5G-I3/RIOT-public that referenced this pull request Sep 25, 2019
Otherwise, a race condition might be triggered where data that was just
received (which is the reason the device might be busy) is overridden
by the sent data.

(cherry picked from commit a7d26f1,
see RIOT-OS#11264)
miri64 added a commit to 5G-I3/RIOT-public that referenced this pull request Sep 25, 2019
Otherwise, there can happen a state transition between the busy check
and the setting do TX_ARET_ON.

(cherry picked from commit 41ee023,
see RIOT-OS#11264)
miri64 added a commit to 5G-I3/RIOT-public that referenced this pull request Sep 25, 2019
@miri64
Copy link
Member Author

miri64 commented Nov 11, 2019

Rebased to current master and dependencies

@miri64
Copy link
Member Author

miri64 commented Jan 21, 2020

Rebased to current master and dependencies to resolve #11263's conflict with #13065.

@benpicco
Copy link
Contributor

Can this be closed now that #12728 is merged?

@miri64
Copy link
Member Author

miri64 commented Feb 19, 2020

If #11068 is able to perform without this PR, yes. Let me check this in the coming days.

@fjmolinas
Copy link
Contributor

If #11068 is able to perform without this PR, yes. Let me check this in the coming days.

ping :)

@miri64
Copy link
Member Author

miri64 commented Apr 14, 2020

Sorry, still did not find the time to test this :(. Will see if I manage today.

@miri64
Copy link
Member Author

miri64 commented Apr 14, 2020

I tested with #11068 rebased to master and without gnrc_netif_pktq and also tested as it is now. I used ping6 -s 120 -i 750 -c 4800 beef::1 over static routes with a single hop.

Rebased to master:

2020-04-14 12:02:40,266 # --- beef::1 PING statistics ---
2020-04-14 12:02:40,272 # 4800 packets transmitted, 423 packets received, 91% packet loss
2020-04-14 12:02:40,276 # round-trip min/avg/max = 84.702/111.848/142.180 ms

In the current version:

2020-04-14 13:10:52,562 # --- beef::1 PING statistics ---
2020-04-14 13:10:52,567 # 4800 packets transmitted, 1265 packets received, 73% packet loss
2020-04-14 13:10:52,572 # round-trip min/avg/max = 75.163/111.254/153.087 ms

So yes, in some form or another, this PR is still needed. However, as I said multiple times before: this should be made configurable and based on a MAC protocol on top. As it is now it basically just requires a minimal MAC queue on top, which might not work for all devices.

miri64 and others added 3 commits September 23, 2020 11:51
Otherwise, a race condition might be triggered where data that was just
received (which is the reason the device might be busy) is overridden
by the sent data.
Otherwise, there can happen a state transition between the busy check
and the setting do TX_ARET_ON.
@miri64
Copy link
Member Author

miri64 commented Sep 23, 2020

Rebased to current master, mainly to reflect the current state. We should discuss in #14954 how to proceed with this in netdev. I will adapt this PR then accordingly.

@jia200x
Copy link
Member

jia200x commented Sep 23, 2020

I think there's still a case of race condition: If the user calls the send function while receiving and at the same moment RX_BUSY goes to RX, the send function will pass through and we will see the same issue :/

I think the proper way to do it would be to either save the IRQ request somewhere or use features like the Frame Buffer Empty Indicator (section 11.7 of datasheet) or the SPI_CMD_MODE to read the IRQ_STATUS register (section 6.3.1)

@miri64
Copy link
Member Author

miri64 commented Oct 28, 2020

IMHO with the radio HAL now merged, it doesn't make much sense to keep this PR. Shout if you disagree.

@miri64 miri64 closed this Oct 28, 2020
@miri64 miri64 deleted the at86rf2xx/enh/busy-ret branch October 28, 2020 14:50
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 State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet 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