review changes

Author: wqx6

src/app/clusters/thread-border-router-management-server/thread-border-router-management-server.cpp

@@ -95,13 +95,9 @@ Status ServerInstance::HandleSetActiveDatasetRequest(bool failSafeArmed,
         return Status::InvalidInState;
     }
 
-    mBreadcrumb    = req.breadcrumb;
-    mRandomNumber  = Crypto::GetRandU32();
-    CHIP_ERROR err = mDelegate->SetActiveDataset(activeDataset, mRandomNumber, this);
-    if (err != CHIP_NO_ERROR)
-    {
-        mRandomNumber = 0;
-    }
+    mBreadcrumb = req.breadcrumb;
+    mSetActiveDatasetSequenceNumber++;
+    CHIP_ERROR err = mDelegate->SetActiveDataset(activeDataset, mSetActiveDatasetSequenceNumber, this);
     return StatusIB(err).mStatus;
 }
 
@@ -161,7 +157,8 @@ void ServerInstance::InvokeCommand(HandlerContext & ctxt)
                 Status status = HandleSetActiveDatasetRequest(IsFailSafeArmed(ctx.mCommandHandler.GetAccessingFabricIndex()), req);
                 if (status != Status::Success)
                 {
-                    OnActivateDatasetComplete(mRandomNumber, ChipError(ChipError::SdkPart::kIMGlobalStatus, to_underlying(status)));
+                    OnActivateDatasetComplete(mSetActiveDatasetSequenceNumber,
+                                              ChipError(ChipError::SdkPart::kIMGlobalStatus, to_underlying(status)));
                 }
             }
             else
@@ -317,20 +314,19 @@ void ServerInstance::CommitSavedBreadcrumb()
     mBreadcrumb.ClearValue();
 }
 
-void ServerInstance::OnActivateDatasetComplete(uint32_t randomNumber, CHIP_ERROR error)
+void ServerInstance::OnActivateDatasetComplete(uint32_t sequenceNum, CHIP_ERROR error)
 {
     auto commandHandleRef = std::move(mAsyncCommandHandle);
     auto commandHandle    = commandHandleRef.Get();
     if (commandHandle == nullptr)
     {
         return;
     }
-    if (mRandomNumber != randomNumber)
+    if (mSetActiveDatasetSequenceNumber != sequenceNum)
     {
         // Previous SetActiveDatasetRequest was handled.
         return;
     }
-    mRandomNumber = 0;
     if (error == CHIP_NO_ERROR)
     {
         // The successful completion of the activation process SHALL disarm the fail-safe timer.
@@ -356,8 +352,6 @@ void ServerInstance::OnFailSafeTimerExpired()
     {
         return;
     }
-    // Reset the RandomNumeber so that the OnActivateDatasetComplete will not handle the previous SetActiveDatasetRequest command.
-    mRandomNumber = 0;
     commandHandle->AddStatus(mPath, Status::Timeout);
 }
 

src/app/clusters/thread-border-router-management-server/thread-border-router-management-server.h

@@ -53,7 +53,7 @@ class ServerInstance : public CommandHandlerInterface, public AttributeAccessInt
     CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;
 
     // ActivateDatasetCallbackInterface
-    void OnActivateDatasetComplete(uint32_t randomNum, CHIP_ERROR error) override;
+    void OnActivateDatasetComplete(uint32_t sequenceNum, CHIP_ERROR error) override;
 
 private:
     friend class TestThreadBorderRouterManagementCluster;
@@ -86,7 +86,7 @@ class ServerInstance : public CommandHandlerInterface, public AttributeAccessInt
     app::CommandHandler::Handle mAsyncCommandHandle;
     ConcreteCommandPath mPath = ConcreteCommandPath(0, 0, 0);
     Optional<uint64_t> mBreadcrumb;
-    uint32_t mRandomNumber;
+    uint32_t mSetActiveDatasetSequenceNumber = 0;
 };
 
 bool IsFailSafeArmed(FabricIndex accessingFabricIndex);

src/app/clusters/thread-border-router-management-server/thread-br-delegate.h

@@ -48,7 +48,7 @@ class Delegate
         // If the dataset is set successfully, OnActivateDatasetComplete should be called with CHIP_NO_ERROR when the
         // Border Router is attached to the Thread network.
         // If an error occurs while setting the active dataset, this callback should be called with the error.
-        virtual void OnActivateDatasetComplete(uint32_t randomNumber, CHIP_ERROR error) = 0;
+        virtual void OnActivateDatasetComplete(uint32_t sequenceNum, CHIP_ERROR error) = 0;
     };
 
     enum class DatasetType : uint8_t
@@ -72,14 +72,16 @@ class Delegate
     virtual CHIP_ERROR GetDataset(Thread::OperationalDataset & dataset, DatasetType type) = 0;
 
     // The Delegate implementation should back up the old active dataset at the beginning of function if no backup already exists.
-    // The Delegate implementation should store the random number and pass it to OnActivateDatasetComplete.
-    virtual CHIP_ERROR SetActiveDataset(const Thread::OperationalDataset & activeDataset, uint32_t randomNum,
+    // The Delegate implementation must store the sequence number and pass it to OnActivateDatasetComplete.
+    virtual CHIP_ERROR SetActiveDataset(const Thread::OperationalDataset & activeDataset, uint32_t sequenceNum,
                                         ActivateDatasetCallback * callback) = 0;
 
     // The Delegate implementation should remove the backup active dataset in this function.
     virtual CHIP_ERROR CommitActiveDataset() = 0;
 
-    // The Delegate implementation should restore the backup active dataset in this function.
+    // The Delegate implementation should restore the backup active dataset in this function. The delegate is allowed to call
+    // OnActivateDatasetComplete for the previous SetActiveDataset request even after this function is called as the sequence
+    // number passed to OnActivateDatasetComplete will be different.
     virtual CHIP_ERROR RevertActiveDataset() = 0;
 
     virtual CHIP_ERROR SetPendingDataset(const Thread::OperationalDataset & pendingDataset) = 0;

src/platform/OpenThread/GenericThreadBorderRouterDelegate.cpp

@@ -119,7 +119,7 @@ CHIP_ERROR GenericOpenThreadBorderRouterDelegate::GetDataset(Thread::Operational
 }
 
 CHIP_ERROR GenericOpenThreadBorderRouterDelegate::SetActiveDataset(const Thread::OperationalDataset & activeDataset,
-                                                                   uint32_t randomNumber, ActivateDatasetCallback * callback)
+                                                                   uint32_t sequenceNum, ActivateDatasetCallback * callback)
 {
     CHIP_ERROR err = BackupActiveDataset();
     if (err == CHIP_NO_ERROR)
@@ -128,8 +128,10 @@ CHIP_ERROR GenericOpenThreadBorderRouterDelegate::SetActiveDataset(const Thread:
     }
     if (err == CHIP_NO_ERROR)
     {
-        mRandomNumber = randomNumber;
-        mCallback     = callback;
+        // We can change the stored sequence number because OnActivateDatasetComplete executed later must be called for this
+        // SetActiveDataset request
+        mSequenceNum = sequenceNum;
+        mCallback    = callback;
     }
     return err;
 }
@@ -142,7 +144,7 @@ void GenericOpenThreadBorderRouterDelegate::OnPlatformEventHandler(const DeviceL
         if (event->Type == DeviceLayer::DeviceEventType::kThreadConnectivityChange &&
             event->ThreadConnectivityChange.Result == DeviceLayer::kConnectivity_Established)
         {
-            delegate->mCallback->OnActivateDatasetComplete(delegate->mRandomNumber, CHIP_NO_ERROR);
+            delegate->mCallback->OnActivateDatasetComplete(delegate->mSequenceNum, CHIP_NO_ERROR);
             // Delete Failsafe Keys after activating dataset is completed
             DeviceLayer::PersistedStorage::KeyValueStoreMgr().Delete(kFailsafeThreadDatasetTlvsKey);
             delegate->mCallback = nullptr;

src/platform/OpenThread/GenericThreadBorderRouterDelegate.h

@@ -67,7 +67,7 @@ class GenericOpenThreadBorderRouterDelegate : public Delegate
 
     CHIP_ERROR GetDataset(Thread::OperationalDataset & dataset, DatasetType type) override;
 
-    CHIP_ERROR SetActiveDataset(const Thread::OperationalDataset & activeDataset, uint32_t randomNumber,
+    CHIP_ERROR SetActiveDataset(const Thread::OperationalDataset & activeDataset, uint32_t sequenceNum,
                                 ActivateDatasetCallback * callback) override;
 
     CHIP_ERROR RevertActiveDataset() override;
@@ -82,7 +82,7 @@ class GenericOpenThreadBorderRouterDelegate : public Delegate
     CHIP_ERROR BackupActiveDataset();
     ActivateDatasetCallback * mCallback        = nullptr;
     Thread::OperationalDataset mStagingDataset = {};
-    uint32_t mRandomNumber;
+    uint32_t mSequenceNum;
     char mThreadBorderRouterName[kBorderRouterNameMaxLength + 1];
 };
 } // namespace ThreadBorderRouterManagement