Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the radio state machine and timing logic in ot-rfsim/src/radio.c to improve delayed sleep handling, state transitions, and channel changes. Feedback highlights a critical issue where bypassing the startup transition during an ongoing Ack transmission allows an instantaneous channel change, violating physical timing constraints. It is recommended to check for pending channel changes when exiting the IFS wait state to correctly trigger the startup ramp-up delay.
| bool isAckOnAir = (sSubState == RFSIM_RADIO_SUBSTATE_RX_ACK_TX_ONGOING); | ||
|
|
||
| if (!isAckOnAir && ((sState == OT_RADIO_STATE_SLEEP && sSubState != RFSIM_RADIO_SUBSTATE_STARTUP) || | ||
| aChannel != sCurrentChannel)) | ||
| { | ||
| // Going from sleep to receive, or a channel change, incurs the ramp-up time. | ||
| setRadioSubState(RFSIM_RADIO_SUBSTATE_STARTUP, RFSIM_RAMPUP_TIME_US); | ||
| } |
There was a problem hiding this comment.
When isAckOnAir is true, the transition to RFSIM_RADIO_SUBSTATE_STARTUP is bypassed to let the ongoing Ack transmission finish. However, sCurrentChannel is still immediately updated to aChannel. When the Ack transmission completes, the substate machine transitions through RX_TX_TO_RX -> IFS_WAIT -> READY and updates sOngoingOperationChannel to sCurrentChannel without ever incurring the RFSIM_RAMPUP_TIME_US ramp-up delay. This allows an instantaneous channel change, bypassing the physical timing constraints of the radio model.\n\nTo fix this, we should check if a channel change is pending when exiting RFSIM_RADIO_SUBSTATE_IFS_WAIT in platformRadioProcess(). If sCurrentChannel != sOngoingOperationChannel, we should transition to RFSIM_RADIO_SUBSTATE_STARTUP instead of RFSIM_RADIO_SUBSTATE_READY.
There was a problem hiding this comment.
The intention of the current code is that the substates RX_TX_TO_RX and IFS_WAIT together are a waiting period in which the radio transitions back to Rx; and the radio does that now on its newly scheduled/selected channel. So physically it's ok per current code: the radio takes 2*sTurnAroundTime and it could do this even faster, in 1 * sTurnAroundTime. The rampup time is equal to sTurnAroundTime.
One option is to document this intended behavior; if so I'll hear it here :)
There was a problem hiding this comment.
(Meanwhile code is replaced - new name for the boolean)
|
The failed CI test is |
When `otPlatRadioSleep()` is called while the radio is receiving an incoming frame, waiting in AIFS, or actively transmitting an auto-ACK, `ot-rfsim` schedules a delayed transition to sleep (`sDelaySleep = true`) which takes effect once the ongoing frame/ACK operation completes. Previously, `otPlatRadioSleep()` returned `OT_ERROR_BUSY` in these substates which would be incorrect. The radio driver must accept the sleep request and transition to sleep after finishing the current atomic operation without returning an error. This commit updates `otPlatRadioSleep()` to return `OT_ERROR_NONE` when scheduling delayed sleep, indicating that the sleep request has been successfully accepted.
8a570bd to
735c62c
Compare
The delayed sleep scheduled by `otPlatRadioSleep()` was not always
correctly applied. Also the channel information was not always
correctly managed by the RFSIM radio, e.g. reporting incorrect Rx
channel to the stack.
This commit fixes these by calling `applyRadioDelayedSleep()` from more
code paths and adds a companion function `applyPendingChannelChange()`
to switch the channel, and apply radio ramp-up, as soon as an ongoing
Rx operation is done.
All radio platform functions are now aligned to comply with
the clarified `otPlatRadio...()` API contract of OT PR #13580.
Delayed sleep related changes:
- `platformRadioTxDone()` now applies a pending sleep once the Ack Tx
is done.
- the `RX_FRAME_ONGOING` timer path in `platformRadioProcess()`
applies a pending sleep too. That path is not only a failsafe: the
simulator sends RxDone just to the frame's addressee, so it is the
normal path for any frame that is overheard by a neighboring node.
- `platformRadioRxDone()` calls `radioReceive()` now before the sub-
state transitions, so that `otPlatRadioReceiveDone()` is invoked
before the radio sleeps, as the API requires. Previously the sleep
state was applied first, after which `radioReceive()` returned
early and the received frame was silently dropped.
API contract related changes:
- `otPlatRadioSleep()` returns `OT_ERROR_BUSY` while transmitting.
- `otPlatRadioDisable()` accepts a pending transition to sleep and
enacts it; `OT_ERROR_INVALID_STATE` now means that the radio is
neither in Sleep state nor transitioning to Sleep state.
- `otPlatRadioReceive()` returns `OT_ERROR_INVALID_STATE` while
transmitting, and models the radio ramp-up time when the receive
channel is changed. An ongoing Rx operation (tested with
`isRxOperationOngoing()` now) is now not aborted, which complies
to the new API.
- `otPlatRadioEnergyScan()` cancels a pending transition to sleep.
Radio state machine:
- `setRadioState(OT_RADIO_STATE_DISABLED)` resets the substate to
Ready instead of Startup; the startup time is applied by
`otPlatRadioEnable()`.
- during ramp-up (STARTUP) of the radio `sOngoingOperationChannel`
is already set to the channel to which it is ramping up.
This fixes a bug that sometimes the node's radio stays registered
on its previous channel in the simulator, causing it to miss
frames.
Channel handling:
- `radioReceive()` tags a received frame with the channel it was
received on (`sOngoingOperationChannel`); previously
`otPlatRadioReceive()` set the requested channel, which could then
mistakenly be used for transmitting the final Ack.
- `otPlatRadioReceive()` models the ramp-up time on a channel change.
During an ongoing Rx operation this change is deferred until that
operation is done, so the node stays registered on the Rx channel in
the simulator and the Ack is still sent.
- `sOngoingOperationChannel` is set when ramp-up starts and again when
it ends. Previously it was only updated in `platformRadioProcess()`,
which runs after the radio state is reported to the simulator, so a
node could sleep while still registered on its previous channel
(from OTNS viewpoint) and miss frames.
735c62c to
ab6bd5f
Compare
Two commits:
[rfsim] fix delayed sleep and channel handling; align with OT radio API
[rfsim] return OT_ERROR_NONE on delayed sleep in
otPlatRadioSleep