From 3fe41db16c39949bb24017bf4232f4be1c704119 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 28 Jun 2022 16:40:15 -0400 Subject: [PATCH] Fix the extended discovery time limit to actually be obeyed properly. (#20019) * Fix the extended discovery time limit to actually be obeyed properly. Fixes https://github.com/project-chip/connectedhomeip/issues/16522 Tested that if CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS is set to CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT then we advertise extended discovery in all the conditions where we are not advertising commissionable discovery. If CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS is set to another value, verified that: 1. We do not start advertising extended discovery on startup. 2. If we start advertising commissionable discovery on startup, then after the commissioning window closes (whether due to being commissioned or timing out) we we start advertising extended discovery until that times out. 3. If we open a new commissioning window and then RevokeCommissioning, we start advertising extended discovery until it times out. 4. If we open a new commissioning window and then commission the device, we start advertising extended discovery until it times out. 5. If in a state with two fabrics commissioned we remove a fabric we do _not_ start advertising extended discovery (unlike before this change). 6. If in a state with two fabrics commissioned and extended discovery advertising ongoing we remove one of the fabrics, we keep advertising extended discovery until it times out. * Fix typo in comment. Co-authored-by: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> * Advertise extended discovery on startup. Addresses review comments. Co-authored-by: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> --- src/app/server/Dnssd.cpp | 108 +++++++++++++++------------------------ src/app/server/Dnssd.h | 21 ++++---- 2 files changed, 49 insertions(+), 80 deletions(-) diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index 3b1d7d7c9f0042..12fe2824ad3019 100644 --- a/src/app/server/Dnssd.cpp +++ b/src/app/server/Dnssd.cpp @@ -80,6 +80,13 @@ void DnssdServer::SetExtendedDiscoveryTimeoutSecs(int32_t secs) { ChipLogDetail(Discovery, "Setting extended discovery timeout to %" PRId32 "s", secs); mExtendedDiscoveryTimeoutSecs = MakeOptional(secs); + + if (mExtendedDiscoveryExpiration != kTimeoutCleared && + mExtendedDiscoveryExpiration > mTimeSource.GetMonotonicTimestamp() + System::Clock::Seconds32(secs)) + { + // Reset our timer to the new (shorter) timeout. + ScheduleExtendedDiscoveryExpiration(); + } } int32_t DnssdServer::GetExtendedDiscoveryTimeoutSecs() @@ -95,72 +102,15 @@ void HandleExtendedDiscoveryExpiration(System::Layer * aSystemLayer, void * aApp void DnssdServer::OnExtendedDiscoveryExpiration(System::Layer * aSystemLayer, void * aAppState) { - if (!DnssdServer::OnExpiration(mExtendedDiscoveryExpiration)) - { - ChipLogDetail(Discovery, "Extended discovery timeout cancelled"); - return; - } - ChipLogDetail(Discovery, "Extended discovery timed out"); mExtendedDiscoveryExpiration = kTimeoutCleared; -} -#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY - -bool DnssdServer::OnExpiration(System::Clock::Timestamp expirationMs) -{ - if (expirationMs == kTimeoutCleared) - { - ChipLogDetail(Discovery, "OnExpiration callback for cleared session"); - return false; - } - System::Clock::Timestamp now = mTimeSource.GetMonotonicTimestamp(); - if (expirationMs > now) - { - ChipLogDetail(Discovery, "OnExpiration callback for reset session"); - return false; - } - - ChipLogDetail(Discovery, "OnExpiration - valid time out"); - - CHIP_ERROR err = Dnssd::ServiceAdvertiser::Instance().Init(chip::DeviceLayer::UDPEndPointManager()); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to initialize advertiser: %" CHIP_ERROR_FORMAT, err.Format()); - } - - // reset advertising - err = Dnssd::ServiceAdvertiser::Instance().RemoveServices(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to remove advertised services: %" CHIP_ERROR_FORMAT, err.Format()); - } - - // restart operational (if needed) - err = AdvertiseOperational(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to advertise operational node: %" CHIP_ERROR_FORMAT, err.Format()); - } -#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY - err = AdvertiseCommissioner(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to advertise commissioner: %" CHIP_ERROR_FORMAT, err.Format()); - } -#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY - - err = Dnssd::ServiceAdvertiser::Instance().FinalizeServiceUpdate(); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to finalize service update: %" CHIP_ERROR_FORMAT, err.Format()); - } - - return true; + // Reset our advertising, now that we have flagged ourselves as possibly not + // needing extended discovery anymore. + StartServer(); } -#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY CHIP_ERROR DnssdServer::ScheduleExtendedDiscoveryExpiration() { int32_t extendedDiscoveryTimeoutSecs = GetExtendedDiscoveryTimeoutSecs(); @@ -362,6 +312,16 @@ CHIP_ERROR DnssdServer::AdvertiseCommissioner() CHIP_ERROR DnssdServer::AdvertiseCommissionableNode(chip::Dnssd::CommissioningMode mode) { +#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY + mCurrentCommissioningMode = mode; + if (mode != Dnssd::CommissioningMode::kDisabled) + { + // We're not doing extended discovery right now. + DeviceLayer::SystemLayer().CancelTimer(HandleExtendedDiscoveryExpiration, nullptr); + mExtendedDiscoveryExpiration = kTimeoutCleared; + } +#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY + return Advertise(true /* commissionableNode */, mode); } @@ -379,8 +339,6 @@ void DnssdServer::StartServer(Dnssd::CommissioningMode mode) { ChipLogProgress(Discovery, "Updating services using commissioning mode %d", static_cast(mode)); - ClearTimeouts(); - DeviceLayer::PlatformMgr().AddEventHandler(OnPlatformEventWrapper, 0); CHIP_ERROR err = Dnssd::ServiceAdvertiser::Instance().Init(chip::DeviceLayer::UDPEndPointManager()); @@ -401,7 +359,7 @@ void DnssdServer::StartServer(Dnssd::CommissioningMode mode) ChipLogError(Discovery, "Failed to advertise operational node: %" CHIP_ERROR_FORMAT, err.Format()); } - if (mode != chip::Dnssd::CommissioningMode::kDisabled) + if (mode != Dnssd::CommissioningMode::kDisabled) { err = AdvertiseCommissionableNode(mode); if (err != CHIP_NO_ERROR) @@ -412,13 +370,27 @@ void DnssdServer::StartServer(Dnssd::CommissioningMode mode) #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY else if (GetExtendedDiscoveryTimeoutSecs() != CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED) { - err = AdvertiseCommissionableNode(mode); - if (err != CHIP_NO_ERROR) + bool alwaysAdvertiseExtended = (GetExtendedDiscoveryTimeoutSecs() == CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT); + // We do extended discovery advertising in three cases: + // 1) We don't have a timeout for extended discovery. + // 2) We are transitioning out of commissioning mode (basic or enhanced) + // and should therefore start extended discovery. + // 3) We are resetting advertising while we are in the middle of an + // existing extended discovery advertising period. + if (alwaysAdvertiseExtended || mCurrentCommissioningMode != Dnssd::CommissioningMode::kDisabled || + mExtendedDiscoveryExpiration != kTimeoutCleared) { - ChipLogError(Discovery, "Failed to advertise extended commissionable node: %" CHIP_ERROR_FORMAT, err.Format()); + err = AdvertiseCommissionableNode(mode); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Discovery, "Failed to advertise extended commissionable node: %" CHIP_ERROR_FORMAT, err.Format()); + } + if (mExtendedDiscoveryExpiration == kTimeoutCleared) + { + // set timeout + ScheduleExtendedDiscoveryExpiration(); + } } - // set timeout - ScheduleExtendedDiscoveryExpiration(); } #endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY diff --git a/src/app/server/Dnssd.h b/src/app/server/Dnssd.h index af9cfaf6122e04..394c928e9c3943 100644 --- a/src/app/server/Dnssd.h +++ b/src/app/server/Dnssd.h @@ -139,13 +139,6 @@ class DLL_EXPORT DnssdServer Time::TimeSource mTimeSource; - void ClearTimeouts() - { -#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY - mExtendedDiscoveryExpiration = kTimeoutCleared; -#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY - } - FabricTable * mFabricTable = nullptr; CommissioningModeProvider * mCommissioningModeProvider = nullptr; @@ -156,11 +149,6 @@ class DLL_EXPORT DnssdServer // Ephemeral discriminator to use instead of the default if set Optional mEphemeralDiscriminator; - Optional mExtendedDiscoveryTimeoutSecs = NullOptional; - - /// return true if expirationMs is valid (not cleared and not in the future) - bool OnExpiration(System::Clock::Timestamp expiration); - #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY /// Get the current extended discovery timeout (set by /// SetExtendedDiscoveryTimeoutSecs, or the configuration default if not set). @@ -169,7 +157,16 @@ class DLL_EXPORT DnssdServer /// schedule next extended discovery expiration CHIP_ERROR ScheduleExtendedDiscoveryExpiration(); + // mExtendedDiscoveryExpiration, if not set to kTimeoutCleared, is used to + // indicate that we should be advertising extended discovery right now. System::Clock::Timestamp mExtendedDiscoveryExpiration = kTimeoutCleared; + Optional mExtendedDiscoveryTimeoutSecs = NullOptional; + + // The commissioning mode we are advertising right now. Used to detect when + // we need to start extended discovery advertisement. We start this off as + // kEnabledBasic, so that when we first start up we do extended discovery + // advertisement if we don't enter commissioning mode. + Dnssd::CommissioningMode mCurrentCommissioningMode = Dnssd::CommissioningMode::kEnabledBasic; #endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY };