From 59e3cc5f6eeed01507477e67fdb32a532557bac6 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Thu, 15 Feb 2024 21:54:29 -0500 Subject: [PATCH] Fix NetworkCommissioning Cluster issues (#32156) * Fix NetworkCommissioning Cluster issues This PR addresses several NetworkCommissioning issues which are all inter-related: - Attributes are never reported on change. - ScanNetworks used to modify attributes that should not be modified - LastNetworkStatus was not set where required - Thread network scanning did not report error on SSID field provided - All ConstraintErrors actually were reported as InvalidCommand - Lacking results of directed scanning did not report NetworkNotFound - Fixes #32024 - Fixes #32022 - Fixes #32021 - Fixes #32019 - Fixes #32018 - Fixes #31870 Testing done: - TC-CNET-4.4 pass on ESP32 and Linux - Commissioning works on ESP32 and Linux - Manual testing of all attribute changes validated with chip-repl - Automated test beyond TC-CNET-4.4 coming as a follow-up * Restyled by clang-format * Fix a Release call missing * Fix one Release too many * Apply review comments * Restyled by clang-format --------- Co-authored-by: Restyled.io --- .../energy-management-app.matter | 1 + .../energy-management-app.zap | 16 + .../network-commissioning.cpp | 201 +++++++++--- .../network-commissioning.h | 12 +- src/app/data-model/Nullable.h | 13 + src/app/tests/BUILD.gn | 1 + src/app/tests/TestNullable.cpp | 303 ++++++++++++++++++ 7 files changed, 492 insertions(+), 55 deletions(-) create mode 100644 src/app/tests/TestNullable.cpp diff --git a/examples/energy-management-app/energy-management-common/energy-management-app.matter b/examples/energy-management-app/energy-management-common/energy-management-app.matter index c56280b2b72f08..1cedefb6c1e43c 100644 --- a/examples/energy-management-app/energy-management-common/energy-management-app.matter +++ b/examples/energy-management-app/energy-management-common/energy-management-app.matter @@ -1770,6 +1770,7 @@ endpoint 0 { ram attribute lastNetworkingStatus; ram attribute lastNetworkID; ram attribute lastConnectErrorValue; + callback attribute supportedWiFiBands; callback attribute generatedCommandList; callback attribute acceptedCommandList; callback attribute eventList; diff --git a/examples/energy-management-app/energy-management-common/energy-management-app.zap b/examples/energy-management-app/energy-management-common/energy-management-app.zap index d795b4fd482448..88514f105c37f1 100644 --- a/examples/energy-management-app/energy-management-common/energy-management-app.zap +++ b/examples/energy-management-app/energy-management-common/energy-management-app.zap @@ -1452,6 +1452,22 @@ "maxInterval": 65534, "reportableChange": 0 }, + { + "name": "SupportedWiFiBands", + "code": 8, + "mfgCode": null, + "side": "server", + "type": "array", + "included": 1, + "storageOption": "External", + "singleton": 0, + "bounded": 0, + "defaultValue": null, + "reportable": 1, + "minInterval": 1, + "maxInterval": 65534, + "reportableChange": 0 + }, { "name": "GeneratedCommandList", "code": 65528, diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index ffaff6bd49253a..cb847a13867d5d 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -89,6 +90,19 @@ static void EnumerateAndRelease(Iterator * iterator, Func func) } } +template +static size_t CountAndRelease(Iterator * iterator) +{ + size_t count = 0; + if (iterator != nullptr) + { + count = iterator->Count(); + iterator->Release(); + } + + return count; +} + BitFlags WiFiFeatures(WiFiDriver * driver) { BitFlags features = Feature::kWiFiNetworkInterface; @@ -102,21 +116,22 @@ BitFlags WiFiFeatures(WiFiDriver * driver) Instance::Instance(EndpointId aEndpointId, WiFiDriver * apDelegate) : CommandHandlerInterface(Optional(aEndpointId), Id), AttributeAccessInterface(Optional(aEndpointId), Id), - mFeatureFlags(WiFiFeatures(apDelegate)), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate) + mEndpointId(aEndpointId), mFeatureFlags(WiFiFeatures(apDelegate)), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate) { mpDriver.Set(apDelegate); } Instance::Instance(EndpointId aEndpointId, ThreadDriver * apDelegate) : CommandHandlerInterface(Optional(aEndpointId), Id), AttributeAccessInterface(Optional(aEndpointId), Id), - mFeatureFlags(Feature::kThreadNetworkInterface), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate) + mEndpointId(aEndpointId), mFeatureFlags(Feature::kThreadNetworkInterface), mpWirelessDriver(apDelegate), + mpBaseDriver(apDelegate) { mpDriver.Set(apDelegate); } Instance::Instance(EndpointId aEndpointId, EthernetDriver * apDelegate) : CommandHandlerInterface(Optional(aEndpointId), Id), AttributeAccessInterface(Optional(aEndpointId), Id), - mFeatureFlags(Feature::kEthernetNetworkInterface), mpWirelessDriver(nullptr), mpBaseDriver(apDelegate) + mEndpointId(aEndpointId), mFeatureFlags(Feature::kEthernetNetworkInterface), mpWirelessDriver(nullptr), mpBaseDriver(apDelegate) {} CHIP_ERROR Instance::Init() @@ -156,6 +171,42 @@ void Instance::SendNonConcurrentConnectNetworkResponse() } #endif // CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION +void Instance::SetLastNetworkingStatusValue(Attributes::LastNetworkingStatus::TypeInfo::Type networkingStatusValue) +{ + if (mLastNetworkingStatusValue.Update(networkingStatusValue)) + { + MatterReportingAttributeChangeCallback(mEndpointId, Clusters::NetworkCommissioning::Id, + Attributes::LastNetworkingStatus::TypeInfo::GetAttributeId()); + } +} + +void Instance::SetLastConnectErrorValue(Attributes::LastConnectErrorValue::TypeInfo::Type connectErrorValue) +{ + if (mLastConnectErrorValue.Update(connectErrorValue)) + { + MatterReportingAttributeChangeCallback(mEndpointId, Clusters::NetworkCommissioning::Id, + Attributes::LastConnectErrorValue::TypeInfo::GetAttributeId()); + } +} + +void Instance::SetLastNetworkId(ByteSpan lastNetworkId) +{ + ByteSpan prevLastNetworkId{ mLastNetworkID, mLastNetworkIDLen }; + VerifyOrReturn(lastNetworkId.size() <= kMaxNetworkIDLen); + VerifyOrReturn(!prevLastNetworkId.data_equal(lastNetworkId)); + + memcpy(mLastNetworkID, lastNetworkId.data(), lastNetworkId.size()); + mLastNetworkIDLen = static_cast(lastNetworkId.size()); + MatterReportingAttributeChangeCallback(mEndpointId, Clusters::NetworkCommissioning::Id, + Attributes::LastNetworkID::TypeInfo::GetAttributeId()); +} + +void Instance::ReportNetworksListChanged() const +{ + MatterReportingAttributeChangeCallback(mEndpointId, Clusters::NetworkCommissioning::Id, + Attributes::Networks::TypeInfo::GetAttributeId()); +} + void Instance::InvokeCommand(HandlerContext & ctxt) { if (mAsyncCommandHandle.Get() != nullptr) @@ -379,34 +430,34 @@ CHIP_ERROR Instance::Write(const ConcreteDataAttributePath & aPath, AttributeVal void Instance::OnNetworkingStatusChange(Status aCommissioningError, Optional aNetworkId, Optional aConnectStatus) { - if (aNetworkId.HasValue() && aNetworkId.Value().size() > kMaxNetworkIDLen) - { - ChipLogError(DeviceLayer, "Invalid network id received when calling OnNetworkingStatusChange"); - return; - } - mLastNetworkingStatusValue.SetNonNull(aCommissioningError); if (aNetworkId.HasValue()) { - memcpy(mLastNetworkID, aNetworkId.Value().data(), aNetworkId.Value().size()); - mLastNetworkIDLen = static_cast(aNetworkId.Value().size()); - } - else - { - mLastNetworkIDLen = 0; + if (aNetworkId.Value().size() > kMaxNetworkIDLen) + { + ChipLogError(DeviceLayer, "Overly large network ID received when calling OnNetworkingStatusChange"); + } + else + { + SetLastNetworkId(aNetworkId.Value()); + } } + + SetLastNetworkingStatusValue(MakeNullable(aCommissioningError)); if (aConnectStatus.HasValue()) { - mLastConnectErrorValue.SetNonNull(aConnectStatus.Value()); + SetLastConnectErrorValue(MakeNullable(aConnectStatus.Value())); } else { - mLastConnectErrorValue.SetNull(); + SetLastConnectErrorValue(NullNullable); } } void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetworks::DecodableType & req) { MATTER_TRACE_SCOPE("HandleScanNetwork", "NetworkCommissioning"); + + mScanningWasDirected = false; if (mFeatureFlags.Has(Feature::kWiFiNetworkInterface)) { ByteSpan ssid; @@ -426,9 +477,13 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw } if (ssid.size() > DeviceLayer::Internal::kMaxWiFiSSIDLength) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); + // This should not happen, it means it's a broken driver. + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); + SetLastNetworkingStatusValue(MakeNullable(Status::kUnknownError)); return; } + + mScanningWasDirected = !ssid.empty(); mCurrentOperationBreadcrumb = req.breadcrumb; mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler); ctx.mCommandHandler.FlushAcksRightAwayOnSlowCommand(); @@ -436,6 +491,13 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw } else if (mFeatureFlags.Has(Feature::kThreadNetworkInterface)) { + // Not allowed to populate SSID for Thread. + if (!req.ssid.HasValue()) + { + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); + return; + } + mCurrentOperationBreadcrumb = req.breadcrumb; mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler); ctx.mCommandHandler.FlushAcksRightAwayOnSlowCommand(); @@ -483,7 +545,7 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands if (req.ssid.empty() || req.ssid.size() > DeviceLayer::Internal::kMaxWiFiSSIDLength) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand, "ssid"); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "ssid"); return; } @@ -497,7 +559,7 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands return; } #endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); return; } @@ -524,7 +586,7 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands { if (!isxdigit(req.credentials.data()[d])) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); return; } } @@ -532,7 +594,7 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands else { // Invalid length - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); return; } @@ -547,6 +609,7 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands if (response.networkingStatus == Status::kSuccess) { UpdateBreadcrumb(req.breadcrumb); + ReportNetworksListChanged(); } } @@ -557,7 +620,7 @@ void Instance::HandleAddOrUpdateWiFiNetworkWithPDC(HandlerContext & ctx, // Credentials must be empty when configuring for PDC, it's only present to keep the command shape compatible. if (!req.credentials.empty()) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand, "credentials"); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "credentials"); return; } @@ -565,20 +628,20 @@ void Instance::HandleAddOrUpdateWiFiNetworkWithPDC(HandlerContext & ctx, if (networkIdentity.size() > kMaxCHIPCompactNetworkIdentityLength || Credentials::ValidateChipNetworkIdentity(networkIdentity) != CHIP_NO_ERROR) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand, "networkIdentity"); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "networkIdentity"); return; } if (req.clientIdentifier.HasValue() && req.clientIdentifier.Value().size() != CertificateKeyId::size()) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand, "clientIdentifier"); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "clientIdentifier"); return; } bool provePossession = req.possessionNonce.HasValue(); if (provePossession && req.possessionNonce.Value().size() != kPossessionNonceSize) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand, "possessionNonce"); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "possessionNonce"); return; } @@ -640,6 +703,7 @@ void Instance::HandleAddOrUpdateWiFiNetworkWithPDC(HandlerContext & ctx, response.possessionSignature.SetValue(possessionSignature.Value().Span()); } + ReportNetworksListChanged(); UpdateBreadcrumb(req.breadcrumb); } @@ -671,6 +735,7 @@ void Instance::HandleAddOrUpdateThreadNetwork(HandlerContext & ctx, const Comman ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response); if (response.networkingStatus == Status::kSuccess) { + ReportNetworksListChanged(); UpdateBreadcrumb(req.breadcrumb); } } @@ -704,7 +769,16 @@ void Instance::HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveN ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response); if (response.networkingStatus == Status::kSuccess) { + ReportNetworksListChanged(); UpdateBreadcrumb(req.breadcrumb); + + // If no networks are left, clear-out errors; + if (CountAndRelease(mpBaseDriver->GetNetworks()) == 0) + { + SetLastNetworkId(ByteSpan{}); + SetLastConnectErrorValue(NullNullable); + SetLastNetworkingStatusValue(NullNullable); + } } } @@ -713,7 +787,7 @@ void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::Connec MATTER_TRACE_SCOPE("HandleConnectNetwork", "NetworkCommissioning"); if (req.networkID.size() > kMaxNetworkIDLen) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidValue); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); return; } @@ -754,6 +828,7 @@ void Instance::HandleReorderNetwork(HandlerContext & ctx, const Commands::Reorde ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response); if (response.networkingStatus == Status::kSuccess) { + ReportNetworksListChanged(); UpdateBreadcrumb(req.breadcrumb); } } @@ -765,7 +840,7 @@ void Instance::HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryId if (req.keyIdentifier.size() != CertificateKeyId::size()) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand, "keyIdentifier"); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "keyIdentifier"); return; } CertificateKeyId keyIdentifier(req.keyIdentifier.data()); @@ -773,7 +848,7 @@ void Instance::HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryId bool provePossession = req.possessionNonce.HasValue(); if (provePossession && req.possessionNonce.Value().size() != kPossessionNonceSize) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand, "possessionNonce"); + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "possessionNonce"); return; } @@ -870,19 +945,23 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i { DeviceLayer::DeviceControlServer::DeviceControlSvr().PostConnectedToOperationalNetworkEvent( ByteSpan(mLastNetworkID, mLastNetworkIDLen)); - mLastConnectErrorValue.SetNull(); + SetLastConnectErrorValue(NullNullable); } else { response.errorValue.SetNonNull(interfaceStatus); - mLastConnectErrorValue.SetNonNull(interfaceStatus); + SetLastConnectErrorValue(MakeNullable(interfaceStatus)); } - mLastNetworkIDLen = mConnectingNetworkIDLen; - memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen); - mLastNetworkingStatusValue.SetNonNull(commissioningError); + SetLastNetworkId(ByteSpan{ mConnectingNetworkID, mConnectingNetworkIDLen }); + SetLastNetworkingStatusValue(MakeNullable(commissioningError)); -#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION +#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 + // TODO(#30576) raised to modify CommandHandler to notify it if no response required + // -----> Is this required here: commandHandle->FinishCommand(); +#else commandHandle->AddResponse(mPath, response); #endif // CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION @@ -904,9 +983,7 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI return; } - mLastNetworkingStatusValue.SetNonNull(status); - mLastConnectErrorValue.SetNull(); - mLastNetworkIDLen = 0; + SetLastNetworkingStatusValue(MakeNullable(status)); TLV::TLVWriter * writer; TLV::TLVType listContainerType; @@ -931,7 +1008,7 @@ void Instance::OnFinished(Status status, CharSpan debugText, ThreadScanResponseI TLV::TLVType::kTLVType_Array, listContainerType)); // If no network was found, we encode an empty list, don't call a zero-sized alloc. - if (networks->Count() > 0) + if ((status == Status::kSuccess) && (networks->Count() > 0)) { VerifyOrExit(scanResponseArray.Alloc(chip::min(networks->Count(), kMaxNetworksInScanResponse)), err = CHIP_ERROR_NO_MEMORY); for (; networks != nullptr && networks->Next(scanResponse);) @@ -1020,9 +1097,14 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte return; } - mLastNetworkingStatusValue.SetNonNull(status); - mLastConnectErrorValue.SetNull(); - mLastNetworkIDLen = 0; + // If drivers are failing to respond NetworkNotFound on empty results, force it for them. + bool resultsMissing = !networks || (networks->Count() == 0); + if ((status == Status::kSuccess) && mScanningWasDirected && resultsMissing) + { + status = Status::kNetworkNotFound; + } + + SetLastNetworkingStatusValue(MakeNullable(status)); TLV::TLVWriter * writer; TLV::TLVType listContainerType; @@ -1044,21 +1126,30 @@ void Instance::OnFinished(Status status, CharSpan debugText, WiFiScanResponseIte SuccessOrExit(err = writer->StartContainer(TLV::ContextTag(Commands::ScanNetworksResponse::Fields::kWiFiScanResults), TLV::TLVType::kTLVType_Array, listContainerType)); - for (; networks != nullptr && networks->Next(scanResponse) && networksEncoded < kMaxNetworksInScanResponse; networksEncoded++) + // Only encode results on success, to avoid stale contents on partial failure. + if ((status == Status::kSuccess) && (networks != nullptr)) { - Structs::WiFiInterfaceScanResultStruct::Type result; - result.security = scanResponse.security; - result.ssid = ByteSpan(scanResponse.ssid, scanResponse.ssidLen); - result.bssid = ByteSpan(scanResponse.bssid, sizeof(scanResponse.bssid)); - result.channel = scanResponse.channel; - result.wiFiBand = scanResponse.wiFiBand; - result.rssi = scanResponse.rssi; - SuccessOrExit(err = DataModel::Encode(*writer, TLV::AnonymousTag(), result)); + while (networks->Next(scanResponse)) + { + Structs::WiFiInterfaceScanResultStruct::Type result; + result.security = scanResponse.security; + result.ssid = ByteSpan(scanResponse.ssid, scanResponse.ssidLen); + result.bssid = ByteSpan(scanResponse.bssid, sizeof(scanResponse.bssid)); + result.channel = scanResponse.channel; + result.wiFiBand = scanResponse.wiFiBand; + result.rssi = scanResponse.rssi; + SuccessOrExit(err = DataModel::Encode(*writer, TLV::AnonymousTag(), result)); + + ++networksEncoded; + if (networksEncoded >= kMaxNetworksInScanResponse) + { + break; + } + } } SuccessOrExit(err = writer->EndContainer(listContainerType)); SuccessOrExit(err = commandHandle->FinishCommand()); - exit: if (err != CHIP_NO_ERROR) { @@ -1110,6 +1201,12 @@ void Instance::OnFailSafeTimerExpired() ChipLogDetail(Zcl, "Failsafe timeout, tell platform driver to revert network credentials."); mpWirelessDriver->RevertConfiguration(); mAsyncCommandHandle.Release(); + + // Reset state on failsafe expiry. + ReportNetworksListChanged(); + SetLastNetworkId(ByteSpan{}); + SetLastConnectErrorValue(NullNullable); + SetLastNetworkingStatusValue(NullNullable); } CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) diff --git a/src/app/clusters/network-commissioning/network-commissioning.h b/src/app/clusters/network-commissioning/network-commissioning.h index a286b7c79c4f3b..cd2d5909c566d1 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.h +++ b/src/app/clusters/network-commissioning/network-commissioning.h @@ -77,10 +77,11 @@ class Instance : public CommandHandlerInterface, static void OnPlatformEventHandler(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg); void OnCommissioningComplete(); void OnFailSafeTimerExpired(); - #if !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION void SendNonConcurrentConnectNetworkResponse(); #endif + + EndpointId mEndpointId = kInvalidEndpointId; const BitFlags mFeatureFlags; DeviceLayer::NetworkCommissioning::Internal::WirelessDriver * const mpWirelessDriver; @@ -96,14 +97,19 @@ class Instance : public CommandHandlerInterface, // Setting these values don't have to care about parallel requests, since we will reject other requests when there is another // request ongoing. // These values can be updated via OnNetworkingStatusChange callback, ScanCallback::OnFinished and ConnectCallback::OnResult. - DataModel::Nullable mLastNetworkingStatusValue; + Attributes::LastNetworkingStatus::TypeInfo::Type mLastNetworkingStatusValue; Attributes::LastConnectErrorValue::TypeInfo::Type mLastConnectErrorValue; uint8_t mConnectingNetworkID[DeviceLayer::NetworkCommissioning::kMaxNetworkIDLen]; uint8_t mConnectingNetworkIDLen = 0; uint8_t mLastNetworkID[DeviceLayer::NetworkCommissioning::kMaxNetworkIDLen]; uint8_t mLastNetworkIDLen = 0; - Optional mCurrentOperationBreadcrumb; + bool mScanningWasDirected = false; + + void SetLastNetworkingStatusValue(Attributes::LastNetworkingStatus::TypeInfo::Type networkingStatusValue); + void SetLastConnectErrorValue(Attributes::LastConnectErrorValue::TypeInfo::Type connectErrorValue); + void SetLastNetworkId(ByteSpan lastNetworkId); + void ReportNetworksListChanged() const; // Commits the breadcrumb value saved in mCurrentOperationBreadcrumb to the breadcrumb attribute in GeneralCommissioning // cluster. Will set mCurrentOperationBreadcrumb to NullOptional. diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h index d926c5465dacf1..cea3dea5b5361b 100644 --- a/src/app/data-model/Nullable.h +++ b/src/app/data-model/Nullable.h @@ -80,6 +80,19 @@ struct Nullable : protected Optional return true; } + // Set the nullable to the `other` nullable, returning true if something actually changed. + // This can be used to determine if changes occurred on assignment, so that reporting can be triggered + // only on actual changes. + constexpr bool Update(const Nullable & other) + { + bool changed = *this != other; + if (changed) + { + *this = other; + } + return changed; + } + // The only fabric-scoped objects in the spec are commands, events and structs inside lists, and none of those can be nullable. static constexpr bool kIsFabricScoped = false; diff --git a/src/app/tests/BUILD.gn b/src/app/tests/BUILD.gn index d8f80e20162fc3..4dad96164804c0 100644 --- a/src/app/tests/BUILD.gn +++ b/src/app/tests/BUILD.gn @@ -143,6 +143,7 @@ chip_test_suite_using_nltest("tests") { "TestFabricScopedEventLogging.cpp", "TestInteractionModelEngine.cpp", "TestMessageDef.cpp", + "TestNullable.cpp", "TestNumericAttributeTraits.cpp", "TestOperationalStateClusterObjects.cpp", "TestPendingNotificationMap.cpp", diff --git a/src/app/tests/TestNullable.cpp b/src/app/tests/TestNullable.cpp new file mode 100644 index 00000000000000..c1290c1a0cef66 --- /dev/null +++ b/src/app/tests/TestNullable.cpp @@ -0,0 +1,303 @@ +/* + * + * Copyright (c) 2024 Project CHIP Authors + * All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include +#include + +#include +#include +#include +#include + +using namespace chip; +using namespace chip::app::DataModel; + +namespace { + +// Counts calls to constructor and destructor, to determine if the right +// semantics applied in cases where destruction is expected. +struct CtorDtorCounter +{ + CtorDtorCounter(int i) : m(i) { ++created; } + ~CtorDtorCounter() { ++destroyed; } + + CtorDtorCounter(const CtorDtorCounter & o) : m(o.m) { ++created; } + CtorDtorCounter & operator=(const CtorDtorCounter &) = default; + + CtorDtorCounter(CtorDtorCounter && o) : m(o.m) { ++created; } + CtorDtorCounter & operator=(CtorDtorCounter &&) = default; + + bool operator==(const CtorDtorCounter & o) const { return m == o.m; } + + int m; + + static void ResetCounter() + { + created = 0; + destroyed = 0; + } + + static int created; + static int destroyed; +}; + +struct MovableCtorDtorCounter : public CtorDtorCounter +{ +public: + MovableCtorDtorCounter(int i) : CtorDtorCounter(i) {} + + MovableCtorDtorCounter(const MovableCtorDtorCounter & o) = delete; + MovableCtorDtorCounter & operator=(const MovableCtorDtorCounter &) = delete; + + MovableCtorDtorCounter(MovableCtorDtorCounter && o) = default; + MovableCtorDtorCounter & operator=(MovableCtorDtorCounter &&) = default; +}; + +int CtorDtorCounter::created = 0; +int CtorDtorCounter::destroyed = 0; + +} // namespace + +static void TestBasic(nlTestSuite * inSuite, void * inContext) +{ + // Set up our test CtorDtorCounter objects, which will mess with counts, before we reset the + // counts. + CtorDtorCounter c100(100), c101(101), c102(102); + + CtorDtorCounter::ResetCounter(); + + { + auto testNullable = MakeNullable(100); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 1 && CtorDtorCounter::destroyed == 0); + NL_TEST_ASSERT(inSuite, !testNullable.IsNull() && testNullable.Value().m == 100); + NL_TEST_ASSERT(inSuite, testNullable == c100); + NL_TEST_ASSERT(inSuite, testNullable != c101); + NL_TEST_ASSERT(inSuite, testNullable != c102); + + testNullable.SetNull(); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 1 && CtorDtorCounter::destroyed == 1); + NL_TEST_ASSERT(inSuite, !!testNullable.IsNull()); + NL_TEST_ASSERT(inSuite, testNullable != c100); + NL_TEST_ASSERT(inSuite, testNullable != c101); + NL_TEST_ASSERT(inSuite, testNullable != c102); + + testNullable.SetNonNull(CtorDtorCounter(101)); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 3 && CtorDtorCounter::destroyed == 2); + NL_TEST_ASSERT(inSuite, !testNullable.IsNull() && testNullable.Value().m == 101); + NL_TEST_ASSERT(inSuite, testNullable != c100); + NL_TEST_ASSERT(inSuite, testNullable == c101); + NL_TEST_ASSERT(inSuite, testNullable != c102); + + testNullable.SetNonNull(102); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 4 && CtorDtorCounter::destroyed == 3); + NL_TEST_ASSERT(inSuite, !testNullable.IsNull() && testNullable.Value().m == 102); + NL_TEST_ASSERT(inSuite, testNullable != c100); + NL_TEST_ASSERT(inSuite, testNullable != c101); + NL_TEST_ASSERT(inSuite, testNullable == c102); + } + + // Our test CtorDtorCounter objects are still in scope here. + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 4 && CtorDtorCounter::destroyed == 4); +} + +static void TestMake(nlTestSuite * inSuite, void * inContext) +{ + CtorDtorCounter::ResetCounter(); + + { + auto testNullable = MakeNullable(200); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 1 && CtorDtorCounter::destroyed == 0); + NL_TEST_ASSERT(inSuite, !testNullable.IsNull() && testNullable.Value().m == 200); + } + + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 1 && CtorDtorCounter::destroyed == 1); +} + +static void TestCopy(nlTestSuite * inSuite, void * inContext) +{ + CtorDtorCounter::ResetCounter(); + + { + auto testSrc = MakeNullable(300); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 1 && CtorDtorCounter::destroyed == 0); + NL_TEST_ASSERT(inSuite, !testSrc.IsNull() && testSrc.Value().m == 300); + + { + Nullable testDst(testSrc); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 0); + NL_TEST_ASSERT(inSuite, !testDst.IsNull() && testDst.Value().m == 300); + } + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 1); + + { + Nullable testDst; + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 1); + NL_TEST_ASSERT(inSuite, !!testDst.IsNull()); + + testDst = testSrc; + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 3 && CtorDtorCounter::destroyed == 1); + NL_TEST_ASSERT(inSuite, !testDst.IsNull() && testDst.Value().m == 300); + } + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 3 && CtorDtorCounter::destroyed == 2); + } + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 3 && CtorDtorCounter::destroyed == 3); +} + +static void TestMove(nlTestSuite * inSuite, void * inContext) +{ + CtorDtorCounter::ResetCounter(); + + { + auto testSrc = MakeNullable(400); + Nullable testDst(std::move(testSrc)); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 1); + NL_TEST_ASSERT(inSuite, !testDst.IsNull() && testDst.Value().m == 400); + } + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 2); + + { + Nullable testDst; + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 2 && CtorDtorCounter::destroyed == 2); + NL_TEST_ASSERT(inSuite, !!testDst.IsNull()); + + auto testSrc = MakeNullable(401); + testDst = std::move(testSrc); + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 4 && CtorDtorCounter::destroyed == 3); + NL_TEST_ASSERT(inSuite, !testDst.IsNull() && testDst.Value().m == 401); + } + NL_TEST_ASSERT(inSuite, CtorDtorCounter::created == 4 && CtorDtorCounter::destroyed == 4); +} + +static void TestUpdate(nlTestSuite * inSuite, void * inContext) +{ + using SmallArray = std::array; + // Arrays + { + auto nullable1 = MakeNullable({ 1, 2, 3 }); + auto nullable2 = MakeNullable({ 1, 2, 3 }); + + NL_TEST_ASSERT(inSuite, !nullable1.IsNull()); + NL_TEST_ASSERT(inSuite, !nullable2.IsNull()); + NL_TEST_ASSERT(inSuite, nullable1 == nullable2); + + // No-op on change to same. + NL_TEST_ASSERT(inSuite, nullable1.Update(nullable2) == false); + NL_TEST_ASSERT(inSuite, nullable1 == nullable2); + + nullable1.Value()[0] = 100; + + NL_TEST_ASSERT(inSuite, nullable1 != nullable2); + NL_TEST_ASSERT(inSuite, nullable2.Update(nullable1) == true); + NL_TEST_ASSERT(inSuite, nullable1 == nullable2); + } + + // Structs + { + struct SomeObject + { + uint8_t a; + uint8_t b; + + bool operator==(const SomeObject & other) const { return (a == other.a) && (b == other.b); } + }; + + auto nullable1 = MakeNullable({ 1, 2 }); + auto nullable2 = MakeNullable({ 1, 2 }); + + NL_TEST_ASSERT(inSuite, !nullable1.IsNull()); + NL_TEST_ASSERT(inSuite, !nullable2.IsNull()); + NL_TEST_ASSERT(inSuite, nullable1 == nullable2); + + // No-op on change to same. + NL_TEST_ASSERT(inSuite, nullable1.Update(nullable2) == false); + NL_TEST_ASSERT(inSuite, nullable1 == nullable2); + + nullable1.Value().a = 100; + + NL_TEST_ASSERT(inSuite, nullable1 != nullable2); + NL_TEST_ASSERT(inSuite, nullable2.Update(nullable1) == true); + NL_TEST_ASSERT(inSuite, nullable1 == nullable2); + } + + // Scalar cases + { + auto nullable1 = MakeNullable(static_cast(1)); + + NL_TEST_ASSERT(inSuite, !nullable1.IsNull()); + + // Non-null to non-null same value + NL_TEST_ASSERT(inSuite, nullable1.Update(nullable1) == false); + NL_TEST_ASSERT(inSuite, !nullable1.IsNull()); + + // Non-null to null + NL_TEST_ASSERT(inSuite, nullable1.Update(NullNullable) == true); + NL_TEST_ASSERT(inSuite, nullable1.IsNull()); + + // Null to null + NL_TEST_ASSERT(inSuite, nullable1.Update(NullNullable) == false); + NL_TEST_ASSERT(inSuite, nullable1.IsNull()); + + // Null to non-null + NL_TEST_ASSERT(inSuite, nullable1.Update(MakeNullable(static_cast(1))) == true); + NL_TEST_ASSERT(inSuite, !nullable1.IsNull()); + NL_TEST_ASSERT(inSuite, nullable1.Value() == 1); + + // Non-null to non-null different value + NL_TEST_ASSERT(inSuite, nullable1.Update(MakeNullable(static_cast(2))) == true); + NL_TEST_ASSERT(inSuite, !nullable1.IsNull()); + NL_TEST_ASSERT(inSuite, nullable1.Value() == 2); + + // Non-null to extent of range --> changes to "invalid" value in range. + NL_TEST_ASSERT(inSuite, nullable1.Update(MakeNullable(static_cast(255))) == true); + NL_TEST_ASSERT(inSuite, !nullable1.IsNull()); + NL_TEST_ASSERT(inSuite, nullable1.Value() == 255); + } +} + +// clang-format off +static const nlTest sTests[] = +{ + NL_TEST_DEF("NullableBasic", TestBasic), + NL_TEST_DEF("NullableMake", TestMake), + NL_TEST_DEF("NullableCopy", TestCopy), + NL_TEST_DEF("NullableMove", TestMove), + NL_TEST_DEF("Nullable Update operation", TestUpdate), + NL_TEST_SENTINEL() +}; +// clang-format on + +int TestNullable() +{ + // clang-format off + nlTestSuite theSuite = + { + "Test for Nullable abstraction", + &sTests[0], + nullptr, + nullptr + }; + // clang-format on + + nlTestRunner(&theSuite, nullptr); + + return (nlTestRunnerStats(&theSuite)); +} + +CHIP_REGISTER_TEST_SUITE(TestNullable)