cleanup fixes relating to udc (#8739)

* cleanup fixes relating to udc

* add tests, fix platform eventloop conflict

* build fix

* restyled fix

* merge with latest

examples/minimal-mdns/server.cpp

@@ -51,7 +51,7 @@ struct Options
     const char * instanceName = "chip-mdns-demo";
 } gOptions;
 
-using namespace chip::ArgParser;
+using namespace ArgParser;
 
 constexpr uint16_t kOptionEnableIpV4   = '4';
 constexpr uint16_t kOptionListenPort   = 'p';
@@ -111,7 +111,7 @@ class ReplyDelegate : public mdns::Minimal::ServerDelegate, public mdns::Minimal
 public:
     ReplyDelegate(mdns::Minimal::ResponseSender * responder) : mResponder(responder) {}
 
-    void OnQuery(const mdns::Minimal::BytesRange & data, const chip::Inet::IPPacketInfo * info) override
+    void OnQuery(const mdns::Minimal::BytesRange & data, const Inet::IPPacketInfo * info) override
     {
         char addr[INET6_ADDRSTRLEN];
         info->SrcAddress.ToString(addr, sizeof(addr));
@@ -127,7 +127,7 @@ class ReplyDelegate : public mdns::Minimal::ServerDelegate, public mdns::Minimal
         mCurrentSource = nullptr;
     }
 
-    void OnResponse(const mdns::Minimal::BytesRange & data, const chip::Inet::IPPacketInfo * info) override
+    void OnResponse(const mdns::Minimal::BytesRange & data, const Inet::IPPacketInfo * info) override
     {
         char addr[INET6_ADDRSTRLEN];
         info->SrcAddress.ToString(addr, sizeof(addr));
@@ -158,8 +158,8 @@ class ReplyDelegate : public mdns::Minimal::ServerDelegate, public mdns::Minimal
     }
 
     mdns::Minimal::ResponseSender * mResponder;
-    const chip::Inet::IPPacketInfo * mCurrentSource = nullptr;
-    uint32_t mMessageId                             = 0;
+    const Inet::IPPacketInfo * mCurrentSource = nullptr;
+    uint32_t mMessageId                       = 0;
 };
 
 } // namespace
@@ -178,7 +178,7 @@ int main(int argc, char ** args)
         return 1;
     }
 
-    if (!chip::ArgParser::ParseArgs(args[0], argc, args, allOptions))
+    if (!ArgParser::ParseArgs(args[0], argc, args, allOptions))
     {
         return 1;
     }
@@ -188,27 +188,22 @@ int main(int argc, char ** args)
     mdns::Minimal::Server<10 /* endpoints */> mdnsServer;
     mdns::Minimal::QueryResponder<16 /* maxRecords */> queryResponder;
 
-    mdns::Minimal::QNamePart tcpServiceName[]       = { chip::Mdns::kOperationalServiceName, chip::Mdns::kOperationalProtocol,
-                                                  chip::Mdns::kLocalDomain };
-    mdns::Minimal::QNamePart tcpServerServiceName[] = { gOptions.instanceName, chip::Mdns::kOperationalServiceName,
-                                                        chip::Mdns::kOperationalProtocol, chip::Mdns::kLocalDomain };
-    mdns::Minimal::QNamePart udpServiceName[]       = { chip::Mdns::kCommissionableServiceName, chip::Mdns::kCommissionProtocol,
-                                                  chip::Mdns::kLocalDomain };
-    mdns::Minimal::QNamePart udpServerServiceName[] = { gOptions.instanceName, chip::Mdns::kCommissionableServiceName,
-                                                        chip::Mdns::kCommissionProtocol, chip::Mdns::kLocalDomain };
+    mdns::Minimal::QNamePart tcpServiceName[] = { Mdns::kOperationalServiceName, Mdns::kOperationalProtocol, Mdns::kLocalDomain };
+    mdns::Minimal::QNamePart tcpServerServiceName[] = { gOptions.instanceName, Mdns::kOperationalServiceName,
+                                                        Mdns::kOperationalProtocol, Mdns::kLocalDomain };
+    mdns::Minimal::QNamePart udpServiceName[] = { Mdns::kCommissionableServiceName, Mdns::kCommissionProtocol, Mdns::kLocalDomain };
+    mdns::Minimal::QNamePart udpServerServiceName[] = { gOptions.instanceName, Mdns::kCommissionableServiceName,
+                                                        Mdns::kCommissionProtocol, Mdns::kLocalDomain };
 
     // several UDP versions for discriminators
-    mdns::Minimal::QNamePart udpDiscriminator1[] = { "S52", chip::Mdns::kSubtypeServiceNamePart,
-                                                     chip::Mdns::kCommissionableServiceName, chip::Mdns::kCommissionProtocol,
-                                                     chip::Mdns::kLocalDomain };
-    mdns::Minimal::QNamePart udpDiscriminator2[] = { "V123", chip::Mdns::kSubtypeServiceNamePart,
-                                                     chip::Mdns::kCommissionableServiceName, chip::Mdns::kCommissionProtocol,
-                                                     chip::Mdns::kLocalDomain };
-    mdns::Minimal::QNamePart udpDiscriminator3[] = { "L840", chip::Mdns::kSubtypeServiceNamePart,
-                                                     chip::Mdns::kCommissionableServiceName, chip::Mdns::kCommissionProtocol,
-                                                     chip::Mdns::kLocalDomain };
-
-    mdns::Minimal::QNamePart serverName[] = { gOptions.instanceName, chip::Mdns::kLocalDomain };
+    mdns::Minimal::QNamePart udpDiscriminator1[] = { "S52", Mdns::kSubtypeServiceNamePart, Mdns::kCommissionableServiceName,
+                                                     Mdns::kCommissionProtocol, Mdns::kLocalDomain };
+    mdns::Minimal::QNamePart udpDiscriminator2[] = { "V123", Mdns::kSubtypeServiceNamePart, Mdns::kCommissionableServiceName,
+                                                     Mdns::kCommissionProtocol, Mdns::kLocalDomain };
+    mdns::Minimal::QNamePart udpDiscriminator3[] = { "L840", Mdns::kSubtypeServiceNamePart, Mdns::kCommissionableServiceName,
+                                                     Mdns::kCommissionProtocol, Mdns::kLocalDomain };
+
+    mdns::Minimal::QNamePart serverName[] = { gOptions.instanceName, Mdns::kLocalDomain };
 
     mdns::Minimal::IPv4Responder ipv4Responder(serverName);
     mdns::Minimal::IPv6Responder ipv6Responder(serverName);
@@ -256,7 +251,7 @@ int main(int argc, char ** args)
     {
         MdnsExample::AllInterfaces allInterfaces(gOptions.enableIpV4);
 
-        if (mdnsServer.Listen(&chip::DeviceLayer::InetLayer, &allInterfaces, gOptions.listenPort) != CHIP_NO_ERROR)
+        if (mdnsServer.Listen(&DeviceLayer::InetLayer, &allInterfaces, gOptions.listenPort) != CHIP_NO_ERROR)
         {
             printf("Server failed to listen on all interfaces\n");
             return 1;

examples/platform/linux/AppMain.cpp

@@ -184,11 +184,16 @@ CHIP_ERROR InitCommissioner()
     ReturnErrorOnFailure(gCommissioner.SetUdpListenPort(CHIP_PORT + 2));
     ReturnErrorOnFailure(gCommissioner.SetUdcListenPort(CHIP_PORT + 3));
     ReturnErrorOnFailure(gCommissioner.Init(localId, params));
-    ReturnErrorOnFailure(gCommissioner.ServiceEvents());
 
     return CHIP_NO_ERROR;
 }
 
+CHIP_ERROR ShutdownCommissioner()
+{
+    gCommissioner.Shutdown();
+    return CHIP_NO_ERROR;
+}
+
 #endif // CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
 
 void ChipLinuxAppMainLoop()
@@ -209,6 +214,11 @@ void ChipLinuxAppMainLoop()
 #endif // CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
 
     chip::DeviceLayer::PlatformMgr().RunEventLoop();
+
+#if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
+    ShutdownCommissioner();
+#endif // CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
+
 #if defined(ENABLE_CHIP_SHELL)
     shellThread.join();
 #endif

examples/platform/linux/CommissioneeShellCommands.cpp

@@ -59,7 +59,7 @@ static CHIP_ERROR PrintAllCommands()
     streamer_printf(sout, "  help                       Usage: commissionee <subcommand>\r\n");
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY_CLIENT
     streamer_printf(sout,
-                    "  sendudc <address> <port>   Send UDC message to address. Usage: commissionee sendudc 127.0.0.1 11100\r\n");
+                    "  sendudc <address> <port>   Send UDC message to address. Usage: commissionee sendudc 127.0.0.1 5543\r\n");
 #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY_CLIENT
     streamer_printf(sout, "\r\n");
 

src/BUILD.gn

@@ -54,6 +54,7 @@ if (chip_build_tests) {
       "${chip_root}/src/messaging/tests",
       "${chip_root}/src/protocols/bdx/tests",
       "${chip_root}/src/protocols/secure_channel/tests",
+      "${chip_root}/src/protocols/user_directed_commissioning/tests",
       "${chip_root}/src/system/tests",
       "${chip_root}/src/transport/retransmit/tests",
       "${chip_root}/src/transport/tests",

src/app/server/Server.cpp

@@ -588,27 +588,21 @@ CHIP_ERROR SendUserDirectedCommissioningRequest(chip::Transport::PeerAddress com
     }
     ChipLogDetail(AppServer, "instanceName=%s", nameBuffer);
 
-    // send UDC message 5 times per spec (no ACK on this message)
-    for (unsigned int i = 0; i < 5; i++)
+    chip::System::PacketBufferHandle payloadBuf = chip::MessagePacketBuffer::NewWithData(nameBuffer, strlen(nameBuffer));
+    if (payloadBuf.IsNull())
     {
-        chip::System::PacketBufferHandle payloadBuf = chip::MessagePacketBuffer::NewWithData(nameBuffer, strlen(nameBuffer));
-        if (payloadBuf.IsNull())
-        {
-            ChipLogError(AppServer, "Unable to allocate packet buffer\n");
-            return CHIP_ERROR_NO_MEMORY;
-        }
-
-        err = gUDCClient.SendUDCMessage(&gTransports, std::move(payloadBuf), commissioner);
-        if (err == CHIP_NO_ERROR)
-        {
-            ChipLogDetail(AppServer, "Send UDC request success");
-        }
-        else
-        {
-            ChipLogError(AppServer, "Send UDC request failed, err: %s\n", chip::ErrorStr(err));
-        }
+        ChipLogError(AppServer, "Unable to allocate packet buffer\n");
+        return CHIP_ERROR_NO_MEMORY;
+    }
 
-        sleep(1);
+    err = gUDCClient.SendUDCMessage(&gTransports, std::move(payloadBuf), commissioner);
+    if (err == CHIP_NO_ERROR)
+    {
+        ChipLogDetail(AppServer, "Send UDC request success");
+    }
+    else
+    {
+        ChipLogError(AppServer, "Send UDC request failed, err: %s\n", chip::ErrorStr(err));
     }
     return err;
 }

src/controller/CHIPDeviceController.cpp

@@ -864,17 +864,18 @@ CHIP_ERROR DeviceCommissioner::Shutdown()
     PersistDeviceList();
 
 #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable
+    if (mUdcTransportMgr != nullptr)
+    {
+        chip::Platform::Delete(mUdcTransportMgr);
+        mUdcTransportMgr = nullptr;
+    }
     if (mUdcServer != nullptr)
     {
         mUdcServer->SetInstanceNameResolver(nullptr);
         mUdcServer->SetUserConfirmationProvider(nullptr);
+        chip::Platform::Delete(mUdcServer);
         mUdcServer = nullptr;
     }
-    if (mUdcTransportMgr != nullptr)
-    {
-        chip::Platform::Delete(mUdcTransportMgr);
-        mUdcTransportMgr = nullptr;
-    }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
 
     DeviceController::Shutdown();

src/controller/python/ChipDeviceController-ScriptBinding.cpp

@@ -284,7 +284,7 @@ ChipError::StorageType pychip_DeviceController_CloseSession(chip::Controller::De
 
 ChipError::StorageType pychip_DeviceController_DiscoverAllCommissionableNodes(chip::Controller::DeviceCommissioner * devCtrl)
 {
-    Mdns::DiscoveryFilter filter(Mdns::DiscoveryFilterType::kNone, (uint16_t) 0);
+    Mdns::DiscoveryFilter filter(Mdns::DiscoveryFilterType::kNone, static_cast<uint16_t>(0));
     return devCtrl->DiscoverCommissionableNodes(filter).AsInteger();
 }
 

src/lib/mdns/Advertiser.h

@@ -197,7 +197,7 @@ class CommissionAdvertisingParameters : public BaseAdvertisingParams<CommissionA
     {
         if (deviceName.HasValue())
         {
-            chip::Platform::CopyString(mDeviceName, sizeof(mDeviceName), deviceName.Value());
+            Platform::CopyString(mDeviceName, sizeof(mDeviceName), deviceName.Value());
             mDeviceNameHasValue = true;
         }
         else
@@ -215,7 +215,7 @@ class CommissionAdvertisingParameters : public BaseAdvertisingParams<CommissionA
     {
         if (rotatingId.HasValue())
         {
-            chip::Platform::CopyString(mRotatingId, sizeof(mRotatingId), rotatingId.Value());
+            Platform::CopyString(mRotatingId, sizeof(mRotatingId), rotatingId.Value());
             mRotatingIdHasValue = true;
         }
         else
@@ -233,7 +233,7 @@ class CommissionAdvertisingParameters : public BaseAdvertisingParams<CommissionA
     {
         if (pairingInstr.HasValue())
         {
-            chip::Platform::CopyString(mPairingInstr, sizeof(mPairingInstr), pairingInstr.Value());
+            Platform::CopyString(mPairingInstr, sizeof(mPairingInstr), pairingInstr.Value());
             mPairingInstrHasValue = true;
         }
         else

src/lib/mdns/Resolver.h

@@ -118,10 +118,10 @@ struct DiscoveryFilter
 {
     DiscoveryFilterType type;
     uint16_t code;
-    char * instanceName;
+    const char * instanceName;
     DiscoveryFilter() : type(DiscoveryFilterType::kNone), code(0) {}
     DiscoveryFilter(DiscoveryFilterType newType, uint16_t newCode) : type(newType), code(newCode) {}
-    DiscoveryFilter(DiscoveryFilterType newType, char * newInstanceName) : type(newType), instanceName(newInstanceName) {}
+    DiscoveryFilter(DiscoveryFilterType newType, const char * newInstanceName) : type(newType), instanceName(newInstanceName) {}
 };
 enum class DiscoveryType
 {

src/protocols/user_directed_commissioning/UserDirectedCommissioning.h

@@ -41,6 +41,9 @@ namespace chip {
 namespace Protocols {
 namespace UserDirectedCommissioning {
 
+// Cache contains 16 clients. This may need to be tweaked.
+constexpr size_t kMaxUDCClients = 16;
+
 /**
  * User Directed Commissioning Protocol Message Types
  */
@@ -101,6 +104,18 @@ class DLL_EXPORT UserDirectedCommissioningClient
 
     CHIP_ERROR SendUDCMessage(TransportMgrBase * transportMgr, System::PacketBufferHandle && payload,
                               chip::Transport::PeerAddress peerAddress);
+
+    /**
+     * Encode a User Directed Commissioning message.
+     *
+     * @param payload       A PacketBufferHandle with the payload.
+     *
+     * @return CHIP_ERROR_NO_MEMORY if allocation fails.
+     *         Other CHIP_ERROR codes as returned by the lower layers.
+     *
+     */
+
+    CHIP_ERROR EncodeUDCMessage(System::PacketBufferHandle && payload);
 };
 
 class DLL_EXPORT UserDirectedCommissioningServer : public TransportMgrDelegate
@@ -164,14 +179,19 @@ class DLL_EXPORT UserDirectedCommissioningServer : public TransportMgrDelegate
      */
     void OnCommissionableNodeFound(const Mdns::DiscoveredNodeData & nodeData);
 
+    /**
+     * Get the cache of UDC Clients
+     *
+     */
+    UDCClients<kMaxUDCClients> GetUDCClients() { return mUdcClients; }
+
 private:
     InstanceNameResolver * mInstanceNameResolver         = nullptr;
     UserConfirmationProvider * mUserConfirmationProvider = nullptr;
 
     void OnMessageReceived(const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf) override;
 
-    // Cache contains 16 clients. This may need to be tweaked.
-    UDCClients<16> mUdcClients; // < Active UDC clients
+    UDCClients<kMaxUDCClients> mUdcClients; // < Active UDC clients
 };
 
 } // namespace UserDirectedCommissioning

src/protocols/user_directed_commissioning/UserDirectedCommissioningClient.cpp

@@ -32,8 +32,30 @@ namespace UserDirectedCommissioning {
 CHIP_ERROR UserDirectedCommissioningClient::SendUDCMessage(TransportMgrBase * transportMgr, System::PacketBufferHandle && payload,
                                                            chip::Transport::PeerAddress peerAddress)
 {
-    CHIP_ERROR err;
+    CHIP_ERROR err = EncodeUDCMessage(std::move(payload));
+    if (err != CHIP_NO_ERROR)
+    {
+        return err;
+    }
+    ChipLogProgress(Inet, "Sending UDC msg");
+
+    // send UDC message 5 times per spec (no ACK on this message)
+    for (unsigned int i = 0; i < 5; i++)
+    {
+        err = transportMgr->SendMessage(peerAddress, std::move(payload));
+        if (err != CHIP_NO_ERROR)
+        {
+            ChipLogError(AppServer, "UDC SendMessage failed, err: %s\n", chip::ErrorStr(err));
+            return err;
+        }
+        sleep(1);
+    }
+    ChipLogProgress(Inet, "UDC msg send status %s", ErrorStr(err));
+    return err;
+}
 
+CHIP_ERROR UserDirectedCommissioningClient::EncodeUDCMessage(System::PacketBufferHandle && payload)
+{
     PayloadHeader payloadHeader;
     PacketHeader packetHeader;
 
@@ -49,11 +71,7 @@ CHIP_ERROR UserDirectedCommissioningClient::SendUDCMessage(TransportMgrBase * tr
 
     ReturnErrorOnFailure(packetHeader.EncodeBeforeData(payload));
 
-    ChipLogProgress(Inet, "Sending UDC msg");
-    err = transportMgr->SendMessage(peerAddress, std::move(payload));
-
-    ChipLogProgress(Inet, "UDC msg send status %s", ErrorStr(err));
-    return err;
+    return CHIP_NO_ERROR;
 }
 
 } // namespace UserDirectedCommissioning

src/protocols/user_directed_commissioning/tests/BUILD.gn

@@ -0,0 +1,34 @@
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import("//build_overrides/build.gni")
+import("//build_overrides/chip.gni")
+import("//build_overrides/nlio.gni")
+import("//build_overrides/nlunit_test.gni")
+
+import("${chip_root}/build/chip/chip_test_suite.gni")
+
+chip_test_suite("tests") {
+  output_name = "libUserDirectedCommissioningTests"
+
+  test_sources = [ "TestUdcMessages.cpp" ]
+
+  public_deps = [
+    "${chip_root}/src/lib/core",
+    "${chip_root}/src/lib/support",
+    "${chip_root}/src/protocols",
+    "${nlio_root}:nlio",
+    "${nlunit_test_root}:nlunit-test",
+  ]
+
+  cflags = [ "-Wconversion" ]
+}