From 78aac89b869ffa4d9137b427202a16efbe1076be Mon Sep 17 00:00:00 2001 From: Yufeng Wang Date: Thu, 3 Oct 2024 10:58:47 -0700 Subject: [PATCH] Address review comments --- .../device_manager/PairingManager.cpp | 58 ++++++---- .../device_manager/PairingManager.h | 100 +++++++++--------- 2 files changed, 89 insertions(+), 69 deletions(-) diff --git a/examples/fabric-admin/device_manager/PairingManager.cpp b/examples/fabric-admin/device_manager/PairingManager.cpp index 64475c652b8552..c46a0ca000897b 100644 --- a/examples/fabric-admin/device_manager/PairingManager.cpp +++ b/examples/fabric-admin/device_manager/PairingManager.cpp @@ -108,7 +108,7 @@ CHIP_ERROR PairingManager::Init(Controller::DeviceCommissioner * commissioner, C VerifyOrReturnError(commissioner != nullptr, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(credIssuerCmds != nullptr, CHIP_ERROR_INCORRECT_STATE); - mCommissioner = commissioner; + mCommissioner = commissioner; mCredIssuerCmds = credIssuerCmds; return CHIP_NO_ERROR; @@ -253,7 +253,6 @@ void PairingManager::OnStatusUpdate(DevicePairingDelegate::Status status) switch (status) { case DevicePairingDelegate::Status::SecurePairingSuccess: - ChipLogProgress(NotSpecified, "Secure Pairing Success"); ChipLogProgress(NotSpecified, "CASE establishment successful"); break; case DevicePairingDelegate::Status::SecurePairingFailed: @@ -266,14 +265,12 @@ void PairingManager::OnPairingComplete(CHIP_ERROR err) { if (err == CHIP_NO_ERROR) { - ChipLogProgress(NotSpecified, "Pairing Success"); ChipLogProgress(NotSpecified, "PASE establishment successful"); } else { ChipLogProgress(NotSpecified, "Pairing Failure: %s", ErrorStr(err)); } - } void PairingManager::OnPairingDeleted(CHIP_ERROR err) @@ -293,8 +290,9 @@ void PairingManager::OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err) if (err == CHIP_NO_ERROR) { // print to console - fprintf(stderr, "New device with Node ID: 0x%lx has been successfully added.\n", nodeId); - // CurrentCommissioner() has a lifetime that is the entire life of the application itself + fprintf(stderr, "New device with Node ID: " ChipLogFormatX64 "has been successfully added.\n", ChipLogValueX64(nodeId)); + + // mCommissioner has a lifetime that is the entire life of the application itself // so it is safe to provide to StartDeviceSynchronization. DeviceSynchronizer::Instance().StartDeviceSynchronization(mCommissioner, nodeId, mDeviceIsICD); } @@ -418,6 +416,7 @@ void PairingManager::OnDiscoveredDevice(const Dnssd::CommissionNodeData & nodeDa err = Pair(mNodeId, peerAddress); if (CHIP_NO_ERROR != err) { + ChipLogProgress(NotSpecified, "Failed to pair device: " ChipLogFormatX64 " %s", ChipLogValueX64(mNodeId), ErrorStr(err)); } } @@ -473,17 +472,19 @@ void PairingManager::OnDeviceAttestationCompleted(Controller::DeviceCommissioner } // NOTE: This will log errors even if the attestion was successful. - auto err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(device, attestationResult); + CHIP_ERROR err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(device, attestationResult); if (CHIP_NO_ERROR != err) { + ChipLogError(NotSpecified, "Failed to continue commissioning after device attestation, error: %s", ErrorStr(err)); } return; } // Don't bypass attestation, continue with error. - auto err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(device, attestationResult); + CHIP_ERROR err = deviceCommissioner->ContinueCommissioningAfterDeviceAttestation(device, attestationResult); if (CHIP_NO_ERROR != err) { + ChipLogError(NotSpecified, "Failed to continue commissioning after device attestation, error: %s", ErrorStr(err)); } } @@ -548,11 +549,13 @@ void PairingManager::OnCurrentFabricRemove(void * context, NodeId nodeId, CHIP_E if (err == CHIP_NO_ERROR) { // print to console - fprintf(stderr, "Device with Node ID: 0x%lx has been successfully removed.\n", nodeId); + fprintf(stderr, "Device with Node ID: " ChipLogFormatX64 "has been successfully removed.\n", ChipLogValueX64(nodeId)); #if defined(PW_RPC_ENABLED) - app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(self->CurrentCommissioner().GetFabricIndex(), nodeId); - RemoveSynchronizedDevice(nodeId); + FabricIndex fabricIndex = self->CurrentCommissioner().GetFabricIndex(); + app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(fabricIndex, nodeId); + ScopedNodeId scopedNodeId(nodeId, fabricIndex); + RemoveSynchronizedDevice(scopedNodeId); #endif } else @@ -571,16 +574,17 @@ void PairingManager::InitPairingCommand() CHIP_ERROR PairingManager::PairDeviceWithCode(NodeId nodeId, const char * payload) { - if (payload == nullptr || strlen(payload) > kMaxManualCodeLength) + if (payload == nullptr || strlen(payload) > kMaxManualCodeLength + 1) { ChipLogError(NotSpecified, "PairDeviceWithCode failed: Invalid pairing payload"); return CHIP_ERROR_INVALID_STRING_LENGTH; } - auto params = Platform::MakeUnique(); - params->nodeId = nodeId; + auto params = Platform::MakeUnique(); + VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY); - Platform::CopyString(params->payloadBuffer, kMaxManualCodeLength, payload); + params->nodeId = nodeId; + Platform::CopyString(params->payloadBuffer, sizeof(params->payloadBuffer), payload); // Schedule work on the Matter thread return DeviceLayer::PlatformMgr().ScheduleWork(OnPairDeviceWithCode, reinterpret_cast(params.release())); @@ -615,12 +619,14 @@ CHIP_ERROR PairingManager::PairDevice(chip::NodeId nodeId, uint32_t setupPINCode return CHIP_ERROR_INVALID_STRING_LENGTH; } - auto params = Platform::MakeUnique(); + auto params = Platform::MakeUnique(); + VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY); + params->nodeId = nodeId; params->setupPINCode = setupPINCode; params->deviceRemotePort = deviceRemotePort; - Platform::CopyString(params->ipAddrBuffer, Inet::IPAddress::kMaxStringLength, deviceRemoteIp); + Platform::CopyString(params->ipAddrBuffer, sizeof(params->ipAddrBuffer), deviceRemoteIp); // Schedule work on the Matter thread return DeviceLayer::PlatformMgr().ScheduleWork(OnPairDevice, reinterpret_cast(params.release())); @@ -652,19 +658,31 @@ void PairingManager::OnPairDevice(intptr_t context) CHIP_ERROR PairingManager::UnpairDevice(NodeId nodeId) { + auto params = Platform::MakeUnique(); + VerifyOrReturnError(params != nullptr, CHIP_ERROR_NO_MEMORY); + + params->nodeId = nodeId; + // Schedule work on the Matter thread - return DeviceLayer::PlatformMgr().ScheduleWork(OnUnpairDevice, static_cast(nodeId)); + return DeviceLayer::PlatformMgr().ScheduleWork(OnUnpairDevice, reinterpret_cast(params.release())); } void PairingManager::OnUnpairDevice(intptr_t context) { - NodeId nodeId = static_cast(context); + Platform::UniquePtr params(reinterpret_cast(context)); PairingManager & self = PairingManager::Instance(); self.InitPairingCommand(); self.mCurrentFabricRemover = Platform::MakeUnique(self.mCommissioner); - CHIP_ERROR err = self.mCurrentFabricRemover->RemoveCurrentFabric(nodeId, &self.mCurrentFabricRemoveCallback); + + if (!self.mCurrentFabricRemover) + { + ChipLogError(NotSpecified, "Failed to unpair device, mCurrentFabricRemover is null"); + return; + } + + CHIP_ERROR err = self.mCurrentFabricRemover->RemoveCurrentFabric(params->nodeId, &self.mCurrentFabricRemoveCallback); if (err != CHIP_NO_ERROR) { ChipLogError(NotSpecified, "Failed to unpair device, error: %s", ErrorStr(err)); diff --git a/examples/fabric-admin/device_manager/PairingManager.h b/examples/fabric-admin/device_manager/PairingManager.h index 6023c8e0080aa7..563d129079d3f1 100644 --- a/examples/fabric-admin/device_manager/PairingManager.h +++ b/examples/fabric-admin/device_manager/PairingManager.h @@ -26,7 +26,7 @@ #include // Constants -constexpr uint16_t kMaxManualCodeLength = 21; +constexpr uint16_t kMaxManualCodeLength = 22; class CommissioningWindowDelegate { @@ -140,14 +140,6 @@ class PairingManager : public chip::Controller::DevicePairingDelegate, CHIP_ERROR UnpairDevice(chip::NodeId nodeId); private: - PairingManager(); - PairingManager(const PairingManager &) = delete; - PairingManager & operator=(const PairingManager &) = delete; - - chip::Controller::DeviceCommissioner * mCommissioner = nullptr; - CredentialIssuerCommands * mCredIssuerCmds; - - /////////// Open Commissioning Window Command Interface ///////// struct CommissioningWindowParams { chip::NodeId nodeId; @@ -162,21 +154,34 @@ class PairingManager : public chip::Controller::DevicePairingDelegate, chip::ByteSpan salt; }; - CommissioningWindowDelegate * mCommissioningWindowDelegate = nullptr; + struct PairDeviceWithCodeParams + { + chip::NodeId nodeId; + char payloadBuffer[kMaxManualCodeLength + 1]; + }; - /** - * Holds the unique_ptr to the current CommissioningWindowOpener. - * Only one commissioning window opener can be active at a time. - * The pointer is reset when the commissioning window is closed or when an error occurs. - */ - chip::Platform::UniquePtr mWindowOpener; - chip::Callback::Callback mOnOpenCommissioningWindowCallback; - chip::Callback::Callback mOnOpenCommissioningWindowVerifierCallback; + struct PairDeviceParams + { + chip::NodeId nodeId; + uint32_t setupPINCode; + uint16_t deviceRemotePort; + char ipAddrBuffer[chip::Inet::IPAddress::kMaxStringLength]; + }; - static void OnOpenCommissioningWindow(intptr_t context); - static void OnOpenCommissioningWindowResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status, - chip::SetupPayload payload); - static void OnOpenCommissioningWindowVerifierResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status); + struct UnpairDeviceParams + { + chip::NodeId nodeId; + }; + + // Constructors + PairingManager(); + PairingManager(const PairingManager &) = delete; + PairingManager & operator=(const PairingManager &) = delete; + + // Private member functions (static and non-static) + chip::Controller::CommissioningParameters GetCommissioningParameters(); + void InitPairingCommand(); + CHIP_ERROR Pair(chip::NodeId remoteId, chip::Transport::PeerAddress address); /////////// DevicePairingDelegate Interface ///////// void OnStatusUpdate(chip::Controller::DevicePairingDelegate::Status status) override; @@ -197,31 +202,28 @@ class PairingManager : public chip::Controller::DevicePairingDelegate, const chip::Credentials::DeviceAttestationVerifier::AttestationDeviceInfo & info, chip::Credentials::AttestationVerificationResult attestationResult) override; - /////////// Pairing Command Interface ///////// - struct PairDeviceWithCodeParams - { - chip::NodeId nodeId; - char payloadBuffer[kMaxManualCodeLength + 1]; - }; + static void OnOpenCommissioningWindow(intptr_t context); + static void OnOpenCommissioningWindowResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status, + chip::SetupPayload payload); + static void OnOpenCommissioningWindowVerifierResponse(void * context, chip::NodeId deviceId, CHIP_ERROR status); + static void OnCurrentFabricRemove(void * context, chip::NodeId remoteNodeId, CHIP_ERROR status); + static void OnPairDeviceWithCode(intptr_t context); + static void OnPairDevice(intptr_t context); + static void OnUnpairDevice(intptr_t context); - struct PairDeviceParams - { - chip::NodeId nodeId; - uint32_t setupPINCode; - uint16_t deviceRemotePort; - char ipAddrBuffer[chip::Inet::IPAddress::kMaxStringLength]; - }; + // Private data members + chip::Controller::DeviceCommissioner * mCommissioner = nullptr; + CredentialIssuerCommands * mCredIssuerCmds = nullptr; - CommissioningDelegate * mCommissioningDelegate = nullptr; - PairingDelegate * mPairingDelegate = nullptr; + CommissioningWindowDelegate * mCommissioningWindowDelegate = nullptr; + CommissioningDelegate * mCommissioningDelegate = nullptr; + PairingDelegate * mPairingDelegate = nullptr; chip::NodeId mNodeId = chip::kUndefinedNodeId; uint16_t mDiscriminator = 0; uint32_t mSetupPINCode = 0; const char * mOnboardingPayload = nullptr; - - // For ICD - bool mDeviceIsICD = false; + bool mDeviceIsICD = false; uint8_t mRandomGeneratedICDSymmetricKey[chip::Crypto::kAES_CCM128_Key_Length]; chip::Optional mICDRegistration; @@ -231,16 +233,16 @@ class PairingManager : public chip::Controller::DevicePairingDelegate, chip::Optional mICDMonitoredSubject; chip::Optional mICDStayActiveDurationMsec; + /** + * Holds the unique_ptr to the current CommissioningWindowOpener. + * Only one commissioning window opener can be active at a time. + * The pointer is reset when the commissioning window is closed or when an error occurs. + */ + chip::Platform::UniquePtr mWindowOpener; + chip::Callback::Callback mOnOpenCommissioningWindowCallback; + chip::Callback::Callback mOnOpenCommissioningWindowVerifierCallback; + // For Unpair chip::Platform::UniquePtr mCurrentFabricRemover; chip::Callback::Callback mCurrentFabricRemoveCallback; - - chip::Controller::CommissioningParameters GetCommissioningParameters(); - void InitPairingCommand(); - CHIP_ERROR Pair(chip::NodeId remoteId, chip::Transport::PeerAddress address); - - static void OnCurrentFabricRemove(void * context, chip::NodeId remoteNodeId, CHIP_ERROR status); - static void OnPairDeviceWithCode(intptr_t context); - static void OnPairDevice(intptr_t context); - static void OnUnpairDevice(intptr_t context); };