From fe683102c3195f21e51282af5744d41c7bbe2e48 Mon Sep 17 00:00:00 2001 From: "tennessee.carmelveilleux@gmail.com" Date: Fri, 14 Jun 2024 15:17:49 -0400 Subject: [PATCH] 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 --- src/controller/CHIPDeviceController.cpp | 4 +- .../ChipDeviceController-ScriptBinding.cpp | 18 ++++++-- src/controller/python/chip/ChipDeviceCtrl.py | 14 +++---- src/lib/support/SetupDiscriminator.h | 6 +-- .../secure_channel/RendezvousParameters.h | 41 +++++++++++++++++-- src/python_testing/matter_testing_support.py | 6 ++- 6 files changed, 67 insertions(+), 22 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 75c95ee20abc06..27b189a41a9dfc 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -793,10 +793,8 @@ 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)); + params.GetSetupDiscriminator(), this, OnDiscoveredDeviceOverBleSuccess, OnDiscoveredDeviceOverBleError)); ExitNow(CHIP_NO_ERROR); } else diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 2086b50dc7fea5..3b3dbb2e26fb81 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -76,6 +76,7 @@ #include #include #include +#include #include #include #include @@ -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); + } + 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 index 612644ca72cf9e..112fae16946157 100644 --- 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 index 5c1fabae71ba1e..78cc0eeac3afdd 100644 --- a/src/lib/support/SetupDiscriminator.h +++ b/src/lib/support/SetupDiscriminator.h @@ -32,7 +32,7 @@ namespace chip { class SetupDiscriminator { public: - constexpr SetupDiscriminator() : mDiscriminator(0), mIsShortDiscriminator(0) {} + constexpr SetupDiscriminator() : mDiscriminator(0xFFFFu), 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 index 5586dbada42791..4539d0c1e46138 100644 --- a/src/protocols/secure_channel/RendezvousParameters.h +++ b/src/protocols/secure_channel/RendezvousParameters.h @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -59,11 +60,42 @@ 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 mHasDiscriminator; } + + uint16_t GetDiscriminator() const { + if (mSetupDiscriminator.IsShortDiscriminator()) { + ChipLogError(Discovery, "Get RendezvousParameters::GetDiscriminator() called with SHORT discriminator (inconsistent). Using value 0 to avoid crash! Call GetSetupDiscriminator() to avoid loss."); + return 0; + } + + if (!mHasDiscriminator) + { + ChipLogError(Discovery, "Get RendezvousParameters::GetDiscriminator() called without discriminator in params (inconsistent). Using value 0 to avoid crash! Ensure discriminator is set!"); + return 0; + } + + return mSetupDiscriminator.GetLongValue(); + } + + SetupDiscriminator GetSetupDiscriminator() const { + if (!mHasDiscriminator) + { + ChipLogError(Discovery, "Get RendezvousParameters::GetSetupDiscriminator() called without discriminator in params (inconsistent). Returning last!"); + } + return mSetupDiscriminator; + } + + RendezvousParameters & SetSetupDiscriminator(SetupDiscriminator discriminator) + { + mSetupDiscriminator = discriminator; + mHasDiscriminator = true; + return *this; + } + RendezvousParameters & SetDiscriminator(uint16_t discriminator) { - mDiscriminator = discriminator; + mSetupDiscriminator.SetLongValue(discriminator); + mHasDiscriminator = true; return *this; } @@ -130,7 +162,8 @@ 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 + bool mHasDiscriminator = false; + 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 index c342d87ef2cf46..e0acefd1c61782 100644 --- 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 ====")