Skip to content

Commit

Permalink
Fix the extended discovery time limit to actually be obeyed properly. (…
Browse files Browse the repository at this point in the history 
…#20019)

* Fix the extended discovery time limit to actually be obeyed properly.

Fixes #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 <[email protected]>

* Advertise extended discovery on startup.

Addresses review comments.

Co-authored-by: chrisdecenzo <[email protected]>
  • Loading branch information
@bzbarsky-apple @chrisdecenzo
bzbarsky-apple and chrisdecenzo authored Jun 28, 2022
1 parent 41e474d commit cab048f
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 80 deletions.
108 changes: 40 additions & 68 deletions src/app/server/Dnssd.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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();
Expand Down Expand Up @@ -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);
}

Expand All @@ -379,8 +339,6 @@ void DnssdServer::StartServer(Dnssd::CommissioningMode mode)
{
ChipLogProgress(Discovery, "Updating services using commissioning mode %d", static_cast<int>(mode));

ClearTimeouts();

DeviceLayer::PlatformMgr().AddEventHandler(OnPlatformEventWrapper, 0);

CHIP_ERROR err = Dnssd::ServiceAdvertiser::Instance().Init(chip::DeviceLayer::UDPEndPointManager());
Expand All @@ -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)
Expand All @@ -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

Expand Down
21 changes: 9 additions & 12 deletions src/app/server/Dnssd.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,6 @@ class DLL_EXPORT DnssdServer

Time::TimeSource<Time::Source::kSystem> 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;

Expand All @@ -156,11 +149,6 @@ class DLL_EXPORT DnssdServer
// Ephemeral discriminator to use instead of the default if set
Optional<uint16_t> mEphemeralDiscriminator;

Optional<int32_t> 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).
Expand All @@ -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<int32_t> 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
};

Expand Down

0 comments on commit cab048f

Please sign in to comment.