From 4fef04bd99fd4363eefe47fcfe16cf872d57b756 Mon Sep 17 00:00:00 2001 From: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com> Date: Tue, 13 Feb 2024 08:51:29 -0500 Subject: [PATCH] [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 <84793247+mkardous-silabs@users.noreply.github.com> * default mICDModeAdvertise to kNone --------- Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> --- src/app/server/Dnssd.cpp | 12 +++++++++++- src/lib/dnssd/Advertiser.h | 15 +++++++++++---- src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp | 14 +++++--------- src/lib/dnssd/Discovery_ImplPlatform.cpp | 15 ++++++++------- .../dnssd/minimal_mdns/tests/TestAdvertiser.cpp | 4 ++-- src/lib/dnssd/platform/tests/TestPlatform.cpp | 4 ++-- 6 files changed, 39 insertions(+), 25 deletions(-) diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index a8040564956a3e..1ec2fa0b8ffdb4 100644 --- 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(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 index 7c6e2953dd78d5..ddd8df259b1364 100644 --- 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 BaseAdvertisingParams { @@ -103,12 +110,12 @@ class BaseAdvertisingParams } Optional GetTcpSupported() const { return mTcpSupported; } - Derived & SetICDOperatingAsLIT(Optional operatesAsLIT) + Derived & SetICDModeToAdvertise(ICDModeAdvertise operatingMode) { - mICDOperatesAsLIT = operatesAsLIT; + mICDModeAdvertise = operatingMode; return *reinterpret_cast(this); } - Optional 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 mLocalMRPConfig; Optional mTcpSupported; - Optional 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 index b3b1f8ea8876a6..4c80723738bcaa 100644 --- 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(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 index 8b1bbbfb7b82e5..abbc4647ff3805 100644 --- 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 index aaf7fc6aa716ab..547d4eecf3bc54 100644 --- 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("Pair me")) .SetProductId(chip::Optional(897)) .SetRotatingDeviceId(chip::Optional("id_that_spins")) - .SetICDOperatingAsLIT(chip::Optional(false)) + .SetICDModeToAdvertise(ICDModeAdvertise::kSIT) // 3600005 is more than the max so should be adjusted down .SetLocalMRPConfig(Optional::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(3)) .SetPairingInstruction(chip::Optional("Pair me")) .SetProductId(chip::Optional(897)) - .SetICDOperatingAsLIT(chip::Optional(true)) + .SetICDModeToAdvertise(ICDModeAdvertise::kLIT) .SetLocalMRPConfig(Optional::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 index f0628d0e691ff1..91a79f1efdc64a 100644 --- 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::Value(32_ms32, 30_ms32, 10_ms16)) // SII and SAI to match below - .SetICDOperatingAsLIT(Optional(false)); + .SetICDModeToAdvertise(ICDModeAdvertise::kSIT); test::ExpectedCall operationalCall2 = test::ExpectedCall() .SetProtocol(DnssdServiceProtocol::kDnssdProtocolTcp) .SetServiceName("_matter") @@ -95,7 +95,7 @@ CommissionAdvertisingParameters commissionableNodeParamsLargeBasic = .SetPairingInstruction(Optional("Pair me")) .SetProductId(Optional(897)) .SetRotatingDeviceId(Optional("id_that_spins")) - .SetICDOperatingAsLIT(Optional(false)) + .SetICDModeToAdvertise(ICDModeAdvertise::kSIT) // 3600005 is over the max, so this should be adjusted by the platform .SetLocalMRPConfig(Optional::Value(3600000_ms32, 3600005_ms32, 65535_ms16));