Skip to content

Commit

Permalink
Fix python Wi-Fi/Thread setup with manual code
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
tcarmelveilleux committed Jun 14, 2024
1 parent a12a0c0 commit fe68310
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 22 deletions.
4 changes: 1 addition & 3 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 15 additions & 3 deletions src/controller/python/ChipDeviceController-ScriptBinding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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));
}

Expand Down
14 changes: 7 additions & 7 deletions src/controller/python/chip/ChipDeviceCtrl.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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.
Expand Down Expand Up @@ -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 = [
Expand Down Expand Up @@ -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.'''
Expand Down
6 changes: 3 additions & 3 deletions src/lib/support/SetupDiscriminator.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
41 changes: 37 additions & 4 deletions src/protocols/secure_channel/RendezvousParameters.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include <crypto/CHIPCryptoPAL.h>
#include <lib/support/logging/CHIPLogging.h>
#include <lib/support/SetupDiscriminator.h>
#include <messaging/ReliableMessageProtocolConfig.h>
#include <protocols/secure_channel/PASESession.h>

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down
6 changes: 4 additions & 2 deletions src/python_testing/matter_testing_support.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 ====")
Expand Down

0 comments on commit fe68310

Please sign in to comment.