[ICD] Add Keep active during MDNS resolution ICD Check-In Sender (#30552

)

* Fix 30492

* apply comments

* fix last comments

src/app/FailSafeContext.cpp

@@ -56,9 +56,7 @@ void FailSafeContext::SetFailSafeArmed(bool armed)
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
     if (IsFailSafeArmed() != armed)
     {
-        ICDListener::KeepActiveFlags activeRequest = ICDListener::KeepActiveFlags::kFailSafeArmed;
-        armed ? ICDNotifier::GetInstance().BroadcastActiveRequestNotification(activeRequest)
-              : ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(activeRequest);
+        ICDNotifier::GetInstance().BroadcastActiveRequest(ICDListener::KeepActiveFlag::kFailSafeArmed, armed);
     }
 #endif
     mFailSafeArmed = armed;

src/app/icd/BUILD.gn

@@ -55,6 +55,7 @@ source_set("sender") {
 
   public_deps = [
     ":cluster",
+    ":notifier",
     "${chip_root}/src/credentials:credentials",
     "${chip_root}/src/lib/address_resolve:address_resolve",
     "${chip_root}/src/protocols/secure_channel",

src/app/icd/ICDCheckInSender.cpp

@@ -17,6 +17,8 @@
 
 #include "ICDCheckInSender.h"
 
+#include "ICDNotifier.h"
+
 #include <system/SystemPacketBuffer.h>
 
 #include <protocols/secure_channel/CheckinMessage.h>
@@ -37,15 +39,17 @@ ICDCheckInSender::ICDCheckInSender(Messaging::ExchangeManager * exchangeManager)
 
 void ICDCheckInSender::OnNodeAddressResolved(const PeerId & peerId, const AddressResolve::ResolveResult & result)
 {
-    mResolveInProgress = false;
+    if (CHIP_NO_ERROR != SendCheckInMsg(result.address))
+    {
+        ChipLogError(AppServer, "Failed to send the ICD Check-In message");
+    }
 
-    VerifyOrReturn(CHIP_NO_ERROR != SendCheckInMsg(result.address),
-                   ChipLogError(AppServer, "Failed to send the ICD Check-In message"));
+    ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress);
 }
 
 void ICDCheckInSender::OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP_ERROR reason)
 {
-    mResolveInProgress = false;
+    ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress);
     ChipLogProgress(AppServer, "Node Address resolution failed for ICD Check-In with Node ID " ChipLogFormatX64,
                     ChipLogValueX64(peerId.GetNodeId()));
 }
@@ -55,12 +59,13 @@ CHIP_ERROR ICDCheckInSender::SendCheckInMsg(const Transport::PeerAddress & addr)
     System::PacketBufferHandle buffer = MessagePacketBuffer::New(CheckinMessage::sMinPayloadSize);
 
     VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_NO_MEMORY);
-    MutableByteSpan output{ buffer->Start(), buffer->DataLength() };
+    MutableByteSpan output{ buffer->Start(), buffer->MaxDataLength() };
 
     // TODO retrieve Check-in counter
     CounterType counter = 0;
 
     ReturnErrorOnFailure(CheckinMessage::GenerateCheckinMessagePayload(mKey, counter, ByteSpan(), output));
+    buffer->SetDataLength(static_cast<uint16_t>(output.size()));
 
     VerifyOrReturnError(mExchangeManager->GetSessionManager() != nullptr, CHIP_ERROR_INTERNAL);
 
@@ -88,13 +93,11 @@ CHIP_ERROR ICDCheckInSender::RequestResolve(ICDMonitoringEntry & entry, FabricTa
     memcpy(mKey.AsMutable<Crypto::Aes128KeyByteArray>(), entry.key.As<Crypto::Aes128KeyByteArray>(),
            sizeof(Crypto::Aes128KeyByteArray));
 
-    // TODO #30492
-    // Device must stay active during MDNS resolution
     CHIP_ERROR err = AddressResolve::Resolver::Instance().LookupNode(request, mAddressLookupHandle);
 
     if (err == CHIP_NO_ERROR)
     {
-        mResolveInProgress = true;
+        ICDNotifier::GetInstance().BroadcastActiveRequestNotification(ICDListener::KeepActiveFlag::kCheckInInProgress);
     }
 
     return err;

src/app/icd/ICDManager.cpp

@@ -40,9 +40,8 @@ using namespace chip::app;
 using namespace chip::app::Clusters;
 using namespace chip::app::Clusters::IcdManagement;
 
-uint8_t ICDManager::OpenExchangeContextCount = 0;
 static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS,
-              "ICDManager::OpenExchangeContextCount cannot hold count for the max exchange count");
+              "ICDManager::mOpenExchangeContextCount cannot hold count for the max exchange count");
 
 void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeystore)
 {
@@ -98,7 +97,6 @@ bool ICDManager::SupportsFeature(Feature feature)
     bool success        = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS);
     return success ? ((featureMap & to_underlying(feature)) != 0) : false;
 #else
-
     return ((mFeatureMap & to_underlying(feature)) != 0);
 #endif // !CONFIG_BUILD_FOR_HOST_UNIT_TEST
 }
@@ -171,7 +169,7 @@ void ICDManager::UpdateOperationState(OperationalState state)
         CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(slowPollInterval);
         if (err != CHIP_NO_ERROR)
         {
-            ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format());
+            ChipLogError(AppServer, "Failed to set Slow Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format());
         }
     }
     else if (state == OperationalState::ActiveMode)
@@ -201,7 +199,7 @@ void ICDManager::UpdateOperationState(OperationalState state)
             CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(GetFastPollingInterval());
             if (err != CHIP_NO_ERROR)
             {
-                ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format());
+                ChipLogError(AppServer, "Failed to set Fast Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format());
             }
 
             postObserverEvent(ObserverEventType::EnterActiveMode);
@@ -274,44 +272,70 @@ void ICDManager::OnKeepActiveRequest(KeepActiveFlags request)
 {
     assertChipStackLockedByCurrentThread();
 
-    if (request == KeepActiveFlags::kExchangeContextOpen)
+    VerifyOrReturn(request < KeepActiveFlagsValues::kInvalidFlag);
+
+    if (request.Has(KeepActiveFlag::kExchangeContextOpen))
     {
         // There can be multiple open exchange contexts at the same time.
         // Keep track of the requests count.
-        this->OpenExchangeContextCount++;
-        this->SetKeepActiveModeRequirements(request, true /* state */);
+        this->mOpenExchangeContextCount++;
     }
-    else /* !kExchangeContextOpen */
+
+    if (request.Has(KeepActiveFlag::kCheckInInProgress))
     {
-        // Only 1 request per type (kCommissioningWindowOpen, kFailSafeArmed)
-        // set requirement directly
-        this->SetKeepActiveModeRequirements(request, true /* state */);
+        // There can be multiple check-in at the same time.
+        // Keep track of the requests count.
+        this->mCheckInRequestCount++;
     }
+
+    this->SetKeepActiveModeRequirements(request, true /* state */);
 }
 
 void ICDManager::OnActiveRequestWithdrawal(KeepActiveFlags request)
 {
     assertChipStackLockedByCurrentThread();
 
-    if (request == KeepActiveFlags::kExchangeContextOpen)
+    VerifyOrReturn(request < KeepActiveFlagsValues::kInvalidFlag);
+
+    if (request.Has(KeepActiveFlag::kExchangeContextOpen))
     {
         // There can be multiple open exchange contexts at the same time.
         // Keep track of the requests count.
-        if (this->OpenExchangeContextCount > 0)
+        if (this->mOpenExchangeContextCount > 0)
         {
-            this->OpenExchangeContextCount--;
+            this->mOpenExchangeContextCount--;
         }
         else
         {
             ChipLogError(DeviceLayer, "The ICD Manager did not account for ExchangeContext closure");
         }
 
-        if (this->OpenExchangeContextCount == 0)
+        if (this->mOpenExchangeContextCount == 0)
         {
-            this->SetKeepActiveModeRequirements(request, false /* state */);
+            this->SetKeepActiveModeRequirements(KeepActiveFlag::kExchangeContextOpen, false /* state */);
         }
     }
-    else /* !kExchangeContextOpen */
+
+    if (request.Has(KeepActiveFlag::kCheckInInProgress))
+    {
+        // There can be multiple open exchange contexts at the same time.
+        // Keep track of the requests count.
+        if (this->mCheckInRequestCount > 0)
+        {
+            this->mCheckInRequestCount--;
+        }
+        else
+        {
+            ChipLogError(DeviceLayer, "The ICD Manager did not account for Check-In Sender start");
+        }
+
+        if (this->mCheckInRequestCount == 0)
+        {
+            this->SetKeepActiveModeRequirements(KeepActiveFlag::kCheckInInProgress, false /* state */);
+        }
+    }
+
+    if (request.Has(KeepActiveFlag::kCommissioningWindowOpen) || request.Has(KeepActiveFlag::kFailSafeArmed))
     {
         // Only 1 request per type (kCommissioningWindowOpen, kFailSafeArmed)
         // remove requirement directly

src/app/icd/ICDManager.h

@@ -118,6 +118,7 @@ class ICDManager : public ICDListener
 
     static void OnIdleModeDone(System::Layer * aLayer, void * appState);
     static void OnActiveModeDone(System::Layer * aLayer, void * appState);
+
     /**
      * @brief Callback function called shortly before the device enters idle mode to allow checks to be made. This is currently only
      * called once to prevent entering in a loop if some events re-trigger this check (for instance if a check for subscription
@@ -126,7 +127,8 @@ class ICDManager : public ICDListener
      */
     static void OnTransitionToIdle(System::Layer * aLayer, void * appState);
 
-    static uint8_t OpenExchangeContextCount;
+    uint8_t mOpenExchangeContextCount = 0;
+    uint8_t mCheckInRequestCount      = 0;
 
 private:
     // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5)
@@ -139,7 +141,7 @@ class ICDManager : public ICDListener
     static constexpr uint32_t kMinActiveModeDuration  = 300;
     static constexpr uint16_t kMinActiveModeThreshold = 300;
 
-    BitFlags<KeepActiveFlags> mKeepActiveFlags{ 0 };
+    KeepActiveFlags mKeepActiveFlags{ 0 };
 
     // Initialize mOperationalState to ActiveMode so the init sequence at bootup triggers the IdleMode behaviour first.
     OperationalState mOperationalState             = OperationalState::ActiveMode;

src/app/icd/ICDNotifier.h

@@ -18,6 +18,7 @@
 
 #include <app/AppConfig.h>
 #include <lib/core/CHIPError.h>
+#include <lib/support/BitFlags.h>
 
 class ICDListener;
 
@@ -38,11 +39,13 @@ static_assert(ICD_MAX_NOTIFICATION_SUBSCRIBERS > 0, "At least 1 Subscriber is re
 class ICDListener
 {
 public:
-    enum class KeepActiveFlags : uint8_t
+    enum class KeepActiveFlagsValues : uint8_t
     {
         kCommissioningWindowOpen = 0x01,
         kFailSafeArmed           = 0x02,
-        kExchangeContextOpen     = 0x03,
+        kExchangeContextOpen     = 0x04,
+        kCheckInInProgress       = 0x08,
+        kInvalidFlag             = 0x10, // Move up when adding more flags
     };
 
     enum class ICDManagementEvents : uint8_t
@@ -51,6 +54,9 @@ class ICDListener
         kStayActiveRequestReceived = 0x02,
     };
 
+    using KeepActiveFlags = BitFlags<KeepActiveFlagsValues>;
+    using KeepActiveFlag  = KeepActiveFlagsValues;
+
     virtual ~ICDListener() {}
 
     /**
@@ -101,6 +107,11 @@ class ICDNotifier
     void BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags request);
     void BroadcastICDManagementEvent(ICDListener::ICDManagementEvents event);
 
+    inline void BroadcastActiveRequest(ICDListener::KeepActiveFlags request, bool notify)
+    {
+        (notify) ? BroadcastActiveRequestNotification(request) : BroadcastActiveRequestWithdrawal(request);
+    }
+
     static ICDNotifier & GetInstance() { return sICDNotifier; }
 
 private:

src/app/server/CommissioningWindowManager.cpp

@@ -544,7 +544,7 @@ void CommissioningWindowManager::UpdateWindowStatus(CommissioningWindowStatusEnu
     {
         mWindowStatus = aNewStatus;
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
-        app::ICDListener::KeepActiveFlags request = app::ICDListener::KeepActiveFlags::kCommissioningWindowOpen;
+        app::ICDListener::KeepActiveFlags request = app::ICDListener::KeepActiveFlag::kCommissioningWindowOpen;
         if (mWindowStatus != CommissioningWindowStatusEnum::kWindowNotOpen)
         {
             app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(request);

src/app/tests/TestICDManager.cpp

@@ -165,7 +165,7 @@ class TestICDManager
     static void TestKeepActivemodeRequests(nlTestSuite * aSuite, void * aContext)
     {
         TestContext * ctx = static_cast<TestContext *>(aContext);
-        typedef ICDListener::KeepActiveFlags ActiveFlag;
+        typedef ICDListener::KeepActiveFlag ActiveFlag;
         ICDNotifier notifier = ICDNotifier::GetInstance();
 
         // Setting a requirement will transition the ICD to active mode.

src/messaging/ExchangeContext.cpp

@@ -326,7 +326,7 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons
     SetAutoRequestAck(!session->IsGroupSession());
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
-    app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlags::kExchangeContextOpen);
+    app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlag::kExchangeContextOpen);
 #endif
 
 #if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING)
@@ -345,7 +345,7 @@ ExchangeContext::~ExchangeContext()
     VerifyOrDie(mFlags.Has(Flags::kFlagClosed));
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
-    app::ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(app::ICDListener::KeepActiveFlags::kExchangeContextOpen);
+    app::ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(app::ICDListener::KeepActiveFlag::kExchangeContextOpen);
 #endif // CHIP_CONFIG_ENABLE_ICD_SERVER
 
     // Ideally, in this scenario, the retransmit table should

src/messaging/ReliableMessageMgr.cpp

@@ -38,7 +38,6 @@
 #include <platform/ConnectivityManager.h>
 
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
-#include <app/icd/ICDManager.h>  // nogncheck
 #include <app/icd/ICDNotifier.h> // nogncheck
 #endif
 
@@ -260,7 +259,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp
     // Implement:
     //   "An ICD sender SHOULD increase t to also account for its own sleepy interval
     //   required to receive the acknowledgment"
-    mrpBackoffTime += app::ICDManager::GetFastPollingInterval();
+    mrpBackoffTime += CHIP_DEVICE_CONFIG_ICD_FAST_POLL_INTERVAL;
 #endif
 
     mrpBackoffTime += CHIP_CONFIG_MRP_RETRY_INTERVAL_SENDER_BOOST;