From 4428050f601e6e3c8b0d39eece313b8e17fb56b2 Mon Sep 17 00:00:00 2001 From: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> Date: Fri, 25 Mar 2022 15:34:11 -0700 Subject: [PATCH] Address DNS-SD issues (#16625) * fix PH, allow for longer extended discovery timeout * ci fix * address comments * fix v6-only tv-app casting * fix CI --- .../tv-common/include/CHIPProjectAppConfig.h | 10 ++++++- src/app/server/Dnssd.cpp | 20 +++++++------- src/app/server/Dnssd.h | 4 +-- src/include/platform/CHIPDeviceConfig.h | 4 ++- .../UserDirectedCommissioningServer.cpp | 26 ++++++++++++++++--- 5 files changed, 47 insertions(+), 17 deletions(-) diff --git a/examples/tv-app/tv-common/include/CHIPProjectAppConfig.h b/examples/tv-app/tv-common/include/CHIPProjectAppConfig.h index e660c0747b93f3..e1e7c4d913d559 100644 --- a/examples/tv-app/tv-common/include/CHIPProjectAppConfig.h +++ b/examples/tv-app/tv-common/include/CHIPProjectAppConfig.h @@ -33,19 +33,27 @@ // TVs need to be both commissioners and commissionees #define CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE 1 +// TVs that are not commissionees, +// or that don't automatically enter commissioning mode should set this to 0 +#define CHIP_DEVICE_CONFIG_ENABLE_PAIRING_AUTOSTART 1 + // TVs do not typically need this - enable for debugging // #define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY_CLIENT 1 +// Enable extended discovery, set timeout to 24 hours #define CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY 1 +#define CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS (24 * 60 * 60) +// Advertise TV device type in DNS-SD #define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DEVICE_TYPE 1 #define CHIP_DEVICE_CONFIG_DEVICE_TYPE 35 // 0x0023 = 35 = Video Player +// Include device name in discovery for casting use case #define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DEVICE_NAME 1 - #define CHIP_DEVICE_CONFIG_DEVICE_NAME "Test TV" +// Enable app platform #define CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED 1 // overrides CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT in CHIPProjectConfig diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index 59cefa261346da..e49255050ce448 100644 --- a/src/app/server/Dnssd.cpp +++ b/src/app/server/Dnssd.cpp @@ -84,15 +84,15 @@ bool DnssdServer::HaveOperationalCredentials() #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY -void DnssdServer::SetExtendedDiscoveryTimeoutSecs(int16_t secs) +void DnssdServer::SetExtendedDiscoveryTimeoutSecs(int32_t secs) { - ChipLogDetail(Discovery, "Setting extended discovery timeout to %" PRId16 "s", secs); + ChipLogDetail(Discovery, "Setting extended discovery timeout to %" PRId32 "s", secs); chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Put(DefaultStorageKeyAllocator::DNSExtendedDiscoveryTimeout(), secs); } -int16_t DnssdServer::GetExtendedDiscoveryTimeoutSecs() +int32_t DnssdServer::GetExtendedDiscoveryTimeoutSecs() { - int16_t secs; + int32_t secs; CHIP_ERROR err = chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get( DefaultStorageKeyAllocator::DNSExtendedDiscoveryTimeout(), &secs); @@ -199,7 +199,7 @@ void DnssdServer::OnDiscoveryExpiration(System::Layer * aSystemLayer, void * aAp ChipLogDetail(Discovery, "OnDiscoveryExpiration callback for valid session"); #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY - int16_t extTimeout = GetExtendedDiscoveryTimeoutSecs(); + int32_t extTimeout = GetExtendedDiscoveryTimeoutSecs(); if (extTimeout != CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED) { CHIP_ERROR err = AdvertiseCommissionableNode(chip::Dnssd::CommissioningMode::kDisabled); @@ -232,16 +232,16 @@ CHIP_ERROR DnssdServer::ScheduleDiscoveryExpiration() #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY CHIP_ERROR DnssdServer::ScheduleExtendedDiscoveryExpiration() { - int16_t extendedDiscoveryTimeoutSecs = GetExtendedDiscoveryTimeoutSecs(); + int32_t extendedDiscoveryTimeoutSecs = GetExtendedDiscoveryTimeoutSecs(); if (extendedDiscoveryTimeoutSecs == CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT) { return CHIP_NO_ERROR; } - ChipLogDetail(Discovery, "Scheduling extended discovery timeout in %" PRId16 "s", extendedDiscoveryTimeoutSecs); + ChipLogDetail(Discovery, "Scheduling extended discovery timeout in %" PRId32 "s", extendedDiscoveryTimeoutSecs); - mExtendedDiscoveryExpiration = mTimeSource.GetMonotonicTimestamp() + System::Clock::Seconds16(extendedDiscoveryTimeoutSecs); + mExtendedDiscoveryExpiration = mTimeSource.GetMonotonicTimestamp() + System::Clock::Seconds32(extendedDiscoveryTimeoutSecs); - return DeviceLayer::SystemLayer().StartTimer(System::Clock::Seconds16(extendedDiscoveryTimeoutSecs), + return DeviceLayer::SystemLayer().StartTimer(System::Clock::Seconds32(extendedDiscoveryTimeoutSecs), HandleExtendedDiscoveryExpiration, nullptr); } #endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY @@ -378,7 +378,7 @@ CHIP_ERROR DnssdServer::Advertise(bool commissionableNode, chip::Dnssd::Commissi advertiseParameters.SetMRPConfig(GetLocalMRPConfig()).SetTcpSupported(Optional(INET_CONFIG_ENABLE_TCP_ENDPOINT)); - if (mode != chip::Dnssd::CommissioningMode::kEnabledEnhanced) + if (!HaveOperationalCredentials()) { if (DeviceLayer::ConfigurationMgr().GetInitialPairingHint(value) != CHIP_NO_ERROR) { diff --git a/src/app/server/Dnssd.h b/src/app/server/Dnssd.h index efc7b86ea5ce08..5724d84a2f3dc1 100644 --- a/src/app/server/Dnssd.h +++ b/src/app/server/Dnssd.h @@ -88,7 +88,7 @@ class DLL_EXPORT DnssdServer #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY /// Sets the extended discovery timeout. Value will be persisted across reboots - void SetExtendedDiscoveryTimeoutSecs(int16_t secs); + void SetExtendedDiscoveryTimeoutSecs(int32_t secs); /// Callback from Extended Discovery Expiration timer /// Checks if extended discovery has expired and if so, @@ -173,7 +173,7 @@ class DLL_EXPORT DnssdServer #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY /// get the current extended discovery timeout (from persistent storage) - int16_t GetExtendedDiscoveryTimeoutSecs(); + int32_t GetExtendedDiscoveryTimeoutSecs(); /// schedule next extended discovery expiration CHIP_ERROR ScheduleExtendedDiscoveryExpiration(); diff --git a/src/include/platform/CHIPDeviceConfig.h b/src/include/platform/CHIPDeviceConfig.h index 7dd950f32a825a..53405932479738 100644 --- a/src/include/platform/CHIPDeviceConfig.h +++ b/src/include/platform/CHIPDeviceConfig.h @@ -1233,11 +1233,13 @@ * Default time in seconds that a device will advertise commissionable node discovery * after commissioning mode ends. This value can be overridden by the user. * - * Only valid when CCHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY==1 + * Only valid when CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY==1 */ #define CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED 0 #define CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT -1 +#ifndef CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS #define CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS (15 * 60) +#endif /** * CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DEVICE_TYPE diff --git a/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp b/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp index b5b7d43a8a507c..cf46a3c6e8afc4 100644 --- a/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp +++ b/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp @@ -111,12 +111,12 @@ void UserDirectedCommissioningServer::OnCommissionableNodeFound(const Dnssd::Dis { if (nodeData.numIPs == 0) { - ChipLogError(AppServer, "SetUDCClientProcessingState no IP addresses returned for instance name=%s", nodeData.instanceName); + ChipLogError(AppServer, "OnCommissionableNodeFound no IP addresses returned for instance name=%s", nodeData.instanceName); return; } if (nodeData.port == 0) { - ChipLogError(AppServer, "SetUDCClientProcessingState no port returned for instance name=%s", nodeData.instanceName); + ChipLogError(AppServer, "OnCommissionableNodeFound no port returned for instance name=%s", nodeData.instanceName); return; } @@ -127,7 +127,8 @@ void UserDirectedCommissioningServer::OnCommissionableNodeFound(const Dnssd::Dis (int) client->GetUDCClientProcessingState(), (int) UDCClientProcessingState::kPromptingUser); client->SetUDCClientProcessingState(UDCClientProcessingState::kPromptingUser); - // currently IPv6 addresses do not work for some reason +#if INET_CONFIG_ENABLE_IPV4 + // prefer IPv4 if its an option bool foundV4 = false; for (unsigned i = 0; i < nodeData.numIPs; ++i) { @@ -143,6 +144,25 @@ void UserDirectedCommissioningServer::OnCommissionableNodeFound(const Dnssd::Dis { client->SetPeerAddress(chip::Transport::PeerAddress::UDP(nodeData.ipAddress[0], nodeData.port)); } +#else // INET_CONFIG_ENABLE_IPV4 + // if we only support V6, then try to find a v6 address + bool foundV6 = false; + for (unsigned i = 0; i < nodeData.numIPs; ++i) + { + if (nodeData.ipAddress[i].IsIPv6()) + { + foundV6 = true; + client->SetPeerAddress(chip::Transport::PeerAddress::UDP(nodeData.ipAddress[i], nodeData.port)); + break; + } + } + // last resort, try with what we have + if (!foundV6) + { + ChipLogError(AppServer, "OnCommissionableNodeFound no v6 returned for instance name=%s", nodeData.instanceName); + client->SetPeerAddress(chip::Transport::PeerAddress::UDP(nodeData.ipAddress[0], nodeData.port)); + } +#endif // INET_CONFIG_ENABLE_IPV4 client->SetDeviceName(nodeData.deviceName); client->SetLongDiscriminator(nodeData.longDiscriminator);