at86rf2xx: fix race conditions and PHY states representation#12069
Closed
jia200x wants to merge 5 commits intoRIOT-OS:masterfrom
Closed
at86rf2xx: fix race conditions and PHY states representation#12069jia200x wants to merge 5 commits intoRIOT-OS:masterfrom
jia200x wants to merge 5 commits intoRIOT-OS:masterfrom
Conversation
jia200x
added
Type: bug
jia200x
added
the
State: waiting for other PR
Member
Author
|
btw the PHY header is ready on RX_START. It's possible to read the packet length there (e.g cache the pkt size there so RX_END already know the size) |
jia200x
force-pushed
the
pr/at86rf2xx_phy_states
branch
from
0e97caf to
a8b911a
Compare
jia200x
force-pushed
the
pr/at86rf2xx_phy_states
branch
from
a8b911a to
c624117
Compare
This commit removes the SLEEP state and adds a dedicated function for turning the radio on and off. The motivation for removing the SLEEP state is to simplify the state machine of `at86rf2xx_set_state`
This change defines PHY states for the radio. Although the internal states differ between Basic Mode and Extended Mode operation, they have an equivalent in PHY states (e.g RX_AACK_ON is equivalent to RX_ON). With this it will be easier to implement Basic Mode support for this radio
Due to the design of netdev, it's more convenient to use static buffer protection instead of dynamic. As soon as the frame buffer is accessed, the SFD detection is disabled until the whole recv procedure finished (dropping or fetching from FB). This way it's not necessary to switch to PLL_ON mode on reception.
The current driver SPI polls the states of the transceiver. This doesn't prevent race conditions to happen, since these states are very volatile. With this change the PHY states of the transceiver are represented with the `dev->state` variable, which syncs to the device via interrupts. Now it's possible to safely check if the device is busy without chances of race conditions.
jia200x
force-pushed
the
pr/at86rf2xx_phy_states
branch
from
c624117 to
76c8d9d
Compare
Member
Author
|
Same as some other PRs, this driver has changed quite a lot. I will re-open if needed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution description
This PR summarizes in:
1. Define PHY states for the at86rf2xx.
Now the at86rf2xx_set_state function receives a PHY state instead of an internal AT86RF2XX representation.This simplifies the state machine and integration of Basic Mode:
E.g
at86rf2xx_set_state(dev, AT86RF2XX_PHY_RX)will internally set the transceiver to AT86RF2XX_STATE_RX_AACK_ON in Extended Mode but to AT86RF2XX_STATE_RX_ON in Basic Mode.2. Now it's possible to retrieve the TRX state with a variable, instead of polling via
at86rf2xx_get_status.This variable is synchronized with the transceiver via interrupts (e.g RX_START interrupt set
dev->state=AT86RF2XX_PHY_RX_BUSY. Then, the RX_DONE event sets the variable back to a non BUSY event).See how this simplifies the _isr function and how this prevents race conditions (send command is rejected if "dev->state" is in one of the BUSY states. No need to poll the device).
3. Move from Dynamic buffer protection to Static buffer protection:
Dynamic buffer protection is released on at86rf2xx_fb_stop. The current work around was to go set the transceiver state to PLL_ON before reading the frame buffer.
With static buffer protection the detection of SFD is disabled on RX_END, and re-enabled on packet read/drop.
This changes fix the issue described on #11256 (it makes @miri64 tests pass), so it's an alternative to #11264
Basic Mode support comes next :)
Discussion/RFC
In practice, the RX_START event tells the PHY layer that the SFD was detected. Thus, this event is mandatory for synchronizing the dev->state variable.
Also, the transmission (regardless of Basic or Extended mode) always ends with the TX_DONE event. So, RX_START and TRX_END must be mandatory if a PHY layer is present (so far, the
at86rf2xx_netdev.cis the PHY layer implementation of the driver).The question is, do we still want the TELL_RX_START, TELL_RX_DONE and TELL_TX_DONE configurations?
If yes, they MUST be activated in the at86rf2xx_netdev layer.
Testing procedure
Run #11256. Check the test passes (that means no "Unexpected payload. This test failed!").
Issues/PRs references
Fixes #11256
Alternative to #11264
Depends on #12062