at86rf2xx: remove SLEEP from phy states #12062
Conversation
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`
3f0fad0 to
23eae5d
Compare
| /* temporarily wake up if sleeping */ | ||
| if (old_state == AT86RF2XX_STATE_SLEEP) { | ||
| at86rf2xx_assert_awake(dev); | ||
| if (dev->flags & AT86RF2XX_OPT_SLEEP) { |
There was a problem hiding this comment.
Did you verify that dev->flags is not been changed elsewhere?
There was a problem hiding this comment.
@jia200x thanks for your contribution! Although I think that I understood the main idea, there are some points on which I would need clarification.
the device will do a TX->SLEEP transition. With this change, if the device was sleeping it will end in TRX_OFF.
- Is this really the case or do we go back to
AT86RF2XX_STATE_RX_AACK_ONas initialized in the setup function? If we don't, then how comes we are always able to receive frames by default? - I understand that the intention is not to handle sleep state from within the device driver but if we actually switch to
TRX_OFFinstead ofSLEEPwithout a higher layer control, this would increase current consumption by means for existing applications. Would this break anything for you @roberthartung or @MichelRottleuthner?
- Try to send data with txtsnd. The device should be able to send data and go back to STANDBY state (TRX_OFF)
So the assumption is, that a device returns to its previous state after sending. If the previous state was TRX_OFF, then we will return to that state again after sending and we won't be able to receive afterwards. Is that correct? If so, do we have any kind of documentation for that behavior?
(@jia200x I know not all questions directly relate to your changes but if we could sort out some misunderstandings here, we should do so :-))
| @@ -418,8 +418,8 @@ static int _get(netdev_t *netdev, netopt_t opt, void *val, size_t max_len) | |||
| } | |||
|
|
|||
| /* go back to sleep if were sleeping */ | |||
| if (old_state == AT86RF2XX_STATE_SLEEP) { | |||
| at86rf2xx_set_state(dev, AT86RF2XX_STATE_SLEEP); | |||
| if (dev->flags & AT86RF2XX_OPT_SLEEP) { | |||
There was a problem hiding this comment.
Did you verify that dev->flags is not been changed elsewhere?
| @@ -222,7 +222,7 @@ static int _set_state(at86rf2xx_t *dev, netopt_state_t state) | |||
| at86rf2xx_set_state(dev, AT86RF2XX_STATE_TRX_OFF); | |||
| break; | |||
| case NETOPT_STATE_SLEEP: | |||
| at86rf2xx_set_state(dev, AT86RF2XX_STATE_SLEEP); | |||
There was a problem hiding this comment.
Do we even need AT86RF2XX_STATE_SLEEP anymore?
|
@PeterKietzmann I will give it a second look to try to leave it with the original behavior |
|
Ping? |
Yes, this is the root. |
|
I will close this one for now, the driver has changed quite a lot. I will re open it if needed |
Contribution description
This commit changes the representation of the SLEEP state in the
at86rf2xxdriver.I'm trying to make a distinction between PHY states (RX_ON, TRX_OFF, TX_ON/PLL_ON) and hardware states (P_ON, RESET, SLEEP). This will simplify a lot the state handling of radios, and also add some mechanism to prevent race conditions when changing between RX, BUSY_RX, TX and BUSY_TX (I will open a PR soon that show this in action).
The first step then is to remove the SLEEP state from
at86rf2xx_set_stateand add some dedicated functions for turning the radio on and off (inspired in the Linux at86rf2xx driver).The API of netdev is the same (NETOPT_STATE set with NETOPT_STATE_SLEEP sets the radio to sleep), but the behavior of the driver changes a bit because of the
dev->idle_statevariable. In current master, it's possible to send data while the device is sleeping, and then the device will do a TX->SLEEP transition. With this change, if the device was sleeping it will end in TRX_OFF.In any case, the
idle_statelogic shouldn't be in the driver (e.g in TSCH networks the MAC layer needs to set the state of the radio without cooperation from the driver. Same applies for LoRaWAN). So I hope it's OK ;)Testing procedure
Test it with the
defaultexample:ifconfig 4 set state SLEEP)txtsnd. The device should be able to send data and go back to STANDBY state (TRX_OFF)ifconfig 4 set state IDLE).It would be possible to verify that the device is actually sleeping measuring the power consumption of the node (I know this is possible with not so much efforts on IoT-LAB)
Issues/PRs references
Related to #11483