More commissionable node work (#6910)

* Discovery: centralize subtype creation, add more.

This PR centralizes the coding from subtype to printed string so
we can more easily keep the advertising and discovery values aligned.
Also adds the remaining subtypes from the second 0.7 ballot.

NOTE: This PR purposefully does NOT include the change made to the
      subtype in spec PR #2429 to remove leading zeros. This is
      because that change will break discovery for devices on
      different versions of the SDK and needs to be put in as a
      separate PR with full PSA on the slack channels.

* Fix build.

I'm not sure how I missed adding these files in the first place.

* Review comments - enum size + tests + sizing.

Also added size checks to the various codes.

* Fix build.

* Couple of minor review comments.

src/controller/CHIPDeviceController.cpp

@@ -1435,7 +1435,7 @@ CHIP_ERROR DeviceCommissioner::DiscoverCommissioningLongDiscriminator(uint16_t l
     {
         mCommissionableNodes[i].Reset();
     }
-    Mdns::CommissionableNodeFilter filter(Mdns::CommissionableNodeFilterType::kLong, long_discriminator);
+    Mdns::DiscoveryFilter filter(Mdns::DiscoveryFilterType::kLong, long_discriminator);
     return Mdns::Resolver::Instance().FindCommissionableNodes(filter);
 }
 

src/lib/mdns/Discovery_ImplPlatform.cpp

@@ -235,28 +235,47 @@ CHIP_ERROR DiscoveryImplPlatform::Advertise(const CommissionAdvertisingParameter
                                              strnlen(pairingInstrBuf, sizeof(pairingInstrBuf)) };
         }
 
-        snprintf(shortDiscriminatorSubtype, sizeof(shortDiscriminatorSubtype), "_S%03u", params.GetShortDiscriminator());
-        subTypes[subTypeSize++] = shortDiscriminatorSubtype;
-        snprintf(longDiscriminatorSubtype, sizeof(longDiscriminatorSubtype), "_L%04u", params.GetLongDiscriminator());
-        subTypes[subTypeSize++] = longDiscriminatorSubtype;
-        snprintf(commissioningModeSubType, sizeof(commissioningModeSubType), "_C%u", params.GetCommissioningMode() ? 1 : 0);
-        subTypes[subTypeSize++] = commissioningModeSubType;
+        if (MakeServiceSubtype(shortDiscriminatorSubtype, sizeof(shortDiscriminatorSubtype),
+                               DiscoveryFilter(DiscoveryFilterType::kShort, params.GetShortDiscriminator())) == CHIP_NO_ERROR)
+        {
+            subTypes[subTypeSize++] = shortDiscriminatorSubtype;
+        }
+        if (MakeServiceSubtype(longDiscriminatorSubtype, sizeof(longDiscriminatorSubtype),
+                               DiscoveryFilter(DiscoveryFilterType::kLong, params.GetLongDiscriminator())) == CHIP_NO_ERROR)
+        {
+            subTypes[subTypeSize++] = longDiscriminatorSubtype;
+        }
+        if (MakeServiceSubtype(commissioningModeSubType, sizeof(commissioningModeSubType),
+                               DiscoveryFilter(DiscoveryFilterType::kCommissioningMode, params.GetCommissioningMode() ? 1 : 0)) ==
+            CHIP_NO_ERROR)
+        {
+            subTypes[subTypeSize++] = commissioningModeSubType;
+        }
         if (params.GetCommissioningMode() && params.GetOpenWindowCommissioningMode())
         {
-            snprintf(openWindowSubType, sizeof(openWindowSubType), "_A1");
-            subTypes[subTypeSize++] = openWindowSubType;
+            if (MakeServiceSubtype(openWindowSubType, sizeof(openWindowSubType),
+                                   DiscoveryFilter(DiscoveryFilterType::kCommissioningModeFromCommand, 1)) == CHIP_NO_ERROR)
+            {
+                subTypes[subTypeSize++] = openWindowSubType;
+            }
         }
     }
 
     if (params.GetVendorId().HasValue())
     {
-        snprintf(vendorSubType, sizeof(vendorSubType), "_V%u", params.GetVendorId().Value());
-        subTypes[subTypeSize++] = vendorSubType;
+        if (MakeServiceSubtype(vendorSubType, sizeof(vendorSubType),
+                               DiscoveryFilter(DiscoveryFilterType::kVendor, params.GetVendorId().Value())) == CHIP_NO_ERROR)
+        {
+            subTypes[subTypeSize++] = vendorSubType;
+        }
     }
     if (params.GetDeviceType().HasValue())
     {
-        snprintf(deviceTypeSubType, sizeof(deviceTypeSubType), "_T%u", params.GetDeviceType().Value());
-        subTypes[subTypeSize++] = deviceTypeSubType;
+        if (MakeServiceSubtype(deviceTypeSubType, sizeof(deviceTypeSubType),
+                               DiscoveryFilter(DiscoveryFilterType::kDeviceType, params.GetDeviceType().Value())) == CHIP_NO_ERROR)
+        {
+            subTypes[subTypeSize++] = deviceTypeSubType;
+        }
     }
 
     service.mTextEntries   = textEntries;

src/lib/mdns/Discovery_ImplPlatform.h

@@ -54,10 +54,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
     /// Requests resolution of a node ID to its address
     CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
 
-    CHIP_ERROR FindCommissionableNodes(CommissionableNodeFilter filter = CommissionableNodeFilter()) override
-    {
-        return CHIP_ERROR_NOT_IMPLEMENTED;
-    }
+    CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; }
 
     static DiscoveryImplPlatform & GetInstance();
 

src/lib/mdns/Resolver.h

@@ -65,19 +65,22 @@ struct CommissionableNodeData
     bool IsValid() const { return !IsHost("") && ipAddress[0] != chip::Inet::IPAddress::Any; }
 };
 
-enum class CommissionableNodeFilterType
+enum class DiscoveryFilterType : uint8_t
 {
     kNone,
     kShort,
     kLong,
     kVendor,
+    kDeviceType,
+    kCommissioningMode,
+    kCommissioningModeFromCommand,
 };
-struct CommissionableNodeFilter
+struct DiscoveryFilter
 {
-    CommissionableNodeFilterType type;
+    DiscoveryFilterType type;
     uint16_t code;
-    CommissionableNodeFilter() : type(CommissionableNodeFilterType::kNone), code(0) {}
-    CommissionableNodeFilter(CommissionableNodeFilterType newType, uint16_t newCode) : type(newType), code(newCode) {}
+    DiscoveryFilter() : type(DiscoveryFilterType::kNone), code(0) {}
+    DiscoveryFilter(DiscoveryFilterType newType, uint16_t newCode) : type(newType), code(newCode) {}
 };
 /// Groups callbacks for CHIP service resolution requests
 class ResolverDelegate
@@ -114,7 +117,7 @@ class Resolver
     virtual CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) = 0;
 
     // Finds all nodes with the given filter that are currently in commissioning mode.
-    virtual CHIP_ERROR FindCommissionableNodes(CommissionableNodeFilter filter = CommissionableNodeFilter()) = 0;
+    virtual CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) = 0;
 
     /// Provides the system-wide implementation of the service resolver
     static Resolver & Instance();

src/lib/mdns/Resolver_ImplMinimalMdns.cpp

@@ -366,14 +366,14 @@ class MinMdnsResolver : public Resolver, public MdnsPacketDelegate
     CHIP_ERROR StartResolver(chip::Inet::InetLayer * inetLayer, uint16_t port) override;
     CHIP_ERROR SetResolverDelegate(ResolverDelegate * delegate) override;
     CHIP_ERROR ResolveNodeId(const PeerId & peerId, Inet::IPAddressType type) override;
-    CHIP_ERROR FindCommissionableNodes(CommissionableNodeFilter filter = CommissionableNodeFilter()) override;
+    CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override;
 
 private:
     ResolverDelegate * mDelegate = nullptr;
     DiscoveryType mDiscoveryType = DiscoveryType::kUnknown;
 
     CHIP_ERROR SendQuery(mdns::Minimal::FullQName qname, mdns::Minimal::QType type);
-    CHIP_ERROR BrowseNodes(DiscoveryType type, CommissionableNodeFilter subtype);
+    CHIP_ERROR BrowseNodes(DiscoveryType type, DiscoveryFilter subtype);
     template <typename... Args>
     mdns::Minimal::FullQName CheckAndAllocateQName(Args &&... parts)
     {
@@ -446,41 +446,28 @@ CHIP_ERROR MinMdnsResolver::SendQuery(mdns::Minimal::FullQName qname, mdns::Mini
     return GlobalMinimalMdnsServer::Server().BroadcastSend(builder.ReleasePacket(), kMdnsPort);
 }
 
-CHIP_ERROR MinMdnsResolver::FindCommissionableNodes(CommissionableNodeFilter filter)
+CHIP_ERROR MinMdnsResolver::FindCommissionableNodes(DiscoveryFilter filter)
 {
     return BrowseNodes(DiscoveryType::kCommissionableNode, filter);
 }
 
 // TODO(cecille): Extend filter and use this for Resolve
-CHIP_ERROR MinMdnsResolver::BrowseNodes(DiscoveryType type, CommissionableNodeFilter filter)
+CHIP_ERROR MinMdnsResolver::BrowseNodes(DiscoveryType type, DiscoveryFilter filter)
 {
     mDiscoveryType    = type;
     char subtypeStr[] = "_Xdddddd";
 
     mdns::Minimal::FullQName qname;
 
-    switch (filter.type)
-    {
-    case CommissionableNodeFilterType::kShort:
-        snprintf(subtypeStr, strlen(subtypeStr), "_S%03u", filter.code);
-        break;
-    case CommissionableNodeFilterType::kLong:
-        snprintf(subtypeStr, strlen(subtypeStr), "_L%04u", filter.code);
-        break;
-    case CommissionableNodeFilterType::kVendor:
-        snprintf(subtypeStr, strlen(subtypeStr), "_V%03u", filter.code);
-        break;
-    case CommissionableNodeFilterType::kNone:
-        break;
-    }
+    MakeServiceSubtype(subtypeStr, sizeof(subtypeStr), filter);
 
     switch (type)
     {
     case DiscoveryType::kOperational:
         qname = CheckAndAllocateQName("_chip", "_tcp", "local");
         break;
     case DiscoveryType::kCommissionableNode:
-        if (filter.type == CommissionableNodeFilterType::kNone)
+        if (filter.type == DiscoveryFilterType::kNone)
         {
             qname = CheckAndAllocateQName("_chipc", "_udp", "local");
         }

src/lib/mdns/Resolver_ImplNone.cpp

@@ -35,10 +35,7 @@ class NoneResolver : public Resolver
         ChipLogError(Discovery, "Failed to resolve node ID: mDNS resolving not available");
         return CHIP_ERROR_NOT_IMPLEMENTED;
     }
-    CHIP_ERROR FindCommissionableNodes(CommissionableNodeFilter filter = CommissionableNodeFilter()) override
-    {
-        return CHIP_ERROR_NOT_IMPLEMENTED;
-    }
+    CHIP_ERROR FindCommissionableNodes(DiscoveryFilter filter = DiscoveryFilter()) override { return CHIP_ERROR_NOT_IMPLEMENTED; }
 };
 
 NoneResolver gResolver;

src/lib/mdns/ServiceNaming.cpp

@@ -119,5 +119,58 @@ CHIP_ERROR MakeHostName(char * buffer, size_t bufferLen, const chip::ByteSpan &
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR MakeServiceSubtype(char * buffer, size_t bufferLen, DiscoveryFilter subtype)
+{
+    size_t requiredSize;
+    switch (subtype.type)
+    {
+    case DiscoveryFilterType::kShort:
+        // 8-bit number
+        if (subtype.code >= 1 << 8)
+        {
+            return CHIP_ERROR_INVALID_ARGUMENT;
+        }
+        requiredSize = snprintf(buffer, bufferLen, "_S%03u", subtype.code);
+        break;
+    case DiscoveryFilterType::kLong:
+        // 12-bit number
+        if (subtype.code >= 1 << 12)
+        {
+            return CHIP_ERROR_INVALID_ARGUMENT;
+        }
+        requiredSize = snprintf(buffer, bufferLen, "_L%04u", subtype.code);
+        break;
+    case DiscoveryFilterType::kVendor:
+        // Vendor ID is 16-bit, so if it fits in the code, it's good.
+        // NOTE: size here is wrong, will be changed in upcming PR to remove leading zeros.
+        requiredSize = snprintf(buffer, bufferLen, "_V%03u", subtype.code);
+        break;
+    case DiscoveryFilterType::kDeviceType:
+        // TODO: Not totally clear the size required here: see spec issue #3226
+        requiredSize = snprintf(buffer, bufferLen, "_T%03u", subtype.code);
+        break;
+    case DiscoveryFilterType::kCommissioningMode:
+        if (subtype.code > 1)
+        {
+            return CHIP_ERROR_INVALID_ARGUMENT;
+        }
+        requiredSize = snprintf(buffer, bufferLen, "_C%u", subtype.code);
+        break;
+    case DiscoveryFilterType::kCommissioningModeFromCommand:
+        // 1 is the only valid value
+        if (subtype.code != 1)
+        {
+            return CHIP_ERROR_INVALID_ARGUMENT;
+        }
+        requiredSize = snprintf(buffer, bufferLen, "_A1");
+        break;
+    case DiscoveryFilterType::kNone:
+        requiredSize = 0;
+        buffer[0]    = '\0';
+        break;
+    }
+    return (requiredSize <= (bufferLen - 1)) ? CHIP_NO_ERROR : CHIP_ERROR_NO_MEMORY;
+}
+
 } // namespace Mdns
 } // namespace chip

src/lib/mdns/ServiceNaming.h

@@ -19,6 +19,7 @@
 
 #include <core/CHIPError.h>
 #include <core/PeerId.h>
+#include <mdns/Resolver.h>
 #include <support/Span.h>
 
 #include <cstddef>
@@ -37,5 +38,7 @@ CHIP_ERROR ExtractIdFromInstanceName(const char * name, PeerId * peerId);
 /// identifier (MAC address or EUI64)
 CHIP_ERROR MakeHostName(char * buffer, size_t bufferLen, const chip::ByteSpan & macOrEui64);
 
+CHIP_ERROR MakeServiceSubtype(char * buffer, size_t bufferLen, DiscoveryFilter subtype);
+
 } // namespace Mdns
 } // namespace chip

src/lib/mdns/tests/TestServiceNaming.cpp

@@ -81,9 +81,82 @@ void TestExtractIdFromInstanceName(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, ExtractIdFromInstanceName("1234-", &peerId) != CHIP_NO_ERROR);
 }
 
+void TestMakeServiceNameSubtype(nlTestSuite * inSuite, void * inContext)
+{
+    // TODO(cecille): These need to be changed to remove leading zeros
+    constexpr size_t kSize = 16;
+    char buffer[kSize];
+    DiscoveryFilter filter;
+
+    // Long tests
+    filter.type = DiscoveryFilterType::kLong;
+    filter.code = 3;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_L0003") == 0);
+
+    filter.code = (1 << 12) - 1;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_L4095") == 0);
+
+    filter.code = 1 << 12;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) != CHIP_NO_ERROR);
+
+    // Short tests
+    filter.type = DiscoveryFilterType::kShort;
+    filter.code = 3;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_S003") == 0);
+
+    filter.code = (1 << 8) - 1;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_S255") == 0);
+
+    filter.code = 1 << 8;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) != CHIP_NO_ERROR);
+
+    // Vendor tests
+    filter.type = DiscoveryFilterType::kVendor;
+    filter.code = 3;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_V003") == 0);
+    // TODO:add tests for longer vendor codes once the leading zero issue is fixed.
+
+    // Device Type tests
+    filter.type = DiscoveryFilterType::kDeviceType;
+    filter.code = 3;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_T003") == 0);
+    // TODO: Add tests for longer device types once the leadng zero issue is fixed.
+
+    // Commisioning mode tests
+    filter.type = DiscoveryFilterType::kCommissioningMode;
+    filter.code = 0;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_C0") == 0);
+    filter.code = 1;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_C1") == 0);
+    filter.code = 2; // only or or 1 allwoed
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) != CHIP_NO_ERROR);
+
+    // Commissioning mode open from command
+    filter.type = DiscoveryFilterType::kCommissioningModeFromCommand;
+    filter.code = 1;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "_A1") == 0);
+    filter.code = 0; // 1 is only value allowed
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) != CHIP_NO_ERROR);
+
+    // None tests.
+    filter.type = DiscoveryFilterType::kNone;
+    NL_TEST_ASSERT(inSuite, MakeServiceSubtype(buffer, sizeof(buffer), filter) == CHIP_NO_ERROR);
+    NL_TEST_ASSERT(inSuite, strcmp(buffer, "") == 0);
+}
+
 const nlTest sTests[] = {
     NL_TEST_DEF("MakeInstanceName", TestMakeInstanceName),                    //
     NL_TEST_DEF("ExtractIdFromInstandceName", TestExtractIdFromInstanceName), //
+    NL_TEST_DEF("TestMakeServiceNameSubtype", TestMakeServiceNameSubtype),    //
     NL_TEST_SENTINEL()                                                        //
 };