[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

@@ -22,6 +22,7 @@
 #include <lib/core/Optional.h>
 #include <lib/dnssd/Advertiser.h>
 #include <lib/dnssd/ServiceNaming.h>
+#include <lib/support/DefaultStorageKeyAllocator.h>
 #include <lib/support/Span.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <messaging/ReliableMessageProtocolConfig.h>
@@ -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<int>(mode));
+    ChipLogProgress(Discovery, "Updating services using commissioning mode %d", static_cast<int>(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();

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

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.

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.

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
 

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.

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

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

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

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
 

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.

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)
 

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)