Fix python Wi-Fi/Thread setup with manual code (project-chip#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 project-chip#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]>

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

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));
 }
 

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.'''

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

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;

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 ====")

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

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)