Fix python Wi-Fi/Thread setup with manual code (#33933)

* Fix python Wi-Fi/Thread setup with manual code - Plumbing was missing to pass down the short discriminator - Passing `--manual-code 1234-567-8901` which only has short discriminator, would always fail to find device over BLE Fixes #26907 This PR: - Adds plumbing to detect short discriminator in Python controller - Improves code-based setup in CHIPDeviceController to honor the SetupDiscriminator value, including whether short/long. Testing done: - Ran `python3 src/python_testing/TC_SC_3_6.py --commissioning-method ble-wifi --wifi-ssid MySsid --wifi-passphrase Secret123 --manual-code 2168-374-4904 --storage-path kvs1` - Before fix, discriminator always mismatched. - After fix, commissioning succeeds. - Unit tests and other integration tests still pass * Restyled by clang-format * Restyled by autopep8 * Add warning about GetDiscriminator * Improve unit test * Fix tests * Address review comments * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · Oct 7, 2024 · 1570978 · 1570978
1 parent a58741d
commit 1570978
Show file tree

Hide file tree

Showing 8 changed files with 103 additions and 39 deletions.
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -793,10 +793,9 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
             // for later.
             mRendezvousParametersForDeviceDiscoveredOverBle = params;
 
-            SetupDiscriminator discriminator;
-            discriminator.SetLongValue(params.GetDiscriminator());
-            SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(
-                              discriminator, this, OnDiscoveredDeviceOverBleSuccess, OnDiscoveredDeviceOverBleError));
+            SuccessOrExit(err = mSystemState->BleLayer()->NewBleConnectionByDiscriminator(params.GetSetupDiscriminator().value(),
+                                                                                          this, OnDiscoveredDeviceOverBleSuccess,
+                                                                                          OnDiscoveredDeviceOverBleError));
             ExitNow(CHIP_NO_ERROR);
         }
         else

diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp
@@ -76,6 +76,7 @@
 #include <lib/support/CodeUtils.h>
 #include <lib/support/DLLUtil.h>
 #include <lib/support/ScopedBuffer.h>
+#include <lib/support/SetupDiscriminator.h>
 #include <platform/CHIPDeviceLayer.h>
 #include <setup_payload/QRCodeSetupPayloadParser.h>
 #include <system/SystemClock.h>
@@ -140,7 +141,7 @@ PyChipError pychip_DeviceController_GetNodeId(chip::Controller::DeviceCommission
 
 // Rendezvous
 PyChipError pychip_DeviceController_ConnectBLE(chip::Controller::DeviceCommissioner * devCtrl, uint16_t discriminator,
-                                               uint32_t setupPINCode, chip::NodeId nodeid);
+                                               bool isShortDiscriminator, uint32_t setupPINCode, chip::NodeId nodeid);
 PyChipError pychip_DeviceController_ConnectIP(chip::Controller::DeviceCommissioner * devCtrl, const char * peerAddrStr,
                                               uint32_t setupPINCode, chip::NodeId nodeid);
 PyChipError pychip_DeviceController_ConnectWithCode(chip::Controller::DeviceCommissioner * devCtrl, const char * onboardingPayload,
@@ -377,13 +378,24 @@ const char * pychip_DeviceController_StatusReportToString(uint32_t profileId, ui
 }
 
 PyChipError pychip_DeviceController_ConnectBLE(chip::Controller::DeviceCommissioner * devCtrl, uint16_t discriminator,
-                                               uint32_t setupPINCode, chip::NodeId nodeid)
+                                               bool isShortDiscriminator, uint32_t setupPINCode, chip::NodeId nodeid)
 {
+    SetupDiscriminator setupDiscriminator;
+
+    if (isShortDiscriminator)
+    {
+        setupDiscriminator.SetShortValue(discriminator & 0xFu);
+    }
+    else
+    {
+        setupDiscriminator.SetLongValue(discriminator);
+    }
+
     return ToPyChipError(devCtrl->PairDevice(nodeid,
                                              chip::RendezvousParameters()
                                                  .SetPeerAddress(Transport::PeerAddress(Transport::Type::kBle))
                                                  .SetSetupPINCode(setupPINCode)
-                                                 .SetDiscriminator(discriminator),
+                                                 .SetSetupDiscriminator(setupDiscriminator),
                                              sCommissioningParameters));
 }
 

diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py
@@ -533,7 +533,7 @@ def IsConnected(self):
                 self.devCtrl)
         )
 
-    def ConnectBLE(self, discriminator, setupPinCode, nodeid) -> PyChipError:
+    def ConnectBLE(self, discriminator: int, setupPinCode: int, nodeid: int, isShortDiscriminator: bool = False) -> PyChipError:
         self.CheckIsActive()
 
         self._commissioning_complete_future = concurrent.futures.Future()
@@ -542,7 +542,7 @@ def ConnectBLE(self, discriminator, setupPinCode, nodeid) -> PyChipError:
             self._enablePairingCompeleteCallback(True)
             self._ChipStack.Call(
                 lambda: self._dmLib.pychip_DeviceController_ConnectBLE(
-                    self.devCtrl, discriminator, setupPinCode, nodeid)
+                    self.devCtrl, discriminator, isShortDiscriminator, setupPinCode, nodeid)
             ).raise_on_error()
 
             # TODO: Change return None. Only returning on success is not useful.
@@ -1590,7 +1590,7 @@ def _InitLib(self):
             self._dmLib.pychip_DeviceController_DeleteDeviceController.restype = PyChipError
 
             self._dmLib.pychip_DeviceController_ConnectBLE.argtypes = [
-                c_void_p, c_uint16, c_uint32, c_uint64]
+                c_void_p, c_uint16, c_bool, c_uint32, c_uint64]
             self._dmLib.pychip_DeviceController_ConnectBLE.restype = PyChipError
 
             self._dmLib.pychip_DeviceController_SetThreadOperationalDataset.argtypes = [
@@ -1882,17 +1882,17 @@ def Commission(self, nodeid) -> PyChipError:
         finally:
             self._commissioning_complete_future = None
 
-    def CommissionThread(self, discriminator, setupPinCode, nodeId, threadOperationalDataset: bytes) -> PyChipError:
+    def CommissionThread(self, discriminator, setupPinCode, nodeId, threadOperationalDataset: bytes, isShortDiscriminator: bool = False) -> PyChipError:
         ''' Commissions a Thread device over BLE
         '''
         self.SetThreadOperationalDataset(threadOperationalDataset)
-        return self.ConnectBLE(discriminator, setupPinCode, nodeId)
+        return self.ConnectBLE(discriminator, setupPinCode, nodeId, isShortDiscriminator)
 
-    def CommissionWiFi(self, discriminator, setupPinCode, nodeId, ssid: str, credentials: str) -> PyChipError:
+    def CommissionWiFi(self, discriminator, setupPinCode, nodeId, ssid: str, credentials: str, isShortDiscriminator: bool = False) -> PyChipError:
         ''' Commissions a Wi-Fi device over BLE.
         '''
         self.SetWiFiCredentials(ssid, credentials)
-        return self.ConnectBLE(discriminator, setupPinCode, nodeId)
+        return self.ConnectBLE(discriminator, setupPinCode, nodeId, isShortDiscriminator)
 
     def SetWiFiCredentials(self, ssid: str, credentials: str):
         ''' Set the Wi-Fi credentials to set during commissioning.'''

diff --git a/src/lib/support/SetupDiscriminator.h b/src/lib/support/SetupDiscriminator.h
@@ -23,16 +23,16 @@
 
 #pragma once
 
-#include <lib/support/CodeUtils.h>
-
 #include <cstdint>
 
+#include <lib/support/CodeUtils.h>
+
 namespace chip {
 
 class SetupDiscriminator
 {
 public:
-    constexpr SetupDiscriminator() : mDiscriminator(0), mIsShortDiscriminator(0) {}
+    constexpr SetupDiscriminator() : mDiscriminator(0), mIsShortDiscriminator(false) {}
 
     // See section 5.1.2. QR Code in the Matter specification
     static constexpr int kLongBits = 12;
@@ -104,8 +104,8 @@ class SetupDiscriminator
     // discriminator).
     static_assert(kLongBits == 12, "Unexpected field length");
     static_assert(kShortBits <= kLongBits, "Unexpected field length");
-    uint16_t mDiscriminator : 12;
-    uint16_t mIsShortDiscriminator : 1;
+    uint16_t mDiscriminator;
+    bool mIsShortDiscriminator;
 };
 
 } // namespace chip
diff --git a/src/protocols/secure_channel/RendezvousParameters.h b/src/protocols/secure_channel/RendezvousParameters.h
@@ -17,13 +17,16 @@
 
 #pragma once
 
+#include <optional>
+
 #include <transport/raw/Base.h>
 #include <transport/raw/PeerAddress.h>
 #if CONFIG_NETWORK_LAYER_BLE
 #include <ble/Ble.h>
 #endif // CONFIG_NETWORK_LAYER_BLE
 
 #include <crypto/CHIPCryptoPAL.h>
+#include <lib/support/SetupDiscriminator.h>
 #include <lib/support/logging/CHIPLogging.h>
 #include <messaging/ReliableMessageProtocolConfig.h>
 #include <protocols/secure_channel/PASESession.h>
@@ -59,11 +62,55 @@ class RendezvousParameters
 
     // Discriminators in RendezvousParameters are always long (12-bit)
     // discriminators.
-    bool HasDiscriminator() const { return mDiscriminator <= kMaxRendezvousDiscriminatorValue; }
-    uint16_t GetDiscriminator() const { return mDiscriminator; }
+    bool HasDiscriminator() const { return mSetupDiscriminator.has_value(); }
+
+    // Obtains the long version of the discriminator, or 0 if short.
+    // WARNING: This is lossy and a bad idea to use. The correct method to use
+    //          is GetSetupDiscriminator(). This method exists for public
+    //          API backwards compatibility.
+    uint16_t GetDiscriminator() const
+    {
+        if (!mSetupDiscriminator.has_value())
+        {
+            ChipLogError(Discovery,
+                         "Get RendezvousParameters::GetDiscriminator() called without discriminator in params (inconsistent). "
+                         "Using value 0 to avoid crash! Ensure discriminator is set!");
+            return 0;
+        }
+
+        if (mSetupDiscriminator.value().IsShortDiscriminator())
+        {
+            ChipLogError(Discovery,
+                         "Get RendezvousParameters::GetDiscriminator() called with SHORT discriminator (inconsistent). Using value "
+                         "0 to avoid crash! Call GetSetupDiscriminator() to avoid loss.");
+            return 0;
+        }
+
+        return mSetupDiscriminator.value().GetLongValue();
+    }
+
+    std::optional<SetupDiscriminator> GetSetupDiscriminator() const
+    {
+        if (!mSetupDiscriminator.has_value())
+        {
+            ChipLogError(
+                Discovery,
+                "Get RendezvousParameters::GetSetupDiscriminator() called without discriminator in params (inconsistent).");
+        }
+        return mSetupDiscriminator;
+    }
+
+    RendezvousParameters & SetSetupDiscriminator(SetupDiscriminator discriminator)
+    {
+        mSetupDiscriminator = discriminator;
+        return *this;
+    }
+
     RendezvousParameters & SetDiscriminator(uint16_t discriminator)
     {
-        mDiscriminator = discriminator;
+        SetupDiscriminator tempDiscriminator;
+        tempDiscriminator.SetLongValue(discriminator);
+        mSetupDiscriminator = tempDiscriminator;
         return *this;
     }
 
@@ -128,9 +175,9 @@ class RendezvousParameters
     }
 
 private:
-    Transport::PeerAddress mPeerAddress;  ///< the peer node address
-    uint32_t mSetupPINCode  = 0;          ///< the target peripheral setup PIN Code
-    uint16_t mDiscriminator = UINT16_MAX; ///< the target peripheral discriminator
+    Transport::PeerAddress mPeerAddress; ///< the peer node address
+    uint32_t mSetupPINCode = 0;          ///< the target peripheral setup PIN Code
+    std::optional<SetupDiscriminator> mSetupDiscriminator;
 
     Crypto::Spake2pVerifier mPASEVerifier;
     bool mHasPASEVerifier = false;

diff --git a/src/python_testing/matter_testing_support.py b/src/python_testing/matter_testing_support.py
@@ -1554,14 +1554,16 @@ def _commission_device(self, i) -> bool:
                 info.passcode,
                 conf.dut_node_ids[i],
                 conf.wifi_ssid,
-                conf.wifi_passphrase
+                conf.wifi_passphrase,
+                isShortDiscriminator=(info.filter_type == DiscoveryFilterType.SHORT_DISCRIMINATOR)
             )
         elif conf.commissioning_method == "ble-thread":
             return dev_ctrl.CommissionThread(
                 info.filter_value,
                 info.passcode,
                 conf.dut_node_ids[i],
-                conf.thread_operational_dataset
+                conf.thread_operational_dataset,
+                isShortDiscriminator=(info.filter_type == DiscoveryFilterType.SHORT_DISCRIMINATOR)
             )
         elif conf.commissioning_method == "on-network-ip":
             logging.warning("==== USING A DIRECT IP COMMISSIONING METHOD NOT SUPPORTED IN THE LONG TERM ====")

diff --git a/src/setup_payload/SetupPayload.h b/src/setup_payload/SetupPayload.h
@@ -126,7 +126,7 @@ struct PayloadContents
     // payload parsed from a QR code would always have a value for
     // rendezvousInformation.
     Optional<RendezvousInformationFlags> rendezvousInformation;
-    SetupDiscriminator discriminator;
+    SetupDiscriminator discriminator{};
     uint32_t setUpPINCode = 0;
 
     enum class ValidationMode : uint8_t

diff --git a/src/setup_payload/tests/TestManualCode.cpp b/src/setup_payload/tests/TestManualCode.cpp
@@ -26,6 +26,7 @@
 #include <stdio.h>
 
 #include <lib/core/StringBuilderAdapters.h>
+#include <lib/support/logging/CHIPLogging.h>
 #include <lib/support/verhoeff/Verhoeff.h>
 #include <setup_payload/ManualSetupPayloadGenerator.h>
 #include <setup_payload/ManualSetupPayloadParser.h>
@@ -397,11 +398,14 @@ TEST(TestManualCode, TestLongCodeReadWrite)
     EXPECT_TRUE(inPayload == outPayload);
 }
 
-void assertEmptyPayloadWithError(CHIP_ERROR actualError, CHIP_ERROR expectedError, const SetupPayload & payload)
+void assertEmptyPayloadWithError(CHIP_ERROR actualError, CHIP_ERROR expectedError, const SetupPayload & payload, int line)
 {
+    ChipLogProgress(Test, "Current check line: %d", line);
     EXPECT_EQ(actualError, expectedError);
-    EXPECT_TRUE(payload.setUpPINCode == 0 && payload.discriminator.GetLongValue() == 0 && payload.productID == 0 &&
-                payload.vendorID == 0);
+    EXPECT_EQ(payload.setUpPINCode, 0u);
+    EXPECT_EQ(payload.discriminator.GetLongValue(), 0u);
+    EXPECT_EQ(payload.productID, 0u);
+    EXPECT_EQ(payload.vendorID, 0u);
 }
 
 TEST(TestManualCode, TestPayloadParser_InvalidEntry)
@@ -413,46 +417,46 @@ TEST(TestManualCode, TestPayloadParser_InvalidEntry)
     decimalString = "";
     decimalString += Verhoeff10::ComputeCheckChar(decimalString.c_str());
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INVALID_STRING_LENGTH,
-                                payload);
+                                payload, __LINE__);
 
     // Invalid character
     decimalString = "24184.2196";
     decimalString += Verhoeff10::ComputeCheckChar(decimalString.c_str());
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INVALID_INTEGER_VALUE,
-                                payload);
+                                payload, __LINE__);
 
     // too short
     decimalString = "2456";
     decimalString += Verhoeff10::ComputeCheckChar(decimalString.c_str());
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INVALID_STRING_LENGTH,
-                                payload);
+                                payload, __LINE__);
 
     // too long for long code
     decimalString = "123456789123456785671";
     decimalString += Verhoeff10::ComputeCheckChar(decimalString.c_str());
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INVALID_STRING_LENGTH,
-                                payload);
+                                payload, __LINE__);
 
     // too long for short code
     decimalString = "12749875380";
     decimalString += Verhoeff10::ComputeCheckChar(decimalString.c_str());
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INVALID_STRING_LENGTH,
-                                payload);
+                                payload, __LINE__);
 
     // bit to indicate short code but long code length
     decimalString = "23456789123456785610";
     decimalString += Verhoeff10::ComputeCheckChar(decimalString.c_str());
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INVALID_STRING_LENGTH,
-                                payload);
+                                payload, __LINE__);
     // no pin code (= 0)
     decimalString = "2327680000";
     decimalString += Verhoeff10::ComputeCheckChar(decimalString.c_str());
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INVALID_ARGUMENT,
-                                payload);
+                                payload, __LINE__);
     // wrong check digit
     decimalString = "02684354589";
     assertEmptyPayloadWithError(ManualSetupPayloadParser(decimalString).populatePayload(payload), CHIP_ERROR_INTEGRITY_CHECK_FAILED,
-                                payload);
+                                payload, __LINE__);
 }
 
 TEST(TestManualCode, TestCheckDecimalStringValidity)