driver/at86rf2xx: Fix possible race condition where at86rf2xx_configure returns to the wrong state if waited on finished state in at86rf2xx_setstate

[DipSwitch]

at86rf2xx_configure() could possibly return to the wrong state after updating the phy's configuration.

[thomaseichinger]

@DipSwitch Sorry, this one totally flew under my radar.

I still fail to see the relation between moving an unmatched check and the mentioned race condition. What was the wrong state you landed in?

[DipSwitch]

It would try to return to AT86RF2XX_STATE_BUSY_TX_ARET in my case.

[thomaseichinger]

Sorry, it took me a while. I think your change is good, the version below might still optimize the code size a bit.

uint8_t at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state)
{
    uint8_t old_state;

    /* make sure there is no ongoing transmission, or state transition already
     * in progress */
    do {
        old_state = at86rf2xx_get_status(dev);
    } while (old_state == AT86RF2XX_STATE_BUSY_RX_AACK ||
             old_state == AT86RF2XX_STATE_BUSY_TX_ARET ||
             old_state == AT86RF2XX_STATE_IN_PROGRESS)

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

    if (state == old_state) {
        return old_state;
    }

    /* we need to go via PLL_ON if we are moving between RX_AACK_ON <-> TX_ARET_ON */
    if ((old_state == AT86RF2XX_STATE_RX_AACK_ON &&
             state == AT86RF2XX_STATE_TX_ARET_ON) ||
        (old_state == AT86RF2XX_STATE_TX_ARET_ON &&
             state == AT86RF2XX_STATE_RX_AACK_ON)) {
        _set_state(dev, AT86RF2XX_STATE_PLL_ON, AT86RF2XX_STATE_PLL_ON);
    }
    /* check if we need to wake up from sleep mode */
    else if (old_state == AT86RF2XX_STATE_SLEEP) {
        DEBUG("at86rf2xx: waking up from sleep mode\n");
        at86rf2xx_assert_awake(dev);
    }

    if (state == AT86RF2XX_STATE_SLEEP) {
        /* First go to TRX_OFF */
        at86rf2xx_set_state(dev, AT86RF2XX_STATE_FORCE_TRX_OFF);
        /* Discard all IRQ flags, framebuffer is lost anyway */
        at86rf2xx_reg_read(dev, AT86RF2XX_REG__IRQ_STATUS);
        /* Go to SLEEP mode from TRX_OFF */
        gpio_set(dev->params.sleep_pin);
        dev->state = state;
    } else {
        _set_state(dev, state, state);
    }

    return old_state;
}

[DipSwitch]

Will update the pr tomorrow.

[thomaseichinger]

Awesome, thanks!

I also saw that at least at86rf2xx_tx_prepare and at86rf2xx_reset_state_machine can be simplified then too.

[aabadie]

Just activated the CI. Can we have a positive review (ACK) and merge this one for the release ? It will also be useful for #7149.

[biboc]

@DipSwitch, can you merge @thomaseichinger PR and let's merge this PR since we've tested it and it works
I'll give my ACK as soon as thomas's PR is merged

[aabadie]

Murdock is complaining for some variable scope reassingment reason. Can you fix this @DipSwitch ?

[aabadie]

@thomaseichinger @DipSwitch I allowed myself to directly push the requested change proposed above and fix Murdock reported issue.

Let me know if that fine for you so we can move forward here and here. Thanks !

[biboc]

@aabadie, Did you check that what you pushed is same as the PR @thomaseichinger suggested? (the on we ACK from my side)
DipSwitch#6

[aabadie]

Did you check that what you pushed is same as the PR @thomaseichinger suggested? (the on we ACK from my side)

Well, I missed it, sorry. Need to compare and will fix asap.

[aabadie]

@biboc, should be fine now.

[miri64]

Does this fix #5486 by any chance?

[thomaseichinger]

@aabadie Could you please also change the commit message?
(Since when can you push to other people's branches?)

[aabadie]

Could you please also change the commit message?

Done

Since when can you push to other people's branches?

Every maintainer of RIOT is able to do that under certain conditions. I don't recommend this practice but sometimes it helps.

[aabadie]

ping @thomaseichinger

[biboc]

ACK for me, wait @thomaseichinger before merging

[aabadie]

when this on is merged, I'll be able to test the OT PR on IoT-LAB tomorrow afternoon.

[thomaseichinger]

Code seems good to me. ACK

@aabadie I think we should do some more thorough release testing for the at86rf2xx though.

[miri64]

Does this fix #5486 by any chance?

Nope :(