From c19f26f800b6554c278ce80eec370b551576c477 Mon Sep 17 00:00:00 2001 From: Jean-Francois Penven <67962328+jepenven-silabs@users.noreply.github.com> Date: Tue, 21 Nov 2023 10:13:51 -0500 Subject: [PATCH 1/7] [ICD] Add Keep active during MDNS resolution ICD Check-In Sender (#30552) * Fix 30492 * apply comments * fix last comments --- src/app/FailSafeContext.cpp | 4 +- src/app/icd/BUILD.gn | 1 + src/app/icd/ICDCheckInSender.cpp | 19 +++--- src/app/icd/ICDManager.cpp | 60 +++++++++++++------ src/app/icd/ICDManager.h | 6 +- src/app/icd/ICDNotifier.h | 15 ++++- src/app/server/CommissioningWindowManager.cpp | 2 +- src/app/tests/TestICDManager.cpp | 2 +- src/messaging/ExchangeContext.cpp | 4 +- src/messaging/ReliableMessageMgr.cpp | 3 +- 10 files changed, 77 insertions(+), 39 deletions(-) diff --git a/src/app/FailSafeContext.cpp b/src/app/FailSafeContext.cpp index 2d886b17b34bc0..647401e7f6b5d4 100644 --- a/src/app/FailSafeContext.cpp +++ b/src/app/FailSafeContext.cpp @@ -56,9 +56,7 @@ void FailSafeContext::SetFailSafeArmed(bool armed) #if CHIP_CONFIG_ENABLE_ICD_SERVER if (IsFailSafeArmed() != armed) { - ICDListener::KeepActiveFlags activeRequest = ICDListener::KeepActiveFlags::kFailSafeArmed; - armed ? ICDNotifier::GetInstance().BroadcastActiveRequestNotification(activeRequest) - : ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(activeRequest); + ICDNotifier::GetInstance().BroadcastActiveRequest(ICDListener::KeepActiveFlag::kFailSafeArmed, armed); } #endif mFailSafeArmed = armed; diff --git a/src/app/icd/BUILD.gn b/src/app/icd/BUILD.gn index 8b8f10fd5c1587..99e8a23b0b699f 100644 --- a/src/app/icd/BUILD.gn +++ b/src/app/icd/BUILD.gn @@ -55,6 +55,7 @@ source_set("sender") { public_deps = [ ":cluster", + ":notifier", "${chip_root}/src/credentials:credentials", "${chip_root}/src/lib/address_resolve:address_resolve", "${chip_root}/src/protocols/secure_channel", diff --git a/src/app/icd/ICDCheckInSender.cpp b/src/app/icd/ICDCheckInSender.cpp index bd2b86bb24fdd3..25637ea1d821dc 100644 --- a/src/app/icd/ICDCheckInSender.cpp +++ b/src/app/icd/ICDCheckInSender.cpp @@ -17,6 +17,8 @@ #include "ICDCheckInSender.h" +#include "ICDNotifier.h" + #include #include @@ -37,15 +39,17 @@ ICDCheckInSender::ICDCheckInSender(Messaging::ExchangeManager * exchangeManager) void ICDCheckInSender::OnNodeAddressResolved(const PeerId & peerId, const AddressResolve::ResolveResult & result) { - mResolveInProgress = false; + if (CHIP_NO_ERROR != SendCheckInMsg(result.address)) + { + ChipLogError(AppServer, "Failed to send the ICD Check-In message"); + } - VerifyOrReturn(CHIP_NO_ERROR != SendCheckInMsg(result.address), - ChipLogError(AppServer, "Failed to send the ICD Check-In message")); + ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress); } void ICDCheckInSender::OnNodeAddressResolutionFailed(const PeerId & peerId, CHIP_ERROR reason) { - mResolveInProgress = false; + ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlag::kCheckInInProgress); ChipLogProgress(AppServer, "Node Address resolution failed for ICD Check-In with Node ID " ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId())); } @@ -55,12 +59,13 @@ CHIP_ERROR ICDCheckInSender::SendCheckInMsg(const Transport::PeerAddress & addr) System::PacketBufferHandle buffer = MessagePacketBuffer::New(CheckinMessage::sMinPayloadSize); VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_NO_MEMORY); - MutableByteSpan output{ buffer->Start(), buffer->DataLength() }; + MutableByteSpan output{ buffer->Start(), buffer->MaxDataLength() }; // TODO retrieve Check-in counter CounterType counter = 0; ReturnErrorOnFailure(CheckinMessage::GenerateCheckinMessagePayload(mKey, counter, ByteSpan(), output)); + buffer->SetDataLength(static_cast(output.size())); VerifyOrReturnError(mExchangeManager->GetSessionManager() != nullptr, CHIP_ERROR_INTERNAL); @@ -88,13 +93,11 @@ CHIP_ERROR ICDCheckInSender::RequestResolve(ICDMonitoringEntry & entry, FabricTa memcpy(mKey.AsMutable(), entry.key.As(), sizeof(Crypto::Aes128KeyByteArray)); - // TODO #30492 - // Device must stay active during MDNS resolution CHIP_ERROR err = AddressResolve::Resolver::Instance().LookupNode(request, mAddressLookupHandle); if (err == CHIP_NO_ERROR) { - mResolveInProgress = true; + ICDNotifier::GetInstance().BroadcastActiveRequestNotification(ICDListener::KeepActiveFlag::kCheckInInProgress); } return err; diff --git a/src/app/icd/ICDManager.cpp b/src/app/icd/ICDManager.cpp index 3d75b0d4dcf74c..fd744a25764402 100644 --- a/src/app/icd/ICDManager.cpp +++ b/src/app/icd/ICDManager.cpp @@ -40,9 +40,8 @@ using namespace chip::app; using namespace chip::app::Clusters; using namespace chip::app::Clusters::IcdManagement; -uint8_t ICDManager::OpenExchangeContextCount = 0; static_assert(UINT8_MAX >= CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS, - "ICDManager::OpenExchangeContextCount cannot hold count for the max exchange count"); + "ICDManager::mOpenExchangeContextCount cannot hold count for the max exchange count"); void ICDManager::Init(PersistentStorageDelegate * storage, FabricTable * fabricTable, Crypto::SymmetricKeystore * symmetricKeystore) { @@ -98,7 +97,6 @@ bool ICDManager::SupportsFeature(Feature feature) bool success = (Attributes::FeatureMap::Get(kRootEndpointId, &featureMap) == EMBER_ZCL_STATUS_SUCCESS); return success ? ((featureMap & to_underlying(feature)) != 0) : false; #else - return ((mFeatureMap & to_underlying(feature)) != 0); #endif // !CONFIG_BUILD_FOR_HOST_UNIT_TEST } @@ -171,7 +169,7 @@ void ICDManager::UpdateOperationState(OperationalState state) CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(slowPollInterval); if (err != CHIP_NO_ERROR) { - ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); + ChipLogError(AppServer, "Failed to set Slow Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); } } else if (state == OperationalState::ActiveMode) @@ -201,7 +199,7 @@ void ICDManager::UpdateOperationState(OperationalState state) CHIP_ERROR err = DeviceLayer::ConnectivityMgr().SetPollingInterval(GetFastPollingInterval()); if (err != CHIP_NO_ERROR) { - ChipLogError(AppServer, "Failed to set Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); + ChipLogError(AppServer, "Failed to set Fast Polling Interval: err %" CHIP_ERROR_FORMAT, err.Format()); } postObserverEvent(ObserverEventType::EnterActiveMode); @@ -274,44 +272,70 @@ void ICDManager::OnKeepActiveRequest(KeepActiveFlags request) { assertChipStackLockedByCurrentThread(); - if (request == KeepActiveFlags::kExchangeContextOpen) + VerifyOrReturn(request < KeepActiveFlagsValues::kInvalidFlag); + + if (request.Has(KeepActiveFlag::kExchangeContextOpen)) { // There can be multiple open exchange contexts at the same time. // Keep track of the requests count. - this->OpenExchangeContextCount++; - this->SetKeepActiveModeRequirements(request, true /* state */); + this->mOpenExchangeContextCount++; } - else /* !kExchangeContextOpen */ + + if (request.Has(KeepActiveFlag::kCheckInInProgress)) { - // Only 1 request per type (kCommissioningWindowOpen, kFailSafeArmed) - // set requirement directly - this->SetKeepActiveModeRequirements(request, true /* state */); + // There can be multiple check-in at the same time. + // Keep track of the requests count. + this->mCheckInRequestCount++; } + + this->SetKeepActiveModeRequirements(request, true /* state */); } void ICDManager::OnActiveRequestWithdrawal(KeepActiveFlags request) { assertChipStackLockedByCurrentThread(); - if (request == KeepActiveFlags::kExchangeContextOpen) + VerifyOrReturn(request < KeepActiveFlagsValues::kInvalidFlag); + + if (request.Has(KeepActiveFlag::kExchangeContextOpen)) { // There can be multiple open exchange contexts at the same time. // Keep track of the requests count. - if (this->OpenExchangeContextCount > 0) + if (this->mOpenExchangeContextCount > 0) { - this->OpenExchangeContextCount--; + this->mOpenExchangeContextCount--; } else { ChipLogError(DeviceLayer, "The ICD Manager did not account for ExchangeContext closure"); } - if (this->OpenExchangeContextCount == 0) + if (this->mOpenExchangeContextCount == 0) { - this->SetKeepActiveModeRequirements(request, false /* state */); + this->SetKeepActiveModeRequirements(KeepActiveFlag::kExchangeContextOpen, false /* state */); } } - else /* !kExchangeContextOpen */ + + if (request.Has(KeepActiveFlag::kCheckInInProgress)) + { + // There can be multiple open exchange contexts at the same time. + // Keep track of the requests count. + if (this->mCheckInRequestCount > 0) + { + this->mCheckInRequestCount--; + } + else + { + ChipLogError(DeviceLayer, "The ICD Manager did not account for Check-In Sender start"); + } + + if (this->mCheckInRequestCount == 0) + { + this->SetKeepActiveModeRequirements(KeepActiveFlag::kCheckInInProgress, false /* state */); + } + } + + if (request.Has(KeepActiveFlag::kCommissioningWindowOpen) || request.Has(KeepActiveFlag::kFailSafeArmed)) { // Only 1 request per type (kCommissioningWindowOpen, kFailSafeArmed) // remove requirement directly diff --git a/src/app/icd/ICDManager.h b/src/app/icd/ICDManager.h index 75001f8f71e0ca..573a20011f7ffc 100644 --- a/src/app/icd/ICDManager.h +++ b/src/app/icd/ICDManager.h @@ -118,6 +118,7 @@ class ICDManager : public ICDListener static void OnIdleModeDone(System::Layer * aLayer, void * appState); static void OnActiveModeDone(System::Layer * aLayer, void * appState); + /** * @brief Callback function called shortly before the device enters idle mode to allow checks to be made. This is currently only * called once to prevent entering in a loop if some events re-trigger this check (for instance if a check for subscription @@ -126,7 +127,8 @@ class ICDManager : public ICDListener */ static void OnTransitionToIdle(System::Layer * aLayer, void * appState); - static uint8_t OpenExchangeContextCount; + uint8_t mOpenExchangeContextCount = 0; + uint8_t mCheckInRequestCount = 0; private: // SIT ICDs should have a SlowPollingThreshold shorter than or equal to 15s (spec 9.16.1.5) @@ -139,7 +141,7 @@ class ICDManager : public ICDListener static constexpr uint32_t kMinActiveModeDuration = 300; static constexpr uint16_t kMinActiveModeThreshold = 300; - BitFlags mKeepActiveFlags{ 0 }; + KeepActiveFlags mKeepActiveFlags{ 0 }; // Initialize mOperationalState to ActiveMode so the init sequence at bootup triggers the IdleMode behaviour first. OperationalState mOperationalState = OperationalState::ActiveMode; diff --git a/src/app/icd/ICDNotifier.h b/src/app/icd/ICDNotifier.h index 9f6032c8616903..4344b72a1d3ac3 100644 --- a/src/app/icd/ICDNotifier.h +++ b/src/app/icd/ICDNotifier.h @@ -18,6 +18,7 @@ #include #include +#include class ICDListener; @@ -38,11 +39,13 @@ static_assert(ICD_MAX_NOTIFICATION_SUBSCRIBERS > 0, "At least 1 Subscriber is re class ICDListener { public: - enum class KeepActiveFlags : uint8_t + enum class KeepActiveFlagsValues : uint8_t { kCommissioningWindowOpen = 0x01, kFailSafeArmed = 0x02, - kExchangeContextOpen = 0x03, + kExchangeContextOpen = 0x04, + kCheckInInProgress = 0x08, + kInvalidFlag = 0x10, // Move up when adding more flags }; enum class ICDManagementEvents : uint8_t @@ -51,6 +54,9 @@ class ICDListener kStayActiveRequestReceived = 0x02, }; + using KeepActiveFlags = BitFlags; + using KeepActiveFlag = KeepActiveFlagsValues; + virtual ~ICDListener() {} /** @@ -101,6 +107,11 @@ class ICDNotifier void BroadcastActiveRequestWithdrawal(ICDListener::KeepActiveFlags request); void BroadcastICDManagementEvent(ICDListener::ICDManagementEvents event); + inline void BroadcastActiveRequest(ICDListener::KeepActiveFlags request, bool notify) + { + (notify) ? BroadcastActiveRequestNotification(request) : BroadcastActiveRequestWithdrawal(request); + } + static ICDNotifier & GetInstance() { return sICDNotifier; } private: diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index 33eaeedb1eb619..cfa201c57a3757 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -544,7 +544,7 @@ void CommissioningWindowManager::UpdateWindowStatus(CommissioningWindowStatusEnu { mWindowStatus = aNewStatus; #if CHIP_CONFIG_ENABLE_ICD_SERVER - app::ICDListener::KeepActiveFlags request = app::ICDListener::KeepActiveFlags::kCommissioningWindowOpen; + app::ICDListener::KeepActiveFlags request = app::ICDListener::KeepActiveFlag::kCommissioningWindowOpen; if (mWindowStatus != CommissioningWindowStatusEnum::kWindowNotOpen) { app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(request); diff --git a/src/app/tests/TestICDManager.cpp b/src/app/tests/TestICDManager.cpp index 7e47996dff78fa..ed6ecb7dc625b4 100644 --- a/src/app/tests/TestICDManager.cpp +++ b/src/app/tests/TestICDManager.cpp @@ -165,7 +165,7 @@ class TestICDManager static void TestKeepActivemodeRequests(nlTestSuite * aSuite, void * aContext) { TestContext * ctx = static_cast(aContext); - typedef ICDListener::KeepActiveFlags ActiveFlag; + typedef ICDListener::KeepActiveFlag ActiveFlag; ICDNotifier notifier = ICDNotifier::GetInstance(); // Setting a requirement will transition the ICD to active mode. diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 1b93bff5a62e62..c9d912daf88091 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -326,7 +326,7 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons SetAutoRequestAck(!session->IsGroupSession()); #if CHIP_CONFIG_ENABLE_ICD_SERVER - app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlags::kExchangeContextOpen); + app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlag::kExchangeContextOpen); #endif #if defined(CHIP_EXCHANGE_CONTEXT_DETAIL_LOGGING) @@ -345,7 +345,7 @@ ExchangeContext::~ExchangeContext() VerifyOrDie(mFlags.Has(Flags::kFlagClosed)); #if CHIP_CONFIG_ENABLE_ICD_SERVER - app::ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(app::ICDListener::KeepActiveFlags::kExchangeContextOpen); + app::ICDNotifier::GetInstance().BroadcastActiveRequestWithdrawal(app::ICDListener::KeepActiveFlag::kExchangeContextOpen); #endif // CHIP_CONFIG_ENABLE_ICD_SERVER // Ideally, in this scenario, the retransmit table should diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index 53d0487b7b826b..4ddf1801da0ade 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -38,7 +38,6 @@ #include #if CHIP_CONFIG_ENABLE_ICD_SERVER -#include // nogncheck #include // nogncheck #endif @@ -260,7 +259,7 @@ System::Clock::Timestamp ReliableMessageMgr::GetBackoff(System::Clock::Timestamp // Implement: // "An ICD sender SHOULD increase t to also account for its own sleepy interval // required to receive the acknowledgment" - mrpBackoffTime += app::ICDManager::GetFastPollingInterval(); + mrpBackoffTime += CHIP_DEVICE_CONFIG_ICD_FAST_POLL_INTERVAL; #endif mrpBackoffTime += CHIP_CONFIG_MRP_RETRY_INTERVAL_SENDER_BOOST; From 57743594b5d6fd6fb4dd8bb8d8275f3cdd61a3dd Mon Sep 17 00:00:00 2001 From: C Freeman Date: Tue, 21 Nov 2023 16:15:47 +0100 Subject: [PATCH 2/7] AutoCommissioner: Set default NTP to internal buffer (#29954) * AutoCommissioner: Set default NTP to internal buffer Also, it would be good to actually set the time zone to our own buffer. * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Size check and fail big strings, add tests, fix bug - neither DefaultNTP nor the time zone name is useful to the device if they're truncated, so don't attempt to send anything unless the value is spec compliant - NOTE: Right now, we still attempt to commission with the remaining parameters, but I'm open to returning an error here too - add tests for too-long names AND max size names - fix a bug in the server default NTP handling that prevented domain names from ever being set - turn on dns resolution in the all-clusters. Note that the all clusters app will never actually DO anything with these names, but it's still nice to have it available for testing. * Restyled by clang-format * Apply suggestions from code review Co-authored-by: Boris Zbarsky * Address review comments * Restyled by clang-format --------- Co-authored-by: Boris Zbarsky Co-authored-by: Restyled.io --- .../all-clusters-app.matter | 2 +- .../all-clusters-common/all-clusters-app.zap | 3 +- .../DefaultTimeSyncDelegate.cpp | 6 ++- .../time-synchronization-server.cpp | 23 ++++---- src/controller/AutoCommissioner.cpp | 30 +++++++++-- src/controller/AutoCommissioner.h | 3 ++ src/controller/CommissioningDelegate.h | 1 + .../ChipDeviceController-ScriptBinding.cpp | 17 +++++- src/controller/python/chip/ChipDeviceCtrl.py | 8 +-- src/python_testing/TC_TIMESYNC_2_6.py | 2 +- .../TestCommissioningTimeSync.py | 53 ++++++++++++++++++- 11 files changed, 119 insertions(+), 29 deletions(-) diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter index 3a6187874db2b0..1161b93c5119ee 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.matter @@ -5525,7 +5525,7 @@ endpoint 0 { ram attribute timeZoneDatabase default = 0; callback attribute timeZoneListMaxSize default = 3; callback attribute DSTOffsetListMaxSize; - ram attribute supportsDNSResolve default = false; + ram attribute supportsDNSResolve default = true; callback attribute generatedCommandList; callback attribute acceptedCommandList; callback attribute attributeList; diff --git a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap index 5f391a52fa267a..fe400ec5019b28 100644 --- a/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap +++ b/examples/all-clusters-app/all-clusters-common/all-clusters-app.zap @@ -4926,7 +4926,7 @@ "storageOption": "RAM", "singleton": 0, "bounded": 0, - "defaultValue": "false", + "defaultValue": "true", "reportable": 1, "minInterval": 1, "maxInterval": 65534, @@ -21585,6 +21585,7 @@ "endpointId": 65534, "networkId": 0 } + ] } diff --git a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp index 6b00f661eea412..82164afc2427b2 100644 --- a/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp +++ b/src/app/clusters/time-synchronization-server/DefaultTimeSyncDelegate.cpp @@ -44,8 +44,10 @@ bool DefaultTimeSyncDelegate::IsNTPAddressValid(chip::CharSpan ntp) bool DefaultTimeSyncDelegate::IsNTPAddressDomain(chip::CharSpan ntp) { - // placeholder implementation - return false; + // For now, assume anything that includes a . is a domain name. + // Delegates are free to evaluate this properly if they actually HAVE domain + // name resolution, rather than just implementing a dummy for testing. + return !IsNTPAddressValid(ntp) && (memchr(ntp.data(), '.', ntp.size()) != nullptr); } CHIP_ERROR DefaultTimeSyncDelegate::UpdateTimeFromPlatformSource(chip::Callback::Callback * callback) diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp index 7750c56be352d0..707abfb875045b 100644 --- a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp +++ b/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp @@ -1284,24 +1284,19 @@ bool emberAfTimeSynchronizationClusterSetDefaultNTPCallback( commandObj->AddStatus(commandPath, Status::ConstraintError); return true; } - if (!GetDelegate()->IsNTPAddressValid(dNtpChar.Value())) + bool dnsResolve; + if (EMBER_ZCL_STATUS_SUCCESS != SupportsDNSResolve::Get(commandPath.mEndpointId, &dnsResolve)) { - commandObj->AddStatus(commandPath, Status::InvalidCommand); + commandObj->AddStatus(commandPath, Status::Failure); return true; } - if (GetDelegate()->IsNTPAddressDomain(dNtpChar.Value())) + bool isDomain = GetDelegate()->IsNTPAddressDomain(dNtpChar.Value()); + bool isIPv6 = GetDelegate()->IsNTPAddressValid(dNtpChar.Value()); + bool useable = isIPv6 || (isDomain && dnsResolve); + if (!useable) { - bool dnsResolve; - if (EMBER_ZCL_STATUS_SUCCESS != SupportsDNSResolve::Get(commandPath.mEndpointId, &dnsResolve)) - { - commandObj->AddStatus(commandPath, Status::Failure); - return true; - } - if (!dnsResolve) - { - commandObj->AddStatus(commandPath, Status::InvalidCommand); - return true; - } + commandObj->AddStatus(commandPath, Status::InvalidCommand); + return true; } } diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index b97b4d544b7b4e..397497ab80f206 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -27,6 +27,8 @@ namespace chip { namespace Controller { using namespace chip::app::Clusters; +using chip::app::DataModel::MakeNullable; +using chip::app::DataModel::NullNullable; AutoCommissioner::AutoCommissioner() { @@ -87,7 +89,9 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam IsUnsafeSpan(params.GetAttestationSignature(), mParams.GetAttestationSignature()) || IsUnsafeSpan(params.GetPAI(), mParams.GetPAI()) || IsUnsafeSpan(params.GetDAC(), mParams.GetDAC()) || IsUnsafeSpan(params.GetTimeZone(), mParams.GetTimeZone()) || - IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets())); + IsUnsafeSpan(params.GetDSTOffsets(), mParams.GetDSTOffsets()) || + (params.GetDefaultNTP().HasValue() && !params.GetDefaultNTP().Value().IsNull() && + params.GetDefaultNTP().Value().Value().data() != mDefaultNtp)); mParams = params; @@ -194,16 +198,36 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam for (size_t i = 0; i < size; ++i) { mTimeZoneBuf[i] = params.GetTimeZone().Value()[i]; - if (mTimeZoneBuf[i].name.HasValue()) + if (params.GetTimeZone().Value()[i].name.HasValue() && + params.GetTimeZone().Value()[i].name.Value().size() <= kMaxTimeZoneNameLen) { auto span = MutableCharSpan(mTimeZoneNames[i], kMaxTimeZoneNameLen); - CopyCharSpanToMutableCharSpan(mTimeZoneBuf[i].name.Value(), span); + // The buffer backing "span" is statically allocated and is of size kMaxSupportedTimeZones, so this should never + // fail. + CopyCharSpanToMutableCharSpan(params.GetTimeZone().Value()[i].name.Value(), span); mTimeZoneBuf[i].name.SetValue(span); } + else + { + mTimeZoneBuf[i].name.ClearValue(); + } } auto list = app::DataModel::List(mTimeZoneBuf, size); mParams.SetTimeZone(list); } + if (params.GetDefaultNTP().HasValue()) + { + ChipLogProgress(Controller, "Setting Default NTP from parameters"); + // This parameter is an optional nullable, so we need to go two levels deep here. + if (!params.GetDefaultNTP().Value().IsNull() && params.GetDefaultNTP().Value().Value().size() <= kMaxDefaultNtpSize) + { + // The buffer backing "span" is statically allocated and is of size kMaxDefaultNtpSize. + auto span = MutableCharSpan(mDefaultNtp, kMaxDefaultNtpSize); + CopyCharSpanToMutableCharSpan(params.GetDefaultNTP().Value().Value(), span); + auto default_ntp = MakeNullable(CharSpan(mDefaultNtp, params.GetDefaultNTP().Value().Value().size())); + mParams.SetDefaultNTP(default_ntp); + } + } return CHIP_NO_ERROR; } diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 2e13445a113f5d..f86ebead1d011a 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -113,6 +113,9 @@ class AutoCommissioner : public CommissioningDelegate static constexpr size_t kMaxSupportedDstStructs = 10; app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type mDstOffsetsBuf[kMaxSupportedDstStructs]; + static constexpr size_t kMaxDefaultNtpSize = 128; + char mDefaultNtp[kMaxDefaultNtpSize]; + bool mNeedsNetworkSetup = false; ReadCommissioningInfo mDeviceCommissioningInfo; bool mNeedsDST = false; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index bb95f5eb8fe8f2..cbfff76adc6b3b 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -527,6 +527,7 @@ class CommissioningParameters mDAC.ClearValue(); mTimeZone.ClearValue(); mDSTOffsets.ClearValue(); + mDefaultNTP.ClearValue(); } private: diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index a7732a48361db0..2d6857d17a4fe6 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -100,6 +100,7 @@ chip::Platform::ScopedMemoryBuffer sThreadBuf; chip::Platform::ScopedMemoryBuffer sDefaultNTPBuf; app::Clusters::TimeSynchronization::Structs::DSTOffsetStruct::Type sDSTBuf; app::Clusters::TimeSynchronization::Structs::TimeZoneStruct::Type sTimeZoneBuf; +chip::Platform::ScopedMemoryBuffer sTimeZoneNameBuf; chip::Controller::CommissioningParameters sCommissioningParameters; } // namespace @@ -140,7 +141,7 @@ PyChipError pychip_DeviceController_UnpairDevice(chip::Controller::DeviceCommiss DeviceUnpairingCompleteFunct callback); PyChipError pychip_DeviceController_SetThreadOperationalDataset(const char * threadOperationalDataset, uint32_t size); PyChipError pychip_DeviceController_SetWiFiCredentials(const char * ssid, const char * credentials); -PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt); +PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt, const char * name); PyChipError pychip_DeviceController_SetDSTOffset(int32_t offset, uint64_t validStarting, uint64_t validUntil); PyChipError pychip_DeviceController_SetDefaultNtp(const char * defaultNTP); PyChipError pychip_DeviceController_SetTrustedTimeSource(chip::NodeId nodeId, chip::EndpointId endpoint); @@ -509,10 +510,22 @@ PyChipError pychip_DeviceController_SetWiFiCredentials(const char * ssid, const return ToPyChipError(CHIP_NO_ERROR); } -PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt) +PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt, const char * name) { sTimeZoneBuf.offset = offset; sTimeZoneBuf.validAt = validAt; + if (strcmp(name, "") == 0) + { + sTimeZoneNameBuf.Free(); + sTimeZoneBuf.name = NullOptional; + } + else + { + size_t len = strlen(name); + ReturnErrorCodeIf(!sTimeZoneNameBuf.Alloc(len), ToPyChipError(CHIP_ERROR_NO_MEMORY)); + memcpy(sTimeZoneNameBuf.Get(), name, len); + sTimeZoneBuf.name.SetValue(CharSpan(sTimeZoneNameBuf.Get(), len)); + } app::DataModel::List list(&sTimeZoneBuf, 1); sCommissioningParameters.SetTimeZone(list); return ToPyChipError(CHIP_NO_ERROR); diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index d6e8faa1742317..4db95205179a46 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -1421,10 +1421,10 @@ def _InitLib(self): c_char_p, c_char_p] self._dmLib.pychip_DeviceController_SetWiFiCredentials.restype = PyChipError - # Currently only supports 1 list item, no name + # Currently only supports 1 list item self._dmLib.pychip_DeviceController_SetTimeZone.restype = PyChipError self._dmLib.pychip_DeviceController_SetTimeZone.argtypes = [ - c_int32, c_uint64] + c_int32, c_uint64, c_char_p] # Currently only supports 1 list item self._dmLib.pychip_DeviceController_SetDSTOffset.restype = PyChipError @@ -1737,11 +1737,11 @@ def ResetCommissioningParameters(self): lambda: self._dmLib.pychip_DeviceController_ResetCommissioningParameters() ).raise_on_error() - def SetTimeZone(self, offset: int, validAt: int): + def SetTimeZone(self, offset: int, validAt: int, name: str = ""): ''' Set the time zone to set during commissioning. Currently only one time zone entry is supported''' self.CheckIsActive() self._ChipStack.Call( - lambda: self._dmLib.pychip_DeviceController_SetTimeZone(offset, validAt) + lambda: self._dmLib.pychip_DeviceController_SetTimeZone(offset, validAt, name.encode("utf-8")) ).raise_on_error() def SetDSTOffset(self, offset: int, validStarting: int, validUntil: int): diff --git a/src/python_testing/TC_TIMESYNC_2_6.py b/src/python_testing/TC_TIMESYNC_2_6.py index 23ed1a5575b6dd..ebd02ff3622f83 100644 --- a/src/python_testing/TC_TIMESYNC_2_6.py +++ b/src/python_testing/TC_TIMESYNC_2_6.py @@ -35,7 +35,7 @@ async def send_set_default_ntp_cmd(self, ntp: typing.Union[Nullable, str]) -> No async def send_set_default_ntp_cmd_expect_error(self, ntp: typing.Union[Nullable, str], error: Status) -> None: try: await self.send_single_cmd(cmd=Clusters.Objects.TimeSynchronization.Commands.SetDefaultNTP(defaultNTP=ntp)) - asserts.assert_true(False, "Unexpected SetTimeZone command success") + asserts.assert_true(False, "Unexpected SetDefaultNTP command success") except InteractionModelError as e: asserts.assert_equal(e.status, error, "Unexpected error returned") pass diff --git a/src/python_testing/TestCommissioningTimeSync.py b/src/python_testing/TestCommissioningTimeSync.py index dfd7c55f8d21f4..509aabfc6aa500 100644 --- a/src/python_testing/TestCommissioningTimeSync.py +++ b/src/python_testing/TestCommissioningTimeSync.py @@ -89,7 +89,7 @@ async def commission_and_base_checks(self): async def create_commissioner(self): if self.commissioner: - self.destroy_current_commissioner() + await self.destroy_current_commissioner() new_certificate_authority = self.certificate_authority_manager.NewCertificateAuthority() new_fabric_admin = new_certificate_authority.NewFabricAdmin(vendorId=0xFFF1, fabricId=2) self.commissioner = new_fabric_admin.NewController(nodeId=112233, useTestCommissioner=True) @@ -230,6 +230,57 @@ async def test_FabricCheckStage(self): kCheckForMatchingFabric), "Incorrectly ran check for matching fabric stage") asserts.assert_equal(self.commissioner.GetFabricCheckResult(), -1, "Fabric check result incorrectly set") + @async_test_body + async def test_TimeZoneName(self): + await self.create_commissioner() + self.commissioner.SetTimeZone(offset=3600, validAt=0, name="test") + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set') + + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone) + expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name="test")] + asserts.assert_equal(received, expected, "Time zone was not correctly set by commissioner") + + await self.create_commissioner() + # name is max 64 per the spec + sixty_five_byte_string = "x" * 65 + self.commissioner.SetTimeZone(offset=3600, validAt=0, name=sixty_five_byte_string) + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set') + + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone) + expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name=None)] + asserts.assert_equal(received, expected, "Commissioner did not ignore too-long name") + + await self.create_commissioner() + # name is max 64 per the spec + sixty_four_byte_string = "x" * 64 + self.commissioner.SetTimeZone(offset=3600, validAt=0, name=sixty_four_byte_string) + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureTimeZone), 'Time zone was not successfully set') + + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.TimeZone) + expected = [Clusters.TimeSynchronization.Structs.TimeZoneStruct(offset=3600, validAt=0, name=sixty_four_byte_string)] + asserts.assert_equal(received, expected, "Time zone 64 byte name was not correctly set") + + @async_test_body + async def test_DefaultNtpSize(self): + await self.create_commissioner() + too_long_name = "x." + "x" * 127 + self.commissioner.SetDefaultNTP(too_long_name) + await self.commission_and_base_checks() + asserts.assert_false(self.commissioner.CheckStageSuccessful(kConfigureDefaultNTP), + 'Commissioner attempted to set default NTP to a too long value') + + await self.create_commissioner() + just_fits_name = "x." + "x" * 126 + self.commissioner.SetDefaultNTP(just_fits_name) + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful(kConfigureDefaultNTP), + 'Commissioner did not correctly set default NTP') + received = await self.read_single_attribute_check_success(cluster=Clusters.TimeSynchronization, attribute=Clusters.TimeSynchronization.Attributes.DefaultNTP) + asserts.assert_equal(received, just_fits_name, 'Commissioner incorrectly set default NTP name') + # TODO(cecille): Test - Add hooks to change the time zone response to indicate no DST is needed # TODO(cecille): Test - Set commissioningParameters TimeZone and DST list size to > node list size to ensure they get truncated From 26df725acdc6575c625666f837ac3f9a50336a33 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 21 Nov 2023 11:24:27 -0500 Subject: [PATCH 3/7] Add comment regarding why platforms depend on address resolve config (#30597) * Add comment regarding why platforms depend on address resolve config * Restyle --------- Co-authored-by: Andrei Litvin --- src/platform/Infineon/CYW30739/BUILD.gn | 5 +++++ src/platform/bouffalolab/BL702/BUILD.gn | 11 ++++++++--- src/platform/bouffalolab/BL702L/BUILD.gn | 11 ++++++++--- src/platform/cc13xx_26xx/cc13x2_26x2/BUILD.gn | 5 +++++ src/platform/cc13xx_26xx/cc13x4_26x4/BUILD.gn | 5 +++++ src/platform/nxp/k32w/k32w0/BUILD.gn | 11 ++++++++--- src/platform/nxp/k32w/k32w1/BUILD.gn | 4 ++++ src/platform/nxp/rt/rw61x/BUILD.gn | 5 ++++- src/platform/qpg/BUILD.gn | 4 ++++ src/platform/silabs/efr32/BUILD.gn | 4 ++++ src/platform/stm32/BUILD.gn | 4 ++++ 11 files changed, 59 insertions(+), 10 deletions(-) diff --git a/src/platform/Infineon/CYW30739/BUILD.gn b/src/platform/Infineon/CYW30739/BUILD.gn index 51a3be470e36c3..ff0704fb37aedc 100644 --- a/src/platform/Infineon/CYW30739/BUILD.gn +++ b/src/platform/Infineon/CYW30739/BUILD.gn @@ -82,6 +82,11 @@ static_library("CYW30739") { "ThreadStackManagerImpl.cpp", "ThreadStackManagerImpl.h", ] + + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h public_configs = [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config", ] diff --git a/src/platform/bouffalolab/BL702/BUILD.gn b/src/platform/bouffalolab/BL702/BUILD.gn index 831f0d27679e21..28cc128fd7fb61 100644 --- a/src/platform/bouffalolab/BL702/BUILD.gn +++ b/src/platform/bouffalolab/BL702/BUILD.gn @@ -101,6 +101,14 @@ static_library("BL702") { ] deps += [ "${chip_root}/src/lib/dnssd:platform_header" ] } + + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h + public_configs = [ + "${chip_root}/src/lib/address_resolve:default_address_resolve_config", + ] } if (chip_enable_ethernet) { @@ -115,7 +123,4 @@ static_library("BL702") { deps += [ "${chip_root}/src/credentials:credentials_header" ] public_deps = [ "${chip_root}/src/platform:platform_base" ] - - public_configs = - [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config" ] } diff --git a/src/platform/bouffalolab/BL702L/BUILD.gn b/src/platform/bouffalolab/BL702L/BUILD.gn index d4e077d5922802..4e018cf8cd65ad 100644 --- a/src/platform/bouffalolab/BL702L/BUILD.gn +++ b/src/platform/bouffalolab/BL702L/BUILD.gn @@ -88,11 +88,16 @@ static_library("BL702L") { ] deps += [ "${chip_root}/src/lib/dnssd:platform_header" ] } + + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h + public_configs = [ + "${chip_root}/src/lib/address_resolve:default_address_resolve_config", + ] } deps += [ "${chip_root}/src/credentials:credentials_header" ] public_deps = [ "${chip_root}/src/platform:platform_base" ] - - public_configs = - [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config" ] } diff --git a/src/platform/cc13xx_26xx/cc13x2_26x2/BUILD.gn b/src/platform/cc13xx_26xx/cc13x2_26x2/BUILD.gn index 18dfc7dff0bf58..f293eee81e3e58 100644 --- a/src/platform/cc13xx_26xx/cc13x2_26x2/BUILD.gn +++ b/src/platform/cc13xx_26xx/cc13x2_26x2/BUILD.gn @@ -52,6 +52,11 @@ static_library("cc13x2_26x2") { "${chip_root}/src/crypto", "${chip_root}/src/platform:platform_base", ] + + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h public_configs = [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config" ] diff --git a/src/platform/cc13xx_26xx/cc13x4_26x4/BUILD.gn b/src/platform/cc13xx_26xx/cc13x4_26x4/BUILD.gn index 1dd6933b2f23ca..d6967347eca720 100644 --- a/src/platform/cc13xx_26xx/cc13x4_26x4/BUILD.gn +++ b/src/platform/cc13xx_26xx/cc13x4_26x4/BUILD.gn @@ -52,6 +52,11 @@ static_library("cc13x4_26x4") { "${chip_root}/src/crypto", "${chip_root}/src/platform:platform_base", ] + + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h public_configs = [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config" ] diff --git a/src/platform/nxp/k32w/k32w0/BUILD.gn b/src/platform/nxp/k32w/k32w0/BUILD.gn index 193ba06d78a271..a5c204a10b956c 100644 --- a/src/platform/nxp/k32w/k32w0/BUILD.gn +++ b/src/platform/nxp/k32w/k32w0/BUILD.gn @@ -140,10 +140,15 @@ static_library("k32w0") { ] deps += [ "${chip_root}/src/lib/dnssd:platform_header" ] } + + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h + public_configs = [ + "${chip_root}/src/lib/address_resolve:default_address_resolve_config", + ] } public_deps += [ "${chip_root}/src/crypto" ] - - public_configs = - [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config" ] } diff --git a/src/platform/nxp/k32w/k32w1/BUILD.gn b/src/platform/nxp/k32w/k32w1/BUILD.gn index 5b42263b8380a0..053f8d784ac180 100644 --- a/src/platform/nxp/k32w/k32w1/BUILD.gn +++ b/src/platform/nxp/k32w/k32w1/BUILD.gn @@ -107,6 +107,10 @@ static_library("k32w1") { public_deps += [ "${chip_root}/src/crypto" ] + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h public_configs = [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config" ] } diff --git a/src/platform/nxp/rt/rw61x/BUILD.gn b/src/platform/nxp/rt/rw61x/BUILD.gn index d8a72d1e449718..1cf0b5ac55f9a7 100644 --- a/src/platform/nxp/rt/rw61x/BUILD.gn +++ b/src/platform/nxp/rt/rw61x/BUILD.gn @@ -147,7 +147,10 @@ static_library("nxp_platform") { public_configs = [ ":nxp_platform_config", - # address_resolve config needed by the include of server.h in ConfigurationManagerImpl.cpp + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h "${chip_root}/src/lib/address_resolve:default_address_resolve_config", ] } diff --git a/src/platform/qpg/BUILD.gn b/src/platform/qpg/BUILD.gn index 9122243c005874..4e1e5241269cc8 100644 --- a/src/platform/qpg/BUILD.gn +++ b/src/platform/qpg/BUILD.gn @@ -70,6 +70,10 @@ static_library("qpg") { public_deps += [ "${openthread_root}:libopenthread-mtd" ] } + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h public_configs += [ "${chip_root}/third_party/openthread/platforms/qpg:openthread_qpg_config", "${chip_root}/src/lib/address_resolve:default_address_resolve_config", diff --git a/src/platform/silabs/efr32/BUILD.gn b/src/platform/silabs/efr32/BUILD.gn index 36d3a05787c16f..5c7eef17f65875 100644 --- a/src/platform/silabs/efr32/BUILD.gn +++ b/src/platform/silabs/efr32/BUILD.gn @@ -124,6 +124,10 @@ static_library("efr32") { deps += [ "${chip_root}/src/lib/dnssd:platform_header" ] } + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h public_configs = [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config", ] diff --git a/src/platform/stm32/BUILD.gn b/src/platform/stm32/BUILD.gn index bc823db59aedf1..f6610d7ff7329a 100644 --- a/src/platform/stm32/BUILD.gn +++ b/src/platform/stm32/BUILD.gn @@ -90,6 +90,10 @@ static_library("stm32") { deps += [ "${chip_root}/src/lib/dnssd:platform_header" ] + # TODO: platform should NOT depend on default_address_resolve_config. This should + # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 + # + # Currently this exists because OpenThread platform includes src/app/Server.h public_configs = [ "${chip_root}/src/lib/address_resolve:default_address_resolve_config", ] From 971e060ed42b6e87552a92e19ab23391ebeb273a Mon Sep 17 00:00:00 2001 From: C Freeman Date: Tue, 21 Nov 2023 19:26:12 +0100 Subject: [PATCH 4/7] Air purifier: Fix conformance (#30601) - admin commissioning doesn't have the BCW feature, but had the command on - turned off command - Identify cluster revision is now 4. It already included the v4 attribute, it was just the revision that was incorrect. --- .../air-purifier-common/air-purifier-app.matter | 8 +------- .../air-purifier-common/air-purifier-app.zap | 10 +--------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/examples/air-purifier-app/air-purifier-common/air-purifier-app.matter b/examples/air-purifier-app/air-purifier-common/air-purifier-app.matter index 99693cb41a99b2..f4f930ac93a015 100644 --- a/examples/air-purifier-app/air-purifier-common/air-purifier-app.matter +++ b/examples/air-purifier-app/air-purifier-common/air-purifier-app.matter @@ -795,12 +795,7 @@ server cluster AdministratorCommissioning = 60 { octet_string<32> salt = 4; } - request struct OpenBasicCommissioningWindowRequest { - int16u commissioningTimeout = 0; - } - timed command access(invoke: administer) OpenCommissioningWindow(OpenCommissioningWindowRequest): DefaultSuccess = 0; - timed command access(invoke: administer) OpenBasicCommissioningWindow(OpenBasicCommissioningWindowRequest): DefaultSuccess = 1; timed command access(invoke: administer) RevokeCommissioning(): DefaultSuccess = 2; } @@ -1917,7 +1912,6 @@ endpoint 0 { ram attribute clusterRevision default = 0x0001; handle command OpenCommissioningWindow; - handle command OpenBasicCommissioningWindow; handle command RevokeCommissioning; } @@ -1973,7 +1967,7 @@ endpoint 1 { callback attribute eventList; callback attribute attributeList default = 0; ram attribute featureMap default = 0; - ram attribute clusterRevision default = 2; + ram attribute clusterRevision default = 4; handle command Identify; handle command TriggerEffect; diff --git a/examples/air-purifier-app/air-purifier-common/air-purifier-app.zap b/examples/air-purifier-app/air-purifier-common/air-purifier-app.zap index 1f0867d7757cbb..4daeefb57b7fa6 100644 --- a/examples/air-purifier-app/air-purifier-common/air-purifier-app.zap +++ b/examples/air-purifier-app/air-purifier-common/air-purifier-app.zap @@ -1633,14 +1633,6 @@ "isIncoming": 1, "isEnabled": 1 }, - { - "name": "OpenBasicCommissioningWindow", - "code": 1, - "mfgCode": null, - "source": "client", - "isIncoming": 1, - "isEnabled": 1 - }, { "name": "RevokeCommissioning", "code": 2, @@ -2302,7 +2294,7 @@ "storageOption": "RAM", "singleton": 0, "bounded": 0, - "defaultValue": "2", + "defaultValue": "4", "reportable": 1, "minInterval": 1, "maxInterval": 65534, From 41971bae8fa8017a90501de4bf1d6c67803e0e25 Mon Sep 17 00:00:00 2001 From: Jean-Francois Penven <67962328+jepenven-silabs@users.noreply.github.com> Date: Tue, 21 Nov 2023 23:13:56 -0500 Subject: [PATCH 5/7] Fix Circular dependency OpenThreadPlatform (#30607) --- src/app/server/Server.cpp | 10 ++++++++++ .../GenericThreadStackManagerImpl_OpenThread.hpp | 12 ------------ src/platform/silabs/efr32/BUILD.gn | 8 +------- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 8aa1d37a99122e..65a45192728833 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -408,6 +408,16 @@ void Server::OnPlatformEvent(const DeviceLayer::ChipDeviceEvent & event) ResumeSubscriptions(); #endif break; +#if CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT + case DeviceEventType::kThreadConnectivityChange: + if (event.ThreadConnectivityChange.Result == kConnectivity_Established) + { + // Refresh Multicast listening + ChipLogDetail(DeviceLayer, "Thread Attached updating Multicast address"); + RejoinExistingMulticastGroups(); + } + break; +#endif // CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT default: break; } diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp index 0d19ae192b5e36..7bc357f0705cc9 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp @@ -65,9 +65,6 @@ #include #include -#if CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT -#include -#endif // CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT extern "C" void otSysProcessDrivers(otInstance * aInstance); #if CHIP_DEVICE_CONFIG_THREAD_ENABLE_CLI @@ -210,15 +207,6 @@ void GenericThreadStackManagerImpl_OpenThread::_OnPlatformEvent(const } } -#if CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT - if (event->ThreadStateChange.AddressChanged && isThreadAttached) - { - // Refresh Multicast listening - ChipLogDetail(DeviceLayer, "Thread Attached updating Multicast address"); - Server::GetInstance().RejoinExistingMulticastGroups(); - } -#endif // CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT - #if CHIP_DETAIL_LOGGING LogOpenThreadStateChange(mOTInst, event->ThreadStateChange.OpenThread.Flags); #endif // CHIP_DETAIL_LOGGING diff --git a/src/platform/silabs/efr32/BUILD.gn b/src/platform/silabs/efr32/BUILD.gn index 5c7eef17f65875..e244ad178a9848 100644 --- a/src/platform/silabs/efr32/BUILD.gn +++ b/src/platform/silabs/efr32/BUILD.gn @@ -124,13 +124,7 @@ static_library("efr32") { deps += [ "${chip_root}/src/lib/dnssd:platform_header" ] } - # TODO: platform should NOT depend on default_address_resolve_config. This should - # be removed. See https://github.com/project-chip/connectedhomeip/issues/30596 - # - # Currently this exists because OpenThread platform includes src/app/Server.h - public_configs = [ - "${chip_root}/src/lib/address_resolve:default_address_resolve_config", - ] + public_configs = [] } if (chip_enable_wifi) { From 4e2d4898735f35555576e0ecccfd3b782c8b320a Mon Sep 17 00:00:00 2001 From: Terence Hampson Date: Tue, 21 Nov 2023 23:16:42 -0500 Subject: [PATCH 6/7] Changes to CommandStatusIB to reflect 1.3 spec (#30595) --- src/app/MessageDef/CommandDataIB.h | 2 +- src/app/MessageDef/CommandStatusIB.cpp | 18 ++++++++++++++++++ src/app/MessageDef/CommandStatusIB.h | 21 +++++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/app/MessageDef/CommandDataIB.h b/src/app/MessageDef/CommandDataIB.h index b3bd2f4181d2c9..9872f5e43584dc 100644 --- a/src/app/MessageDef/CommandDataIB.h +++ b/src/app/MessageDef/CommandDataIB.h @@ -71,7 +71,7 @@ class Parser : public StructParser /** * @brief Get the provided command reference associated with the CommandData * - * @param [in] apRef A pointer to apRef + * @param [out] apRef A pointer to apRef * * @return #CHIP_NO_ERROR on success * #CHIP_ERROR_WRONG_TLV_TYPE if there is such element but it's not any of the defined unsigned integer types diff --git a/src/app/MessageDef/CommandStatusIB.cpp b/src/app/MessageDef/CommandStatusIB.cpp index 4f38bb292e657e..566ea0a7d83058 100644 --- a/src/app/MessageDef/CommandStatusIB.cpp +++ b/src/app/MessageDef/CommandStatusIB.cpp @@ -75,6 +75,14 @@ CHIP_ERROR CommandStatusIB::Parser::PrettyPrint() const PRETTY_PRINT_DECDEPTH(); } break; + case to_underlying(Tag::kRef): + VerifyOrReturnError(TLV::kTLVType_UnsignedInteger == reader.GetType(), CHIP_ERROR_WRONG_TLV_TYPE); + { + uint16_t reference; + ReturnErrorOnFailure(reader.Get(reference)); + PRETTY_PRINT("\tRef = 0x%x,", reference); + } + break; default: PRETTY_PRINT("Unknown tag num %" PRIu32, tagNum); break; @@ -108,6 +116,11 @@ CHIP_ERROR CommandStatusIB::Parser::GetErrorStatus(StatusIB::Parser * const apEr return apErrorStatus->Init(reader); } +CHIP_ERROR CommandStatusIB::Parser::GetRef(uint16_t * const apRef) const +{ + return GetUnsignedInteger(to_underlying(Tag::kRef), apRef); +} + CommandPathIB::Builder & CommandStatusIB::Builder::CreatePath() { if (mError == CHIP_NO_ERROR) @@ -126,6 +139,11 @@ StatusIB::Builder & CommandStatusIB::Builder::CreateErrorStatus() return mErrorStatus; } +CHIP_ERROR CommandStatusIB::Builder::Ref(const uint16_t aRef) +{ + return mpWriter->Put(TLV::ContextTag(Tag::kRef), aRef); +} + CHIP_ERROR CommandStatusIB::Builder::EndOfCommandStatusIB() { EndOfContainer(); diff --git a/src/app/MessageDef/CommandStatusIB.h b/src/app/MessageDef/CommandStatusIB.h index 8614df28cab4b5..a780e02683eec0 100644 --- a/src/app/MessageDef/CommandStatusIB.h +++ b/src/app/MessageDef/CommandStatusIB.h @@ -37,6 +37,7 @@ enum class Tag : uint8_t { kPath = 0, kErrorStatus = 1, + kRef = 2, }; class Parser : public StructParser @@ -67,6 +68,17 @@ class Parser : public StructParser * #CHIP_END_OF_TLV if there is no such element */ CHIP_ERROR GetErrorStatus(StatusIB::Parser * const apErrorStatus) const; + + /** + * @brief Get the provided command reference associated with the CommandStatus + * + * @param [out] apRef A pointer to apRef + * + * @return #CHIP_NO_ERROR on success + * #CHIP_ERROR_WRONG_TLV_TYPE if there is such element but it's not any of the defined unsigned integer types + * #CHIP_END_OF_TLV if there is no such element + */ + CHIP_ERROR GetRef(uint16_t * const apRef) const; }; class Builder : public StructBuilder @@ -86,6 +98,15 @@ class Builder : public StructBuilder */ StatusIB::Builder & CreateErrorStatus(); + /** + * @brief Inject Command Ref into the TLV stream. + * + * @param [in] aRef refer to the CommandRef to set in CommandStatusIB. + * + * @return #CHIP_NO_ERROR on success + */ + CHIP_ERROR Ref(const uint16_t aRef); + /** * @brief Mark the end of this CommandStatusIB * From 90f36a4acd8b8a478e11f3f14558071d07966ee9 Mon Sep 17 00:00:00 2001 From: simonhmorris1 <112178216+simonhmorris1@users.noreply.github.com> Date: Wed, 22 Nov 2023 09:29:14 +0000 Subject: [PATCH 7/7] Implement Non concurrent mode (#30201) * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Correct intermittent missing SSID(#29865) * ConnectNetworkResponse Supression (#29865) * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Fix darwin-tests workflow file syntax. (#30162) There were extra curly braces that failed on some (but not all?) CI runs. * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Only start WiFi on BLE Commissioning pass(#29865) * Remove use of Breadcrumb::Get from BLE (#29865) * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Only start WiFi on BLE Commissioning pass(#29865) * Remove use of Breadcrumb::Get from BLE (#29865) * Tidy comments (#29865) * Default on error, check timeout, tidy up (#29865) * Make log messages user friendly (#29865) * Change to compile time non-concurrent mode(#29865) * Restyled by whitespace * Restyled by clang-format * Improve code style (#29865) * Restyled by clang-format * Add CommandHandler issue reference 30576 (#29865) --------- Co-authored-by: Boris Zbarsky Co-authored-by: Restyled.io --- examples/platform/linux/AppMain.cpp | 17 ++-- .../network-commissioning.cpp | 32 +++++++- .../network-commissioning.h | 1 + src/app/server/CommissioningWindowManager.cpp | 10 ++- src/controller/AutoCommissioner.cpp | 41 +++++----- src/controller/CHIPDeviceController.cpp | 81 +++++++++++++++---- src/controller/CommissioningDelegate.h | 22 +++-- src/include/platform/CHIPDeviceConfig.h | 13 +++ src/include/platform/CHIPDeviceEvent.h | 12 +++ src/include/platform/DeviceControlServer.h | 3 +- src/platform/DeviceControlServer.cpp | 14 ++++ src/platform/Linux/BLEManagerImpl.cpp | 4 + .../Linux/ConnectivityManagerImpl.cpp | 18 +++++ src/platform/Linux/ConnectivityManagerImpl.h | 1 + 14 files changed, 220 insertions(+), 49 deletions(-) diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp index 56e6edd44687f2..b4eb6ecfd3baab 100644 --- a/examples/platform/linux/AppMain.cpp +++ b/examples/platform/linux/AppMain.cpp @@ -20,6 +20,7 @@ #include #include "app/clusters/network-commissioning/network-commissioning.h" +#include #include #include #include @@ -238,15 +239,15 @@ void InitNetworkCommissioning() using chip::Shell::Engine; #endif -#if CHIP_DEVICE_CONFIG_ENABLE_WPA +#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION /* * The device shall check every kWiFiStartCheckTimeUsec whether Wi-Fi management * has been fully initialized. If after kWiFiStartCheckAttempts Wi-Fi management * still hasn't been initialized, the device configuration is reset, and device * needs to be paired again. */ -static constexpr useconds_t kWiFiStartCheckTimeUsec = 100 * 1000; // 100 ms -static constexpr uint8_t kWiFiStartCheckAttempts = 5; +static constexpr useconds_t kWiFiStartCheckTimeUsec = WIFI_START_CHECK_TIME_USEC; +static constexpr uint8_t kWiFiStartCheckAttempts = WIFI_START_CHECK_ATTEMPTS; #endif namespace { @@ -264,6 +265,11 @@ void EventHandler(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg) { ChipLogProgress(DeviceLayer, "Receive kCHIPoBLEConnectionEstablished"); } + else if ((event->Type == chip::DeviceLayer::DeviceEventType::kInternetConnectivityChange)) + { + // Restart the server on connectivity change + app::DnssdServer::Instance().StartServer(); + } } void Cleanup() @@ -298,7 +304,7 @@ void StopSignalHandler(int signal) } // namespace -#if CHIP_DEVICE_CONFIG_ENABLE_WPA +#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION static bool EnsureWiFiIsStarted() { for (int cnt = 0; cnt < kWiFiStartCheckAttempts; cnt++) @@ -462,9 +468,10 @@ int ChipLinuxAppInit(int argc, char * const argv[], OptionSet * customOptions, DeviceLayer::ConnectivityMgr().SetBLEAdvertisingEnabled(true); #endif -#if CHIP_DEVICE_CONFIG_ENABLE_WPA +#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION if (LinuxDeviceOptions::GetInstance().mWiFi) { + // Start WiFi management in Concurrent mode DeviceLayer::ConnectivityMgrImpl().StartWiFiManagement(); if (!EnsureWiFiIsStarted()) { diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index 68dfed344e0da6..471b6038b85b09 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -171,11 +171,18 @@ void Instance::InvokeCommand(HandlerContext & ctxt) ctxt, [this](HandlerContext & ctx, const auto & req) { HandleRemoveNetwork(ctx, req); }); return; - case Commands::ConnectNetwork::Id: + case Commands::ConnectNetwork::Id: { VerifyOrReturn(mFeatureFlags.Has(Feature::kWiFiNetworkInterface) || mFeatureFlags.Has(Feature::kThreadNetworkInterface)); +#if CONFIG_NETWORK_LAYER_BLE && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + // If commissionee does not support Concurrent Connections, request the BLE to be stopped. + // Start the ConnectNetwork, but this will not complete until the BLE is off. + ChipLogProgress(NetworkProvisioning, "Closing BLE connections due to non-concurrent mode"); + DeviceLayer::DeviceControlServer::DeviceControlSvr().PostCloseAllBLEConnectionsToOperationalNetworkEvent(); +#endif HandleCommand( ctxt, [this](HandlerContext & ctx, const auto & req) { HandleConnectNetwork(ctx, req); }); return; + } case Commands::ReorderNetwork::Id: VerifyOrReturn(mFeatureFlags.Has(Feature::kWiFiNetworkInterface) || mFeatureFlags.Has(Feature::kThreadNetworkInterface)); @@ -623,7 +630,20 @@ void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::Connec memcpy(mConnectingNetworkID, req.networkID.data(), mConnectingNetworkIDLen); mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler); mCurrentOperationBreadcrumb = req.breadcrumb; + + // In Non-concurrent mode postpone the final execution of ConnectNetwork until the operational + // network has been fully brought up and kWiFiDeviceAvailable is delivered. + // mConnectingNetworkIDLen and mConnectingNetworkID contains the received SSID +#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION mpWirelessDriver->ConnectNetwork(req.networkID, this); +#endif +} + +void Instance::HandleNonConcurrentConnectNetwork() +{ + ByteSpan nonConcurrentNetworkID = ByteSpan(mConnectingNetworkID, mConnectingNetworkIDLen); + ChipLogProgress(NetworkProvisioning, "HandleNonConcurrentConnectNetwork() SSID=%s", mConnectingNetworkID); + mpWirelessDriver->ConnectNetwork(nonConcurrentNetworkID, this); } void Instance::HandleReorderNetwork(HandlerContext & ctx, const Commands::ReorderNetwork::DecodableType & req) @@ -759,7 +779,13 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen); mLastNetworkingStatusValue.SetNonNull(commissioningError); +#if CONFIG_NETWORK_LAYER_BLE && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + ChipLogProgress(NetworkProvisioning, "Non-concurrent mode, ConnectNetworkResponse will NOT be sent"); + // Do not send the ConnectNetworkResponse if in non-concurrent mode + // Issue #30576 raised to modify CommandHandler to notify it if no response required +#else commandHandle->AddResponse(mPath, response); +#endif if (commissioningError == Status::kSuccess) { CommitSavedBreadcrumb(); @@ -956,6 +982,10 @@ void Instance::OnPlatformEventHandler(const DeviceLayer::ChipDeviceEvent * event { this_->OnFailSafeTimerExpired(); } + else if (event->Type == DeviceLayer::DeviceEventType::kWiFiDeviceAvailable) + { + this_->HandleNonConcurrentConnectNetwork(); + } } void Instance::OnCommissioningComplete() diff --git a/src/app/clusters/network-commissioning/network-commissioning.h b/src/app/clusters/network-commissioning/network-commissioning.h index d11b118a5be854..03fa72f6bfbd26 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.h +++ b/src/app/clusters/network-commissioning/network-commissioning.h @@ -117,6 +117,7 @@ class Instance : public CommandHandlerInterface, void HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveNetwork::DecodableType & req); void HandleConnectNetwork(HandlerContext & ctx, const Commands::ConnectNetwork::DecodableType & req); void HandleReorderNetwork(HandlerContext & ctx, const Commands::ReorderNetwork::DecodableType & req); + void HandleNonConcurrentConnectNetwork(void); void HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryIdentity::DecodableType & req); public: diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp index cfa201c57a3757..c4257fe7e2ef02 100644 --- a/src/app/server/CommissioningWindowManager.cpp +++ b/src/app/server/CommissioningWindowManager.cpp @@ -64,7 +64,8 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv Cleanup(); mServer->GetSecureSessionManager().ExpireAllPASESessions(); // That should have cleared out mPASESession. -#if CONFIG_NETWORK_LAYER_BLE +#if CONFIG_NETWORK_LAYER_BLE && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + // If in NonConcurrentConnection, this will already have been completed mServer->GetBleLayerObject()->CloseAllBleConnections(); #endif } @@ -82,6 +83,13 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv app::DnssdServer::Instance().AdvertiseOperational(); ChipLogProgress(AppServer, "Operational advertising enabled"); } +#if CONFIG_NETWORK_LAYER_BLE + else if (event->Type == DeviceLayer::DeviceEventType::kCloseAllBleConnections) + { + ChipLogProgress(AppServer, "Received kCloseAllBleConnections"); + mServer->GetBleLayerObject()->CloseAllBleConnections(); + } +#endif } void CommissioningWindowManager::Shutdown() diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 397497ab80f206..075c5eeb243b23 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -687,35 +687,32 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio mNeedsDST = false; break; case CommissioningStage::kReadCommissioningInfo2: { - bool shouldReadCommissioningInfo2 = - mParams.GetCheckForMatchingFabric() || (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore); - if (shouldReadCommissioningInfo2) + + if (!report.Is()) { - if (!report.Is()) - { - ChipLogError( - Controller, - "[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG."); - } + ChipLogError( + Controller, + "[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG."); + } - ReadCommissioningInfo2 commissioningInfo = report.Get(); + ReadCommissioningInfo2 commissioningInfo = report.Get(); + mParams.SetSupportsConcurrentConnection(commissioningInfo.supportsConcurrentConnection); - if (mParams.GetCheckForMatchingFabric()) + if (mParams.GetCheckForMatchingFabric()) + { + chip::NodeId nodeId = commissioningInfo.nodeId; + if (nodeId != kUndefinedNodeId) { - chip::NodeId nodeId = commissioningInfo.nodeId; - if (nodeId != kUndefinedNodeId) - { - mParams.SetRemoteNodeId(nodeId); - } + mParams.SetRemoteNodeId(nodeId); } + } - if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore) + if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore) + { + if (commissioningInfo.isIcd) { - if (commissioningInfo.isIcd) - { - mNeedIcdRegistraion = true; - ChipLogDetail(Controller, "AutoCommissioner: Device is ICD"); - } + mNeedIcdRegistraion = true; + ChipLogDetail(Controller, "AutoCommissioner: Device is ICD"); } } break; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 29c6cabb72184e..11c966cb84f207 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1611,6 +1611,27 @@ void OnBasicFailure(void * context, CHIP_ERROR error) commissioner->CommissioningStageComplete(error); } +static void NonConcurrentTimeout(void * context, CHIP_ERROR error) +{ + if (error == CHIP_ERROR_TIMEOUT) + { + ChipLogProgress(Controller, "Non-concurrent mode: Expected NetworkResponse Timeout, do nothing"); + } + else + { + ChipLogProgress(Controller, "Non-concurrent mode: Received failure response %" CHIP_ERROR_FORMAT, error.Format()); + } +} + +static void NonConcurrentNetworkResponse(void * context, + const NetworkCommissioning::Commands::ConnectNetworkResponse::DecodableType & data) +{ + // In Non Concurrent mode the commissioning network should have been shut down and not sent the + // ConnectNetworkResponse. In case it does send it this handles the message + ChipLogError(Controller, "Non-concurrent Mode : Received Unexpected ConnectNetwork response, ignoring. Status=%u", + to_underlying(data.networkingStatus)); +} + void DeviceCommissioner::CleanupCommissioning(DeviceProxy * proxy, NodeId nodeId, const CompletionStatus & completionStatus) { commissioningCompletionStatus = completionStatus; @@ -2111,6 +2132,16 @@ void DeviceCommissioner::ParseCommissioningInfo2() ReadCommissioningInfo2 info; CHIP_ERROR return_err = CHIP_NO_ERROR; + using namespace chip::app::Clusters::GeneralCommissioning::Attributes; + CHIP_ERROR err = + mAttributeCache->Get(kRootEndpointId, info.supportsConcurrentConnection); + if (err != CHIP_NO_ERROR) + { + // May not be present so don't return the error code, non fatal, default concurrent + ChipLogError(Controller, "Failed to read SupportsConcurrentConnection: %" CHIP_ERROR_FORMAT, err.Format()); + info.supportsConcurrentConnection = true; + } + return_err = ParseFabrics(info); if (return_err == CHIP_NO_ERROR) @@ -2206,6 +2237,8 @@ CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info) } else if (err == CHIP_ERROR_KEY_NOT_FOUND) { + // This key is optional so not an error + err = CHIP_NO_ERROR; info.isIcd = false; } else if (err == CHIP_ERROR_IM_STATUS_CODE_RECEIVED) @@ -2440,6 +2473,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio case CommissioningStage::kReadCommissioningInfo: { ChipLogProgress(Controller, "Sending read request for commissioning information"); // NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9 + // See R1.1, 2.11.2 Interaction Model Limits app::AttributePathParams readPaths[9]; // Read all the feature maps for all the networking clusters on any endpoint to determine what is supported readPaths[0] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id, @@ -2471,7 +2505,14 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio case CommissioningStage::kReadCommissioningInfo2: { size_t numberOfAttributes = 0; // This is done in a separate step since we've already used up all the available read paths in the previous read step - app::AttributePathParams readPaths[9]; + // NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9 + // See R1.1, 2.11.2 Interaction Model Limits + app::AttributePathParams readPaths[3]; + + // Mandatory attribute + readPaths[numberOfAttributes++] = + app::AttributePathParams(endpoint, app::Clusters::GeneralCommissioning::Id, + app::Clusters::GeneralCommissioning::Attributes::SupportsConcurrentConnection::Id); // Read the current fabrics if (params.GetCheckForMatchingFabric()) @@ -2486,18 +2527,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio app::AttributePathParams(endpoint, IcdManagement::Id, IcdManagement::Attributes::FeatureMap::Id); } - // Current implementation makes sense when we only have a few attributes to read with conditions. Should revisit this if we - // are adding more attributes here. - - if (numberOfAttributes == 0) - { - // We don't actually want to do this step, so just bypass it - ChipLogProgress(Controller, "kReadCommissioningInfo2 step called without parameter set, skipping"); - CommissioningStageComplete(CHIP_NO_ERROR); - return; - } - - ChipLogProgress(Controller, "Sending request for commissioning information -- Part 2"); SendCommissioningReadRequest(proxy, timeout, readPaths, numberOfAttributes); } break; @@ -2918,7 +2947,31 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio NetworkCommissioning::Commands::ConnectNetwork::Type request; request.networkID = params.GetWiFiCredentials().Value().ssid; request.breadcrumb.Emplace(breadcrumb); - CHIP_ERROR err = SendCommand(proxy, request, OnConnectNetworkResponse, OnBasicFailure, endpoint, timeout); + + CHIP_ERROR err = CHIP_NO_ERROR; + GeneralCommissioning::Attributes::SupportsConcurrentConnection::TypeInfo::Type supportsConcurrentConnection; + supportsConcurrentConnection = params.GetSupportsConcurrentConnection().Value(); + ChipLogProgress(Controller, "SendCommand kWiFiNetworkEnable, supportsConcurrentConnection=%d", + supportsConcurrentConnection); + if (supportsConcurrentConnection) + { + err = SendCommand(proxy, request, OnConnectNetworkResponse, OnBasicFailure, endpoint, timeout); + } + else + { + // Concurrent Connections not allowed. Send the ConnectNetwork command but do not wait for the + // ConnectNetworkResponse on the Commissioning network as it will not be present. Log the expected timeout + // and run what would have been in the onConnectNetworkResponse callback. + err = SendCommand(proxy, request, NonConcurrentNetworkResponse, NonConcurrentTimeout, endpoint, NullOptional); + if (err == CHIP_NO_ERROR) + { + // As there will be no ConnectNetworkResponse, it is an implicit kSuccess so a default report is fine + CommissioningDelegate::CommissioningReport report; + CommissioningStageComplete(CHIP_NO_ERROR, report); + return; + } + } + if (err != CHIP_NO_ERROR) { // We won't get any async callbacks here, so just complete our stage. diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index cbfff76adc6b3b..2919a026701bd0 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -35,7 +35,7 @@ enum CommissioningStage : uint8_t kError, kSecurePairing, ///< Establish a PASE session with the device kReadCommissioningInfo, ///< Query General Commissioning Attributes, Network Features and Time Synchronization Cluster - kReadCommissioningInfo2, ///< Query ICD state, check for matching fabric + kReadCommissioningInfo2, ///< Query SupportsConcurrentConnection, ICD state, check for matching fabric kArmFailsafe, ///< Send ArmFailSafe (0x30:0) command to the device kConfigRegulatory, ///< Send SetRegulatoryConfig (0x30:2) command to the device kConfigureUTCTime, ///< SetUTCTime if the DUT has a time cluster @@ -144,6 +144,10 @@ class CommissioningParameters return mDeviceRegulatoryLocation; } + // Value to determine whether the node supports Concurrent Connections as read from the GeneralCommissioning cluster. + // In the AutoCommissioner, this is automatically set from from the kReadCommissioningInfo2 stage. + Optional GetSupportsConcurrentConnection() const { return mSupportsConcurrentConnection; } + // The country code to be used for the node, if set. Optional GetCountryCode() const { return mCountryCode; } @@ -305,6 +309,12 @@ class CommissioningParameters return *this; } + CommissioningParameters & SetSupportsConcurrentConnection(bool concurrentConnection) + { + mSupportsConcurrentConnection.SetValue(concurrentConnection); + return *this; + } + // The lifetime of the buffer countryCode is pointing to should exceed the // lifetime of CommissioningParameters object. CommissioningParameters & SetCountryCode(CharSpan countryCode) @@ -561,6 +571,7 @@ class CommissioningParameters Optional mRemoteProductId; Optional mDefaultRegulatoryLocation; Optional mLocationCapability; + Optional mSupportsConcurrentConnection; CompletionStatus completionStatus; Credentials::DeviceAttestationDelegate * mDeviceAttestationDelegate = nullptr; // Delegate to handle device attestation failures during commissioning @@ -653,9 +664,10 @@ struct ReadCommissioningInfo struct ReadCommissioningInfo2 { - NodeId nodeId = kUndefinedNodeId; - bool isIcd = false; - bool checkInProtocolSupport = false; + NodeId nodeId = kUndefinedNodeId; + bool isIcd = false; + bool checkInProtocolSupport = false; + bool supportsConcurrentConnection = true; }; struct TimeZoneResponseInfo @@ -688,7 +700,7 @@ class CommissioningDelegate public: virtual ~CommissioningDelegate(){}; /* CommissioningReport is returned after each commissioning step is completed. The reports for each step are: - * kReadCommissioningInfo - ReadCommissioningInfo + * kReadCommissioningInfo: ReadCommissioningInfo * kReadCommissioningInfo2: ReadCommissioningInfo2 * kArmFailsafe: CommissioningErrorInfo if there is an error * kConfigRegulatory: CommissioningErrorInfo if there is an error diff --git a/src/include/platform/CHIPDeviceConfig.h b/src/include/platform/CHIPDeviceConfig.h index c5b2359fdcab89..f6989ac2590942 100644 --- a/src/include/platform/CHIPDeviceConfig.h +++ b/src/include/platform/CHIPDeviceConfig.h @@ -302,6 +302,19 @@ #define CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION 1 #endif // CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION +/** + * The device shall check every WIFI_START_CHECK_TIME_USEC whether Wi-Fi management + * has been fully initialized. If after WIFI_START_CHECK_ATTEMPTS Wi-Fi management + * still hasn't been initialized, the device configuration is reset, and device + * needs to be paired again. + */ +#ifndef WIFI_START_CHECK_TIME_USEC +#define WIFI_START_CHECK_TIME_USEC 100000 // 100ms +#endif +#ifndef WIFI_START_CHECK_ATTEMPTS +#define WIFI_START_CHECK_ATTEMPTS 5 +#endif + /** * CHIP_DEVICE_CONFIG_USER_SELECTED_MODE_TIMEOUT_SEC * diff --git a/src/include/platform/CHIPDeviceEvent.h b/src/include/platform/CHIPDeviceEvent.h index 561cd5f539367b..28e5c22400c0a7 100644 --- a/src/include/platform/CHIPDeviceEvent.h +++ b/src/include/platform/CHIPDeviceEvent.h @@ -168,6 +168,18 @@ enum PublicEventTypes */ kCHIPoBLEConnectionClosed, + /** + * Request BLE connections to be closed. + * This is used in the supportsConcurrentConnection = False case. + */ + kCloseAllBleConnections, + + /** + * When supportsConcurrentConnection = False, the ConnectNetwork command cannot start until + * the BLE device is closed and the WiFi device has been started. + */ + kWiFiDeviceAvailable, + /** * Thread State Change * diff --git a/src/include/platform/DeviceControlServer.h b/src/include/platform/DeviceControlServer.h index ce98110c2154d4..073f6a035e95b0 100644 --- a/src/include/platform/DeviceControlServer.h +++ b/src/include/platform/DeviceControlServer.h @@ -36,7 +36,8 @@ class DeviceControlServer final CHIP_ERROR PostCommissioningCompleteEvent(NodeId peerNodeId, FabricIndex accessingFabricIndex); CHIP_ERROR SetRegulatoryConfig(uint8_t location, const CharSpan & countryCode); CHIP_ERROR PostConnectedToOperationalNetworkEvent(ByteSpan networkID); - + CHIP_ERROR PostCloseAllBLEConnectionsToOperationalNetworkEvent(); + CHIP_ERROR PostWiFiDeviceAvailableNetworkEvent(); static DeviceControlServer & DeviceControlSvr(); private: diff --git a/src/platform/DeviceControlServer.cpp b/src/platform/DeviceControlServer.cpp index da27293ada6008..76008965498028 100644 --- a/src/platform/DeviceControlServer.cpp +++ b/src/platform/DeviceControlServer.cpp @@ -73,5 +73,19 @@ CHIP_ERROR DeviceControlServer::PostConnectedToOperationalNetworkEvent(ByteSpan return PlatformMgr().PostEvent(&event); } +CHIP_ERROR DeviceControlServer::PostCloseAllBLEConnectionsToOperationalNetworkEvent() +{ + ChipDeviceEvent event; + event.Type = DeviceEventType::kCloseAllBleConnections; + return PlatformMgr().PostEvent(&event); +} + +CHIP_ERROR DeviceControlServer::PostWiFiDeviceAvailableNetworkEvent() +{ + ChipDeviceEvent event; + event.Type = DeviceEventType::kWiFiDeviceAvailable; + return PlatformMgr().PostEvent(&event); +} + } // namespace DeviceLayer } // namespace chip diff --git a/src/platform/Linux/BLEManagerImpl.cpp b/src/platform/Linux/BLEManagerImpl.cpp index 196dd480872f84..978dc158124c12 100644 --- a/src/platform/Linux/BLEManagerImpl.cpp +++ b/src/platform/Linux/BLEManagerImpl.cpp @@ -635,6 +635,10 @@ void BLEManagerImpl::DriveBLEState(intptr_t arg) void BLEManagerImpl::NotifyChipConnectionClosed(BLE_CONNECTION_OBJECT conId) { ChipLogProgress(Ble, "Got notification regarding chip connection closure"); +#if CHIP_DEVICE_CONFIG_ENABLE_WPA && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION + // In Non-Concurrent mode start the Wi-Fi, as BLE has been stopped + DeviceLayer::ConnectivityMgrImpl().StartNonConcurrentWiFiManagement(); +#endif } void BLEManagerImpl::InitiateScan(BleScanState scanType) diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp index c93a723628fa81..01d7a9668ff615 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.cpp +++ b/src/platform/Linux/ConnectivityManagerImpl.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -765,6 +766,23 @@ bool ConnectivityManagerImpl::IsWiFiManagementStarted() return ret; } +void ConnectivityManagerImpl::StartNonConcurrentWiFiManagement() +{ + StartWiFiManagement(); + + for (int cnt = 0; cnt < WIFI_START_CHECK_ATTEMPTS; cnt++) + { + if (IsWiFiManagementStarted()) + { + DeviceControlServer::DeviceControlSvr().PostWiFiDeviceAvailableNetworkEvent(); + ChipLogProgress(DeviceLayer, "Non-concurrent mode Wi-Fi Management Started."); + return; + } + usleep(WIFI_START_CHECK_TIME_USEC); + } + ChipLogError(Ble, "Non-concurrent mode Wi-Fi Management taking too long to start."); +} + void ConnectivityManagerImpl::DriveAPState() { CHIP_ERROR err = CHIP_NO_ERROR; diff --git a/src/platform/Linux/ConnectivityManagerImpl.h b/src/platform/Linux/ConnectivityManagerImpl.h index df9e893f8ec5a6..1325c6cd83f21d 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.h +++ b/src/platform/Linux/ConnectivityManagerImpl.h @@ -137,6 +137,7 @@ class ConnectivityManagerImpl final : public ConnectivityManager, void StartWiFiManagement(); bool IsWiFiManagementStarted(); + void StartNonConcurrentWiFiManagement(); int32_t GetDisconnectReason(); CHIP_ERROR GetWiFiBssId(MutableByteSpan & value); CHIP_ERROR GetWiFiSecurityType(app::Clusters::WiFiNetworkDiagnostics::SecurityTypeEnum & securityType);