at86rf2xx/Check at86rf2xx state before changing phy

[biboc]

at86rf2xx: Changing phy involve to save current transceiver state, set transceiver to OFF and set it back again to its previous state. This PR avoids to save transceiver state AT86RF2XX_STATE_IN_PROGRESS and wait that transceiver is in stable state

[thomaseichinger]

@biboc Could you please elaborate on what this change is actually fixing? Is there a bug associated with this?

Also please give this PR a descriptive title.

[biboc]

Yes when trying to change channel with OpenThread, at86rf2xx_configure_phy is called.
The driver saves current state, set state to AT86RF2XX_STATE_TRX_OFF, configure PHY and re-set latest state.
Problem is that we can't change at86rf2xx state if it is in AT86RF2XX_STATE_IN_PROGRESS state so with this PR we make sure transceiver is in the right state before changing it and also, we make sure that we do try to set the transceiver to AT86RF2XX_STATE_IN_PROGRESS at the end of at86rf2xx_configure_phy.
Without this fix, the driver loops forever.

[biboc]

@thomaseichinger, better?

[thomaseichinger]

I can't see how moving this block helps. at86rf2xx_configure_phy uses AT86RF2XX_STATE_TRX_OFF not AT86RF2XX_STATE_FORCE_TRX_OFF thus it shouldn't have an effect.

[thomaseichinger]

@biboc Thanks for changing the title.

Could you help me understand by telling me which loop the driver gets stuck in?

[DipSwitch]

Related #7115

…

On 19 Jun 2017 18:19, "biboc" ***@***.***> wrote: Yes when trying to change channel with OpenThread, at86rf2xx_configure_phy is called. The driver saves current state, set state to AT86RF2XX_STATE_TRX_OFF, configure PHY and re-set latest state. Problem is that we can't change at86rf2xx state if it is in AT86RF2XX_STATE_IN_PROGRESS state so with this PR we make sure transceiver is in the right state before changing it. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7207 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAdFRFpR2wsQ7HwVflK_zr1UBjc1R04xks5sFp-ogaJpZM4N-aKp> .

[thomaseichinger]

@biboc Could you please check if #7115 solves your problem?

[biboc]

AT86RF212B Datasheet Extract : "Do not try to initiate a further state change while the radio transceiver is in STATE_TRANSITION_IN_PROGRESS state. "

During a channel setting we use :
void at86rf2xx_set_chan(at86rf2xx_t *dev, uint8_t channel) :
which calls :
void at86rf2xx_configure_phy(at86rf2xx_t *dev)
which calls :
void at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state)
which calls :
static inline void _set_state(at86rf2xx_t *dev, uint8_t state, uint8_t cmd)
{
at86rf2xx_reg_write(dev, AT86RF2XX_REG__TRX_STATE, cmd);

/* To prevent a possible race condition when changing to
* RX_AACK_ON state the state doesn't get read back in that
* case. See discussion
* in #5244
*/
if (state != AT86RF2XX_STATE_RX_AACK_ON) {
while (at86rf2xx_get_status(dev) != state);
}

dev->state = state;
}
We get stuck in the while sometimes. Because in
void at86rf2xx_configure_phy(at86rf2xx_t *dev)
we store the current state, to restore it after phy configuration
but unfortunately in my case we save AT86RF2XX_STATE_IN_PROGRESS instead of a real state.
When this "state" is restored at the end of phy configuration, we get stuck in : static inline void _set_state(at86rf2xx_t dev, uint8_t state, uint8_t cmd)
Because the transceiver can't never reach AT86RF2XX_STATE_IN_PROGRESS state. In my case the transceiver only reach the AT86RF2XX_STATE_TRX_OFF state.
That's why before any state change we need to check that we are NOT in AT86RF2XX_STATE_IN_PROGRESS.
That's why I also inverted those two blocks to prevent any freeze:
/ make sure there is no ongoing transmission, or state transition already
* in progress */
while (old_state == AT86RF2XX_STATE_BUSY_RX_AACK ||
old_state == AT86RF2XX_STATE_BUSY_TX_ARET ||
old_state == AT86RF2XX_STATE_IN_PROGRESS) {
old_state = at86rf2xx_get_status(dev);
}

if (state == AT86RF2XX_STATE_FORCE_TRX_OFF) {
_set_state(dev, AT86RF2XX_STATE_TRX_OFF, state);
return;
}

Blocks inversion should not have any effect.
I agree with you if the transceiver has the same behavior, it should not be freeze because it will reach the AT86RF2XX_STATE_TRX_OFF state. According to the datasheet, it is better to prevention so no weird behaviour can happen.

#7115 answers to the second change (inverting the two blocks in get/set file) but not to my first problem "update phy".

Is it clearer @thomaseichinger ?

[thomaseichinger]

@biboc Thanks for your explanation.

I think however that #7115 actually also handles the first part of your problem because it waits for the module to be in a "reachable" state (no busy or transition state) and returns the state the hardware is in just after that.

The problem is two fold, (i) trying to return to an unreachable state and (ii) a race between getting the state and then setting it. (i) is handled by waiting and only return reachable states and (ii) should be handled by storing the state the module is in just after assuring (i).

With those changes we can actually remove some of the extra checks before calling at86rf2xx_configure_phy as you can see in DipSwitch#6.

Unless I am totally missing something, I'd appreciate if you could test #7115 (including DipSwitch#6) and see if that fixes your problem.

[biboc]

Merge DipSwitch#6 inside #7115 and I'll try

[biboc]

We've tested your branch @thomaseichinger, it works! Apart from a missing definition in a .h so I close this PR and let you merge the other PR with your changes