From 132be6e370e7c59d27aecd8727f8eeb37d9b0f05 Mon Sep 17 00:00:00 2001 From: Timothy Maes Date: Tue, 30 Aug 2022 20:22:34 +0200 Subject: [PATCH 1/5] Re-enable bloat reporting for QPG builds (#22221) --- .github/workflows/examples-qpg.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/examples-qpg.yaml b/.github/workflows/examples-qpg.yaml index d43855ab74c4a2..46d84ce5a538d9 100644 --- a/.github/workflows/examples-qpg.yaml +++ b/.github/workflows/examples-qpg.yaml @@ -81,6 +81,17 @@ jobs: run: | config/qpg/chip-gn/build.sh + - name: Prepare some bloat report from the previous builds + run: | + .environment/pigweed-venv/bin/python3 scripts/tools/memory/gh_sizes.py \ + qpg qpg6105+debug lighting-app \ + out/qpg-light/chip-qpg6105-lighting-example.out \ + /tmp/bloat_reports/ + .environment/pigweed-venv/bin/python3 scripts/tools/memory/gh_sizes.py \ + qpg qpg6105+debug lock-app \ + out/qpg-lock/chip-qpg6105-lock-example.out \ + /tmp/bloat_reports/ + - name: Uploading Size Reports uses: actions/upload-artifact@v2 if: ${{ !env.ACT }} From bb1ebbab66759e3d3d0b70852e7c0d601dfd471d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 30 Aug 2022 14:25:15 -0400 Subject: [PATCH 2/5] Don't notify OnSessionEstablishmentError after OnSessionEstablished. (#22261) A PairingSession that has sent an OnSessionEstablished notification should not send an OnSessionEstablishmentError notification after that (e.g. if the session gets evicted). The session is established at that point, and the _establishment_ cannot hit an error. This is needed to allow PASE sessions to be sanely evicted (e.g. when we're done with them) without triggering spurious errors. --- src/protocols/secure_channel/CASESession.cpp | 4 ++-- src/protocols/secure_channel/PASESession.cpp | 6 +++--- .../secure_channel/PairingSession.cpp | 21 +++++++++++++++++-- src/protocols/secure_channel/PairingSession.h | 6 ++++++ .../SessionEstablishmentDelegate.h | 8 +++++-- .../secure_channel/tests/TestPASESession.cpp | 17 +++++++++++++++ 6 files changed, 53 insertions(+), 9 deletions(-) diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 5f4c04752fe389..623265d160349a 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -131,7 +131,7 @@ void CASESession::OnSessionReleased() { Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); + NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); } void CASESession::Clear() @@ -294,7 +294,7 @@ void CASESession::AbortPendingEstablish(CHIP_ERROR err) { Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(err); + NotifySessionEstablishmentError(err); } CHIP_ERROR CASESession::DeriveSecureSession(CryptoContext & session) const diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 6c48957a14d275..05fc3b38915983 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -72,7 +72,7 @@ void PASESession::OnSessionReleased() { Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); + NotifySessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED); } void PASESession::Finish() @@ -242,7 +242,7 @@ void PASESession::OnResponseTimeout(ExchangeContext * ec) DiscardExchange(); Clear(); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); + NotifySessionEstablishmentError(CHIP_ERROR_TIMEOUT); } CHIP_ERROR PASESession::DeriveSecureSession(CryptoContext & session) const @@ -859,7 +859,7 @@ CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const Payl Clear(); ChipLogError(SecureChannel, "Failed during PASE session setup: %" CHIP_ERROR_FORMAT, err.Format()); // Do this last in case the delegate frees us. - mDelegate->OnSessionEstablishmentError(err); + NotifySessionEstablishmentError(err); } return err; } diff --git a/src/protocols/secure_channel/PairingSession.cpp b/src/protocols/secure_channel/PairingSession.cpp index 8c1dee5968ed4a..67d7229b462709 100644 --- a/src/protocols/secure_channel/PairingSession.cpp +++ b/src/protocols/secure_channel/PairingSession.cpp @@ -63,11 +63,15 @@ void PairingSession::Finish() if (err == CHIP_NO_ERROR) { VerifyOrDie(mSecureSessionHolder); - mDelegate->OnSessionEstablished(mSecureSessionHolder.Get().Value()); + // Make sure to null out mDelegate so we don't send it any other + // notifications. + auto * delegate = mDelegate; + mDelegate = nullptr; + delegate->OnSessionEstablished(mSecureSessionHolder.Get().Value()); } else { - mDelegate->OnSessionEstablishmentError(err); + NotifySessionEstablishmentError(err); } } @@ -165,4 +169,17 @@ void PairingSession::Clear() mSessionManager = nullptr; } +void PairingSession::NotifySessionEstablishmentError(CHIP_ERROR error) +{ + if (mDelegate == nullptr) + { + // Already notified success or error. + return; + } + + auto * delegate = mDelegate; + mDelegate = nullptr; + delegate->OnSessionEstablishmentError(error); +} + } // namespace chip diff --git a/src/protocols/secure_channel/PairingSession.h b/src/protocols/secure_channel/PairingSession.h index f8a707aa3af2d1..e220346e11b4a6 100644 --- a/src/protocols/secure_channel/PairingSession.h +++ b/src/protocols/secure_channel/PairingSession.h @@ -192,6 +192,12 @@ class DLL_EXPORT PairingSession : public SessionDelegate // TODO: remove Clear, we should create a new instance instead reset the old instance. void Clear(); + /** + * Notify our delegate about a session establishment error, if we have not + * notified it of an error or success before. + */ + void NotifySessionEstablishmentError(CHIP_ERROR error); + protected: CryptoContext::SessionRole mRole; SessionHolderWithDelegate mSecureSessionHolder; diff --git a/src/protocols/secure_channel/SessionEstablishmentDelegate.h b/src/protocols/secure_channel/SessionEstablishmentDelegate.h index f14566dae5a092..30cc8cac59f76b 100644 --- a/src/protocols/secure_channel/SessionEstablishmentDelegate.h +++ b/src/protocols/secure_channel/SessionEstablishmentDelegate.h @@ -36,7 +36,9 @@ class DLL_EXPORT SessionEstablishmentDelegate { public: /** - * Called when session establishment fails with an error + * Called when session establishment fails with an error. This will be + * called at most once per session establishment and will not be called if + * OnSessionEstablished is called. */ virtual void OnSessionEstablishmentError(CHIP_ERROR error) {} @@ -46,7 +48,9 @@ class DLL_EXPORT SessionEstablishmentDelegate virtual void OnSessionEstablishmentStarted() {} /** - * Called when the new secure session has been established + * Called when the new secure session has been established. This is + * mututally exclusive with OnSessionEstablishmentError for a give session + * establishment. */ virtual void OnSessionEstablished(const SessionHandle & session) {} diff --git a/src/protocols/secure_channel/tests/TestPASESession.cpp b/src/protocols/secure_channel/tests/TestPASESession.cpp index 61830fa174b530..ab97624e39e473 100644 --- a/src/protocols/secure_channel/tests/TestPASESession.cpp +++ b/src/protocols/secure_channel/tests/TestPASESession.cpp @@ -290,7 +290,9 @@ void SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inContext, S // Let's make sure atleast number is >= than the minimum messages required to complete the // handshake. NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount >= sTestPaseMessageCount); + NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0); NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 1); + NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 0); NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 1); if (mrpCommissionerConfig.HasValue()) @@ -313,6 +315,21 @@ void SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inContext, S mrpAccessoryConfig.Value().mActiveRetransTimeout); } + // Now evict the PASE sessions. + auto session = pairingCommissioner.CopySecureSession(); + NL_TEST_ASSERT(inSuite, session.HasValue()); + session.Value()->AsSecureSession()->MarkForEviction(); + + session = pairingAccessory.CopySecureSession(); + NL_TEST_ASSERT(inSuite, session.HasValue()); + session.Value()->AsSecureSession()->MarkForEviction(); + + // And check that this did not result in any new notifications. + NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0); + NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 1); + NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 0); + NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 1); + loopback.SetLoopbackTransportDelegate(nullptr); } From 8a0aad177fc127b6fc4d02b3954b1312d902e8a5 Mon Sep 17 00:00:00 2001 From: adabreuti <76965454+adabreuti@users.noreply.github.com> Date: Tue, 30 Aug 2022 13:37:49 -0500 Subject: [PATCH 3/5] Add DevinfoInit to Relevant CC13xx apps (#22184) * Add DevinfoInit to CC13xx all clusters * Add Devinfo init to lock-app * Update style --- examples/all-clusters-app/cc13x2x7_26x2x7/BUILD.gn | 2 ++ examples/all-clusters-app/cc13x2x7_26x2x7/main/AppTask.cpp | 7 +++++++ examples/all-clusters-minimal-app/cc13x2x7_26x2x7/BUILD.gn | 2 ++ .../cc13x2x7_26x2x7/main/AppTask.cpp | 7 +++++++ examples/lock-app/cc13x2x7_26x2x7/BUILD.gn | 2 ++ examples/lock-app/cc13x2x7_26x2x7/main/AppTask.cpp | 7 +++++++ 6 files changed, 27 insertions(+) diff --git a/examples/all-clusters-app/cc13x2x7_26x2x7/BUILD.gn b/examples/all-clusters-app/cc13x2x7_26x2x7/BUILD.gn index 48f3ef5b7583f9..3194d47bb0f188 100644 --- a/examples/all-clusters-app/cc13x2x7_26x2x7/BUILD.gn +++ b/examples/all-clusters-app/cc13x2x7_26x2x7/BUILD.gn @@ -78,6 +78,7 @@ ti_simplelink_executable("all-clusters-app") { "${chip_root}/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/bridged-actions-stub.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/static-supported-modes-manager.cpp", + "${chip_root}/examples/providers/DeviceInfoProviderImpl.cpp", "${project_dir}/main/AppTask.cpp", "${project_dir}/main/ClusterManager.cpp", "${project_dir}/main/Globals.cpp", @@ -102,6 +103,7 @@ ti_simplelink_executable("all-clusters-app") { "${project_dir}", "${project_dir}/main", "${chip_root}/examples/all-clusters-app/all-clusters-common/include", + "${chip_root}/examples/providers/", ] cflags = [ diff --git a/examples/all-clusters-app/cc13x2x7_26x2x7/main/AppTask.cpp b/examples/all-clusters-app/cc13x2x7_26x2x7/main/AppTask.cpp index b00e11a766a6da..09762791f4cf1a 100644 --- a/examples/all-clusters-app/cc13x2x7_26x2x7/main/AppTask.cpp +++ b/examples/all-clusters-app/cc13x2x7_26x2x7/main/AppTask.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #if CHIP_DEVICE_CONFIG_ENABLE_OTA_REQUESTOR @@ -64,6 +65,7 @@ static QueueHandle_t sAppEventQueue; static Button_Handle sAppLeftHandle; static Button_Handle sAppRightHandle; +static DeviceInfoProviderImpl sExampleDeviceInfoProvider; AppTask AppTask::sAppTask; @@ -243,6 +245,11 @@ int AppTask::Init() PLAT_LOG("Initialize Server"); static chip::CommonCaseDeviceServerInitParams initParams; (void) initParams.InitializeStaticResourcesBeforeServerInit(); + + // Initialize info provider + sExampleDeviceInfoProvider.SetStorageDelegate(initParams.persistentStorageDelegate); + SetDeviceInfoProvider(&sExampleDeviceInfoProvider); + chip::Server::GetInstance().Init(initParams); ConfigurationMgr().LogDeviceConfig(); diff --git a/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/BUILD.gn b/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/BUILD.gn index 9c870ec32fa84e..6c20ed346ec530 100644 --- a/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/BUILD.gn +++ b/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/BUILD.gn @@ -78,6 +78,7 @@ ti_simplelink_executable("all-clusters-minimal-app") { "${chip_root}/examples/all-clusters-app/all-clusters-common/src/binding-handler.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/bridged-actions-stub.cpp", "${chip_root}/examples/all-clusters-app/all-clusters-common/src/static-supported-modes-manager.cpp", + "${chip_root}/examples/providers/DeviceInfoProviderImpl.cpp", "${project_dir}/main/AppTask.cpp", "${project_dir}/main/ClusterManager.cpp", "${project_dir}/main/Globals.cpp", @@ -102,6 +103,7 @@ ti_simplelink_executable("all-clusters-minimal-app") { "${project_dir}", "${project_dir}/main", "${chip_root}/examples/all-clusters-app/all-clusters-common/include", + "${chip_root}/examples/providers/", ] cflags = [ diff --git a/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/main/AppTask.cpp b/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/main/AppTask.cpp index b00e11a766a6da..09762791f4cf1a 100644 --- a/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/main/AppTask.cpp +++ b/examples/all-clusters-minimal-app/cc13x2x7_26x2x7/main/AppTask.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #if CHIP_DEVICE_CONFIG_ENABLE_OTA_REQUESTOR @@ -64,6 +65,7 @@ static QueueHandle_t sAppEventQueue; static Button_Handle sAppLeftHandle; static Button_Handle sAppRightHandle; +static DeviceInfoProviderImpl sExampleDeviceInfoProvider; AppTask AppTask::sAppTask; @@ -243,6 +245,11 @@ int AppTask::Init() PLAT_LOG("Initialize Server"); static chip::CommonCaseDeviceServerInitParams initParams; (void) initParams.InitializeStaticResourcesBeforeServerInit(); + + // Initialize info provider + sExampleDeviceInfoProvider.SetStorageDelegate(initParams.persistentStorageDelegate); + SetDeviceInfoProvider(&sExampleDeviceInfoProvider); + chip::Server::GetInstance().Init(initParams); ConfigurationMgr().LogDeviceConfig(); diff --git a/examples/lock-app/cc13x2x7_26x2x7/BUILD.gn b/examples/lock-app/cc13x2x7_26x2x7/BUILD.gn index 57792e12f69e0c..ba2b327339e516 100644 --- a/examples/lock-app/cc13x2x7_26x2x7/BUILD.gn +++ b/examples/lock-app/cc13x2x7_26x2x7/BUILD.gn @@ -74,6 +74,7 @@ ti_simplelink_executable("lock_app") { output_name = "chip-${ti_simplelink_board}-lock-example.out" sources = [ + "${chip_root}/examples/providers/DeviceInfoProviderImpl.cpp", "${project_dir}/main/AppTask.cpp", "${project_dir}/main/BoltLockManager.cpp", "${project_dir}/main/ZclCallbacks.cpp", @@ -96,6 +97,7 @@ ti_simplelink_executable("lock_app") { include_dirs = [ "${project_dir}", "${project_dir}/main", + "${chip_root}/examples/providers/", ] cflags = [ diff --git a/examples/lock-app/cc13x2x7_26x2x7/main/AppTask.cpp b/examples/lock-app/cc13x2x7_26x2x7/main/AppTask.cpp index a0d30130644e43..4854eb4c5af0d8 100644 --- a/examples/lock-app/cc13x2x7_26x2x7/main/AppTask.cpp +++ b/examples/lock-app/cc13x2x7_26x2x7/main/AppTask.cpp @@ -27,6 +27,7 @@ #include #include +#include #include #if CHIP_DEVICE_CONFIG_ENABLE_OTA_REQUESTOR @@ -63,6 +64,7 @@ static LED_Handle sAppRedHandle; static LED_Handle sAppGreenHandle; static Button_Handle sAppLeftHandle; static Button_Handle sAppRightHandle; +static DeviceInfoProviderImpl sExampleDeviceInfoProvider; AppTask AppTask::sAppTask; @@ -166,6 +168,11 @@ int AppTask::Init() PLAT_LOG("Initialize Server"); static chip::CommonCaseDeviceServerInitParams initParams; (void) initParams.InitializeStaticResourcesBeforeServerInit(); + + // Initialize info provider + sExampleDeviceInfoProvider.SetStorageDelegate(initParams.persistentStorageDelegate); + SetDeviceInfoProvider(&sExampleDeviceInfoProvider); + chip::Server::GetInstance().Init(initParams); // Initialize device attestation config From 70143fe152a5dc97d6544315daa018cfe5fc6e77 Mon Sep 17 00:00:00 2001 From: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> Date: Tue, 30 Aug 2022 11:38:45 -0700 Subject: [PATCH 4/5] Fixes related to PASE-only commissioning on Android (#22140) * Draft: fixes related to PASE-only commissioning on Android * fix build * avoid null pointer in cleanup --- src/controller/AutoCommissioner.cpp | 18 +++++++++++++ src/controller/CHIPDeviceController.cpp | 1 - src/controller/CommissioningDelegate.cpp | 4 +++ src/controller/CommissioningDelegate.h | 10 ++++++++ .../java/AndroidDeviceControllerWrapper.cpp | 3 ++- .../java/AndroidDeviceControllerWrapper.h | 3 ++- .../java/CHIPDeviceController-JNI.cpp | 15 ++++++++--- .../devicecontroller/ControllerParams.java | 25 +++++++++++++++++++ 8 files changed, 72 insertions(+), 7 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index e9cc301f45b904..b7d7f0c70755f4 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -139,6 +139,12 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam } mParams.SetCSRNonce(ByteSpan(mCSRNonce, sizeof(mCSRNonce))); + if (params.GetSkipCommissioningComplete().HasValue()) + { + ChipLogProgress(Controller, "Setting PASE-only commissioning from parameters"); + mParams.SetSkipCommissioningComplete(params.GetSkipCommissioningComplete().Value()); + } + return CHIP_NO_ERROR; } @@ -252,6 +258,10 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio } else { + if (mParams.GetSkipCommissioningComplete().ValueOr(false)) + { + return CommissioningStage::kCleanup; + } return CommissioningStage::kFindOperational; } case CommissioningStage::kWiFiNetworkSetup: @@ -280,11 +290,19 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio { return CommissioningStage::kThreadNetworkEnable; } + else if (mParams.GetSkipCommissioningComplete().ValueOr(false)) + { + return CommissioningStage::kCleanup; + } else { return CommissioningStage::kFindOperational; } case CommissioningStage::kThreadNetworkEnable: + if (mParams.GetSkipCommissioningComplete().ValueOr(false)) + { + return CommissioningStage::kCleanup; + } return CommissioningStage::kFindOperational; case CommissioningStage::kFindOperational: return CommissioningStage::kSendComplete; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 88bef7820efd71..0452835651aec8 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1572,7 +1572,6 @@ void DeviceCommissioner::CommissioningStageComplete(CHIP_ERROR err, Commissionin { // Once this stage is complete, reset mDeviceBeingCommissioned - this will be reset when the delegate calls the next step. MATTER_TRACE_EVENT_SCOPE("CommissioningStageComplete", "DeviceCommissioner"); - if (mDeviceBeingCommissioned == nullptr) { // We are getting a stray callback (e.g. due to un-cancellable diff --git a/src/controller/CommissioningDelegate.cpp b/src/controller/CommissioningDelegate.cpp index 01d9c6ecd90f88..b6b83980130e98 100644 --- a/src/controller/CommissioningDelegate.cpp +++ b/src/controller/CommissioningDelegate.cpp @@ -113,6 +113,10 @@ const char * StageToString(CommissioningStage stage) return "Cleanup"; break; + case kNeedsNetworkCreds: + return "NeedsNetworkCreds"; + break; + default: return "???"; break; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index cf1743c2d21bae..1e3fdef1276e44 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -378,6 +378,15 @@ class CommissioningParameters return *this; } + // Only perform the PASE steps of commissioning. + // Commissioning will be completed by another admin on the network. + Optional GetSkipCommissioningComplete() const { return mSkipCommissioningComplete; } + CommissioningParameters & SetSkipCommissioningComplete(bool skipCommissioningComplete) + { + mSkipCommissioningComplete = MakeOptional(skipCommissioningComplete); + return *this; + } + private: // Items that can be set by the commissioner Optional mFailsafeTimerSeconds; @@ -407,6 +416,7 @@ class CommissioningParameters nullptr; // Delegate to handle device attestation failures during commissioning Optional mAttemptWiFiNetworkScan; Optional mAttemptThreadNetworkScan; // This automatically gets set to false when a ThreadOperationalDataset is set + Optional mSkipCommissioningComplete; }; struct RequestedCertificate diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp index b4a8c6714ec9eb..2be556d5a2bfd9 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.cpp +++ b/src/controller/java/AndroidDeviceControllerWrapper.cpp @@ -75,7 +75,7 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew( chip::Inet::EndPointManager * udpEndPointManager, AndroidOperationalCredentialsIssuerPtr opCredsIssuerPtr, jobject keypairDelegate, jbyteArray rootCertificate, jbyteArray intermediateCertificate, jbyteArray nodeOperationalCertificate, jbyteArray ipkEpochKey, uint16_t listenPort, uint16_t controllerVendorId, uint16_t failsafeTimerSeconds, - bool attemptNetworkScanWiFi, bool attemptNetworkScanThread, CHIP_ERROR * errInfoOnFailure) + bool attemptNetworkScanWiFi, bool attemptNetworkScanThread, bool skipCommissioningComplete, CHIP_ERROR * errInfoOnFailure) { if (errInfoOnFailure == nullptr) { @@ -156,6 +156,7 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew( params.SetFailsafeTimerSeconds(failsafeTimerSeconds); params.SetAttemptWiFiNetworkScan(attemptNetworkScanWiFi); params.SetAttemptThreadNetworkScan(attemptNetworkScanThread); + params.SetSkipCommissioningComplete(skipCommissioningComplete); wrapper->UpdateCommissioningParameters(params); CHIP_ERROR err = wrapper->mGroupDataProvider.Init(); diff --git a/src/controller/java/AndroidDeviceControllerWrapper.h b/src/controller/java/AndroidDeviceControllerWrapper.h index 799cd374e32439..8c0697b024170a 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.h +++ b/src/controller/java/AndroidDeviceControllerWrapper.h @@ -138,6 +138,7 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel * @param[in] failsafeTimerSeconds the failsafe timer in seconds * @param[in] attemptNetworkScanWiFi whether to attempt a network scan when configuring the network for a WiFi device * @param[in] attemptNetworkScanThread whether to attempt a network scan when configuring the network for a Thread device + * @param[in] skipCommissioningComplete whether to skip the CASE commissioningComplete command during commissioning * @param[out] errInfoOnFailure a pointer to a CHIP_ERROR that will be populated if this method returns nullptr */ static AndroidDeviceControllerWrapper * @@ -148,7 +149,7 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel AndroidOperationalCredentialsIssuerPtr opCredsIssuer, jobject keypairDelegate, jbyteArray rootCertificate, jbyteArray intermediateCertificate, jbyteArray nodeOperationalCertificate, jbyteArray ipkEpochKey, uint16_t listenPort, uint16_t controllerVendorId, uint16_t failsafeTimerSeconds, bool attemptNetworkScanWiFi, - bool attemptNetworkScanThread, CHIP_ERROR * errInfoOnFailure); + bool attemptNetworkScanThread, bool skipCommissioningComplete, CHIP_ERROR * errInfoOnFailure); chip::Controller::AndroidOperationalCredentialsIssuer * GetAndroidOperationalCredentialsIssuer() { diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index 9a46972dd54d79..5f2b6fb0b19caf 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -285,6 +285,11 @@ JNI_METHOD(jlong, newDeviceController)(JNIEnv * env, jobject self, jobject contr &getAttemptNetworkScanThread); SuccessOrExit(err); + jmethodID getSkipCommissioningComplete; + err = chip::JniReferences::GetInstance().FindMethod(env, controllerParams, "getSkipCommissioningComplete", "()Z", + &getSkipCommissioningComplete); + SuccessOrExit(err); + jmethodID getKeypairDelegate; err = chip::JniReferences::GetInstance().FindMethod(env, controllerParams, "getKeypairDelegate", "()Lchip/devicecontroller/KeypairDelegate;", &getKeypairDelegate); @@ -324,6 +329,7 @@ JNI_METHOD(jlong, newDeviceController)(JNIEnv * env, jobject self, jobject contr uint16_t failsafeTimerSeconds = env->CallIntMethod(controllerParams, getFailsafeTimerSeconds); bool attemptNetworkScanWiFi = env->CallBooleanMethod(controllerParams, getAttemptNetworkScanWiFi); bool attemptNetworkScanThread = env->CallBooleanMethod(controllerParams, getAttemptNetworkScanThread); + bool skipCommissioningComplete = env->CallBooleanMethod(controllerParams, getSkipCommissioningComplete); uint64_t adminSubject = env->CallLongMethod(controllerParams, getAdminSubject); std::unique_ptr opCredsIssuer( @@ -332,7 +338,7 @@ JNI_METHOD(jlong, newDeviceController)(JNIEnv * env, jobject self, jobject contr sJVM, self, kLocalDeviceId, fabricId, chip::kUndefinedCATs, &DeviceLayer::SystemLayer(), DeviceLayer::TCPEndPointManager(), DeviceLayer::UDPEndPointManager(), std::move(opCredsIssuer), keypairDelegate, rootCertificate, intermediateCertificate, operationalCertificate, ipk, listenPort, controllerVendorId, - failsafeTimerSeconds, attemptNetworkScanWiFi, attemptNetworkScanThread, &err); + failsafeTimerSeconds, attemptNetworkScanWiFi, attemptNetworkScanThread, skipCommissioningComplete, &err); SuccessOrExit(err); if (adminSubject != kUndefinedNodeId) @@ -384,7 +390,7 @@ JNI_METHOD(void, commissionDevice) ChipLogProgress(Controller, "commissionDevice() called"); - CommissioningParameters commissioningParams = CommissioningParameters(); + CommissioningParameters commissioningParams = wrapper->GetCommissioningParameters(); if (networkCredentials != nullptr) { err = wrapper->ApplyNetworkCredentials(commissioningParams, networkCredentials); @@ -422,7 +428,7 @@ JNI_METHOD(void, pairDevice) #endif .SetPeerAddress(Transport::PeerAddress::BLE()); - CommissioningParameters commissioningParams = CommissioningParameters(); + CommissioningParameters commissioningParams = wrapper->GetCommissioningParameters(); wrapper->ApplyNetworkCredentials(commissioningParams, networkCredentials); if (csrNonce != nullptr) @@ -456,7 +462,8 @@ JNI_METHOD(void, pairDeviceWithAddress) .SetDiscriminator(discriminator) .SetSetupPINCode(pinCode) .SetPeerAddress(Transport::PeerAddress::UDP(const_cast(addrJniString.c_str()), port)); - CommissioningParameters commissioningParams = CommissioningParameters(); + + CommissioningParameters commissioningParams = wrapper->GetCommissioningParameters(); if (csrNonce != nullptr) { JniByteArray jniCsrNonce(env, csrNonce); diff --git a/src/controller/java/src/chip/devicecontroller/ControllerParams.java b/src/controller/java/src/chip/devicecontroller/ControllerParams.java index 612079bfa29361..e2e429a6ad083d 100644 --- a/src/controller/java/src/chip/devicecontroller/ControllerParams.java +++ b/src/controller/java/src/chip/devicecontroller/ControllerParams.java @@ -11,6 +11,7 @@ public final class ControllerParams { private final int failsafeTimerSeconds; private final boolean attemptNetworkScanWiFi; private final boolean attemptNetworkScanThread; + private final boolean skipCommissioningComplete; @Nullable private final KeypairDelegate keypairDelegate; @Nullable private final byte[] rootCertificate; @Nullable private final byte[] intermediateCertificate; @@ -28,6 +29,7 @@ private ControllerParams(Builder builder) { this.failsafeTimerSeconds = builder.failsafeTimerSeconds; this.attemptNetworkScanWiFi = builder.attemptNetworkScanWiFi; this.attemptNetworkScanThread = builder.attemptNetworkScanThread; + this.skipCommissioningComplete = builder.skipCommissioningComplete; this.keypairDelegate = builder.keypairDelegate; this.rootCertificate = builder.rootCertificate; this.intermediateCertificate = builder.intermediateCertificate; @@ -61,6 +63,10 @@ public boolean getAttemptNetworkScanThread() { return attemptNetworkScanThread; } + public boolean getSkipCommissioningComplete() { + return skipCommissioningComplete; + } + public KeypairDelegate getKeypairDelegate() { return keypairDelegate; } @@ -112,6 +118,7 @@ public static class Builder { private int failsafeTimerSeconds = 30; private boolean attemptNetworkScanWiFi = false; private boolean attemptNetworkScanThread = false; + private boolean skipCommissioningComplete = false; @Nullable private KeypairDelegate keypairDelegate = null; @Nullable private byte[] rootCertificate = null; @Nullable private byte[] intermediateCertificate = null; @@ -197,6 +204,24 @@ public Builder setAttemptNetworkScanThread(boolean attemptNetworkScanThread) { return this; } + /** + * Disable the CASE phase of commissioning when the CommissioningComplete command is sent by + * this ChipDeviceCommissioner. + * + *

Specifically, this sets SkipCommissioningComplete in the CommissioningParameters passed to + * the CommissioningDelegate. + * + *

A controller will set this to true when the CASE phase of commissioning is done by a + * separate process, for example, by a Hub on the network. + * + * @param skipCommissioningComplete + * @return + */ + public Builder setSkipCommissioningComplete(boolean skipCommissioningComplete) { + this.skipCommissioningComplete = skipCommissioningComplete; + return this; + } + public Builder setKeypairDelegate(KeypairDelegate keypairDelegate) { this.keypairDelegate = keypairDelegate; return this; From b444d25462331714bf633a0f3715124786d0bf74 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 30 Aug 2022 15:30:59 -0400 Subject: [PATCH 5/5] Ignore DeviceCommissioner::OnDeviceConnectionFailureFn for unexpected devices. (#22247) Just like we ignore DeviceCommissioner::OnDeviceConnectedFn if the device id does not match mDeviceBeingCommissioned, we should ignore OnDeviceConnectionFailureFn. Fixes https://github.com/project-chip/connectedhomeip/issues/22244 --- src/controller/CHIPDeviceController.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 0452835651aec8..0bf9ee911359d9 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1653,6 +1653,13 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, const Scope error = CHIP_ERROR_INTERNAL; } + if (commissioner->mDeviceBeingCommissioned == nullptr || + commissioner->mDeviceBeingCommissioned->GetDeviceId() != peerId.GetNodeId()) + { + // Not the device we are trying to commission. + return; + } + if (commissioner->mCommissioningStage == CommissioningStage::kFindOperational && commissioner->mCommissioningDelegate != nullptr) {