-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
5bf30ed
to
63f9573
Compare
Rebased and added #11263 as a dependency when |
63f9573
to
25fee2d
Compare
Rebased to current master and dependencies. |
Maybe this will be fixed less hacky with #12069. Until this is confirmed, I keep this open. |
25fee2d
to
c8c2df4
Compare
Rebased to current #11263 |
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)
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)
(cherry picked from commit c8c2df4, see RIOT-OS#11264)
c8c2df4
to
dead52c
Compare
Rebased to current master and dependencies |
dead52c
to
0656a72
Compare
Can this be closed now that #12728 is merged? |
If #11068 is able to perform without this PR, yes. Let me check this in the coming days. |
ping :) |
Sorry, still did not find the time to test this :(. Will see if I manage today. |
I tested with #11068 rebased to master and without Rebased to master:
In the current version:
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. |
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.
0656a72
to
d91e63c
Compare
Rebased to current master, mainly to reflect the current state. We should discuss in #14954 how to proceed with this in |
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) |
IMHO with the radio HAL now merged, it doesn't make much sense to keep this PR. Shout if you disagree. |
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 includegnrc_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