diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 75c95ee20abc06..21cd71e2932b0a 100644 --- 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 index 2086b50dc7fea5..323c3ad0784027 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 & 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 index 612644ca72cf9e..a3a9b3040223be 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..2083cdc7cb23e7 100644 --- a/src/lib/support/SetupDiscriminator.h +++ b/src/lib/support/SetupDiscriminator.h @@ -23,16 +23,16 @@ #pragma once -#include - #include +#include + 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 index 5586dbada42791..c056a89a836139 100644 --- a/src/protocols/secure_channel/RendezvousParameters.h +++ b/src/protocols/secure_channel/RendezvousParameters.h @@ -17,6 +17,8 @@ #pragma once +#include + #include #include #if CONFIG_NETWORK_LAYER_BLE @@ -24,6 +26,7 @@ #endif // CONFIG_NETWORK_LAYER_BLE #include +#include #include #include #include @@ -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 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 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 ====") diff --git a/src/setup_payload/SetupPayload.h b/src/setup_payload/SetupPayload.h index cc71547671fd8f..dc67206dd51867 100644 --- 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 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 index abf6a530b51b3e..8b597fc666f4ba 100644 --- a/src/setup_payload/tests/TestManualCode.cpp +++ b/src/setup_payload/tests/TestManualCode.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -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)