[discussion] provide unified radio platform APIs for all radio drivers to process/generate Header IEs

[zhanglongxia]

The ACK frame is generated by the radio driver, and some of the Header IEs of the data frame are updated by the radio driver. These part of code are implemented by radio drivers. Each chip vendor may have a different implementation. This way of generating ACK and handling Header IEs causes some problems：

<1> Once a Header IE needs to be added or updated, a new radio platform API needs to be added or updated.
<2> Requires chip vendors to take extra effort to implement new added or updated radio platform APIs.
<3> As time goes by, there will be more and more radio platform APIs.

---

My proposal is to provide unified platform radio APIs for all radio driver to generate/process Header IEs and encrypt frames.

typedef struct otTxIeInfo {
  int16_t mSfdEndTimeOffset;   ///< The time offset between calling `otPlatRadioTxStartedAtEndSfd()` and the end time
                           ///<  of sending the SFD, in unit of microsecond.
} otTxIeInfo;

/*
 * Updates Header IEs and encrypt farme when the SFD of the frame is sent out.
 *
 * This method should be called when the SFD of the frame is sent out. If the radio driver can't call this function
 *  at the exact time when the SFD is sent out due to hardware limitations, the radio driver should set
 * `otTxFrameInfo.mSfdEndTimeOffset` to a appropriate value.
 *
 * This method will change the `IE present` bit of the FCF, update Header IEs and encrypt the frame.
 * 
 * Note: If this method is executed from an interrupt context, the radio driver MUST implement
 * @fun otPlatRadioCriticalRegionEnter() and @fun otPlatRadioCriticalRegionExit() to gurantee this method to access
 * shared resources safely.
*/
extern otError otPlatRadioTxStartedAtEndSfd(otInstance *aInstance, otRadioFrame *aTxFrame, otTxIeInfo *aTxIeInfo);

/**
 * Represents the info that is used to set the Header IE of the ACK frame.
 *
 */
typedef struct otAckInfo {
  bool mRssiValid:1;  ///< Indicates wether the mLinkMetricsRssi is valid.
  bool mLqiValid:1; ///< Indicates wether the mLinkMetricsLqi is valid.

  int8_t mLinkMetricsRssi;  ///< Received Signal Strength Indicator.
  int8_t mLinkMetricsLqi;    ///<  Link Quality Indicator.
  int16_t mSfdEndTimeOffset;   ///< The time offset between calling `otPlatRadioTxAckStartedAtEndSfd()` and the end time
                           ///<  of sending the SFD, in unit of microsecond.
} otAckIeInfo;

/*
 * Processes the ACK frames when the SFD of the ACK is sent out.
 *
 * This method should be called when the SFD of the ACK frame is sent out. If the radio driver can't call this function
 * at the exact time when the SFD is sent out due to hardware limitations, the radio driver should set
 * `otAckInfo.mSfdEndTimeOffset` to a appropriate value.
 *
 * The radio driver should prepare the FCF, sequence number, addressing filed and security header before calling this
 * method. This method will change the `IE present` bit of the FCF,  add Header IEs and encrypt the ACK frame.
 * 
 * Note: If this method is executed from an interrupt context, the radio driver MUST implement
 * @fun otPlatRadioCriticalRegionEnter() and @fun otPlatRadioCriticalRegionExit() to gurantee this method to access
 * shared resources safely.
*/
extern otError otPlatRadioTxAckStartedAtEndSfd(otInstance *aInstance, otRadioFrame *aAckFrame, otAckIeInfo *aAckIeInfo);

/*
 * Enters the critical region.
 */
void otPlatRadioCriticalRegionEnter(otInstance *aInstance);

/*
 * Exits the critical region.
*/
void otPlatRadioCriticalRegionExit(otInstance *aInstance);

If we implement above methods, chip vendors do not need to implement the following APIs anymore. And we do not need to add new radio platform APIs to process Header IEs. Vendors automatically have new features after synchronizing the code.

otPlatRadioEnableCsl()
otPlatRadioResetCsl()
otPlatRadioUpdateCslSampleTime()
otPlatRadioSetMacKey()
otPlatRadioSetMacFrameCounter()
otPlatRadioSetMacFrameCounterIfLarger()
otPlatRadioConfigureEnhAckProbing()

[zhanglongxia]

@jwhui @abtink @bukepo @Irving-cl Any comment for this proposal?

[bukepo]

Sounds great!

• To prevent from misusage of otInstance in the ISR context, we probably should add something like otRadioContext which maintains all the context needed in the ISR context, and we provide otRadioContext instead of otInstance for otPlatRadioTxStartedAtEndSfd.
• Instead of passing otAckIeInfo to otPlatRadioTxAckStartedAtEndSfd, perhaps we should pass the whole RxFrame which already captures the needed info except the SFD time offset.
• It seems the link metrics are not time sensitive, and there may be other similar IEs, so perhaps we can have an API like otPlatRadioPrepareAckFrame(otRadioContext *aRadioContext, otRadioFrame *aAckFrame, const otRadioFrame *aRxFrame) to update IEs before SFD interrupt. Thus, we can save the API otPlatRadioTxAckStartedAtEndSfd().

[zhanglongxia]

@bukepo
<1> Agree with you that we should not pass the otInstance to the function otPlatRadioTxStartedAtEndSfd.
<2> The RxFrame includes parts of info that is needed by the otPlatRadioTxAckStartedAtEndSfd(). If we set the RxFrame to the parameter of otPlatRadioTxAckStartedAtEndSfd(), it is difficult for developers to know which part of information will be used by the function. And we still need to add otAckIeInfo if the info (such as the mSfdEndTimeOffset) is not included in the RxFrame.
<3> Some platforms have their private methods to generate the ACK header. Adding a new API otPlatRadioPrepareAckFrame() requires more changes on the radio driver. It is not easy for radio drivers to switch to this method.

[abtink]

Thanks @zhanglongxia,

I like the idea of providing more elaborate utility functions (that do more complex actions) to help with radio driver implementation. But I'm not sure if we want to introduce these as otPlatRadio APIs.

Regarding extern otError otPlatRadioTxStartedAtEndSfd(otInstance *aInstance, otRadioFrame *aTxFrame, otTxIeInfo *aTxIeInfo), it could be placed under a new otRadioUtil helper function (similar to otMac utility functions) which implements common actions to help radio platform.

My main concern is that we should not use otInstance and not allow such a function to access any core modules or data.

If it is a standalone function that does some processing on aTxFrame itself, it should be fine. But if it needs to access any OpenThread internal data or core modules, it will not work as OpenThread code is not re-entrant safe.

Regarding

* Note: If this method is executed from an interrupt context, the radio driver MUST implement
* @fun otPlatRadioCriticalRegionEnter() and @fun otPlatRadioCriticalRegionExit() to guarantee this method to access
* shared resources safely.

I am not in favor of introducing functions like CriticalRegionEnter(). My view is that these are really platform-specific functions, and we should leave it to the radio driver implementation to decide how it wants to protect its aTxFrame, ... parameters when it passes them to OpenThread utility functions.

We can document that certain helper functions can be called from different OS contexts, and whether or not they are re-entrant safe. If they are not re-entrant safe, we can suggest in the documentation that the platform should protect against this. Each radio driver implementation can then handle this however it chooses, using locks, critical regions, etc.

[zhanglongxia]

@abtink The reason I do not use the otRadioUtil helper function is that the function otPlatRadioTxStartedAtEndSfd() needs to access some shared resources in SubMac (such as mCslSampleTime, mPrevKey and so on). Another reason is that helper functions are optional, vendors may choose not to use them. In the future, we may add new features without adding new radio APIs, which actually requires vendors to must use this function.

I agree with you that we should not use otInstance in the function otPlatRadioTxStartedAtEndSfd. Just like @bukepo suggested, we can define a structure otRadioContext to represent the shared resources (the resource could be accessed in ISR). When OT changes these shared resources, it should protect them.

The question here is how to protect these shared resources. In my proposal, we need the radio driver to provide the function otPlatRadioCriticalRegionEnter and otPlatRadioCriticalRegionExit to protect shared resources. The name otPlatRadioCriticalRegionXXX may be not good, we can change them to otPlatRadioConextChangeStart and otPlatRadioConextChangeStop. Radio drivers could implement these methods via using lock/unlock, CriticalRegionsEnter/CriticalRegionsExit, etc.

/* Represents the radio context */
typedef struct otRadioContext {
 uint32_t       mMacFrameCounter;
 uint8_t          mKeyId;
 otMacKeyMaterial  mPrevKey;
 otMacKeyMaterial  mCurrKey;
 otMacKeyMaterial   mNextKey;
 uint16_t          mCslPeriod;
 uint32_t         mCslSampleTime;
 otShortAddress      mCslShortAddress;
 otExtAddress       mCslExtAddress
...
} otRadioContext;

/*
 * Updates Header IEs and encrypt farme when the SFD of the frame is sent out.
 *
 * This method should be called when the SFD of the frame is sent out. If the radio driver can't call this function
 *  at the exact time when the SFD is sent out due to hardware limitations, the radio driver should set
 * `otTxFrameInfo.mSfdEndTimeOffset` to a appropriate value.
 *
 * This method will change the `IE present` bit of the FCF, update Header IEs and encrypt the frame.
 * 
 * Note: If this method is executed from an interrupt context, the radio driver MUST implement
 * @fun otPlatRadioConextChangeStart() and @fun otPlatRadioConextChangeStop() to gurantee this method to access
 * @p aRadioContext safely.
*/
extern otError otPlatRadioTxStartedAtEndSfd(otRadioFrame *aTxFrame, otTxIeInfo *aTxIeInfo, otRadioContext *aRadioContext);

/*
 * Processes the ACK frames when the SFD of the ACK is sent out.
 *
 * This method should be called when the SFD of the ACK frame is sent out. If the radio driver can't call this function
 * at the exact time when the SFD is sent out due to hardware limitations, the radio driver should set
 * `otAckInfo.mSfdEndTimeOffset` to a appropriate value.
 *
 * The radio driver should prepare the FCF, sequence number, addressing filed and security header before calling this
 * method. This method will change the `IE present` bit of the FCF,  add Header IEs and encrypt the ACK frame.
 * 
 * Note: If this method is executed from an interrupt context, the radio driver MUST implement
 * @fun otPlatRadioConextChangeStart() and @fun otPlatRadioConextChangeStop() to gurantee this method to access
 * @p aRadioContext safely.
*/
extern otError otPlatRadioTxAckStartedAtEndSfd(otRadioFrame *aAckFrame, otAckIeInfo *aAckIeInfo, otRadioContext *aRadioContext);

/*
 * Notifies the radio driver that the radio context is start to be changed.
 *
 * The radio context will be accessed by `otPlatRadioTxStartedAtEndSfd` and `otPlatRadioTxAckStartedAtEndSfd`
 * from an interrupt context. The radio driver should gurantee that it won't call these two functions from an
 * interrupt context after this method is called.
 *  
 */
void otPlatRadioConextChangeStart(otInstance *aInstance);

/*
 * Notifies the radio driver that changing the radio context has finished.
 *
 * The radio driver is able to call  `otPlatRadioTxStartedAtEndSfd` and `otPlatRadioTxAckStartedAtEndSfd` from an
 * interrupt context after this method is called.
 *  
 */
void otPlatRadioConextChangeStop(otInstance *aInstance);

Here is the example of how we use the otPlatRadioConextChangeStart and otPlatRadioConextChangeStop in SubMac.

void SubMac::SetMacKey(uint8_t            aKeyIdMode,
                       uint8_t            aKeyId,
                       const KeyMaterial &aPrevKey,
                       const KeyMaterial &aCurrKey,
                       const KeyMaterial &aNextKey)
{
   RadioConextChangeStart();
   mRadioContext.mKeyMode   = aKeyMode;
   mRadioContext.mKeyId   = aKeyId;
   mRadioContext.mPrevKey = aPrevKey;
   mRadioContext.mCurrKey = aCurrKey;
   mRadioContext.mNextKey = aNextKey;
   RadioConextChangeExit()
}

How do you think about this option? We limit the radio driver to protect only the radio context, and platforms have their own options for how to protect the radio context.

[Damian-Nordic]

@zhanglongxia
I am not sure if I understand the statement: "And we do not need to add new radio platform APIs to process Header IEs". The proposed new APIs may work in some architectures but in others like the RCP where the radio is running on a separate device or core, there will be no time to fetch OpenThread state (like CSL) lazily from the otPlatRadioTxStartedAtEndSfd()/otPlatRadioTxAckStartedAtEndSfd() callbacks. In such architectures, OpenThread has to still proactively communicate to the radio all the parameters needed to build the header IEs. Would you agree?

Thus, I suspect the proposal won't remove the need to add new platform APIs, but these APIs might have default implementations (weak or enabled with a build-time configuation) that would assume the otPlatRadioTxStartedAtEndSfd() and otPlatRadioTxAckStartedAtEndSfd() callbacks invoked at the TX start. Is my understanding correct?

[zhanglongxia]

@Damian-Nordic And we do not need to add new radio platform APIs to process Header IEs means the radio driver do not need to implement new radio driver APIs to process the Header IEs. The Header IEs will be processed in SubMac by calling otPlatRadioTxStartedAtEndSfd or otPlatRadioTxAckStartedAtEndSfd. In the RCP mode, the otPlatRadioTxStartedAtEndSfd and otPlatRadioTxAckStartedAtEndSfd won't be implemented on host, they are radio driver dedicated functions. All the radio parameters have already been stored in SubMac before calling the otPlatRadioTxStartedAtEndSfd or otPlatRadioTxAckStartedAtEndSfd.

You are correct, we still need to add new radio platform APIs to handle the RCP mode. But in this case, the newly added radio platform API should be only used to handle the communication between Host and RCP, and the radio driver do not need to implement them. We can optimize this type of radio platform APIs later. If we are using a single chip solution, there is no need to add new radio platform APIs.

[Damian-Nordic]

I would argue that we still have to preserve existing APIs, such as otPlatRadioEnableCsl not just to handle the RCP mode but also to support the SoC architecture using more advanced SoCs, where certain radio operations may be delegated to another core that doesn't run any OpenThread code, or certain TX tasks are implemented with the hardware support.

That's why I wanted to have this stated clearly in the proposal that the goal is not to deprecate any existing APIs but to provide the default implementation that may accelerate the adoption of CSL and similar features. :)