From c554f44a681e09e45724b604e27b6ecb69859590 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 7 Mar 2024 16:46:21 -0500 Subject: [PATCH 1/7] Stop treating BUSY response as fatal when CASE re-attempts are enabled. (#32490) 1. We now communicate the requested delay to the session establishment delegate when we get a BUSY response. 2. OperationalSessionSetup, when it has CASE re-attempts enabled, treats a BUSY response as a trigger to reattempt, not a fatal error. 3. In CASEServer, set the requested delay to a better value if we have sent Sigma2 and are waiting for a response to that. Fixes https://github.com/project-chip/connectedhomeip/issues/28288 --- src/app/OperationalSessionSetup.cpp | 43 ++++++++++++++++--- src/app/OperationalSessionSetup.h | 3 ++ src/protocols/secure_channel/CASEServer.cpp | 28 ++++++++++-- src/protocols/secure_channel/CASESession.cpp | 7 ++- src/protocols/secure_channel/CASESession.h | 3 +- src/protocols/secure_channel/PASESession.cpp | 3 +- src/protocols/secure_channel/PASESession.h | 3 +- src/protocols/secure_channel/PairingSession.h | 12 ++++-- .../SessionEstablishmentDelegate.h | 10 +++++ 9 files changed, 97 insertions(+), 15 deletions(-) diff --git a/src/app/OperationalSessionSetup.cpp b/src/app/OperationalSessionSetup.cpp index 179e2a3df5e120..6df07ca6b0568e 100644 --- a/src/app/OperationalSessionSetup.cpp +++ b/src/app/OperationalSessionSetup.cpp @@ -430,7 +430,7 @@ void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error, Sess // member instead of having a boolean // mTryingNextResultDueToSessionEstablishmentError, so we can recover the // error in UpdateDeviceData. - if (CHIP_ERROR_TIMEOUT == error) + if (CHIP_ERROR_TIMEOUT == error || CHIP_ERROR_BUSY == error) { #if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES // Make a copy of the ReliableMessageProtocolConfig, since our @@ -480,6 +480,15 @@ void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error, Sess // Do not touch `this` instance anymore; it has been destroyed in DequeueConnectionCallbacks. } +void OperationalSessionSetup::OnResponderBusy(System::Clock::Milliseconds16 requestedDelay) +{ +#if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES + // Store the requested delay, so that we can use it for scheduling our + // retry. + mRequestedBusyDelay = requestedDelay; +#endif +} + void OperationalSessionSetup::OnSessionEstablished(const SessionHandle & session) { VerifyOrReturn(mState == State::Connecting, @@ -705,9 +714,22 @@ CHIP_ERROR OperationalSessionSetup::ScheduleSessionSetupReattempt(System::Clock: static_assert(UINT16_MAX / CHIP_DEVICE_CONFIG_AUTOMATIC_CASE_RETRY_INITIAL_DELAY_SECONDS >= (1 << CHIP_DEVICE_CONFIG_AUTOMATIC_CASE_RETRY_MAX_BACKOFF), "Our backoff calculation will overflow."); - timerDelay = System::Clock::Seconds16( + System::Clock::Timeout actualTimerDelay = System::Clock::Seconds16( static_cast(CHIP_DEVICE_CONFIG_AUTOMATIC_CASE_RETRY_INITIAL_DELAY_SECONDS << min((mAttemptsDone - 1), CHIP_DEVICE_CONFIG_AUTOMATIC_CASE_RETRY_MAX_BACKOFF))); + const bool responseWasBusy = mRequestedBusyDelay != System::Clock::kZero; + if (responseWasBusy) + { + if (mRequestedBusyDelay > actualTimerDelay) + { + actualTimerDelay = mRequestedBusyDelay; + } + + // Reset mRequestedBusyDelay now that we have consumed it, so it does + // not affect future reattempts not triggered by a busy response. + mRequestedBusyDelay = System::Clock::kZero; + } + if (mAttemptsDone % 2 == 0) { // It's possible that the other side received one of our Sigma1 messages @@ -716,11 +738,22 @@ CHIP_ERROR OperationalSessionSetup::ScheduleSessionSetupReattempt(System::Clock: // listening for Sigma1 messages again. // // To handle that, on every other retry, add the amount of time it would - // take the other side to time out. + // take the other side to time out. It would be nice if we could rely + // on the delay reported in a BUSY response to just tell us that value, + // but in practice for old devices BUSY often sends some hardcoded value + // that tells us nothing about when the other side will decide it has + // timed out. auto additionalTimeout = CASESession::ComputeSigma2ResponseTimeout(GetLocalMRPConfig().ValueOr(GetDefaultMRPConfig())); - timerDelay += std::chrono::duration_cast(additionalTimeout); + actualTimerDelay += additionalTimeout; } - CHIP_ERROR err = mInitParams.exchangeMgr->GetSessionManager()->SystemLayer()->StartTimer(timerDelay, TrySetupAgain, this); + timerDelay = std::chrono::duration_cast(actualTimerDelay); + + CHIP_ERROR err = mInitParams.exchangeMgr->GetSessionManager()->SystemLayer()->StartTimer(actualTimerDelay, TrySetupAgain, this); + + // TODO: If responseWasBusy, should we increment, mRemainingAttempts and + // mResolveAttemptsAllowed, since we were explicitly told to retry? Hard to + // tell what consumers expect out of a capped retry count here. + // The cast on count() is needed because the type count() returns might not // actually be uint16_t; on some platforms it's int. ChipLogProgress(Discovery, diff --git a/src/app/OperationalSessionSetup.h b/src/app/OperationalSessionSetup.h index 45b571a08aa633..9d24faad5efabb 100644 --- a/src/app/OperationalSessionSetup.h +++ b/src/app/OperationalSessionSetup.h @@ -227,6 +227,7 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate, //////////// SessionEstablishmentDelegate Implementation /////////////// void OnSessionEstablished(const SessionHandle & session) override; void OnSessionEstablishmentError(CHIP_ERROR error, SessionEstablishmentStage stage) override; + void OnResponderBusy(System::Clock::Milliseconds16 requestedDelay) override; ScopedNodeId GetPeerId() const { return mPeerId; } @@ -319,6 +320,8 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate, uint8_t mResolveAttemptsAllowed = 0; + System::Clock::Milliseconds16 mRequestedBusyDelay = System::Clock::kZero; + Callback::CallbackDeque mConnectionRetry; #endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES diff --git a/src/protocols/secure_channel/CASEServer.cpp b/src/protocols/secure_channel/CASEServer.cpp index 2ad196a31b9461..df0984d4d94eee 100644 --- a/src/protocols/secure_channel/CASEServer.cpp +++ b/src/protocols/secure_channel/CASEServer.cpp @@ -90,9 +90,31 @@ CHIP_ERROR CASEServer::OnMessageReceived(Messaging::ExchangeContext * ec, const // Handshake wasn't stuck, send the busy status report and let the existing handshake continue. // A successful CASE handshake can take several seconds and some may time out (30 seconds or more). - // TODO: Come up with better estimate: https://github.com/project-chip/connectedhomeip/issues/28288 - // For now, setting minimum wait time to 5000 milliseconds. - CHIP_ERROR err = SendBusyStatusReport(ec, System::Clock::Milliseconds16(5000)); + + System::Clock::Milliseconds16 delay = System::Clock::kZero; + if (GetSession().GetState() == CASESession::State::kSentSigma2) + { + // The delay should be however long we think it will take for + // that to time out. + auto sigma2Timeout = CASESession::ComputeSigma2ResponseTimeout(GetSession().GetRemoteMRPConfig()); + if (sigma2Timeout < System::Clock::Milliseconds16::max()) + { + delay = std::chrono::duration_cast(sigma2Timeout); + } + else + { + // Avoid overflow issues, just wait for as long as we can to + // get close to our expected Sigma2 timeout. + delay = System::Clock::Milliseconds16::max(); + } + } + else + { + // For now, setting minimum wait time to 5000 milliseconds if we + // have no other information. + delay = System::Clock::Milliseconds16(5000); + } + CHIP_ERROR err = SendBusyStatusReport(ec, delay); if (err != CHIP_NO_ERROR) { ChipLogError(Inet, "Failed to send the busy status report, err:%" CHIP_ERROR_FORMAT, err.Format()); diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 30d1f71cb0ff64..a9b62489783794 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -1966,7 +1966,8 @@ void CASESession::OnSuccessStatusReport() Finish(); } -CHIP_ERROR CASESession::OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) +CHIP_ERROR CASESession::OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode, + Optional protocolData) { CHIP_ERROR err = CHIP_NO_ERROR; switch (protocolCode) @@ -1981,6 +1982,10 @@ CHIP_ERROR CASESession::OnFailureStatusReport(Protocols::SecureChannel::GeneralS case kProtocolCodeBusy: err = CHIP_ERROR_BUSY; + if (protocolData.HasValue()) + { + mDelegate->OnResponderBusy(System::Clock::Milliseconds16(static_cast(protocolData.Value()))); + } break; default: diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index cb7f3c7f13380d..9e41f6c69fbe84 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -272,7 +272,8 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler, const ByteSpan & skInfo, const ByteSpan & nonce); void OnSuccessStatusReport() override; - CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) override; + CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode, + Optional protocolData) override; void AbortPendingEstablish(CHIP_ERROR err); diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 50b186100841e0..40dc67793604e2 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -759,7 +759,8 @@ void PASESession::OnSuccessStatusReport() Finish(); } -CHIP_ERROR PASESession::OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) +CHIP_ERROR PASESession::OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode, + Optional protocolData) { CHIP_ERROR err = CHIP_NO_ERROR; switch (protocolCode) diff --git a/src/protocols/secure_channel/PASESession.h b/src/protocols/secure_channel/PASESession.h index 393d3a65fc958a..e270baf42e80f1 100644 --- a/src/protocols/secure_channel/PASESession.h +++ b/src/protocols/secure_channel/PASESession.h @@ -203,7 +203,8 @@ class DLL_EXPORT PASESession : public Messaging::UnsolicitedMessageHandler, CHIP_ERROR HandleMsg3(System::PacketBufferHandle && msg); void OnSuccessStatusReport() override; - CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) override; + CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode, + Optional protocolData) override; void Finish(); diff --git a/src/protocols/secure_channel/PairingSession.h b/src/protocols/secure_channel/PairingSession.h index 8ed9f269b9a32c..f49dbf7997f4e2 100644 --- a/src/protocols/secure_channel/PairingSession.h +++ b/src/protocols/secure_channel/PairingSession.h @@ -26,6 +26,7 @@ #pragma once #include +#include #include #include #include @@ -129,7 +130,11 @@ class DLL_EXPORT PairingSession : public SessionDelegate void SetPeerSessionId(uint16_t id) { mPeerSessionId.SetValue(id); } virtual void OnSuccessStatusReport() {} - virtual CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) + + // Handle a failure StatusReport message from the server. protocolData will + // depend on exactly what the generalCode/protocolCode are. + virtual CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode, + Optional protocolData) { return CHIP_ERROR_INTERNAL; } @@ -174,6 +179,7 @@ class DLL_EXPORT PairingSession : public SessionDelegate return CHIP_NO_ERROR; } + Optional protocolData; if (report.GetGeneralCode() == Protocols::SecureChannel::GeneralStatusCode::kBusy && report.GetProtocolCode() == Protocols::SecureChannel::kProtocolCodeBusy) { @@ -189,15 +195,15 @@ class DLL_EXPORT PairingSession : public SessionDelegate } else { - // TODO: CASE: Notify minimum wait time to clients on receiving busy status report #28290 ChipLogProgress(SecureChannel, "Received busy status report with minimum wait time: %u ms", minimumWaitTime); + protocolData.Emplace(minimumWaitTime); } } } // It's very important that we propagate the return value from // OnFailureStatusReport out to the caller. Make sure we return it directly. - return OnFailureStatusReport(report.GetGeneralCode(), report.GetProtocolCode()); + return OnFailureStatusReport(report.GetGeneralCode(), report.GetProtocolCode(), protocolData); } /** diff --git a/src/protocols/secure_channel/SessionEstablishmentDelegate.h b/src/protocols/secure_channel/SessionEstablishmentDelegate.h index a50949e6f6b2e8..c0ba56d9b01e09 100644 --- a/src/protocols/secure_channel/SessionEstablishmentDelegate.h +++ b/src/protocols/secure_channel/SessionEstablishmentDelegate.h @@ -25,6 +25,7 @@ #pragma once +#include #include #include #include @@ -80,6 +81,15 @@ class DLL_EXPORT SessionEstablishmentDelegate */ virtual void OnSessionEstablished(const SessionHandle & session) {} + /** + * Called when the responder has responded with a "busy" status code and + * provided a requested delay. + * + * This call will be followed by an OnSessionEstablishmentError with + * CHIP_ERROR_BUSY as the error. + */ + virtual void OnResponderBusy(System::Clock::Milliseconds16 requestedDelay) {} + virtual ~SessionEstablishmentDelegate() {} }; From 0aa24427aca9b66c2711c96fd4d3f13856a50566 Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Thu, 7 Mar 2024 18:58:41 -0500 Subject: [PATCH 2/7] Add OperationalState to chef examples (#32495) --- .../chef-operational-state-delegate-impl.cpp | 219 ++++++++++++++++++ .../chef-operational-state-delegate-impl.h | 147 ++++++++++++ examples/chef/linux/BUILD.gn | 1 + examples/chef/nrfconnect/CMakeLists.txt | 1 + 4 files changed, 368 insertions(+) create mode 100644 examples/chef/common/chef-operational-state-delegate-impl.cpp create mode 100644 examples/chef/common/chef-operational-state-delegate-impl.h diff --git a/examples/chef/common/chef-operational-state-delegate-impl.cpp b/examples/chef/common/chef-operational-state-delegate-impl.cpp new file mode 100644 index 00000000000000..48863b0cfa6c26 --- /dev/null +++ b/examples/chef/common/chef-operational-state-delegate-impl.cpp @@ -0,0 +1,219 @@ +/* + * Copyright (c) 2023 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include +#include + +using namespace chip; +using namespace chip::app; +using namespace chip::app::Clusters; +using namespace chip::app::Clusters::OperationalState; +using namespace chip::app::Clusters::RvcOperationalState; + +static void onOperationalStateTimerTick(System::Layer * systemLayer, void * data); + +DataModel::Nullable GenericOperationalStateDelegateImpl::GetCountdownTime() +{ + if (mCountDownTime.IsNull()) + return DataModel::NullNullable; + + return DataModel::MakeNullable((uint32_t) (mCountDownTime.Value() - mRunningTime)); +} + +CHIP_ERROR GenericOperationalStateDelegateImpl::GetOperationalStateAtIndex(size_t index, GenericOperationalState & operationalState) +{ + if (index >= mOperationalStateList.size()) + { + return CHIP_ERROR_NOT_FOUND; + } + operationalState = mOperationalStateList[index]; + return CHIP_NO_ERROR; +} + +CHIP_ERROR GenericOperationalStateDelegateImpl::GetOperationalPhaseAtIndex(size_t index, MutableCharSpan & operationalPhase) +{ + if (index >= mOperationalPhaseList.size()) + { + return CHIP_ERROR_NOT_FOUND; + } + return CopyCharSpanToMutableCharSpan(mOperationalPhaseList[index], operationalPhase); +} + +void GenericOperationalStateDelegateImpl::HandlePauseStateCallback(GenericOperationalError & err) +{ + OperationalState::OperationalStateEnum state = + static_cast(GetInstance()->GetCurrentOperationalState()); + + if (state == OperationalState::OperationalStateEnum::kStopped || state == OperationalState::OperationalStateEnum::kError) + { + err.Set(to_underlying(OperationalState::ErrorStateEnum::kCommandInvalidInState)); + return; + } + + // placeholder implementation + auto error = GetInstance()->SetOperationalState(to_underlying(OperationalState::OperationalStateEnum::kPaused)); + if (error == CHIP_NO_ERROR) + { + err.Set(to_underlying(ErrorStateEnum::kNoError)); + } + else + { + err.Set(to_underlying(ErrorStateEnum::kUnableToCompleteOperation)); + } +} + +void GenericOperationalStateDelegateImpl::HandleResumeStateCallback(GenericOperationalError & err) +{ + OperationalState::OperationalStateEnum state = + static_cast(GetInstance()->GetCurrentOperationalState()); + + if (state == OperationalState::OperationalStateEnum::kStopped || state == OperationalState::OperationalStateEnum::kError) + { + err.Set(to_underlying(OperationalState::ErrorStateEnum::kCommandInvalidInState)); + return; + } + + // placeholder implementation + auto error = GetInstance()->SetOperationalState(to_underlying(OperationalStateEnum::kRunning)); + if (error == CHIP_NO_ERROR) + { + err.Set(to_underlying(ErrorStateEnum::kNoError)); + } + else + { + err.Set(to_underlying(ErrorStateEnum::kUnableToCompleteOperation)); + } +} + +void GenericOperationalStateDelegateImpl::HandleStartStateCallback(GenericOperationalError & err) +{ + OperationalState::GenericOperationalError current_err(to_underlying(OperationalState::ErrorStateEnum::kNoError)); + GetInstance()->GetCurrentOperationalError(current_err); + + if (current_err.errorStateID != to_underlying(OperationalState::ErrorStateEnum::kNoError)) + { + err.Set(to_underlying(OperationalState::ErrorStateEnum::kUnableToStartOrResume)); + return; + } + + // placeholder implementation + auto error = GetInstance()->SetOperationalState(to_underlying(OperationalStateEnum::kRunning)); + if (error == CHIP_NO_ERROR) + { + (void) DeviceLayer::SystemLayer().StartTimer(System::Clock::Seconds16(1), onOperationalStateTimerTick, this); + err.Set(to_underlying(ErrorStateEnum::kNoError)); + } + else + { + err.Set(to_underlying(ErrorStateEnum::kUnableToCompleteOperation)); + } +} + +void GenericOperationalStateDelegateImpl::HandleStopStateCallback(GenericOperationalError & err) +{ + // placeholder implementation + auto error = GetInstance()->SetOperationalState(to_underlying(OperationalStateEnum::kStopped)); + if (error == CHIP_NO_ERROR) + { + (void) DeviceLayer::SystemLayer().CancelTimer(onOperationalStateTimerTick, this); + + OperationalState::GenericOperationalError current_err(to_underlying(OperationalState::ErrorStateEnum::kNoError)); + GetInstance()->GetCurrentOperationalError(current_err); + + Optional> totalTime((DataModel::Nullable(mRunningTime + mPausedTime))); + Optional> pausedTime((DataModel::Nullable(mPausedTime))); + + GetInstance()->OnOperationCompletionDetected(static_cast(current_err.errorStateID), totalTime, pausedTime); + + mRunningTime = 0; + mPausedTime = 0; + err.Set(to_underlying(ErrorStateEnum::kNoError)); + } + else + { + err.Set(to_underlying(ErrorStateEnum::kUnableToCompleteOperation)); + } +} + +static void onOperationalStateTimerTick(System::Layer * systemLayer, void * data) +{ + GenericOperationalStateDelegateImpl * delegate = reinterpret_cast(data); + + OperationalState::Instance * instance = OperationalState::GetOperationalStateInstance(); + OperationalState::OperationalStateEnum state = + static_cast(instance->GetCurrentOperationalState()); + + auto countdown_time = delegate->GetCountdownTime(); + + if (countdown_time.IsNull() || (!countdown_time.IsNull() && countdown_time.Value() > 0)) + { + if (state == OperationalState::OperationalStateEnum::kRunning) + { + delegate->mRunningTime++; + } + else if (state == OperationalState::OperationalStateEnum::kPaused) + { + delegate->mPausedTime++; + } + } + + if (state == OperationalState::OperationalStateEnum::kRunning || state == OperationalState::OperationalStateEnum::kPaused) + { + (void) DeviceLayer::SystemLayer().StartTimer(System::Clock::Seconds16(1), onOperationalStateTimerTick, delegate); + } + else + { + (void) DeviceLayer::SystemLayer().CancelTimer(onOperationalStateTimerTick, delegate); + } +} + +// Init Operational State cluster + +static OperationalState::Instance * gOperationalStateInstance = nullptr; +static OperationalStateDelegate * gOperationalStateDelegate = nullptr; + +OperationalState::Instance * OperationalState::GetOperationalStateInstance() +{ + return gOperationalStateInstance; +} + +void OperationalState::Shutdown() +{ + if (gOperationalStateInstance != nullptr) + { + delete gOperationalStateInstance; + gOperationalStateInstance = nullptr; + } + if (gOperationalStateDelegate != nullptr) + { + delete gOperationalStateDelegate; + gOperationalStateDelegate = nullptr; + } +} + +void emberAfOperationalStateClusterInitCallback(chip::EndpointId endpointId) +{ + VerifyOrDie(endpointId == 1); // this cluster is only enabled for endpoint 1. + VerifyOrDie(gOperationalStateInstance == nullptr && gOperationalStateDelegate == nullptr); + + gOperationalStateDelegate = new OperationalStateDelegate; + EndpointId operationalStateEndpoint = 0x01; + gOperationalStateInstance = new OperationalState::Instance(gOperationalStateDelegate, operationalStateEndpoint); + + gOperationalStateInstance->SetOperationalState(to_underlying(OperationalState::OperationalStateEnum::kStopped)); + + gOperationalStateInstance->Init(); +} diff --git a/examples/chef/common/chef-operational-state-delegate-impl.h b/examples/chef/common/chef-operational-state-delegate-impl.h new file mode 100644 index 00000000000000..60b6b09e9b6511 --- /dev/null +++ b/examples/chef/common/chef-operational-state-delegate-impl.h @@ -0,0 +1,147 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +#include + +namespace chip { +namespace app { +namespace Clusters { + +namespace OperationalState { + +// This is an application level delegate to handle operational state commands according to the specific business logic. +class GenericOperationalStateDelegateImpl : public Delegate +{ +public: + uint32_t mRunningTime = 0; + uint32_t mPausedTime = 0; + app::DataModel::Nullable mCountDownTime; + + /** + * Get the countdown time. This attribute is not used in this application. + * @return The current countdown time. + */ + app::DataModel::Nullable GetCountdownTime() override; + + /** + * Fills in the provided GenericOperationalState with the state at index `index` if there is one, + * or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of states. + * Note: This is used by the SDK to populate the operational state list attribute. If the contents of this list changes, + * the device SHALL call the Instance's ReportOperationalStateListChange method to report that this attribute has changed. + * @param index The index of the state, with 0 representing the first state. + * @param operationalState The GenericOperationalState is filled. + */ + CHIP_ERROR GetOperationalStateAtIndex(size_t index, GenericOperationalState & operationalState) override; + + /** + * Fills in the provided MutableCharSpan with the phase at index `index` if there is one, + * or returns CHIP_ERROR_NOT_FOUND if the index is out of range for the list of phases. + * + * If CHIP_ERROR_NOT_FOUND is returned for index 0, that indicates that the PhaseList attribute is null + * (there are no phases defined at all). + * + * Note: This is used by the SDK to populate the phase list attribute. If the contents of this list changes, the + * device SHALL call the Instance's ReportPhaseListChange method to report that this attribute has changed. + * @param index The index of the phase, with 0 representing the first phase. + * @param operationalPhase The MutableCharSpan is filled. + */ + CHIP_ERROR GetOperationalPhaseAtIndex(size_t index, MutableCharSpan & operationalPhase) override; + + // command callback + /** + * Handle Command Callback in application: Pause + * @param[out] get operational error after callback. + */ + void HandlePauseStateCallback(GenericOperationalError & err) override; + + /** + * Handle Command Callback in application: Resume + * @param[out] get operational error after callback. + */ + void HandleResumeStateCallback(GenericOperationalError & err) override; + + /** + * Handle Command Callback in application: Start + * @param[out] get operational error after callback. + */ + void HandleStartStateCallback(GenericOperationalError & err) override; + + /** + * Handle Command Callback in application: Stop + * @param[out] get operational error after callback. + */ + void HandleStopStateCallback(GenericOperationalError & err) override; + +protected: + Span mOperationalStateList; + Span mOperationalPhaseList; +}; + +// This is an application level delegate to handle operational state commands according to the specific business logic. +class OperationalStateDelegate : public GenericOperationalStateDelegateImpl +{ +private: + const GenericOperationalState opStateList[4] = { + GenericOperationalState(to_underlying(OperationalStateEnum::kStopped)), + GenericOperationalState(to_underlying(OperationalStateEnum::kRunning)), + GenericOperationalState(to_underlying(OperationalStateEnum::kPaused)), + GenericOperationalState(to_underlying(OperationalStateEnum::kError)), + }; + + const uint32_t kExampleCountDown = 30; + +public: + OperationalStateDelegate() + { + GenericOperationalStateDelegateImpl::mOperationalStateList = Span(opStateList); + } + + /** + * Handle Command Callback in application: Start + * @param[out] get operational error after callback. + */ + void HandleStartStateCallback(GenericOperationalError & err) override + { + mCountDownTime.SetNonNull(static_cast(kExampleCountDown)); + GenericOperationalStateDelegateImpl::HandleStartStateCallback(err); + } + + /** + * Handle Command Callback in application: Stop + * @param[out] get operational error after callback. + */ + void HandleStopStateCallback(GenericOperationalError & err) override + { + GenericOperationalStateDelegateImpl::HandleStopStateCallback(err); + mCountDownTime.SetNull(); + } +}; + +Instance * GetOperationalStateInstance(); + +void Shutdown(); + +} // namespace OperationalState +} // namespace Clusters +} // namespace app +} // namespace chip diff --git a/examples/chef/linux/BUILD.gn b/examples/chef/linux/BUILD.gn index 02fa77dac864e5..0a4e2385f28dda 100644 --- a/examples/chef/linux/BUILD.gn +++ b/examples/chef/linux/BUILD.gn @@ -43,6 +43,7 @@ executable("${sample_name}") { "${project_dir}/common/chef-air-quality.cpp", "${project_dir}/common/chef-concentration-measurement.cpp", "${project_dir}/common/chef-fan-control-manager.cpp", + "${project_dir}/common/chef-operational-state-delegate-impl.cpp", "${project_dir}/common/chef-resource-monitoring-delegates.cpp", "${project_dir}/common/chef-rvc-mode-delegate.cpp", "${project_dir}/common/chef-rvc-operational-state-delegate.cpp", diff --git a/examples/chef/nrfconnect/CMakeLists.txt b/examples/chef/nrfconnect/CMakeLists.txt index 0a408e829dd04f..25d663211b9f15 100644 --- a/examples/chef/nrfconnect/CMakeLists.txt +++ b/examples/chef/nrfconnect/CMakeLists.txt @@ -84,6 +84,7 @@ target_sources(app PRIVATE ${CHEF}/common/chef-air-quality.cpp ${CHEF}/common/chef-concentration-measurement.cpp ${CHEF}/common/chef-fan-control-manager.cpp + ${CHEF}/common/chef-operational-state-delegate-impl.cpp ${CHEF}/common/chef-resource-monitoring-delegates.cpp ${CHEF}/common/chef-rvc-mode-delegate.cpp ${CHEF}/common/chef-rvc-operational-state-delegate.cpp From 3dc23203d0a43ddbf4acd0deae18431cc91b5418 Mon Sep 17 00:00:00 2001 From: joonhaengHeo <85541460+joonhaengHeo@users.noreply.github.com> Date: Fri, 8 Mar 2024 09:51:47 +0900 Subject: [PATCH 3/7] [Android] Add Discovered Device information (#32472) * Add Android Discovered Device infor * Restyled by whitespace * Restyled by google-java-format * Restyled by clang-format --------- Co-authored-by: Restyled.io --- src/controller/java/BUILD.gn | 2 + .../java/CHIPDeviceController-JNI.cpp | 36 +++++++++++- .../CommissioningWindowStatus.java | 39 +++++++++++++ .../devicecontroller/DiscoveredDevice.java | 58 +++++++++++++++++++ .../devicecontroller/PairingHintBitmap.java | 57 ++++++++++++++++++ 5 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 src/controller/java/src/chip/devicecontroller/CommissioningWindowStatus.java create mode 100644 src/controller/java/src/chip/devicecontroller/PairingHintBitmap.java diff --git a/src/controller/java/BUILD.gn b/src/controller/java/BUILD.gn index 3b573daa93028c..ff42d1b43a42f4 100644 --- a/src/controller/java/BUILD.gn +++ b/src/controller/java/BUILD.gn @@ -450,6 +450,7 @@ android_library("java") { "src/chip/devicecontroller/ChipCommandType.java", "src/chip/devicecontroller/ChipDeviceController.java", "src/chip/devicecontroller/ChipDeviceControllerException.java", + "src/chip/devicecontroller/CommissioningWindowStatus.java", "src/chip/devicecontroller/ConnectionFailureException.java", "src/chip/devicecontroller/ControllerParams.java", "src/chip/devicecontroller/DeviceAttestationDelegate.java", @@ -469,6 +470,7 @@ android_library("java") { "src/chip/devicecontroller/OTAProviderDelegate.java", "src/chip/devicecontroller/OpenCommissioningCallback.java", "src/chip/devicecontroller/OperationalKeyConfig.java", + "src/chip/devicecontroller/PairingHintBitmap.java", "src/chip/devicecontroller/PaseVerifierParams.java", "src/chip/devicecontroller/ReportCallback.java", "src/chip/devicecontroller/ReportCallbackJni.java", diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index fc2c0694b29849..99520d1a32c9a0 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -1897,9 +1897,19 @@ JNI_METHOD(jobject, getDiscoveredDevice)(JNIEnv * env, jobject self, jlong handl jclass discoveredDeviceCls = env->FindClass("chip/devicecontroller/DiscoveredDevice"); jmethodID constructor = env->GetMethodID(discoveredDeviceCls, "", "()V"); - jfieldID discrminatorID = env->GetFieldID(discoveredDeviceCls, "discriminator", "J"); - jfieldID ipAddressID = env->GetFieldID(discoveredDeviceCls, "ipAddress", "Ljava/lang/String;"); - jfieldID portID = env->GetFieldID(discoveredDeviceCls, "port", "I"); + jfieldID discrminatorID = env->GetFieldID(discoveredDeviceCls, "discriminator", "J"); + jfieldID ipAddressID = env->GetFieldID(discoveredDeviceCls, "ipAddress", "Ljava/lang/String;"); + jfieldID portID = env->GetFieldID(discoveredDeviceCls, "port", "I"); + jfieldID deviceTypeID = env->GetFieldID(discoveredDeviceCls, "deviceType", "J"); + jfieldID vendorIdID = env->GetFieldID(discoveredDeviceCls, "vendorId", "I"); + jfieldID productIdID = env->GetFieldID(discoveredDeviceCls, "productId", "I"); + jfieldID rotatingIdID = env->GetFieldID(discoveredDeviceCls, "rotatingId", "[B"); + jfieldID instanceNameID = env->GetFieldID(discoveredDeviceCls, "instanceName", "Ljava/lang/String;"); + jfieldID deviceNameID = env->GetFieldID(discoveredDeviceCls, "deviceName", "Ljava/lang/String;"); + jfieldID pairingInstructionID = env->GetFieldID(discoveredDeviceCls, "pairingInstruction", "Ljava/lang/String;"); + + jmethodID setCommissioningModeID = env->GetMethodID(discoveredDeviceCls, "setCommissioningMode", "(I)V"); + jmethodID setPairingHintID = env->GetMethodID(discoveredDeviceCls, "setPairingHint", "(I)V"); jobject discoveredObj = env->NewObject(discoveredDeviceCls, constructor); @@ -1911,6 +1921,26 @@ JNI_METHOD(jobject, getDiscoveredDevice)(JNIEnv * env, jobject self, jlong handl env->SetObjectField(discoveredObj, ipAddressID, jniipAdress); env->SetIntField(discoveredObj, portID, static_cast(data->resolutionData.port)); + env->SetLongField(discoveredObj, deviceTypeID, static_cast(data->commissionData.deviceType)); + env->SetIntField(discoveredObj, vendorIdID, static_cast(data->commissionData.vendorId)); + env->SetIntField(discoveredObj, productIdID, static_cast(data->commissionData.productId)); + + jbyteArray jRotatingId; + CHIP_ERROR err = JniReferences::GetInstance().N2J_ByteArray( + env, data->commissionData.rotatingId, static_cast(data->commissionData.rotatingIdLen), jRotatingId); + + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "jRotatingId N2J_ByteArray error : %" CHIP_ERROR_FORMAT, err.Format()); + return nullptr; + } + env->SetObjectField(discoveredObj, rotatingIdID, static_cast(jRotatingId)); + env->SetObjectField(discoveredObj, instanceNameID, env->NewStringUTF(data->commissionData.instanceName)); + env->SetObjectField(discoveredObj, deviceNameID, env->NewStringUTF(data->commissionData.deviceName)); + env->SetObjectField(discoveredObj, pairingInstructionID, env->NewStringUTF(data->commissionData.pairingInstruction)); + + env->CallVoidMethod(discoveredObj, setCommissioningModeID, static_cast(data->commissionData.commissioningMode)); + env->CallVoidMethod(discoveredObj, setPairingHintID, static_cast(data->commissionData.pairingHint)); return discoveredObj; } diff --git a/src/controller/java/src/chip/devicecontroller/CommissioningWindowStatus.java b/src/controller/java/src/chip/devicecontroller/CommissioningWindowStatus.java new file mode 100644 index 00000000000000..bfced35f778b78 --- /dev/null +++ b/src/controller/java/src/chip/devicecontroller/CommissioningWindowStatus.java @@ -0,0 +1,39 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package chip.devicecontroller; + +public enum CommissioningWindowStatus { + WindowNotOpen(0), + EnhancedWindowOpen(1), + BasicWindowOpen(2); + + private final int value; + + CommissioningWindowStatus(int value) { + this.value = value; + } + + public static CommissioningWindowStatus value(int value) { + for (CommissioningWindowStatus status : CommissioningWindowStatus.values()) { + if (status.value == value) { + return status; + } + } + throw new IllegalArgumentException("Invalid value: " + value); + } +} diff --git a/src/controller/java/src/chip/devicecontroller/DiscoveredDevice.java b/src/controller/java/src/chip/devicecontroller/DiscoveredDevice.java index 01d6ecc28cce42..2fb8bcf4e79842 100644 --- a/src/controller/java/src/chip/devicecontroller/DiscoveredDevice.java +++ b/src/controller/java/src/chip/devicecontroller/DiscoveredDevice.java @@ -17,8 +17,66 @@ */ package chip.devicecontroller; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + public class DiscoveredDevice { public long discriminator; public String ipAddress; public int port; + public long deviceType; + public int vendorId; + public int productId; + public Set pairingHint; + public CommissioningWindowStatus commissioningMode; + public byte[] rotatingId; + public String instanceName; + public String deviceName; + public String pairingInstruction; + + // For use in JNI. + private void setCommissioningMode(int value) { + this.commissioningMode = CommissioningWindowStatus.value(value); + } + + private void setPairingHint(int value) { + this.pairingHint = new HashSet<>(); + for (PairingHintBitmap mode : PairingHintBitmap.values()) { + int bitmask = 1 << mode.getBitIndex(); + if ((value & bitmask) != 0) { + pairingHint.add(mode); + } + } + } + + @Override + public String toString() { + return "DiscoveredDevice : {" + + "\n\tdiscriminator : " + + discriminator + + "\n\tipAddress : " + + ipAddress + + "\n\tport : " + + port + + "\n\tdeviceType : " + + deviceType + + "\n\tvendorId : " + + vendorId + + "\n\tproductId : " + + productId + + "\n\tpairingHint : " + + pairingHint + + "\n\tcommissioningMode : " + + commissioningMode + + "\n\trotatingId : " + + (rotatingId != null ? Arrays.toString(rotatingId) : "null") + + "\n\tinstanceName : " + + instanceName + + "\n\tdeviceName : " + + deviceName + + "\n\tpairingInstruction : " + + pairingInstruction + + "\n}"; + } } diff --git a/src/controller/java/src/chip/devicecontroller/PairingHintBitmap.java b/src/controller/java/src/chip/devicecontroller/PairingHintBitmap.java new file mode 100644 index 00000000000000..d13b710c9c1b98 --- /dev/null +++ b/src/controller/java/src/chip/devicecontroller/PairingHintBitmap.java @@ -0,0 +1,57 @@ +/* + * Copyright (c) 2024 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +package chip.devicecontroller; + +public enum PairingHintBitmap { + PowerCycle(0, false), + DeviceManufacturerURL(1, false), + Administrator(2, false), + SettingsMenuOnTheNode(3, false), + CustomInstruction(4, true), + DeviceManual(5, false), + PressResetButton(6, false), + PressResetButtonWithApplicationOfPower(7, false), + PressResetButtonForNseconds(8, true), + PressResetButtonUntilLightBlinks(9, true), + PressResetButtonForNsecondsWithApplicationOfPower(10, true), + PressResetButtonUntilLightBlinksWithApplicationOfPower(11, true), + PressResetButtonNTimes(12, true), + PressSetupButton(13, false), + PressSetupButtonWithApplicationOfPower(14, false), + PressSetupButtonForNseconds(15, true), + PressSetupButtonUntilLightBlinks(16, true), + PressSetupButtonForNsecondsWithApplicationOfPower(17, true), + PressSetupButtonUntilLightBlinksWithApplicationOfPower(18, true), + PressSetupButtonNtimes(19, true); + + private final int bitIndex; + private final boolean isRequirePairingInstruction; + + PairingHintBitmap(int bitIndex, boolean isRequirePairingInstruction) { + this.bitIndex = bitIndex; + this.isRequirePairingInstruction = isRequirePairingInstruction; + } + + public int getBitIndex() { + return bitIndex; + } + + public boolean getRequirePairingInstruction() { + return isRequirePairingInstruction; + } +} From 968cea2949ca48298f23c2dd3f3d514515eb4f24 Mon Sep 17 00:00:00 2001 From: joonhaengHeo <85541460+joonhaengHeo@users.noreply.github.com> Date: Fri, 8 Mar 2024 13:21:39 +0900 Subject: [PATCH 4/7] Remove unset threadNetwork Scan (#31704) * divide android commissioning parameter * Fix countrycode copy issue * Restyled by clang-format * Update commissioningParameter --------- Co-authored-by: Restyled.io --- src/controller/CommissioningDelegate.h | 3 ++ .../java/AndroidDeviceControllerWrapper.cpp | 24 ++++++++++- .../java/AndroidDeviceControllerWrapper.h | 32 ++++++++------- .../java/CHIPDeviceController-JNI.cpp | 41 +++++-------------- 4 files changed, 53 insertions(+), 47 deletions(-) diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 175c246e908f9d..2a78663541dd49 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -370,9 +370,12 @@ class CommissioningParameters mAttestationNonce.SetValue(attestationNonce); return *this; } + + // If a WiFiCredentials is provided, then the WiFiNetworkScan will not be attempted CommissioningParameters & SetWiFiCredentials(WiFiCredentials wifiCreds) { mWiFiCreds.SetValue(wifiCreds); + mAttemptWiFiNetworkScan.SetValue(false); return *this; } diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp index 74e1187ef2a03b..c965bb0f17584b 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.cpp +++ b/src/controller/java/AndroidDeviceControllerWrapper.cpp @@ -104,7 +104,7 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew( jobject keypairDelegate, jbyteArray rootCertificate, jbyteArray intermediateCertificate, jbyteArray nodeOperationalCertificate, jbyteArray ipkEpochKey, uint16_t listenPort, uint16_t controllerVendorId, uint16_t failsafeTimerSeconds, bool attemptNetworkScanWiFi, bool attemptNetworkScanThread, bool skipCommissioningComplete, - bool skipAttestationCertificateValidation, CHIP_ERROR * errInfoOnFailure) + bool skipAttestationCertificateValidation, jstring countryCode, CHIP_ERROR * errInfoOnFailure) { if (errInfoOnFailure == nullptr) { @@ -207,11 +207,30 @@ AndroidDeviceControllerWrapper * AndroidDeviceControllerWrapper::AllocateNew( wrapper->mGroupDataProvider.SetStorageDelegate(wrapperStorage); wrapper->mGroupDataProvider.SetSessionKeystore(initParams.sessionKeystore); - CommissioningParameters params = wrapper->mAutoCommissioner.GetCommissioningParameters(); + CommissioningParameters params = wrapper->GetCommissioningParameters(); params.SetFailsafeTimerSeconds(failsafeTimerSeconds); params.SetAttemptWiFiNetworkScan(attemptNetworkScanWiFi); params.SetAttemptThreadNetworkScan(attemptNetworkScanThread); params.SetSkipCommissioningComplete(skipCommissioningComplete); + + if (countryCode != nullptr) + { + JniUtfString countryCodeJniString(env, countryCode); + if (countryCodeJniString.size() != kCountryCodeBufferLen) + { + *errInfoOnFailure = CHIP_ERROR_INVALID_ARGUMENT; + return nullptr; + } + + MutableCharSpan copiedCode(wrapper->mCountryCode); + if (CopyCharSpanToMutableCharSpan(countryCodeJniString.charSpan(), copiedCode) != CHIP_NO_ERROR) + { + *errInfoOnFailure = CHIP_ERROR_INVALID_ARGUMENT; + return nullptr; + } + params.SetCountryCode(copiedCode); + } + wrapper->UpdateCommissioningParameters(params); CHIP_ERROR err = wrapper->mGroupDataProvider.Init(); @@ -526,6 +545,7 @@ CHIP_ERROR AndroidDeviceControllerWrapper::UpdateCommissioningParameters(const c { // this will wipe out any custom attestationNonce and csrNonce that was being used. // however, Android APIs don't allow these to be set to custom values today. + mCommissioningParameter = params; return mAutoCommissioner.SetCommissioningParameters(params); } diff --git a/src/controller/java/AndroidDeviceControllerWrapper.h b/src/controller/java/AndroidDeviceControllerWrapper.h index ac279370f62c4a..5ccb2edb3b2050 100644 --- a/src/controller/java/AndroidDeviceControllerWrapper.h +++ b/src/controller/java/AndroidDeviceControllerWrapper.h @@ -50,6 +50,8 @@ constexpr uint8_t kUserActiveModeTriggerInstructionBufferLen = 128 + 1; // 128bytes is max UserActiveModeTriggerInstruction size and 1 byte is for escape sequence. + +constexpr uint8_t kCountryCodeBufferLen = 2; /** * This class contains all relevant information for the JNI view of CHIPDeviceController * to handle all controller-related processing. @@ -123,10 +125,7 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel chip::Credentials::PartialDACVerifier * GetPartialDACVerifier() { return &mPartialDACVerifier; } - const chip::Controller::CommissioningParameters & GetCommissioningParameters() const - { - return mAutoCommissioner.GetCommissioningParameters(); - } + const chip::Controller::CommissioningParameters & GetCommissioningParameters() const { return mCommissioningParameter; } static AndroidDeviceControllerWrapper * FromJNIHandle(jlong handle) { @@ -171,20 +170,19 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel * @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 * - AllocateNew(JavaVM * vm, jobject deviceControllerObj, chip::NodeId nodeId, chip::FabricId fabricId, - const chip::CATValues & cats, chip::System::Layer * systemLayer, - chip::Inet::EndPointManager * tcpEndPointManager, - chip::Inet::EndPointManager * udpEndPointManager, + static AndroidDeviceControllerWrapper * AllocateNew( + JavaVM * vm, jobject deviceControllerObj, chip::NodeId nodeId, chip::FabricId fabricId, const chip::CATValues & cats, + chip::System::Layer * systemLayer, chip::Inet::EndPointManager * tcpEndPointManager, + chip::Inet::EndPointManager * udpEndPointManager, #ifdef JAVA_MATTER_CONTROLLER_TEST - ExampleOperationalCredentialsIssuerPtr opCredsIssuer, + ExampleOperationalCredentialsIssuerPtr opCredsIssuer, #else - AndroidOperationalCredentialsIssuerPtr opCredsIssuer, + AndroidOperationalCredentialsIssuerPtr opCredsIssuer, #endif - jobject keypairDelegate, jbyteArray rootCertificate, jbyteArray intermediateCertificate, - jbyteArray nodeOperationalCertificate, jbyteArray ipkEpochKey, uint16_t listenPort, uint16_t controllerVendorId, - uint16_t failsafeTimerSeconds, bool attemptNetworkScanWiFi, bool attemptNetworkScanThread, - bool skipCommissioningComplete, bool skipAttestationCertificateValidation, CHIP_ERROR * errInfoOnFailure); + jobject keypairDelegate, jbyteArray rootCertificate, jbyteArray intermediateCertificate, + jbyteArray nodeOperationalCertificate, jbyteArray ipkEpochKey, uint16_t listenPort, uint16_t controllerVendorId, + uint16_t failsafeTimerSeconds, bool attemptNetworkScanWiFi, bool attemptNetworkScanThread, bool skipCommissioningComplete, + bool skipAttestationCertificateValidation, jstring countryCode, CHIP_ERROR * errInfoOnFailure); void Shutdown(); @@ -246,6 +244,8 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel std::vector mIcacCertificate; std::vector mRcacCertificate; + char mCountryCode[kCountryCodeBufferLen]; + chip::Controller::AutoCommissioner mAutoCommissioner; chip::Credentials::PartialDACVerifier mPartialDACVerifier; @@ -262,6 +262,8 @@ class AndroidDeviceControllerWrapper : public chip::Controller::DevicePairingDel chip::MutableCharSpan mUserActiveModeTriggerInstruction = chip::MutableCharSpan(mUserActiveModeTriggerInstructionBuffer); chip::BitMask mUserActiveModeTriggerHint; + chip::Controller::CommissioningParameters mCommissioningParameter; + AndroidDeviceControllerWrapper(ChipDeviceControllerPtr controller, #ifdef JAVA_MATTER_CONTROLLER_TEST ExampleOperationalCredentialsIssuerPtr opCredsIssuer diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index 99520d1a32c9a0..f1c7487e4e3f87 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -371,6 +371,10 @@ JNI_METHOD(jlong, newDeviceController)(JNIEnv * env, jobject self, jobject contr jobject countryCodeOptional = env->CallObjectMethod(controllerParams, getCountryCode); jobject regulatoryLocationOptional = env->CallObjectMethod(controllerParams, getRegulatoryLocation); + jobject countryCode; + err = chip::JniReferences::GetInstance().GetOptionalValue(countryCodeOptional, countryCode); + SuccessOrExit(err); + #ifdef JAVA_MATTER_CONTROLLER_TEST std::unique_ptr opCredsIssuer( new chip::Controller::ExampleOperationalCredentialsIssuer()); @@ -383,7 +387,7 @@ JNI_METHOD(jlong, newDeviceController)(JNIEnv * env, jobject self, jobject contr DeviceLayer::TCPEndPointManager(), DeviceLayer::UDPEndPointManager(), std::move(opCredsIssuer), keypairDelegate, rootCertificate, intermediateCertificate, operationalCertificate, ipk, listenPort, controllerVendorId, failsafeTimerSeconds, attemptNetworkScanWiFi, attemptNetworkScanThread, skipCommissioningComplete, - skipAttestationCertificateValidation, &err); + skipAttestationCertificateValidation, static_cast(countryCode), &err); SuccessOrExit(err); if (caseFailsafeTimerSeconds > 0) @@ -411,29 +415,6 @@ JNI_METHOD(jlong, newDeviceController)(JNIEnv * env, jobject self, jobject contr } } - jobject countryCode; - err = chip::JniReferences::GetInstance().GetOptionalValue(countryCodeOptional, countryCode); - SuccessOrExit(err); - - if (countryCode != nullptr) - { - jstring countryCodeStr = static_cast(countryCode); - JniUtfString countryCodeJniString(env, countryCodeStr); - - VerifyOrExit(countryCodeJniString.size() == 2, err = CHIP_ERROR_INVALID_ARGUMENT); - - chip::Controller::CommissioningParameters commissioningParams = wrapper->GetCommissioningParameters(); - commissioningParams.SetCountryCode(countryCodeJniString.charSpan()); - - // The wrapper internally has reserved storage for the country code and will copy the value. - err = wrapper->UpdateCommissioningParameters(commissioningParams); - if (err != CHIP_NO_ERROR) - { - ChipLogError(Controller, "UpdateCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format()); - SuccessOrExit(err); - } - } - jobject regulatoryLocation; err = chip::JniReferences::GetInstance().GetOptionalValue(regulatoryLocationOptional, regulatoryLocation); SuccessOrExit(err); @@ -876,7 +857,7 @@ JNI_METHOD(void, updateCommissioningNetworkCredentials) chip::DeviceLayer::StackLock lock; AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle); - CommissioningParameters commissioningParams = wrapper->GetCommissioningParameters(); + CommissioningParameters commissioningParams = wrapper->GetAutoCommissioner()->GetCommissioningParameters(); CHIP_ERROR err = wrapper->ApplyNetworkCredentials(commissioningParams, networkCredentials); if (err != CHIP_NO_ERROR) { @@ -884,10 +865,10 @@ JNI_METHOD(void, updateCommissioningNetworkCredentials) JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err); return; } - err = wrapper->UpdateCommissioningParameters(commissioningParams); + err = wrapper->GetAutoCommissioner()->SetCommissioningParameters(commissioningParams); if (err != CHIP_NO_ERROR) { - ChipLogError(Controller, "UpdateCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format()); + ChipLogError(Controller, "SetCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format()); JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err); return; } @@ -911,7 +892,7 @@ JNI_METHOD(void, updateCommissioningICDRegistrationInfo) chip::DeviceLayer::StackLock lock; AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle); - CommissioningParameters commissioningParams = wrapper->GetCommissioningParameters(); + CommissioningParameters commissioningParams = wrapper->GetAutoCommissioner()->GetCommissioningParameters(); CHIP_ERROR err = wrapper->ApplyICDRegistrationInfo(commissioningParams, icdRegistrationInfo); if (err != CHIP_NO_ERROR) { @@ -919,10 +900,10 @@ JNI_METHOD(void, updateCommissioningICDRegistrationInfo) JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err); return; } - err = wrapper->UpdateCommissioningParameters(commissioningParams); + err = wrapper->GetAutoCommissioner()->SetCommissioningParameters(commissioningParams); if (err != CHIP_NO_ERROR) { - ChipLogError(Controller, "UpdateCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format()); + ChipLogError(Controller, "SetCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format()); JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err); return; } From 610435a8968eac51b3d2658f554e65ab14c088fe Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 8 Mar 2024 06:26:19 +0100 Subject: [PATCH 5/7] [Linux] Use reinterpret_cast<> for trivial glib casting (#32376) * Use reinterpret_cast for trivial glib casting * Replace BLUEZ_OBJECT with reinterpret_cast * More cast fixes * Restyled by clang-format --------- Co-authored-by: Restyled.io --- .../Linux/bluez/BluezAdvertisement.cpp | 8 ++--- src/platform/Linux/bluez/BluezConnection.cpp | 31 ++++++++++--------- src/platform/Linux/bluez/BluezEndpoint.cpp | 9 +++--- .../Linux/bluez/BluezObjectIterator.h | 4 +-- .../Linux/bluez/ChipDeviceScanner.cpp | 4 +-- 5 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/platform/Linux/bluez/BluezAdvertisement.cpp b/src/platform/Linux/bluez/BluezAdvertisement.cpp index 06f831bff8b58f..0fed99478cdaf9 100644 --- a/src/platform/Linux/bluez/BluezAdvertisement.cpp +++ b/src/platform/Linux/bluez/BluezAdvertisement.cpp @@ -224,7 +224,7 @@ void BluezAdvertisement::Shutdown() void BluezAdvertisement::StartDone(GObject * aObject, GAsyncResult * aResult) { - BluezLEAdvertisingManager1 * advMgr = BLUEZ_LEADVERTISING_MANAGER1(aObject); + auto * advMgr = reinterpret_cast(aObject); GAutoPtr error; gboolean success = FALSE; @@ -252,7 +252,7 @@ CHIP_ERROR BluezAdvertisement::StartImpl() adapterObject = g_dbus_interface_get_object(G_DBUS_INTERFACE(mAdapter.get())); VerifyOrExit(adapterObject != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adapterObject in %s", __func__)); - advMgr.reset(bluez_object_get_leadvertising_manager1(BLUEZ_OBJECT(adapterObject))); + advMgr.reset(bluez_object_get_leadvertising_manager1(reinterpret_cast(adapterObject))); VerifyOrExit(advMgr.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL advMgr in %s", __func__)); g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE("a{sv}")); @@ -282,7 +282,7 @@ CHIP_ERROR BluezAdvertisement::Start() void BluezAdvertisement::StopDone(GObject * aObject, GAsyncResult * aResult) { - BluezLEAdvertisingManager1 * advMgr = BLUEZ_LEADVERTISING_MANAGER1(aObject); + auto * advMgr = reinterpret_cast(aObject); GAutoPtr error; gboolean success = FALSE; @@ -308,7 +308,7 @@ CHIP_ERROR BluezAdvertisement::StopImpl() adapterObject = g_dbus_interface_get_object(G_DBUS_INTERFACE(mAdapter.get())); VerifyOrExit(adapterObject != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adapterObject in %s", __func__)); - advMgr.reset(bluez_object_get_leadvertising_manager1(BLUEZ_OBJECT(adapterObject))); + advMgr.reset(bluez_object_get_leadvertising_manager1(reinterpret_cast(adapterObject))); VerifyOrExit(advMgr.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL advMgr in %s", __func__)); bluez_leadvertising_manager1_call_unregister_advertisement( diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index d2db5edc005eba..88595bd86c720c 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -305,9 +305,10 @@ CHIP_ERROR BluezConnection::SendIndication(chip::System::PacketBufferHandle apBu void BluezConnection::SendWriteRequestDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { - BluezGattCharacteristic1 * c1 = BLUEZ_GATT_CHARACTERISTIC1(aObject); + auto * pC1 = reinterpret_cast(aObject); + GAutoPtr error; - gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c1, aResult, &error.GetReceiver()); + gboolean success = bluez_gatt_characteristic1_call_write_value_finish(pC1, aResult, &error.GetReceiver()); VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: SendWriteRequest : %s", error->message)); BLEManagerImpl::HandleWriteComplete(static_cast(apConnection)); @@ -354,9 +355,10 @@ void BluezConnection::OnCharacteristicChanged(GDBusProxy * aInterface, GVariant void BluezConnection::SubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { - BluezGattCharacteristic1 * c2 = BLUEZ_GATT_CHARACTERISTIC1(aObject); + auto * pC2 = reinterpret_cast(aObject); + GAutoPtr error; - gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &error.GetReceiver()); + gboolean success = bluez_gatt_characteristic1_call_write_value_finish(pC2, aResult, &error.GetReceiver()); VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: SubscribeCharacteristic : %s", error->message)); @@ -365,13 +367,12 @@ void BluezConnection::SubscribeCharacteristicDone(GObject * aObject, GAsyncResul CHIP_ERROR BluezConnection::SubscribeCharacteristicImpl(BluezConnection * connection) { - BluezGattCharacteristic1 * c2 = nullptr; - VerifyOrExit(connection->mpC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); - c2 = BLUEZ_GATT_CHARACTERISTIC1(connection->mpC2); + BluezGattCharacteristic1 * pC2 = connection->mpC2; + VerifyOrExit(pC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); // Get notifications on the TX characteristic change (e.g. indication is received) - g_signal_connect(c2, "g-properties-changed", G_CALLBACK(OnCharacteristicChanged), connection); - bluez_gatt_characteristic1_call_start_notify(connection->mpC2, nullptr, SubscribeCharacteristicDone, connection); + g_signal_connect(pC2, "g-properties-changed", G_CALLBACK(OnCharacteristicChanged), connection); + bluez_gatt_characteristic1_call_start_notify(pC2, nullptr, SubscribeCharacteristicDone, connection); exit: return CHIP_NO_ERROR; @@ -386,22 +387,24 @@ CHIP_ERROR BluezConnection::SubscribeCharacteristic() void BluezConnection::UnsubscribeCharacteristicDone(GObject * aObject, GAsyncResult * aResult, gpointer apConnection) { - BluezGattCharacteristic1 * c2 = BLUEZ_GATT_CHARACTERISTIC1(aObject); + auto * pC2 = reinterpret_cast(aObject); + GAutoPtr error; - gboolean success = bluez_gatt_characteristic1_call_write_value_finish(c2, aResult, &error.GetReceiver()); + gboolean success = bluez_gatt_characteristic1_call_write_value_finish(pC2, aResult, &error.GetReceiver()); VerifyOrReturn(success == TRUE, ChipLogError(DeviceLayer, "FAIL: UnsubscribeCharacteristic : %s", error->message)); // Stop listening to the TX characteristic changes - g_signal_handlers_disconnect_by_data(c2, apConnection); + g_signal_handlers_disconnect_by_data(pC2, apConnection); BLEManagerImpl::HandleSubscribeOpComplete(static_cast(apConnection), false); } CHIP_ERROR BluezConnection::UnsubscribeCharacteristicImpl(BluezConnection * connection) { - VerifyOrExit(connection->mpC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); + BluezGattCharacteristic1 * pC2 = connection->mpC2; + VerifyOrExit(pC2 != nullptr, ChipLogError(DeviceLayer, "C2 is NULL in %s", __func__)); - bluez_gatt_characteristic1_call_stop_notify(connection->mpC2, nullptr, UnsubscribeCharacteristicDone, connection); + bluez_gatt_characteristic1_call_stop_notify(pC2, nullptr, UnsubscribeCharacteristicDone, connection); exit: return CHIP_NO_ERROR; diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 2fdd1e1788af6b..8d37b1363e84ad 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -262,9 +262,8 @@ BluezGattCharacteristic1 * BluezEndpoint::CreateGattCharacteristic(BluezGattServ void BluezEndpoint::RegisterGattApplicationDone(GObject * aObject, GAsyncResult * aResult) { GAutoPtr error; - BluezGattManager1 * gattMgr = BLUEZ_GATT_MANAGER1(aObject); - - gboolean success = bluez_gatt_manager1_call_register_application_finish(gattMgr, aResult, &error.GetReceiver()); + gboolean success = bluez_gatt_manager1_call_register_application_finish(reinterpret_cast(aObject), aResult, + &error.GetReceiver()); VerifyOrReturn(success == TRUE, { ChipLogError(DeviceLayer, "FAIL: RegisterApplication : %s", error->message); @@ -287,7 +286,7 @@ CHIP_ERROR BluezEndpoint::RegisterGattApplicationImpl() adapterObject = g_dbus_interface_get_object(G_DBUS_INTERFACE(mAdapter.get())); VerifyOrExit(adapterObject != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adapterObject in %s", __func__)); - gattMgr.reset(bluez_object_get_gatt_manager1(BLUEZ_OBJECT(adapterObject))); + gattMgr.reset(bluez_object_get_gatt_manager1(reinterpret_cast(adapterObject))); VerifyOrExit(gattMgr.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL gattMgr in %s", __func__)); g_variant_builder_init(&optionsBuilder, G_VARIANT_TYPE("a{sv}")); @@ -383,7 +382,7 @@ void BluezEndpoint::BluezSignalOnObjectAdded(GDBusObjectManager * aManager, GDBu { // TODO: right now we do not handle addition/removal of adapters // Primary focus here is to handle addition of a device - GAutoPtr device(bluez_object_get_device1(BLUEZ_OBJECT(aObject))); + GAutoPtr device(bluez_object_get_device1(reinterpret_cast(aObject))); VerifyOrReturn(device.get() != nullptr); if (BluezIsDeviceOnAdapter(device.get(), mAdapter.get()) == TRUE) diff --git a/src/platform/Linux/bluez/BluezObjectIterator.h b/src/platform/Linux/bluez/BluezObjectIterator.h index ae5e01c6b9e34d..0e09cbcf217fc0 100644 --- a/src/platform/Linux/bluez/BluezObjectIterator.h +++ b/src/platform/Linux/bluez/BluezObjectIterator.h @@ -41,8 +41,8 @@ class BluezObjectIterator BluezObjectIterator() = default; explicit BluezObjectIterator(GList * position) : mPosition(position) {} - reference operator*() const { return *BLUEZ_OBJECT(mPosition->data); } - pointer operator->() const { return BLUEZ_OBJECT(mPosition->data); } + reference operator*() const { return *reinterpret_cast(mPosition->data); } + pointer operator->() const { return reinterpret_cast(mPosition->data); } bool operator==(const BluezObjectIterator & other) const { return mPosition == other.mPosition; } bool operator!=(const BluezObjectIterator & other) const { return mPosition != other.mPosition; } diff --git a/src/platform/Linux/bluez/ChipDeviceScanner.cpp b/src/platform/Linux/bluez/ChipDeviceScanner.cpp index 0d248058c3d1a0..5523bf87dfdf66 100644 --- a/src/platform/Linux/bluez/ChipDeviceScanner.cpp +++ b/src/platform/Linux/bluez/ChipDeviceScanner.cpp @@ -215,7 +215,7 @@ CHIP_ERROR ChipDeviceScanner::MainLoopStopScan(ChipDeviceScanner * self) void ChipDeviceScanner::SignalObjectAdded(GDBusObjectManager * manager, GDBusObject * object, ChipDeviceScanner * self) { - GAutoPtr device(bluez_object_get_device1(BLUEZ_OBJECT(object))); + GAutoPtr device(bluez_object_get_device1(reinterpret_cast(object))); VerifyOrReturn(device.get() != nullptr); self->ReportDevice(*device.get()); @@ -225,7 +225,7 @@ void ChipDeviceScanner::SignalInterfaceChanged(GDBusObjectManagerClient * manage GDBusProxy * aInterface, GVariant * aChangedProperties, const gchar * const * aInvalidatedProps, ChipDeviceScanner * self) { - GAutoPtr device(bluez_object_get_device1(BLUEZ_OBJECT(object))); + GAutoPtr device(bluez_object_get_device1(reinterpret_cast(object))); VerifyOrReturn(device.get() != nullptr); self->ReportDevice(*device.get()); From b278978aa3fd92e2f6aec39b7f007fdf79ad8a5c Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Fri, 8 Mar 2024 06:28:51 +0100 Subject: [PATCH 6/7] [Linux] Reuse AdapterIterator in all places where listing managed objects (#32372) * Use dedicated iterator for listing BlueZ objects * Use GAutoPtr to manage GDBusObjectManager instance * Wrap static function with lambda * Fix iterator initialization for AdapterIterator --- src/platform/GLibTypeDeleter.h | 6 ++ src/platform/Linux/bluez/AdapterIterator.cpp | 64 +++++-------------- src/platform/Linux/bluez/AdapterIterator.h | 14 ++-- src/platform/Linux/bluez/BluezConnection.cpp | 17 ++--- src/platform/Linux/bluez/BluezEndpoint.cpp | 13 ++-- src/platform/Linux/bluez/BluezEndpoint.h | 2 +- .../Linux/bluez/BluezObjectIterator.h | 2 + src/platform/Linux/bluez/BluezObjectList.h | 29 ++++----- 8 files changed, 58 insertions(+), 89 deletions(-) diff --git a/src/platform/GLibTypeDeleter.h b/src/platform/GLibTypeDeleter.h index f083a6c5e460d5..b1cec176ad4715 100644 --- a/src/platform/GLibTypeDeleter.h +++ b/src/platform/GLibTypeDeleter.h @@ -120,6 +120,12 @@ struct GAutoPtrDeleter using deleter = GObjectDeleter; }; +template <> +struct GAutoPtrDeleter +{ + using deleter = GObjectDeleter; +}; + template <> struct GAutoPtrDeleter { diff --git a/src/platform/Linux/bluez/AdapterIterator.cpp b/src/platform/Linux/bluez/AdapterIterator.cpp index 5510ceae996ce2..ced172446882b8 100644 --- a/src/platform/Linux/bluez/AdapterIterator.cpp +++ b/src/platform/Linux/bluez/AdapterIterator.cpp @@ -26,69 +26,38 @@ namespace chip { namespace DeviceLayer { namespace Internal { -AdapterIterator::~AdapterIterator() -{ - if (mManager != nullptr) - { - g_object_unref(mManager); - } - - if (mObjectList != nullptr) - { - g_list_free_full(mObjectList, g_object_unref); - } -} - -CHIP_ERROR AdapterIterator::Initialize(AdapterIterator * self) +CHIP_ERROR AdapterIterator::Initialize() { // When creating D-Bus proxy object, the thread default context must be initialized. Otherwise, // all D-Bus signals will be delivered to the GLib global default main context. VerifyOrDie(g_main_context_get_thread_default() != nullptr); - CHIP_ERROR err = CHIP_NO_ERROR; GAutoPtr error; - - self->mManager = g_dbus_object_manager_client_new_for_bus_sync( + mManager.reset(g_dbus_object_manager_client_new_for_bus_sync( G_BUS_TYPE_SYSTEM, G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_NONE, BLUEZ_INTERFACE, "/", bluez_object_manager_client_get_proxy_type, nullptr /* unused user data in the Proxy Type Func */, - nullptr /* destroy notify */, nullptr /* cancellable */, &error.GetReceiver()); + nullptr /* destroy notify */, nullptr /* cancellable */, &error.GetReceiver())); - VerifyOrExit(self->mManager != nullptr, ChipLogError(DeviceLayer, "Failed to get DBUS object manager for listing adapters."); - err = CHIP_ERROR_INTERNAL); + VerifyOrReturnError(mManager, CHIP_ERROR_INTERNAL, + ChipLogError(DeviceLayer, "Failed to get D-Bus object manager for listing adapters: %s", error->message)); - self->mObjectList = g_dbus_object_manager_get_objects(self->mManager); - self->mCurrentListItem = self->mObjectList; + mObjectList.Init(mManager.get()); + mIterator = mObjectList.begin(); -exit: - if (error != nullptr) - { - ChipLogError(DeviceLayer, "DBus error: %s", error->message); - } - - return err; + return CHIP_NO_ERROR; } bool AdapterIterator::Advance() { - if (mCurrentListItem == nullptr) + for (; mIterator != BluezObjectList::end(); ++mIterator) { - return false; - } - - while (mCurrentListItem != nullptr) - { - BluezAdapter1 * adapter = bluez_object_get_adapter1(BLUEZ_OBJECT(mCurrentListItem->data)); - if (adapter == nullptr) + BluezAdapter1 * adapter = bluez_object_get_adapter1(&(*mIterator)); + if (adapter != nullptr) { - mCurrentListItem = mCurrentListItem->next; - continue; + mCurrentAdapter.reset(adapter); + ++mIterator; + return true; } - - mCurrentAdapter.reset(adapter); - - mCurrentListItem = mCurrentListItem->next; - - return true; } return false; @@ -112,9 +81,10 @@ uint32_t AdapterIterator::GetIndex() const bool AdapterIterator::Next() { - if (mManager == nullptr) + if (!mManager) { - CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync(Initialize, this); + CHIP_ERROR err = PlatformMgrImpl().GLibMatterContextInvokeSync( + +[](AdapterIterator * self) { return self->Initialize(); }, this); VerifyOrReturnError(err == CHIP_NO_ERROR, false, ChipLogError(DeviceLayer, "Failed to initialize adapter iterator")); } diff --git a/src/platform/Linux/bluez/AdapterIterator.h b/src/platform/Linux/bluez/AdapterIterator.h index 0d44074889773b..38af64f7ecc21c 100644 --- a/src/platform/Linux/bluez/AdapterIterator.h +++ b/src/platform/Linux/bluez/AdapterIterator.h @@ -17,13 +17,15 @@ #pragma once -#include +#include #include #include +#include #include +#include "BluezObjectList.h" #include "Types.h" namespace chip { @@ -46,8 +48,6 @@ namespace Internal { class AdapterIterator { public: - ~AdapterIterator(); - /// Moves to the next DBUS interface. /// /// MUST be called before any of the 'current value' methods are @@ -65,7 +65,7 @@ class AdapterIterator private: /// Sets up the DBUS manager and loads the list - static CHIP_ERROR Initialize(AdapterIterator * self); + CHIP_ERROR Initialize(); /// Loads the next value in the list. /// @@ -73,9 +73,9 @@ class AdapterIterator /// iterate through. bool Advance(); - GDBusObjectManager * mManager = nullptr; // DBus connection - GList * mObjectList = nullptr; // listing of objects on the bus - GList * mCurrentListItem = nullptr; // current item viewed in the list + GAutoPtr mManager; + BluezObjectList mObjectList; + BluezObjectIterator mIterator; // Data valid only if Next() returns true GAutoPtr mCurrentAdapter; }; diff --git a/src/platform/Linux/bluez/BluezConnection.cpp b/src/platform/Linux/bluez/BluezConnection.cpp index 88595bd86c720c..dd4f9b108e2eb8 100644 --- a/src/platform/Linux/bluez/BluezConnection.cpp +++ b/src/platform/Linux/bluez/BluezConnection.cpp @@ -34,6 +34,7 @@ #include #include "BluezEndpoint.h" +#include "BluezObjectList.h" #include "Types.h" namespace chip { @@ -95,10 +96,6 @@ BluezConnection::ConnectionDataBundle::ConnectionDataBundle(const BluezConnectio CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) { - // populate the service and the characteristics - GList * objects = nullptr; - GList * l; - if (!aEndpoint.mIsCentral) { mpService = reinterpret_cast(g_object_ref(aEndpoint.mpService)); @@ -107,11 +104,9 @@ CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) } else { - objects = g_dbus_object_manager_get_objects(aEndpoint.mpObjMgr); - - for (l = objects; l != nullptr; l = l->next) + for (BluezObject & object : BluezObjectList(aEndpoint.mpObjMgr)) { - BluezGattService1 * service = bluez_object_get_gatt_service1(BLUEZ_OBJECT(l->data)); + BluezGattService1 * service = bluez_object_get_gatt_service1(&object); if (service != nullptr) { if ((BluezIsServiceOnDevice(service, mpDevice)) == TRUE && @@ -126,9 +121,9 @@ CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) VerifyOrExit(mpService != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL service in %s", __func__)); - for (l = objects; l != nullptr; l = l->next) + for (BluezObject & object : BluezObjectList(aEndpoint.mpObjMgr)) { - BluezGattCharacteristic1 * char1 = bluez_object_get_gatt_characteristic1(BLUEZ_OBJECT(l->data)); + BluezGattCharacteristic1 * char1 = bluez_object_get_gatt_characteristic1(&object); if (char1 != nullptr) { if ((BluezIsCharOnService(char1, mpService) == TRUE) && @@ -164,8 +159,6 @@ CHIP_ERROR BluezConnection::Init(const BluezEndpoint & aEndpoint) } exit: - if (objects != nullptr) - g_list_free_full(objects, g_object_unref); return CHIP_NO_ERROR; } diff --git a/src/platform/Linux/bluez/BluezEndpoint.cpp b/src/platform/Linux/bluez/BluezEndpoint.cpp index 8d37b1363e84ad..fe7a5a25171f73 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.cpp +++ b/src/platform/Linux/bluez/BluezEndpoint.cpp @@ -72,6 +72,7 @@ #include #include "BluezConnection.h" +#include "BluezObjectList.h" #include "Types.h" namespace chip { @@ -420,15 +421,14 @@ BluezGattService1 * BluezEndpoint::CreateGattService(const char * aUUID) return service; } -void BluezEndpoint::SetupAdapter() +CHIP_ERROR BluezEndpoint::SetupAdapter() { char expectedPath[32]; snprintf(expectedPath, sizeof(expectedPath), BLUEZ_PATH "/hci%u", mAdapterId); - GList * objects = g_dbus_object_manager_get_objects(mpObjMgr); - for (auto l = objects; l != nullptr && mAdapter.get() == nullptr; l = l->next) + for (BluezObject & object : BluezObjectList(mpObjMgr)) { - GAutoPtr adapter(bluez_object_get_adapter1(BLUEZ_OBJECT(l->data))); + GAutoPtr adapter(bluez_object_get_adapter1(&object)); if (adapter.get() != nullptr) { if (mpAdapterAddr == nullptr) // no adapter address provided, bind to the hci indicated by nodeid @@ -450,7 +450,7 @@ void BluezEndpoint::SetupAdapter() } } - VerifyOrExit(mAdapter.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mAdapter in %s", __func__)); + VerifyOrReturnError(mAdapter, CHIP_ERROR_INTERNAL, ChipLogError(DeviceLayer, "FAIL: NULL mAdapter in %s", __func__)); bluez_adapter1_set_powered(mAdapter.get(), TRUE); @@ -459,8 +459,7 @@ void BluezEndpoint::SetupAdapter() // and the flag is necessary to force using LE transport. bluez_adapter1_set_discoverable(mAdapter.get(), FALSE); -exit: - g_list_free_full(objects, g_object_unref); + return CHIP_NO_ERROR; } BluezConnection * BluezEndpoint::GetBluezConnection(const char * aPath) diff --git a/src/platform/Linux/bluez/BluezEndpoint.h b/src/platform/Linux/bluez/BluezEndpoint.h index d35cebdf6a6cfe..687f0002fe64b0 100644 --- a/src/platform/Linux/bluez/BluezEndpoint.h +++ b/src/platform/Linux/bluez/BluezEndpoint.h @@ -85,7 +85,7 @@ class BluezEndpoint private: CHIP_ERROR StartupEndpointBindings(); - void SetupAdapter(); + CHIP_ERROR SetupAdapter(); void SetupGattServer(GDBusConnection * aConn); void SetupGattService(); diff --git a/src/platform/Linux/bluez/BluezObjectIterator.h b/src/platform/Linux/bluez/BluezObjectIterator.h index 0e09cbcf217fc0..6b177acd03027b 100644 --- a/src/platform/Linux/bluez/BluezObjectIterator.h +++ b/src/platform/Linux/bluez/BluezObjectIterator.h @@ -17,6 +17,8 @@ #pragma once +#include + #include #include diff --git a/src/platform/Linux/bluez/BluezObjectList.h b/src/platform/Linux/bluez/BluezObjectList.h index ee7f6c001514e6..5831f07a3c6c85 100644 --- a/src/platform/Linux/bluez/BluezObjectList.h +++ b/src/platform/Linux/bluez/BluezObjectList.h @@ -35,27 +35,26 @@ namespace Internal { class BluezObjectList { public: - explicit BluezObjectList(GDBusObjectManager * manager) { Initialize(manager); } + BluezObjectList() = default; + explicit BluezObjectList(GDBusObjectManager * manager) { Init(manager); } - ~BluezObjectList() { g_list_free_full(mObjectList, g_object_unref); } - - BluezObjectIterator begin() const { return BluezObjectIterator(mObjectList); } - BluezObjectIterator end() const { return BluezObjectIterator(); } - -protected: - BluezObjectList() {} - - void Initialize(GDBusObjectManager * manager) + ~BluezObjectList() { - if (manager == nullptr) - { - ChipLogError(DeviceLayer, "Manager is NULL in %s", __func__); - return; - } + if (mObjectList != nullptr) + g_list_free_full(mObjectList, g_object_unref); + } + CHIP_ERROR Init(GDBusObjectManager * manager) + { + VerifyOrReturnError(manager != nullptr, CHIP_ERROR_INVALID_ARGUMENT, + ChipLogError(DeviceLayer, "Manager is NULL in %s", __func__)); mObjectList = g_dbus_object_manager_get_objects(manager); + return CHIP_NO_ERROR; } + BluezObjectIterator begin() const { return BluezObjectIterator(mObjectList); } + static BluezObjectIterator end() { return BluezObjectIterator(); } + private: GList * mObjectList = nullptr; }; From 4601714eac222d0ef63944f194ce2164be230e6b Mon Sep 17 00:00:00 2001 From: Wang Qixiang <43193572+wqx6@users.noreply.github.com> Date: Fri, 8 Mar 2024 22:57:46 +0800 Subject: [PATCH 7/7] Add checks for mOTInst in GenericThreadStackManagerImpl_OpenThread (#32482) * Add checks for mOTInst in GenericThreadStackManagerImpl_OpenThread * review changes --- ...nericThreadStackManagerImpl_OpenThread.hpp | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp index 21c44dd00279e3..707e1f710743ec 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp @@ -149,6 +149,7 @@ void GenericThreadStackManagerImpl_OpenThread::_ProcessThreadActivity template bool GenericThreadStackManagerImpl_OpenThread::_HaveRouteToAddress(const Inet::IPAddress & destAddr) { + VerifyOrReturnValue(mOTInst, false); bool res = false; // Lock OpenThread @@ -233,6 +234,7 @@ void GenericThreadStackManagerImpl_OpenThread::_OnPlatformEvent(const template bool GenericThreadStackManagerImpl_OpenThread::_IsThreadEnabled(void) { + VerifyOrReturnValue(mOTInst, false); otDeviceRole curRole; Impl()->LockThreadStack(); @@ -245,6 +247,7 @@ bool GenericThreadStackManagerImpl_OpenThread::_IsThreadEnabled(void) template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetThreadEnabled(bool val) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); otError otErr = OT_ERROR_NONE; Impl()->LockThreadStack(); @@ -279,6 +282,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetThreadEnable template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetThreadProvision(ByteSpan netInfo) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); otError otErr = OT_ERROR_FAILED; otOperationalDatasetTlvs tlvs; @@ -305,6 +309,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetThreadProvis template bool GenericThreadStackManagerImpl_OpenThread::_IsThreadProvisioned(void) { + VerifyOrReturnValue(mOTInst, false); bool provisioned; Impl()->LockThreadStack(); @@ -317,6 +322,7 @@ bool GenericThreadStackManagerImpl_OpenThread::_IsThreadProvisioned(v template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetThreadProvision(Thread::OperationalDataset & dataset) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(Impl()->IsThreadProvisioned(), CHIP_ERROR_INCORRECT_STATE); otOperationalDatasetTlvs datasetTlv; @@ -336,6 +342,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetThreadProvis template bool GenericThreadStackManagerImpl_OpenThread::_IsThreadAttached(void) { + VerifyOrReturnValue(mOTInst, false); otDeviceRole curRole; Impl()->LockThreadStack(); @@ -380,6 +387,7 @@ template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_StartThreadScan(NetworkCommissioning::ThreadDriver::ScanCallback * callback) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR error = CHIP_NO_ERROR; #if CHIP_CONFIG_ENABLE_ICD_SERVER otLinkModeConfig linkMode; @@ -488,6 +496,7 @@ void GenericThreadStackManagerImpl_OpenThread::_OnNetworkScanFinished template ConnectivityManager::ThreadDeviceType GenericThreadStackManagerImpl_OpenThread::_GetThreadDeviceType(void) { + VerifyOrReturnValue(mOTInst, ConnectivityManager::kThreadDeviceType_NotSupported); ConnectivityManager::ThreadDeviceType deviceType; Impl()->LockThreadStack(); @@ -524,6 +533,7 @@ template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetThreadDeviceType(ConnectivityManager::ThreadDeviceType deviceType) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR err = CHIP_NO_ERROR; otLinkModeConfig linkMode; @@ -612,6 +622,7 @@ GenericThreadStackManagerImpl_OpenThread::_SetThreadDeviceType(Connec template bool GenericThreadStackManagerImpl_OpenThread::_HaveMeshConnectivity(void) { + VerifyOrReturnValue(mOTInst, false); bool res; otDeviceRole curRole; @@ -660,6 +671,7 @@ bool GenericThreadStackManagerImpl_OpenThread::_HaveMeshConnectivity( template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetAndLogThreadStatsCounters(void) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR err = CHIP_NO_ERROR; otError otErr; otOperationalDataset activeDataset; @@ -754,6 +766,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetAndLogThread template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetAndLogThreadTopologyMinimal(void) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR err = CHIP_NO_ERROR; #if CHIP_PROGRESS_LOGGING @@ -822,6 +835,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetAndLogThread template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetAndLogThreadTopologyFull() { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR err = CHIP_NO_ERROR; #if CHIP_PROGRESS_LOGGING @@ -991,6 +1005,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetAndLogThread template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetPrimary802154MACAddress(uint8_t * buf) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); const otExtAddress * extendedAddr = otLinkGetExtendedAddress(mOTInst); memcpy(buf, extendedAddr, sizeof(otExtAddress)); return CHIP_NO_ERROR; @@ -999,6 +1014,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetPrimary80215 template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetExternalIPv6Address(chip::Inet::IPAddress & addr) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); const otNetifAddress * otAddresses = otIp6GetUnicastAddresses(mOTInst); // Look only for the global unicast addresses, not internally assigned by Thread. @@ -1034,6 +1050,7 @@ void GenericThreadStackManagerImpl_OpenThread::_ResetThreadNetworkDia template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_GetPollPeriod(uint32_t & buf) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); Impl()->LockThreadStack(); buf = otLinkGetPollPeriod(mOTInst); Impl()->UnlockThreadStack(); @@ -1121,6 +1138,7 @@ bool GenericThreadStackManagerImpl_OpenThread::IsThreadInterfaceUpNoL template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetPollingInterval(System::Clock::Milliseconds32 pollingInterval) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR err = CHIP_NO_ERROR; Impl()->LockThreadStack(); @@ -1173,6 +1191,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetPollingInter template void GenericThreadStackManagerImpl_OpenThread::_ErasePersistentInfo(void) { + VerifyOrReturn(mOTInst); ChipLogProgress(DeviceLayer, "Erasing Thread persistent info..."); Impl()->LockThreadStack(); otThreadSetEnabled(mOTInst, false); @@ -1205,6 +1224,7 @@ void GenericThreadStackManagerImpl_OpenThread::OnJoinerComplete(otErr template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_JoinerStart(void) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR error = CHIP_NO_ERROR; Impl()->LockThreadStack(); @@ -1254,6 +1274,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_JoinerStart(voi template void GenericThreadStackManagerImpl_OpenThread::_UpdateNetworkStatus() { + VerifyOrReturn(mOTInst); // Thread is not enabled, then we are not trying to connect to the network. VerifyOrReturn(ThreadStackMgrImpl().IsThreadEnabled() && mpStatusChangeCallback != nullptr); @@ -1636,6 +1657,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_RemoveInvalidSr template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_ClearAllSrpHostAndServices() { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR error = CHIP_NO_ERROR; Impl()->LockThreadStack(); if (!mIsSrpClearAllRequested) @@ -1684,6 +1706,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_SetupSrpHost(co template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_ClearSrpHost(const char * aHostName) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR error = CHIP_NO_ERROR; Impl()->LockThreadStack(); @@ -1798,6 +1821,7 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::FromOtDnsRespons template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::ResolveAddress(intptr_t context, otDnsAddressCallback callback) { + VerifyOrReturnError(ThreadStackMgrImpl().OTInstance(), CHIP_ERROR_INCORRECT_STATE); DnsResult * dnsResult = reinterpret_cast(context); ThreadStackMgrImpl().LockThreadStack(); @@ -1952,6 +1976,7 @@ template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_DnsBrowse(const char * aServiceName, DnsBrowseCallback aCallback, void * aContext) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR error = CHIP_NO_ERROR; Impl()->LockThreadStack(); @@ -2062,6 +2087,7 @@ template CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_DnsResolve(const char * aServiceName, const char * aInstanceName, DnsResolveCallback aCallback, void * aContext) { + VerifyOrReturnError(mOTInst, CHIP_ERROR_INCORRECT_STATE); CHIP_ERROR error = CHIP_NO_ERROR; Impl()->LockThreadStack();