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/6] [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/6] 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/6] 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/6] 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/6] 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/6] 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 *