[ICD]Convert the ICD DNS advertiser variable from optional bool to an…

… enum class (#32080) * Convert the ICD DNS advertiser variable from optional bool to an enum class * Apply suggestions from code review Co-authored-by: mkardous-silabs <[email protected]> * default mICDModeAdvertise to kNone --------- Co-authored-by: mkardous-silabs <[email protected]>
project-chip · Feb 13, 2024 · 4fef04b · 4fef04b
1 parent f216481
commit 4fef04b
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 25 deletions.
diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp
@@ -147,11 +147,21 @@ void DnssdServer::AddICDKeyToAdvertisement(AdvertisingParams & advParams)
     VerifyOrDieWithMsg(mICDManager != nullptr, Discovery,
                        "Invalid pointer to the ICDManager which is required for the LIT operating mode");
 
+    Dnssd::ICDModeAdvertise ICDModeToAdvertise = Dnssd::ICDModeAdvertise::kNone;
     // Only advertise the ICD key if the device can operate as a LIT
     if (mICDManager->SupportsFeature(Clusters::IcdManagement::Feature::kLongIdleTimeSupport))
     {
-        advParams.SetICDOperatingAsLIT(Optional<bool>(mICDManager->GetICDMode() == ICDConfigurationData::ICDMode::LIT));
+        if (mICDManager->GetICDMode() == ICDConfigurationData::ICDMode::LIT)
+        {
+            ICDModeToAdvertise = Dnssd::ICDModeAdvertise::kLIT;
+        }
+        else
+        {
+            ICDModeToAdvertise = Dnssd::ICDModeAdvertise::kSIT;
+        }
     }
+
+    advParams.SetICDModeToAdvertise(ICDModeToAdvertise);
 }
 #endif
 

diff --git a/src/lib/dnssd/Advertiser.h b/src/lib/dnssd/Advertiser.h
@@ -48,6 +48,13 @@ enum class CommissioningMode
     kEnabledEnhanced // Enhanced Commissioning Mode, CM=2 in DNS-SD key/value pairs
 };
 
+enum class ICDModeAdvertise : uint8_t
+{
+    kNone, // The device does not support the LIT feature-set. No ICD= key is advertised in DNS-SD.
+    kSIT,  // The ICD supports the LIT feature-set, but is currently operating as a SIT. ICD=0 in DNS-SD key/value pairs.
+    kLIT,  // The ICD is currently operating as a LIT. ICD=1 in DNS-SD key/value pairs.
+};
+
 template <class Derived>
 class BaseAdvertisingParams
 {
@@ -103,12 +110,12 @@ class BaseAdvertisingParams
     }
     Optional<bool> GetTcpSupported() const { return mTcpSupported; }
 
-    Derived & SetICDOperatingAsLIT(Optional<bool> operatesAsLIT)
+    Derived & SetICDModeToAdvertise(ICDModeAdvertise operatingMode)
     {
-        mICDOperatesAsLIT = operatesAsLIT;
+        mICDModeAdvertise = operatingMode;
         return *reinterpret_cast<Derived *>(this);
     }
-    Optional<bool> GetICDOperatingAsLIT() const { return mICDOperatesAsLIT; }
+    ICDModeAdvertise GetICDModeToAdvertise() const { return mICDModeAdvertise; }
 
 private:
     uint16_t mPort                   = CHIP_PORT;
@@ -118,7 +125,7 @@ class BaseAdvertisingParams
     size_t mMacLength                = 0;
     Optional<ReliableMessageProtocolConfig> mLocalMRPConfig;
     Optional<bool> mTcpSupported;
-    Optional<bool> mICDOperatesAsLIT;
+    ICDModeAdvertise mICDModeAdvertise = ICDModeAdvertise::kNone;
 };
 
 /// Defines parameters required for advertising a CHIP node

diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
@@ -232,13 +232,9 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
         {
             auto mrp = optionalMrp.Value();
 
-#if CHIP_CONFIG_ENABLE_ICD_SERVER
-            // An ICD operating as a LIT should not advertise its slow polling interval.
-            // When the ICD doesn't support the LIT feature, it doesn't set nor advertise the GetICDOperatingAsLIT entry.
-            // Therefore when GetICDOperatingAsLIT has no value or a value of 0, we advertise the slow polling interval
-            // otherwise we don't include the SII key in the advertisement.
-            if (!params.GetICDOperatingAsLIT().ValueOr(false))
-#endif
+            // An ICD operating as a LIT shall not advertise its slow polling interval.
+            // Don't include the SII key in the advertisement when operating as so.
+            if (params.GetICDModeToAdvertise() != ICDModeAdvertise::kLIT)
             {
                 if (mrp.mIdleRetransTimeout > kMaxRetryInterval)
                 {
@@ -290,11 +286,11 @@ class AdvertiserMinMdns : public ServiceAdvertiser,
                                 CHIP_ERROR_INVALID_STRING_LENGTH);
             txtFields[numTxtFields++] = storage.tcpSupportedBuf;
         }
-        if (params.GetICDOperatingAsLIT().HasValue())
+        if (params.GetICDModeToAdvertise() != ICDModeAdvertise::kNone)
         {
             size_t writtenCharactersNumber =
                 static_cast<size_t>(snprintf(storage.operatingICDAsLITBuf, sizeof(storage.operatingICDAsLITBuf), "ICD=%d",
-                                             params.GetICDOperatingAsLIT().Value()));
+                                             (params.GetICDModeToAdvertise() == ICDModeAdvertise::kLIT)));
             VerifyOrReturnError((writtenCharactersNumber > 0) && (writtenCharactersNumber < sizeof(storage.operatingICDAsLITBuf)),
                                 CHIP_ERROR_INVALID_STRING_LENGTH);
             txtFields[numTxtFields++] = storage.operatingICDAsLITBuf;

diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp
@@ -213,19 +213,20 @@ CHIP_ERROR CopyTxtRecord(TxtFieldKey key, char * buffer, size_t bufferLen, const
     case TxtFieldKey::kSessionIdleInterval:
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
         // A ICD operating as a LIT should not advertise its slow polling interval
-        if (params.GetICDOperatingAsLIT().HasValue() && params.GetICDOperatingAsLIT().Value())
-        {
-            // Returning UNINITIALIZED ensures that the SII string isn't added by the AddTxtRecord
-            // without erroring out the action.
-            return CHIP_ERROR_UNINITIALIZED;
-        }
+        // Returning UNINITIALIZED ensures that the SII string isn't added by the AddTxtRecord
+        // without erroring out the action.
+        VerifyOrReturnError(params.GetICDModeToAdvertise() != ICDModeAdvertise::kLIT, CHIP_ERROR_UNINITIALIZED);
         FALLTHROUGH;
 #endif
     case TxtFieldKey::kSessionActiveInterval:
     case TxtFieldKey::kSessionActiveThreshold:
         return CopyTextRecordValue(buffer, bufferLen, params.GetLocalMRPConfig(), key);
     case TxtFieldKey::kLongIdleTimeICD:
-        return CopyTextRecordValue(buffer, bufferLen, params.GetICDOperatingAsLIT());
+        // The ICD key is only added to the advertissment when the device supports the ICD LIT feature-set.
+        // Return UNINITIALIZED when the operating mode is kNone to ensure that the ICD string isn't added
+        // by the AddTxtRecord without erroring out the action.
+        VerifyOrReturnError(params.GetICDModeToAdvertise() != ICDModeAdvertise::kNone, CHIP_ERROR_UNINITIALIZED);
+        return CopyTextRecordValue(buffer, bufferLen, (params.GetICDModeToAdvertise() == ICDModeAdvertise::kLIT));
     default:
         return CHIP_ERROR_INVALID_ARGUMENT;
     }

diff --git a/src/lib/dnssd/minimal_mdns/tests/TestAdvertiser.cpp b/src/lib/dnssd/minimal_mdns/tests/TestAdvertiser.cpp
@@ -180,7 +180,7 @@ CommissionAdvertisingParameters commissionableNodeParamsLargeEnhanced =
         .SetPairingInstruction(chip::Optional<const char *>("Pair me"))
         .SetProductId(chip::Optional<uint16_t>(897))
         .SetRotatingDeviceId(chip::Optional<const char *>("id_that_spins"))
-        .SetICDOperatingAsLIT(chip::Optional<bool>(false))
+        .SetICDModeToAdvertise(ICDModeAdvertise::kSIT)
         // 3600005 is more than the max so should be adjusted down
         .SetLocalMRPConfig(Optional<ReliableMessageProtocolConfig>::Value(3600000_ms32, 3600005_ms32, 65535_ms16));
 QNamePart txtCommissionableNodeParamsLargeEnhancedParts[] = { "D=22",          "VP=555+897",       "CM=2",       "DT=70000",
@@ -204,7 +204,7 @@ CommissionAdvertisingParameters commissionableNodeParamsEnhancedAsICDLIT =
         .SetPairingHint(chip::Optional<uint16_t>(3))
         .SetPairingInstruction(chip::Optional<const char *>("Pair me"))
         .SetProductId(chip::Optional<uint16_t>(897))
-        .SetICDOperatingAsLIT(chip::Optional<bool>(true))
+        .SetICDModeToAdvertise(ICDModeAdvertise::kLIT)
         .SetLocalMRPConfig(Optional<ReliableMessageProtocolConfig>::Value(3600000_ms32, 3600000_ms32, 65535_ms16));
 // With ICD Operation as LIT, SII key will not be added to the advertisement
 QNamePart txtCommissionableNodeParamsEnhancedAsICDLITParts[] = { "D=22",          "VP=555+897", "CM=2", "DT=70000",

diff --git a/src/lib/dnssd/platform/tests/TestPlatform.cpp b/src/lib/dnssd/platform/tests/TestPlatform.cpp
@@ -53,7 +53,7 @@ OperationalAdvertisingParameters operationalParams2 =
         .SetPort(CHIP_PORT)
         .EnableIpV4(true)
         .SetLocalMRPConfig(Optional<ReliableMessageProtocolConfig>::Value(32_ms32, 30_ms32, 10_ms16)) // SII and SAI to match below
-        .SetICDOperatingAsLIT(Optional<bool>(false));
+        .SetICDModeToAdvertise(ICDModeAdvertise::kSIT);
 test::ExpectedCall operationalCall2 = test::ExpectedCall()
                                           .SetProtocol(DnssdServiceProtocol::kDnssdProtocolTcp)
                                           .SetServiceName("_matter")
@@ -95,7 +95,7 @@ CommissionAdvertisingParameters commissionableNodeParamsLargeBasic =
         .SetPairingInstruction(Optional<const char *>("Pair me"))
         .SetProductId(Optional<uint16_t>(897))
         .SetRotatingDeviceId(Optional<const char *>("id_that_spins"))
-        .SetICDOperatingAsLIT(Optional<bool>(false))
+        .SetICDModeToAdvertise(ICDModeAdvertise::kSIT)
         // 3600005 is over the max, so this should be adjusted by the platform
         .SetLocalMRPConfig(Optional<ReliableMessageProtocolConfig>::Value(3600000_ms32, 3600005_ms32, 65535_ms16));