From 5d510f1523c06e2485fd2ca52c0fb4ca9d520e36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Wed, 23 Mar 2022 06:39:38 +0100 Subject: [PATCH] [dns-sd] Extended discovery improvements (#16290) * [dns-sd] Change extended discovery condition According to the spec, each time a commissionable node service is advertised and the device is not in the commissioning mode, the advertising should be treated as Extended Discovery. The current code, however, makes a distinction whether the device is on any fabric or not. * [dns-sd] Set default extended discovery timeout to 15 minutes The spec recommends that the extended discovery by default time out after a period recommended in the "Announcement duration" section. Change the default from "no timeout" to 15 minutes. Use DefaultStorageKeyAllocator when persisting the timeout. Make extended discovery logs more meaningful for a non-implementer and remove some irrelevant messages. * Fix Cirque tests Discriminator must be now explicitly specified in Linux apps. Previously the Cirque tests were passing only by chance. * Remove CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY Most platforms have it enabled, by default, and those that don't, have extended discovery disabled, too, which covers the case when the commissioning window is closed. On the other hand, when the commissioning window is open, the device should always try to advertise over DNS-SD. * Fix build * Follow KVS key convention --- src/app/server/Dnssd.cpp | 74 ++++++++----------- src/include/platform/CHIPDeviceConfig.h | 23 +----- src/lib/support/DefaultStorageKeyAllocator.h | 3 + .../Darwin/CHIPDevicePlatformConfig.h | 2 - src/platform/EFR32/CHIPDevicePlatformConfig.h | 1 - src/platform/Linux/CHIPDevicePlatformConfig.h | 2 - .../cc13x2_26x2/CHIPDevicePlatformConfig.h | 1 - .../nrfconnect/CHIPDevicePlatformConfig.h | 1 - .../nxp/k32w/k32w0/CHIPDevicePlatformConfig.h | 1 - src/platform/qpg/CHIPDevicePlatformConfig.h | 1 - src/platform/webos/CHIPDevicePlatformConfig.h | 2 - .../linux-cirque/CommissioningTest.py | 5 +- .../linux-cirque/MobileDeviceTest.py | 5 +- 13 files changed, 43 insertions(+), 78 deletions(-) diff --git a/src/app/server/Dnssd.cpp b/src/app/server/Dnssd.cpp index f2804f0908d637..e544b43e99ba7e 100644 --- a/src/app/server/Dnssd.cpp +++ b/src/app/server/Dnssd.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -78,34 +79,34 @@ bool DnssdServer::HaveOperationalCredentials() } } - ChipLogProgress(Discovery, "Failed to find a valid admin pairing. Node ID unknown"); return false; } #if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY -constexpr const char kExtendedDiscoveryTimeoutKeypairStorage[] = "ExtDiscKey"; - void DnssdServer::SetExtendedDiscoveryTimeoutSecs(int16_t secs) { - ChipLogDetail(Discovery, "SetExtendedDiscoveryTimeoutSecs %d", secs); - chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Put(kExtendedDiscoveryTimeoutKeypairStorage, &secs, sizeof(secs)); + ChipLogDetail(Discovery, "Setting extended discovery timeout to %" PRId16 "s", secs); + chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Put(DefaultStorageKeyAllocator::DNSExtendedDiscoveryTimeout(), secs); } int16_t DnssdServer::GetExtendedDiscoveryTimeoutSecs() { - CHIP_ERROR err = CHIP_NO_ERROR; int16_t secs; + CHIP_ERROR err = chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get( + DefaultStorageKeyAllocator::DNSExtendedDiscoveryTimeout(), &secs); - err = chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Get(kExtendedDiscoveryTimeoutKeypairStorage, &secs, sizeof(secs)); - if (err != CHIP_NO_ERROR) + if (err == CHIP_NO_ERROR) + { + return secs; + } + + if (err != CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND) { - ChipLogError(Discovery, "Failed to get extended timeout configuration err: %s", chip::ErrorStr(err)); - secs = CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS; + ChipLogError(Discovery, "Failed to load extended discovery timeout: %" CHIP_ERROR_FORMAT, err.Format()); } - ChipLogDetail(Discovery, "GetExtendedDiscoveryTimeoutSecs %d", secs); - return secs; + return CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS; } /// Callback from Extended Discovery Expiration timer @@ -118,11 +119,11 @@ void DnssdServer::OnExtendedDiscoveryExpiration(System::Layer * aSystemLayer, vo { if (!DnssdServer::OnExpiration(mExtendedDiscoveryExpiration)) { - ChipLogDetail(Discovery, "OnExtendedDiscoveryExpiration callback for cleared session"); + ChipLogDetail(Discovery, "Extended discovery timeout cancelled"); return; } - ChipLogDetail(Discovery, "OnExtendedDiscoveryExpiration callback for valid session"); + ChipLogDetail(Discovery, "Extended discovery timed out"); mExtendedDiscoveryExpiration = kTimeoutCleared; } @@ -220,7 +221,7 @@ CHIP_ERROR DnssdServer::ScheduleDiscoveryExpiration() { return CHIP_NO_ERROR; } - ChipLogDetail(Discovery, "Scheduling Discovery timeout in secs=%d", mDiscoveryTimeoutSecs); + ChipLogDetail(Discovery, "Scheduling discovery timeout in %" PRId16 "s", mDiscoveryTimeoutSecs); mDiscoveryExpiration = mTimeSource.GetMonotonicTimestamp() + System::Clock::Seconds16(mDiscoveryTimeoutSecs); @@ -236,7 +237,7 @@ CHIP_ERROR DnssdServer::ScheduleExtendedDiscoveryExpiration() { return CHIP_NO_ERROR; } - ChipLogDetail(Discovery, "Scheduling Extended Discovery timeout in secs=%d", extendedDiscoveryTimeoutSecs); + ChipLogDetail(Discovery, "Scheduling extended discovery timeout in %" PRId16 "s", extendedDiscoveryTimeoutSecs); mExtendedDiscoveryExpiration = mTimeSource.GetMonotonicTimestamp() + System::Clock::Seconds16(extendedDiscoveryTimeoutSecs); @@ -448,7 +449,7 @@ void DnssdServer::StartServer() void DnssdServer::StartServer(Dnssd::CommissioningMode mode) { - ChipLogDetail(Discovery, "DNS-SD StartServer mode=%d", static_cast(mode)); + ChipLogProgress(Discovery, "Updating services using commissioning mode %d", static_cast(mode)); ClearTimeouts(); @@ -472,44 +473,33 @@ void DnssdServer::StartServer(Dnssd::CommissioningMode mode) ChipLogError(Discovery, "Failed to advertise operational node: %s", chip::ErrorStr(err)); } - if (HaveOperationalCredentials()) + if (mode != chip::Dnssd::CommissioningMode::kDisabled) { - ChipLogProgress(Discovery, "Have operational credentials"); - if (mode != chip::Dnssd::CommissioningMode::kDisabled) + err = AdvertiseCommissionableNode(mode); + if (err != CHIP_NO_ERROR) { - err = AdvertiseCommissionableNode(mode); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to advertise commissionable node: %s", chip::ErrorStr(err)); - } - // no need to set timeout because callers are currently doing that and their timeout might be longer than the default + ChipLogError(Discovery, "Failed to advertise commissionable node: %s", chip::ErrorStr(err)); } -#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY - else if (GetExtendedDiscoveryTimeoutSecs() != CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED) + + // If any fabrics exist, the commissioning window must have been opened by the administrator + // commissioning cluster commands which take care of the timeout. + if (!HaveOperationalCredentials()) { - err = AdvertiseCommissionableNode(mode); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Discovery, "Failed to advertise extended commissionable node: %s", chip::ErrorStr(err)); - } - // set timeout - ScheduleExtendedDiscoveryExpiration(); + ScheduleDiscoveryExpiration(); } -#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY } - else +#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY + else if (GetExtendedDiscoveryTimeoutSecs() != CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED) { -#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY - ChipLogProgress(Discovery, "Start dns-sd server - no current nodeId"); err = AdvertiseCommissionableNode(mode); if (err != CHIP_NO_ERROR) { - ChipLogError(Discovery, "Failed to advertise unprovisioned commissionable node: %s", chip::ErrorStr(err)); + ChipLogError(Discovery, "Failed to advertise extended commissionable node: %s", chip::ErrorStr(err)); } // set timeout - ScheduleDiscoveryExpiration(); -#endif + ScheduleExtendedDiscoveryExpiration(); } +#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY err = AdvertiseCommissioner(); diff --git a/src/include/platform/CHIPDeviceConfig.h b/src/include/platform/CHIPDeviceConfig.h index 921c042e74203b..7dd950f32a825a 100644 --- a/src/include/platform/CHIPDeviceConfig.h +++ b/src/include/platform/CHIPDeviceConfig.h @@ -1162,32 +1162,13 @@ // -------------------- Device DNS-SD Configuration -------------------- -/** - * CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY - * - * Enable MDNS commissionable node advertising when not yet provisioned. - * - * This should be 1 for WiFi SoftAP devices, ethernet devices, and (probably) bridge devices - * - * This should be 0 for Thread/BLE devices and WiFi/BLE devices - */ -#ifndef CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY -#if CHIP_DEVICE_CONFIG_ENABLE_THREAD -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 0 -#else -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 -#endif -#endif - /** * CHIP_DEVICE_CONFIG_DISCOVERY_TIMEOUT_SECS * * Time in seconds that a factory new device will advertise commissionable node discovery. - * - * Only valid when CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY==1 */ #ifndef CHIP_DEVICE_CONFIG_DISCOVERY_TIMEOUT_SECS -#define CHIP_DEVICE_CONFIG_DISCOVERY_TIMEOUT_SECS 15 * 60 +#define CHIP_DEVICE_CONFIG_DISCOVERY_TIMEOUT_SECS (15 * 60) #endif /** @@ -1256,7 +1237,7 @@ */ #define CHIP_DEVICE_CONFIG_DISCOVERY_DISABLED 0 #define CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT -1 -#define CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS CHIP_DEVICE_CONFIG_DISCOVERY_NO_TIMEOUT +#define CHIP_DEVICE_CONFIG_EXTENDED_DISCOVERY_TIMEOUT_SECS (15 * 60) /** * CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DEVICE_TYPE diff --git a/src/lib/support/DefaultStorageKeyAllocator.h b/src/lib/support/DefaultStorageKeyAllocator.h index d9df01301ef9b2..bd46209f3c8f6c 100644 --- a/src/lib/support/DefaultStorageKeyAllocator.h +++ b/src/lib/support/DefaultStorageKeyAllocator.h @@ -80,6 +80,9 @@ class DefaultStorageKeyAllocator static const char * OTACurrentProvider() { return "o/cp"; } static const char * OTAUpdateToken() { return "o/ut"; } + // [G]lobal [D]NS-related keys + static const char * DNSExtendedDiscoveryTimeout() { return "g/d/edt"; } + private: // The ENFORCE_FORMAT args are "off by one" because this is a class method, // with an implicit "this" as first arg. diff --git a/src/platform/Darwin/CHIPDevicePlatformConfig.h b/src/platform/Darwin/CHIPDevicePlatformConfig.h index 2912fd016b4ec8..690a5eaf97b36d 100644 --- a/src/platform/Darwin/CHIPDevicePlatformConfig.h +++ b/src/platform/Darwin/CHIPDevicePlatformConfig.h @@ -36,8 +36,6 @@ #define CHIP_DEVICE_CONFIG_ENABLE_CHIP_TIME_SERVICE_TIME_SYNC 0 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 - // ========== Platform-specific Configuration ========= // These are configuration options that are unique to Darwin platforms. diff --git a/src/platform/EFR32/CHIPDevicePlatformConfig.h b/src/platform/EFR32/CHIPDevicePlatformConfig.h index acdbf0c7bff8ac..ba22beda6ab6d9 100644 --- a/src/platform/EFR32/CHIPDevicePlatformConfig.h +++ b/src/platform/EFR32/CHIPDevicePlatformConfig.h @@ -45,7 +45,6 @@ #endif /* defined(SL_WIFI) */ #define CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY 1 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 #define CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE 1 diff --git a/src/platform/Linux/CHIPDevicePlatformConfig.h b/src/platform/Linux/CHIPDevicePlatformConfig.h index 7c9a5d6beb417c..23aaff4ace2ab2 100644 --- a/src/platform/Linux/CHIPDevicePlatformConfig.h +++ b/src/platform/Linux/CHIPDevicePlatformConfig.h @@ -43,8 +43,6 @@ #define CHIP_DEVICE_CONFIG_ENABLE_CHIP_TIME_SERVICE_TIME_SYNC 0 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 - // ========== Platform-specific Configuration ========= // These are configuration options that are unique to Linux platforms. diff --git a/src/platform/cc13x2_26x2/CHIPDevicePlatformConfig.h b/src/platform/cc13x2_26x2/CHIPDevicePlatformConfig.h index cae88c46eb1559..5927b58b3c2c60 100644 --- a/src/platform/cc13x2_26x2/CHIPDevicePlatformConfig.h +++ b/src/platform/cc13x2_26x2/CHIPDevicePlatformConfig.h @@ -55,4 +55,3 @@ #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT 1 #define CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY 1 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 diff --git a/src/platform/nrfconnect/CHIPDevicePlatformConfig.h b/src/platform/nrfconnect/CHIPDevicePlatformConfig.h index 8ddf607d5d8239..3748d3bd724a1f 100644 --- a/src/platform/nrfconnect/CHIPDevicePlatformConfig.h +++ b/src/platform/nrfconnect/CHIPDevicePlatformConfig.h @@ -102,7 +102,6 @@ #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT 1 #define CHIP_DEVICE_CONFIG_THREAD_SRP_MAX_SERVICES (CHIP_CONFIG_MAX_FABRICS + 1) #define CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY 1 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 #ifdef CONFIG_CHIP_ENABLE_DNS_CLIENT #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT 1 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_COMMISSIONABLE_DISCOVERY 1 diff --git a/src/platform/nxp/k32w/k32w0/CHIPDevicePlatformConfig.h b/src/platform/nxp/k32w/k32w0/CHIPDevicePlatformConfig.h index 600b065f7f3e03..13232a2819b4d2 100644 --- a/src/platform/nxp/k32w/k32w0/CHIPDevicePlatformConfig.h +++ b/src/platform/nxp/k32w/k32w0/CHIPDevicePlatformConfig.h @@ -99,5 +99,4 @@ #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_SRP_CLIENT 1 #define CHIP_DEVICE_CONFIG_ENABLE_THREAD_DNS_CLIENT 1 #define CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY 1 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 #endif diff --git a/src/platform/qpg/CHIPDevicePlatformConfig.h b/src/platform/qpg/CHIPDevicePlatformConfig.h index a4342e49f33d77..0f5367c7c69252 100644 --- a/src/platform/qpg/CHIPDevicePlatformConfig.h +++ b/src/platform/qpg/CHIPDevicePlatformConfig.h @@ -39,7 +39,6 @@ #endif #define CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY 1 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 #define CHIP_DEVICE_CONFIG_ENABLE_CHIP_TIME_SERVICE_TIME_SYNC 0 diff --git a/src/platform/webos/CHIPDevicePlatformConfig.h b/src/platform/webos/CHIPDevicePlatformConfig.h index 9a8fe4f699ebaa..2abb98fe27cb56 100644 --- a/src/platform/webos/CHIPDevicePlatformConfig.h +++ b/src/platform/webos/CHIPDevicePlatformConfig.h @@ -43,8 +43,6 @@ #define CHIP_DEVICE_CONFIG_ENABLE_CHIP_TIME_SERVICE_TIME_SYNC 0 -#define CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONABLE_DISCOVERY 1 - // ========== Platform-specific Configuration ========= // These are configuration options that are unique to webOs platforms. diff --git a/src/test_driver/linux-cirque/CommissioningTest.py b/src/test_driver/linux-cirque/CommissioningTest.py index e9e311d17daf4b..9ef3a838ea9859 100755 --- a/src/test_driver/linux-cirque/CommissioningTest.py +++ b/src/test_driver/linux-cirque/CommissioningTest.py @@ -38,6 +38,7 @@ CHIP_REPO = os.path.join(os.path.abspath( os.path.dirname(__file__)), "..", "..", "..") TEST_EXTPANID = "fedcba9876543210" +TEST_DISCRIMINATOR = 3840 DEVICE_CONFIG = { 'device0': { @@ -81,8 +82,8 @@ def run_controller_test(self): if device['type'] == 'MobileDevice'] for server in server_ids: - self.execute_device_cmd(server, "CHIPCirqueDaemon.py -- run gdb -return-child-result -q -ex \"set pagination off\" -ex run -ex \"bt 25\" --args {} --thread".format( - os.path.join(CHIP_REPO, "out/debug/standalone/chip-all-clusters-app"))) + self.execute_device_cmd(server, "CHIPCirqueDaemon.py -- run gdb -return-child-result -q -ex \"set pagination off\" -ex run -ex \"bt 25\" --args {} --thread --discriminator {}".format( + os.path.join(CHIP_REPO, "out/debug/standalone/chip-all-clusters-app"), TEST_DISCRIMINATOR)) self.reset_thread_devices(server_ids) diff --git a/src/test_driver/linux-cirque/MobileDeviceTest.py b/src/test_driver/linux-cirque/MobileDeviceTest.py index 9af296978b760d..90af77227bb533 100755 --- a/src/test_driver/linux-cirque/MobileDeviceTest.py +++ b/src/test_driver/linux-cirque/MobileDeviceTest.py @@ -38,6 +38,7 @@ CHIP_REPO = os.path.join(os.path.abspath( os.path.dirname(__file__)), "..", "..", "..") TEST_EXTPANID = "fedcba9876543210" +TEST_DISCRIMINATOR = 3840 DEVICE_CONFIG = { 'device0': { @@ -81,8 +82,8 @@ def run_controller_test(self): if device['type'] == 'MobileDevice'] for server in server_ids: - self.execute_device_cmd(server, "CHIPCirqueDaemon.py -- run gdb -return-child-result -q -ex \"set pagination off\" -ex run -ex \"bt 25\" --args {} --thread".format( - os.path.join(CHIP_REPO, "out/debug/standalone/chip-all-clusters-app"))) + self.execute_device_cmd(server, "CHIPCirqueDaemon.py -- run gdb -return-child-result -q -ex \"set pagination off\" -ex run -ex \"bt 25\" --args {} --thread --discriminator {}".format( + os.path.join(CHIP_REPO, "out/debug/standalone/chip-all-clusters-app"), TEST_DISCRIMINATOR)) self.reset_thread_devices(server_ids)