From 17dcef69a5f89698e4504b79a6d83533ce9d151e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:27:32 -0400 Subject: [PATCH 01/26] A first pass to implement better invoke logic decoupling --- src/app/CommandHandlerImpl.cpp | 82 ++++------------- src/app/CommandHandlerImpl.h | 27 ++++-- src/app/CommandResponseSender.cpp | 6 +- src/app/CommandResponseSender.h | 2 +- src/app/InteractionModelEngine.cpp | 93 +++++++++++++++++++- src/app/InteractionModelEngine.h | 7 +- src/app/data-model-provider/OperationTypes.h | 2 + 7 files changed, 140 insertions(+), 79 deletions(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index 366199a3b9f1d4..a38e5752b80d6a 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -15,6 +15,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +#include "app/data-model-provider/OperationTypes.h" #include #include @@ -390,55 +391,17 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); { - Status commandExists = mpCallback->CommandExists(concretePath); - if (commandExists != Status::Success) - { - ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", - ChipLogValueMEI(concretePath.mCommandId), ChipLogValueMEI(concretePath.mClusterId), - concretePath.mEndpointId); - return FallibleAddStatus(concretePath, commandExists) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - } + DataModel::InvokeRequest request; - { - Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); - Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, - .endpoint = concretePath.mEndpointId, - .requestType = Access::RequestType::kCommandInvokeRequest, - .entityId = concretePath.mCommandId }; - Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); - err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); - if (err != CHIP_NO_ERROR) - { - if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL)) - { - return FallibleAddStatus(concretePath, Status::Failure) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - // TODO: when wildcard invokes are supported, handle them to discard rather than fail with status - Status status = err == CHIP_ERROR_ACCESS_DENIED ? Status::UnsupportedAccess : Status::AccessRestricted; - return FallibleAddStatus(concretePath, status) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - } + request.path = concretePath; + request.subjectDescriptor = GetSubjectDescriptor(); + request.accessingFabricIndex = GetAccessingFabricIndex(); + request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); - if (CommandNeedsTimedInvoke(concretePath.mClusterId, concretePath.mCommandId) && !IsTimedInvoke()) - { - // TODO: when wildcard invokes are supported, discard a - // wildcard-expanded path instead of returning a status. - return FallibleAddStatus(concretePath, Status::NeedsTimedInteraction) != CHIP_NO_ERROR ? Status::Failure : Status::Success; - } - - if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId)) - { - // SPEC: Else if the command in the path is fabric-scoped and there is no accessing fabric, - // a CommandStatusIB SHALL be generated with the UNSUPPORTED_ACCESS Status Code. - - // Fabric-scoped commands are not allowed before a specific accessing fabric is available. - // This is mostly just during a PASE session before AddNOC. - if (GetAccessingFabricIndex() == kUndefinedFabricIndex) + Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); + if (preCheckStatus != Status::Success) { - // TODO: when wildcard invokes are supported, discard a - // wildcard-expanded path instead of returning a status. - return FallibleAddStatus(concretePath, Status::UnsupportedAccess) != CHIP_NO_ERROR ? Status::Failure : Status::Success; + return FallibleAddStatus(concretePath, preCheckStatus) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } @@ -542,30 +505,21 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo const ConcreteCommandPath concretePath(mapping.endpoint_id, clusterId, commandId); - if (mpCallback->CommandExists(concretePath) != Status::Success) { - ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", - ChipLogValueMEI(commandId), ChipLogValueMEI(clusterId), mapping.endpoint_id); + DataModel::InvokeRequest request; - continue; - } + request.path = concretePath; + request.subjectDescriptor = GetSubjectDescriptor(); + request.accessingFabricIndex = GetAccessingFabricIndex(); + request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); - { - Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); - Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, - .endpoint = concretePath.mEndpointId, - .requestType = Access::RequestType::kCommandInvokeRequest, - .entityId = concretePath.mCommandId }; - Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); - err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); - if (err != CHIP_NO_ERROR) + Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); + if (preCheckStatus != Status::Success) { - // NOTE: an expected error is CHIP_ERROR_ACCESS_DENIED, but there could be other unexpected errors; - // therefore, keep processing subsequent commands, and if any errors continue, those subsequent - // commands will likewise fail. - continue; + return FallibleAddStatus(concretePath, preCheckStatus) != CHIP_NO_ERROR ? Status::Failure : Status::Success; } } + if ((err = DataModelCallbacks::GetInstance()->PreCommandReceived(concretePath, GetSubjectDescriptor())) == CHIP_NO_ERROR) { TLV::TLVReader dataReader(commandDataReader); diff --git a/src/app/CommandHandlerImpl.h b/src/app/CommandHandlerImpl.h index ddd499658e15d9..2603b96f223c7e 100644 --- a/src/app/CommandHandlerImpl.h +++ b/src/app/CommandHandlerImpl.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -50,21 +51,29 @@ class CommandHandlerImpl : public CommandHandler */ virtual void OnDone(CommandHandlerImpl & apCommandObj) = 0; + /** + * Perform pre-validation that the command dispatch can be performed. In particular: + * - check command existence/validity + * - validate ACL + * - validate timed-invoke and fabric-scoped requirements + * + * Returns Status::Success if the command can be dispatched, otherwise it will + * return the status to be forwarded to the client on failure. + * + * Possible error return codes: + * - UnsupportedEndpoint/UnsupportedCluster/UnsupportedCommand if the command path is invalid + * - NeedsTimedInteraction + * - UnsupportedAccess (ACL failure or fabric scoped without a valid fabric index) + * - AccessRestricted + */ + virtual Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) = 0; + /* * Upon processing of a CommandDataIB, this method is invoked to dispatch the command * to the right server-side handler provided by the application. */ virtual void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) = 0; - - /* - * Check to see if a command implementation exists for a specific - * concrete command path. If it does, Success will be returned. If - * not, one of UnsupportedEndpoint, UnsupportedCluster, or - * UnsupportedCommand will be returned, depending on how the command - * fails to exist. - */ - virtual Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) = 0; }; struct InvokeResponseParameters diff --git a/src/app/CommandResponseSender.cpp b/src/app/CommandResponseSender.cpp index f23786e28dd318..0b028510c0da16 100644 --- a/src/app/CommandResponseSender.cpp +++ b/src/app/CommandResponseSender.cpp @@ -132,10 +132,10 @@ void CommandResponseSender::DispatchCommand(CommandHandlerImpl & apCommandObj, c mpCommandHandlerCallback->DispatchCommand(apCommandObj, aCommandPath, apPayload); } -Status CommandResponseSender::CommandExists(const ConcreteCommandPath & aCommandPath) +Protocols::InteractionModel::Status CommandResponseSender::ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) { - VerifyOrReturnValue(mpCommandHandlerCallback, Protocols::InteractionModel::Status::UnsupportedCommand); - return mpCommandHandlerCallback->CommandExists(aCommandPath); + VerifyOrReturnValue(mpCommandHandlerCallback, Protocols::InteractionModel::Status::Failure); + return mpCommandHandlerCallback->ValidateCommandCanBeDispatched(request); } CHIP_ERROR CommandResponseSender::SendCommandResponse() diff --git a/src/app/CommandResponseSender.h b/src/app/CommandResponseSender.h index 0e03a87f1e8cab..f0b8f11d40ca86 100644 --- a/src/app/CommandResponseSender.h +++ b/src/app/CommandResponseSender.h @@ -64,7 +64,7 @@ class CommandResponseSender : public Messaging::ExchangeDelegate, void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) override; - Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; + Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override; /** * Gets the inner exchange context object, without ownership. diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index b122333ac27a17..6d15b8a6940f4f 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -24,6 +24,9 @@ */ #include "InteractionModelEngine.h" +#include "app/data-model-provider/MetadataTypes.h" +#include "app/data-model-provider/OperationTypes.h" +#include "protocols/interaction_model/StatusCode.h" #include @@ -1646,6 +1649,8 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, DataModel::InvokeRequest request; request.path = aCommandPath; + request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, apCommandObj.IsTimedInvoke()); + request.subjectDescriptor = apCommandObj.GetSubjectDescriptor(); std::optional status = GetDataModelProvider()->Invoke(request, apPayload, &apCommandObj); @@ -1678,7 +1683,93 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, #endif // CHIP_CONFIG_USE_DATA_MODEL_INTERFACE } -Protocols::InteractionModel::Status InteractionModelEngine::CommandExists(const ConcreteCommandPath & aCommandPath) +Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) +{ + + Status status = CommandCheckExists(request.path); + + if (status != Status::Success) + { + ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", + ChipLogValueMEI(request.path.mCommandId), ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId); + return status; + } + + status = CommandCheckACL(request); + VerifyOrReturnValue(status == Status::Success, status); + + return CommandCheckFlags(request); +} + +Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckACL(const DataModel::InvokeRequest & aRequest) +{ + if (!aRequest.subjectDescriptor.has_value()) + { + return Status::UnsupportedAccess; // we require a subject for invoke + } + + Access::RequestPath requestPath{ .cluster = aRequest.path.mClusterId, + .endpoint = aRequest.path.mEndpointId, + .requestType = Access::RequestType::kCommandInvokeRequest, + .entityId = aRequest.path.mCommandId }; +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + std::optional commandInfo = mDataModelProvider->GetAcceptedCommandInfo(aRequest.path); + // This is checked by previous validations, so it should not happen + VerifyOrReturnValue(commandInfo.has_value(), Status::UnsupportedCommand); + + Access::Privilege requestPrivilege = commandInfo->invokePrivilege; +#else + Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(aRequest.path); +#endif + CHIP_ERROR err = Access::GetAccessControl().Check(*aRequest.subjectDescriptor, requestPath, requestPrivilege); + if (err != CHIP_NO_ERROR) + { + if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL)) + { + return Status::Failure; + } + return err == CHIP_ERROR_ACCESS_DENIED ? Status::UnsupportedAccess : Status::AccessRestricted; + } + + return Status::Success; +} + +Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckFlags(const DataModel::InvokeRequest & aRequest) +{ +#if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + std::optional commandInfo = mDataModelProvider->GetAcceptedCommandInfo(aRequest.path); + // This is checked by previous validations, so it should not happen + VerifyOrReturnValue(commandInfo.has_value(), Status::UnsupportedCommand); + + const bool commandNeedsTimedInvoke = commandInfo->flags.Has(DataModel::CommandQualityFlags::kTimed); + const bool CommandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); +#else + const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool CommandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); +#endif + + if (commandNeedsTimedInvoke && !aRequest.invokeFlags.Has(DataModel::InvokeFlags::kTimed)) + { + return Status::NeedsTimedInteraction; + } + + if (CommandIsFabricScoped) + { + // SPEC: Else if the command in the path is fabric-scoped and there is no accessing fabric, + // a CommandStatusIB SHALL be generated with the UNSUPPORTED_ACCESS Status Code. + + // Fabric-scoped commands are not allowed before a specific accessing fabric is available. + // This is mostly just during a PASE session before AddNOC. + if (!aRequest.subjectDescriptor.has_value() || aRequest.accessingFabricIndex == kUndefinedFabricIndex) + { + return Status::UnsupportedAccess; + } + } + + return Status::Success; +} + +Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckExists(const ConcreteCommandPath & aCommandPath) { #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE auto provider = GetDataModelProvider(); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index ba0a099bb7bdb8..493941d4c979b2 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -26,6 +26,7 @@ #pragma once // TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed +#include "app/data-model-provider/OperationTypes.h" #include #include @@ -508,7 +509,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) override; - Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) override; + Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override; bool HasActiveRead(); @@ -612,6 +613,10 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, void ShutdownMatchingSubscriptions(const Optional & aFabricIndex = NullOptional, const Optional & aPeerNodeId = NullOptional); + Status CommandCheckExists(const ConcreteCommandPath & aCommandPath); + Status CommandCheckACL(const DataModel::InvokeRequest & aRequest); + Status CommandCheckFlags(const DataModel::InvokeRequest & aRequest); + /** * Check if the given attribute path is a valid path in the data model provider. */ diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h index 8758e02ef89c5d..b13ec8f224b4fe 100644 --- a/src/app/data-model-provider/OperationTypes.h +++ b/src/app/data-model-provider/OperationTypes.h @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -99,6 +100,7 @@ struct InvokeRequest : OperationRequest { ConcreteCommandPath path; BitFlags invokeFlags; + FabricIndex accessingFabricIndex = kUndefinedFabricIndex; }; } // namespace DataModel From 3ed30eab3ad08c3834c7153ae75d376ac384e26e Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:30:09 -0400 Subject: [PATCH 02/26] Fix some includes --- src/app/InteractionModelEngine.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 6d15b8a6940f4f..bd6fcfb404abe9 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -24,9 +24,6 @@ */ #include "InteractionModelEngine.h" -#include "app/data-model-provider/MetadataTypes.h" -#include "app/data-model-provider/OperationTypes.h" -#include "protocols/interaction_model/StatusCode.h" #include @@ -35,6 +32,8 @@ #include #include #include +#include +#include #include #include #include @@ -45,6 +44,7 @@ #include #include #include +#include #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE #include From 6bd969d4adfa9ec6976c496c90bdd119ba71463c Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:30:36 -0400 Subject: [PATCH 03/26] Fix some includes --- src/app/InteractionModelEngine.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index bd6fcfb404abe9..84840897826196 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -47,8 +48,6 @@ #include #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE -#include - // TODO: defaulting to codegen should eventually be an application choice and not // hard-coded in the interaction model #include From b63c918965010ba34e6a51b2d27b4cb22293083d Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:39:06 -0400 Subject: [PATCH 04/26] More include fixes --- src/app/CommandHandlerImpl.cpp | 2 +- src/app/InteractionModelEngine.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index a38e5752b80d6a..d977f42ecc3316 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -15,7 +15,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -#include "app/data-model-provider/OperationTypes.h" #include #include @@ -23,6 +22,7 @@ #include #include #include +#include #include #include #include diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 493941d4c979b2..eed10d60fafd9b 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -26,7 +26,6 @@ #pragma once // TODO(#32628): Remove the CHIPCore.h header when the esp32 build is correctly fixed -#include "app/data-model-provider/OperationTypes.h" #include #include @@ -50,6 +49,7 @@ #include #include #include +#include #include #include #include From 0912e9ad37f7fcac04dcac32c74789cc89be241b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:40:44 -0400 Subject: [PATCH 05/26] Fix dependency to make things build --- src/app/BUILD.gn | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/BUILD.gn b/src/app/BUILD.gn index 98e7225549aaa4..f3565cf5d9a5e1 100644 --- a/src/app/BUILD.gn +++ b/src/app/BUILD.gn @@ -417,6 +417,7 @@ source_set("command-handler-impl") { "${chip_root}/src/access:types", "${chip_root}/src/app/MessageDef", "${chip_root}/src/app/data-model", + "${chip_root}/src/app/data-model-provider", "${chip_root}/src/app/util:callbacks", "${chip_root}/src/lib/support", "${chip_root}/src/messaging", From 6386bf71340a99debd109d66c6f89af97ce72037 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:49:15 -0400 Subject: [PATCH 06/26] Fixed tests --- src/app/tests/TestCommandInteraction.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index e4465c119d14f8..329ca387b56461 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -372,9 +372,21 @@ class MockCommandHandlerCallback : public CommandHandlerImpl::Callback { DispatchSingleClusterCommand(aCommandPath, apPayload, &apCommandObj); } - Protocols::InteractionModel::Status CommandExists(const ConcreteCommandPath & aCommandPath) + + Protocols::InteractionModel::Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override { - return ServerClusterCommandExists(aCommandPath); + using Protocols::InteractionModel::Status; + + Status status = ServerClusterCommandExists(request.path); + if (status != Status::Success) + { + return status; + } + + // NOTE: IM does more validation here, however for now we do minimal options + // to pass the test. + + return Status::Success; } void ResetCounter() { onFinalCalledTimes = 0; } From 4188ae5b11c164c2a71c198a1f228e4d28025337 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:49:29 -0400 Subject: [PATCH 07/26] Fix includes --- src/app/tests/TestCommandInteraction.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index 329ca387b56461..aa60f65f8de886 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -25,9 +25,9 @@ #include #include -#include #include +#include #include #include #include From af7552fe1a5ede4094f579af54838b5b08644c11 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 13:50:21 -0400 Subject: [PATCH 08/26] Restyle --- src/app/tests/TestCommandInteraction.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index aa60f65f8de886..a44d76d1fc6258 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -27,7 +27,6 @@ #include -#include #include #include #include @@ -38,6 +37,7 @@ #include #include #include +#include #include #include #include From 129ad24bfd6d4a2ff586f765f6294ccdf7aaad6a Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 15:14:29 -0400 Subject: [PATCH 09/26] Fix naming typo --- src/app/InteractionModelEngine.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 84840897826196..4b8cbff40e3605 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1741,10 +1741,10 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckFlags(co VerifyOrReturnValue(commandInfo.has_value(), Status::UnsupportedCommand); const bool commandNeedsTimedInvoke = commandInfo->flags.Has(DataModel::CommandQualityFlags::kTimed); - const bool CommandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); + const bool commandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); #else const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); - const bool CommandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); #endif if (commandNeedsTimedInvoke && !aRequest.invokeFlags.Has(DataModel::InvokeFlags::kTimed)) @@ -1752,7 +1752,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckFlags(co return Status::NeedsTimedInteraction; } - if (CommandIsFabricScoped) + if (commandIsFabricScoped) { // SPEC: Else if the command in the path is fabric-scoped and there is no accessing fabric, // a CommandStatusIB SHALL be generated with the UNSUPPORTED_ACCESS Status Code. From b08d16ebffc729fe4114c5e56765dcc2a1868bfa Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 15:34:30 -0400 Subject: [PATCH 10/26] Fix tizen build --- examples/lighting-app/tizen/src/DBusInterface.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/examples/lighting-app/tizen/src/DBusInterface.cpp b/examples/lighting-app/tizen/src/DBusInterface.cpp index 36c0f19c35bb7a..5484c7daff3793 100644 --- a/examples/lighting-app/tizen/src/DBusInterface.cpp +++ b/examples/lighting-app/tizen/src/DBusInterface.cpp @@ -42,9 +42,12 @@ class CommandHandlerImplCallback : public CommandHandlerImpl::Callback { public: using Status = Protocols::InteractionModel::Status; - void OnDone(CommandHandlerImpl & apCommandObj) {} - void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, TLV::TLVReader & apPayload) {} - Status CommandExists(const ConcreteCommandPath & aCommandPath) { return Status::Success; } + + void OnDone(CommandHandlerImpl & apCommandObj) override {} + void DispatchCommand(CommandHandlerImpl & apCommandObj, const ConcreteCommandPath & aCommandPath, + TLV::TLVReader & apPayload) override + {} + Status ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) override { return Status::Success; } }; DBusInterface::DBusInterface(chip::EndpointId endpointId) : mEndpointId(endpointId) From 03dcc8138aeb7e864d6a98640aae36f0e05b7710 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 20:52:08 -0400 Subject: [PATCH 11/26] Fix condition typo --- src/app/InteractionModelEngine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 4b8cbff40e3605..869902a5bb937a 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1759,7 +1759,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckFlags(co // Fabric-scoped commands are not allowed before a specific accessing fabric is available. // This is mostly just during a PASE session before AddNOC. - if (!aRequest.subjectDescriptor.has_value() || aRequest.accessingFabricIndex == kUndefinedFabricIndex) + if (aRequest.accessingFabricIndex == kUndefinedFabricIndex) { return Status::UnsupportedAccess; } From d197a9405f442de26856716b64c50813eeb6f618 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 20:57:06 -0400 Subject: [PATCH 12/26] Keep group command logic to use continue instead of status return ... there is no status to return --- src/app/CommandHandlerImpl.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index d977f42ecc3316..d2eae50020fd0e 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -516,7 +516,10 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); if (preCheckStatus != Status::Success) { - return FallibleAddStatus(concretePath, preCheckStatus) != CHIP_NO_ERROR ? Status::Failure : Status::Success; + // NOTE: an expected error is CHIP_ERROR_ACCESS_DENIED, but there could be other unexpected errors; + // therefore, keep processing subsequent commands, and if any errors continue, those subsequent + // commands will likewise fail. + continue; } } From 4ec72e5ea90fdfd69a2df9ecba5d1d01be0b4dad Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Thu, 3 Oct 2024 21:02:04 -0400 Subject: [PATCH 13/26] Minor update to kick restyler --- src/app/CommandHandlerImpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index d2eae50020fd0e..7f736dae1c0aae 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020 Project CHIP Authors + * Copyright (c) 2020-2024 Project CHIP Authors * All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); From 3d3e7f6391d8e47f62ede815f6e53956d14cd1eb Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 4 Oct 2024 08:30:40 -0400 Subject: [PATCH 14/26] one more decoupling update: sligtly more inefficient, however likely this is not important --- src/app/CommandHandlerImpl.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index 7f736dae1c0aae..f5eb159838111d 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020-2024 Project CHIP Authors + * Copyright (c) 2020 Project CHIP Authors * All rights reserved. * * Licensed under the Apache License, Version 2.0 (the "License"); @@ -479,11 +479,19 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo // once up front and discard all the paths at once. Ordering with respect // to ACL and command presence checks does not matter, because the behavior // is the same for all of them: ignore the path. +#if !CHIP_CONFIG_USE_DATA_MODEL_INTERFACE + + // Without data model interface, we can query individual commands. + // Data model interface queries commands by a full path so we need endpointID as well. + // + // Since this is a performance update and group commands are never timed, + // missing this should not be that noticeable. if (CommandNeedsTimedInvoke(clusterId, commandId)) { // Group commands are never timed. return Status::Success; } +#endif // No check for `CommandIsFabricScoped` unlike in `ProcessCommandDataIB()` since group commands // always have an accessing fabric, by definition. From 5ca78dff9e268a256b0531a81f2b5665f556984b Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 4 Oct 2024 10:00:17 -0400 Subject: [PATCH 15/26] Renamed based on code review feedback --- src/app/InteractionModelEngine.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 869902a5bb937a..95ef3c92fb5d5c 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1716,11 +1716,11 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckACL(cons // This is checked by previous validations, so it should not happen VerifyOrReturnValue(commandInfo.has_value(), Status::UnsupportedCommand); - Access::Privilege requestPrivilege = commandInfo->invokePrivilege; + Access::Privilege minimumRequiredPrivilege = commandInfo->invokePrivilege; #else - Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(aRequest.path); + Access::Privilege minimumRequiredPrivilege = RequiredPrivilege::ForInvokeCommand(aRequest.path); #endif - CHIP_ERROR err = Access::GetAccessControl().Check(*aRequest.subjectDescriptor, requestPath, requestPrivilege); + CHIP_ERROR err = Access::GetAccessControl().Check(*aRequest.subjectDescriptor, requestPath, minimumRequiredPrivilege); if (err != CHIP_NO_ERROR) { if ((err != CHIP_ERROR_ACCESS_DENIED) && (err != CHIP_ERROR_ACCESS_RESTRICTED_BY_ARL)) From e03197db405f2e12e421347cff5d101745b1edee Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 4 Oct 2024 14:00:36 +0000 Subject: [PATCH 16/26] Restyled by clang-format --- src/app/InteractionModelEngine.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 95ef3c92fb5d5c..c70ef36733ec4e 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1743,8 +1743,8 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckFlags(co const bool commandNeedsTimedInvoke = commandInfo->flags.Has(DataModel::CommandQualityFlags::kTimed); const bool commandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); #else - const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); - const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); #endif if (commandNeedsTimedInvoke && !aRequest.invokeFlags.Has(DataModel::InvokeFlags::kTimed)) From 3cf6ff3c55b8cce4f41923efe18316190b375e78 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 8 Oct 2024 09:28:01 -0400 Subject: [PATCH 17/26] Fix some code review comments --- src/app/CommandHandlerImpl.cpp | 4 +--- src/app/InteractionModelEngine.cpp | 12 ++++++------ src/app/InteractionModelEngine.h | 6 +++--- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index f5eb159838111d..a57d9395968174 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -524,9 +524,7 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); if (preCheckStatus != Status::Success) { - // NOTE: an expected error is CHIP_ERROR_ACCESS_DENIED, but there could be other unexpected errors; - // therefore, keep processing subsequent commands, and if any errors continue, those subsequent - // commands will likewise fail. + // Command failed for a specific path, keep trying the rest of the commands still continue; } } diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index c70ef36733ec4e..e929f3847be12c 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1689,7 +1689,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe if (status != Status::Success) { - ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%x", + ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%u", ChipLogValueMEI(request.path.mCommandId), ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId); return status; } @@ -1700,7 +1700,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe return CommandCheckFlags(request); } -Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckACL(const DataModel::InvokeRequest & aRequest) +Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(const DataModel::InvokeRequest & aRequest) { if (!aRequest.subjectDescriptor.has_value()) { @@ -1714,7 +1714,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckACL(cons #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE std::optional commandInfo = mDataModelProvider->GetAcceptedCommandInfo(aRequest.path); // This is checked by previous validations, so it should not happen - VerifyOrReturnValue(commandInfo.has_value(), Status::UnsupportedCommand); + VerifyOrDie(commandInfo.has_value()); Access::Privilege minimumRequiredPrivilege = commandInfo->invokePrivilege; #else @@ -1733,12 +1733,12 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckACL(cons return Status::Success; } -Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckFlags(const DataModel::InvokeRequest & aRequest) +Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandFlags(const DataModel::InvokeRequest & aRequest) { #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE std::optional commandInfo = mDataModelProvider->GetAcceptedCommandInfo(aRequest.path); // This is checked by previous validations, so it should not happen - VerifyOrReturnValue(commandInfo.has_value(), Status::UnsupportedCommand); + VerifyOrDie(commandInfo.has_value()); const bool commandNeedsTimedInvoke = commandInfo->flags.Has(DataModel::CommandQualityFlags::kTimed); const bool commandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); @@ -1768,7 +1768,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckFlags(co return Status::Success; } -Protocols::InteractionModel::Status InteractionModelEngine::CommandCheckExists(const ConcreteCommandPath & aCommandPath) +Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandExistence(const ConcreteCommandPath & aCommandPath) { #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE auto provider = GetDataModelProvider(); diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index eed10d60fafd9b..a35023d9553c3a 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -613,9 +613,9 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, void ShutdownMatchingSubscriptions(const Optional & aFabricIndex = NullOptional, const Optional & aPeerNodeId = NullOptional); - Status CommandCheckExists(const ConcreteCommandPath & aCommandPath); - Status CommandCheckACL(const DataModel::InvokeRequest & aRequest); - Status CommandCheckFlags(const DataModel::InvokeRequest & aRequest); + Status CheckCommandExistence(const ConcreteCommandPath & aCommandPath); + Status CheckCommandAccess(const DataModel::InvokeRequest & aRequest); + Status CheckCommandFlags(const DataModel::InvokeRequest & aRequest); /** * Check if the given attribute path is a valid path in the data model provider. From 6e777137e6b1e748f44f57a17db0e92728cff7c1 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 8 Oct 2024 09:31:26 -0400 Subject: [PATCH 18/26] Review comment: default privilege to operate --- src/app/InteractionModelEngine.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index e929f3847be12c..ed76d3916698ed 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1713,10 +1713,8 @@ Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(c .entityId = aRequest.path.mCommandId }; #if CHIP_CONFIG_USE_DATA_MODEL_INTERFACE std::optional commandInfo = mDataModelProvider->GetAcceptedCommandInfo(aRequest.path); - // This is checked by previous validations, so it should not happen - VerifyOrDie(commandInfo.has_value()); - - Access::Privilege minimumRequiredPrivilege = commandInfo->invokePrivilege; + Access::Privilege minimumRequiredPrivilege = + commandInfo.has_value() ? commandInfo->invokePrivilege : Access::Privilege::kOperate; #else Access::Privilege minimumRequiredPrivilege = RequiredPrivilege::ForInvokeCommand(aRequest.path); #endif @@ -1743,8 +1741,8 @@ Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandFlags(co const bool commandNeedsTimedInvoke = commandInfo->flags.Has(DataModel::CommandQualityFlags::kTimed); const bool commandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); #else - const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); - const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); #endif if (commandNeedsTimedInvoke && !aRequest.invokeFlags.Has(DataModel::InvokeFlags::kTimed)) From 99d062944176b9ad10420baa8468567056a73898 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 8 Oct 2024 09:32:26 -0400 Subject: [PATCH 19/26] Fix up renames --- src/app/InteractionModelEngine.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index ed76d3916698ed..80c1c234e58955 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1685,7 +1685,7 @@ void InteractionModelEngine::DispatchCommand(CommandHandlerImpl & apCommandObj, Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBeDispatched(const DataModel::InvokeRequest & request) { - Status status = CommandCheckExists(request.path); + Status status = CheckCommandExistence(request.path); if (status != Status::Success) { @@ -1694,10 +1694,10 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe return status; } - status = CommandCheckACL(request); + status = CheckCommandAccess(request); VerifyOrReturnValue(status == Status::Success, status); - return CommandCheckFlags(request); + return CheckCommandFlags(request); } Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandAccess(const DataModel::InvokeRequest & aRequest) From 5981a576d470ed5621660f7e35e7bccbdc2fd0ad Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 8 Oct 2024 09:49:02 -0400 Subject: [PATCH 20/26] Fix hex prefix --- src/app/InteractionModelEngine.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 80c1c234e58955..4ccbf254c11263 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1689,7 +1689,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::ValidateCommandCanBe if (status != Status::Success) { - ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint 0x%u", + ChipLogDetail(DataManagement, "No command " ChipLogFormatMEI " in Cluster " ChipLogFormatMEI " on Endpoint %u", ChipLogValueMEI(request.path.mCommandId), ChipLogValueMEI(request.path.mClusterId), request.path.mEndpointId); return status; } From f0968aeb9d4b07f48479e06d690e676a8a4819ad Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Tue, 8 Oct 2024 13:49:30 +0000 Subject: [PATCH 21/26] Restyled by clang-format --- src/app/InteractionModelEngine.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 4ccbf254c11263..77e9475d1b22b5 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1741,8 +1741,8 @@ Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandFlags(co const bool commandNeedsTimedInvoke = commandInfo->flags.Has(DataModel::CommandQualityFlags::kTimed); const bool commandIsFabricScoped = commandInfo->flags.Has(DataModel::CommandQualityFlags::kFabricScoped); #else - const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); - const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandNeedsTimedInvoke = CommandNeedsTimedInvoke(aRequest.path.mClusterId, aRequest.path.mCommandId); + const bool commandIsFabricScoped = CommandIsFabricScoped(aRequest.path.mClusterId, aRequest.path.mCommandId); #endif if (commandNeedsTimedInvoke && !aRequest.invokeFlags.Has(DataModel::InvokeFlags::kTimed)) From c8efbbfb53513d1f6016f2afa108d7c662685faa Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 8 Oct 2024 16:02:13 -0400 Subject: [PATCH 22/26] Fix cirque after log formatting change. Load bearing log .... --- .../linux-cirque/MobileDeviceTest.py | 2 +- .../linux-cirque/helper/CHIPTestBase.py | 278 +++++++++++------- 2 files changed, 178 insertions(+), 102 deletions(-) diff --git a/src/test_driver/linux-cirque/MobileDeviceTest.py b/src/test_driver/linux-cirque/MobileDeviceTest.py index a206dc31c25c03..c8a73baf0c2a78 100755 --- a/src/test_driver/linux-cirque/MobileDeviceTest.py +++ b/src/test_driver/linux-cirque/MobileDeviceTest.py @@ -126,7 +126,7 @@ def run_controller_test(self): "Toggle ep1 on/off from state 0 to 1", "Received command for Endpoint=1 Cluster=0x0000_0006 Command=0x0000_0000", "Toggle ep1 on/off from state 1 to 0", - "No command 0x0000_0001 in Cluster 0x0000_0006 on Endpoint 0xe9"]), + "No command 0x0000_0001 in Cluster 0x0000_0006 on Endpoint 233"]), "Datamodel test failed: cannot find matching string from device {}".format(device_id)) diff --git a/src/test_driver/linux-cirque/helper/CHIPTestBase.py b/src/test_driver/linux-cirque/helper/CHIPTestBase.py index ea45a62221e7e5..74c69bc441f969 100644 --- a/src/test_driver/linux-cirque/helper/CHIPTestBase.py +++ b/src/test_driver/linux-cirque/helper/CHIPTestBase.py @@ -35,13 +35,13 @@ class TestResult(IntEnum): SYSTEM_FAILURE = 2 -''' +""" CHIPVirtualHome is a base class for single home tests child classes should implement: - setup() - test_routine() - tear_down() -''' +""" class CHIPVirtualHome: @@ -95,31 +95,31 @@ def query_api(self, end_point, args=[], binary=False): def execute_device_cmd(self, device_id, cmd, stream=False): self.logger.info( - "device: {} exec: {}".format(self.get_device_pretty_id(device_id), cmd)) - ret = requests.get(self._build_request_url('device_cmd', [self.home_id, device_id, cmd]), - params={'stream': stream}, - stream=stream) + "device: {} exec: {}".format(self.get_device_pretty_id(device_id), cmd) + ) + ret = requests.get( + self._build_request_url("device_cmd", [self.home_id, device_id, cmd]), + params={"stream": stream}, + stream=stream, + ) if stream: return ret ret_struct = ret.json() - command_ret_code = ret_struct.get('return_code', None) + command_ret_code = ret_struct.get("return_code", None) if command_ret_code is None: # could be 0 self.logger.error("cannot get command return code") raise Exception("cannot get command return code") self.logger.info( - "command return code: {}".format( - ret_struct.get('return_code', 'Unknown')) + "command return code: {}".format(ret_struct.get("return_code", "Unknown")) ) - command_output = ret_struct.get('output', None) + command_output = ret_struct.get("output", None) if command_output is None: # could be empty string self.logger.error("cannot get command output") raise Exception("cannot get command output") - self.logger.info( - "command output: \n{}".format(ret_struct.get('output', '')) - ) + self.logger.info("command output: \n{}".format(ret_struct.get("output", ""))) return ret_struct def sequenceMatch(self, string, patterns): @@ -128,7 +128,7 @@ def sequenceMatch(self, string, patterns): self.logger.info('Finding string: "{}"'.format(s)) this_find = string.find(s, last_find) if this_find < 0: - self.logger.info('Not found') + self.logger.info("Not found") return False self.logger.info("Found at index={}".format(this_find)) last_find = this_find + len(s) @@ -142,49 +142,66 @@ def reset_thread_devices(self, devices: Union[List[str], str]): devices = [devices] for device_id in devices: # Wait for otbr-agent and CHIP server start - self.assertTrue(self.wait_for_device_output( - device_id, "Thread Border Router started on AIL", 10)) - self.assertTrue(self.wait_for_device_output( - device_id, "[SVR] Server Listening...", 15)) + self.assertTrue( + self.wait_for_device_output( + device_id, "Thread Border Router started on AIL", 10 + ) + ) + self.assertTrue( + self.wait_for_device_output(device_id, "[SVR] Server Listening...", 15) + ) # Clear default Thread network commissioning data - self.logger.info("Resetting thread network on {}".format( - self.get_device_pretty_id(device_id))) - self.execute_device_cmd(device_id, 'ot-ctl factoryreset') + self.logger.info( + "Resetting thread network on {}".format( + self.get_device_pretty_id(device_id) + ) + ) + self.execute_device_cmd(device_id, "ot-ctl factoryreset") self.check_device_thread_state( - device_id=device_id, expected_role="disabled", timeout=10) + device_id=device_id, expected_role="disabled", timeout=10 + ) def check_device_thread_state(self, device_id, expected_role, timeout): if isinstance(expected_role, str): expected_role = [expected_role] self.logger.info( - f"Waiting for expected role. {self.get_device_pretty_id(device_id)}: {expected_role}") + f"Waiting for expected role. {self.get_device_pretty_id(device_id)}: {expected_role}" + ) start = time.time() while time.time() < (start + timeout): - reply = self.execute_device_cmd(device_id, 'ot-ctl state') - if reply['output'].split()[0] in expected_role: + reply = self.execute_device_cmd(device_id, "ot-ctl state") + if reply["output"].split()[0] in expected_role: return time.sleep(0.5) self.logger.error( - f"Device {self.get_device_pretty_id(device_id)} does not reach expected role") + f"Device {self.get_device_pretty_id(device_id)} does not reach expected role" + ) raise AssertionError - def form_thread_network(self, device_id: str, expected_role: Union[str, List[str]], timeout: int = 15, - dataset: str = ""): + def form_thread_network( + self, + device_id: str, + expected_role: Union[str, List[str]], + timeout: int = 15, + dataset: str = "", + ): """ Start Thread Network with provided dataset. If dataset is not provided then default will be set. Function that will be also verifying if device start in expected role. """ if not dataset: - dataset = "0e080000000000010000" + \ - "000300000c" + \ - "35060004001fffe0" + \ - "0208fedcba9876543210" + \ - "0708fd00000000001234" + \ - "0510ffeeddccbbaa99887766554433221100" + \ - "030e54657374696e674e6574776f726b" + \ - "0102d252" + \ - "041081cb3b2efa781cc778397497ff520fa50c0302a0ff" + dataset = ( + "0e080000000000010000" + + "000300000c" + + "35060004001fffe0" + + "0208fedcba9876543210" + + "0708fd00000000001234" + + "0510ffeeddccbbaa99887766554433221100" + + "030e54657374696e674e6574776f726b" + + "0102d252" + + "041081cb3b2efa781cc778397497ff520fa50c0302a0ff" + ) ot_init_commands = [ "ot-ctl thread stop", @@ -195,30 +212,33 @@ def form_thread_network(self, device_id: str, expected_role: Union[str, List[str "ot-ctl dataset active", ] self.logger.info( - f"Setting Thread dataset for {self.get_device_pretty_id(device_id)}: {dataset}") + f"Setting Thread dataset for {self.get_device_pretty_id(device_id)}: {dataset}" + ) for cmd in ot_init_commands: self.execute_device_cmd(device_id, cmd) self.check_device_thread_state( - device_id=device_id, expected_role=expected_role, timeout=timeout) + device_id=device_id, expected_role=expected_role, timeout=timeout + ) def connect_to_thread_network(self): - ''' + """ The dataset in this function is used to replace the default dataset generated by openthread. When the test writer is calling this function to setup a thread network, it means they just want a working IPv6 network or a working thread network and don't care about the detail of this network. - ''' + """ self.logger.info("Running commands to form default Thread network") for device in self.thread_devices: - self.wait_for_device_output( - device['id'], "Border router agent started.", 5) + self.wait_for_device_output(device["id"], "Border router agent started.", 5) otInitCommands = [ "ot-ctl thread stop", "ot-ctl ifconfig down", - ("ot-ctl dataset set active 0e080000000000010000000300000d35060004001fffe00208d" - "ead00beef00cafe0708fd01234567890abc051000112233445566778899aabbccddeeff030a4f" - "70656e546872656164010212340410ad463152f9622c7297ec6c6c543a63e70c0302a0ff"), + ( + "ot-ctl dataset set active 0e080000000000010000000300000d35060004001fffe00208d" + "ead00beef00cafe0708fd01234567890abc051000112233445566778899aabbccddeeff030a4f" + "70656e546872656164010212340410ad463152f9622c7297ec6c6c543a63e70c0302a0ff" + ), "ot-ctl ifconfig up", "ot-ctl thread start", "ot-ctl dataset active", # Emit @@ -226,17 +246,19 @@ def connect_to_thread_network(self): for device in self.thread_devices: # Set default openthread provisioning for cmd in otInitCommands: - self.execute_device_cmd(device['id'], cmd) + self.execute_device_cmd(device["id"], cmd) self.logger.info("Waiting for Thread network to be formed...") threadNetworkFormed = False for i in range(30): roles = list() for device in self.thread_devices: # We can only check the status of ot-agent by query its state. - reply = self.execute_device_cmd(device['id'], 'ot-ctl state') - roles.append(reply['output'].split()[0]) - threadNetworkFormed = (roles.count('leader') == 1) and (roles.count( - 'leader') + roles.count('router') + roles.count('child') == len(self.thread_devices)) + reply = self.execute_device_cmd(device["id"], "ot-ctl state") + roles.append(reply["output"].split()[0]) + threadNetworkFormed = (roles.count("leader") == 1) and ( + roles.count("leader") + roles.count("router") + roles.count("child") + == len(self.thread_devices) + ) if threadNetworkFormed: break time.sleep(1) @@ -244,33 +266,46 @@ def connect_to_thread_network(self): self.logger.info("Thread network formed") def enable_wifi_on_device(self): - ssid, psk = self.query_api('wifi_ssid_psk', [self.home_id]) + ssid, psk = self.query_api("wifi_ssid_psk", [self.home_id]) self.logger.info("wifi ap ssid: {}, psk: {}".format(ssid, psk)) for device in self.non_ap_devices: self.logger.info( "device: {} connecting to desired ssid: {}".format( - self.get_device_pretty_id(device['id']), ssid)) - self.write_psk_to_wpa_supplicant_config(device['id'], ssid, psk) - self.kill_existing_wpa_supplicant(device['id']) - self.start_wpa_supplicant(device['id']) + self.get_device_pretty_id(device["id"]), ssid + ) + ) + self.write_psk_to_wpa_supplicant_config(device["id"], ssid, psk) + self.kill_existing_wpa_supplicant(device["id"]) + self.start_wpa_supplicant(device["id"]) time.sleep(5) def get_device_thread_ip(self, device_id): - ret = self.execute_device_cmd(device_id, 'ot-ctl ipaddr') + ret = self.execute_device_cmd(device_id, "ot-ctl ipaddr") ipaddr_list = ret["output"].splitlines() for ipstr in ipaddr_list: try: self.logger.info( - "device: {} thread ip: {}".format(self.get_device_pretty_id(device_id), ipstr)) + "device: {} thread ip: {}".format( + self.get_device_pretty_id(device_id), ipstr + ) + ) ipaddr = ipaddress.ip_address(ipstr) if ipaddr.is_link_local: continue if not ipaddr.is_private: continue - if re.match(("fd[0-9a-f]{2}:[0-9a-f]{4}:[0-9a-f]" - "{4}:[0-9a-f]{4}:0000:00ff:fe00:[0-9a-f]{4}"), ipaddr.exploded) is not None: + if ( + re.match( + ( + "fd[0-9a-f]{2}:[0-9a-f]{4}:[0-9a-f]" + "{4}:[0-9a-f]{4}:0000:00ff:fe00:[0-9a-f]{4}" + ), + ipaddr.exploded, + ) + is not None + ): continue self.logger.info("Get Mesh-Local EID: {}".format(ipstr)) return str(ipaddr) @@ -280,12 +315,17 @@ def get_device_thread_ip(self, device_id): return None def get_device_log(self, device_id): - return self.query_api('device_log', [self.home_id, device_id], binary=True) + return self.query_api("device_log", [self.home_id, device_id], binary=True) def wait_for_device_output(self, device_id, pattern, timeout=1): due = time.time() + timeout while True: - if self.sequenceMatch(self.get_device_log(device_id).decode(), [pattern, ]): + if self.sequenceMatch( + self.get_device_log(device_id).decode(), + [ + pattern, + ], + ): return True if time.time() < due: time.sleep(1) @@ -294,11 +334,11 @@ def wait_for_device_output(self, device_id, pattern, timeout=1): return False def assertTrue(self, exp, note=None): - ''' + """ assert{True|False} assert(Not)Equal python unittest style functions that raise exceptions when condition not met - ''' + """ if exp is not True: if note: self.logger.error(note) @@ -325,49 +365,69 @@ def assertNotEqual(self, val1, val2, note=None): def _build_request_url(self, end_point, args=[]): if len(args) == 0: return urljoin(self.cirque_url, end_point) - return urljoin(self.cirque_url, "{}/{}".format(end_point, '/'.join([str(argv) for argv in args]))) + return urljoin( + self.cirque_url, + "{}/{}".format(end_point, "/".join([str(argv) for argv in args])), + ) def destroy_home(self): self.logger.info("destroying home: {}".format(self.home_id)) - self.query_api('destroy_home', [self.home_id]) + self.query_api("destroy_home", [self.home_id]) def initialize_home(self): home_id = requests.post( - self._build_request_url('create_home'), json=self.device_config).json() + self._build_request_url("create_home"), json=self.device_config + ).json() self.logger.info("home id: {} created!".format(home_id)) - self.assertTrue(home_id in - list(self.query_api('get_homes')), - "created home_id did not match id from get_homes!!") + self.assertTrue( + home_id in list(self.query_api("get_homes")), + "created home_id did not match id from get_homes!!", + ) self.home_id = home_id device_types = set() - created_devices = self.query_api('home_devices', [home_id]) + created_devices = self.query_api("home_devices", [home_id]) - self.logger.info("home id: {} devices: {}".format( - home_id, json.dumps(created_devices, indent=4, sort_keys=True))) + self.logger.info( + "home id: {} devices: {}".format( + home_id, json.dumps(created_devices, indent=4, sort_keys=True) + ) + ) for device in created_devices.values(): - device_types.add(device['type']) + device_types.add(device["type"]) wanted_device_types = set() for device in self.device_config.values(): - wanted_device_types.add(device['type']) + wanted_device_types.add(device["type"]) - self.assertEqual(device_types, wanted_device_types, - "created device does not match to device config!!") + self.assertEqual( + device_types, + wanted_device_types, + "created device does not match to device config!!", + ) self.device_config = created_devices self.device_ids = [device_id for device_id in self.device_config] - self.non_ap_devices = [device for device in self.device_config.values() - if device['type'] != 'wifi_ap'] - self.thread_devices = [device for device in self.device_config.values() - if device['capability'].get('Thread', None) is not None] - self.ap_devices = [device for device in self.device_config.values() - if device['type'] == 'wifi_ap'] + self.non_ap_devices = [ + device + for device in self.device_config.values() + if device["type"] != "wifi_ap" + ] + self.thread_devices = [ + device + for device in self.device_config.values() + if device["capability"].get("Thread", None) is not None + ] + self.ap_devices = [ + device + for device in self.device_config.values() + if device["type"] == "wifi_ap" + ] def save_device_logs(self): timestamp = int(time.time()) @@ -376,47 +436,61 @@ def save_device_logs(self): os.makedirs("logs") for device in self.non_ap_devices: - ret_log = self.get_device_log(device['id']) + ret_log = self.get_device_log(device["id"]) # Use this format for easier sort - f_name = '{}-{}-{}.log'.format(device['type'], - timestamp, device['id'][:8]) + f_name = "{}-{}-{}.log".format(device["type"], timestamp, device["id"][:8]) self.logger.debug("device log name: \n{}".format(f_name)) - with open(os.path.join(log_dir, f_name), 'wb') as fp: + with open(os.path.join(log_dir, f_name), "wb") as fp: fp.write(ret_log) def start_wpa_supplicant(self, device_id): - self.logger.info("device: {}: starting wpa_supplicant on device" - .format(self.get_device_pretty_id(device_id))) + self.logger.info( + "device: {}: starting wpa_supplicant on device".format( + self.get_device_pretty_id(device_id) + ) + ) start_wpa_supplicant_command = "".join( - ["wpa_supplicant -B -i wlan0 ", - "-c /etc/wpa_supplicant/wpa_supplicant.conf ", - "-f /var/log/wpa_supplicant.log -t -dd"]) + [ + "wpa_supplicant -B -i wlan0 ", + "-c /etc/wpa_supplicant/wpa_supplicant.conf ", + "-f /var/log/wpa_supplicant.log -t -dd", + ] + ) return self.execute_device_cmd(device_id, start_wpa_supplicant_command) def write_psk_to_wpa_supplicant_config(self, device_id, ssid, psk): - self.logger.info("device: {}: writing ssid, psk to wpa_supplicant config" - .format(self.get_device_pretty_id(device_id))) + self.logger.info( + "device: {}: writing ssid, psk to wpa_supplicant config".format( + self.get_device_pretty_id(device_id) + ) + ) write_psk_command = "".join( - ["sh -c 'wpa_passphrase {} {} >> ".format(ssid, psk), - "/etc/wpa_supplicant/wpa_supplicant.conf'"]) + [ + "sh -c 'wpa_passphrase {} {} >> ".format(ssid, psk), + "/etc/wpa_supplicant/wpa_supplicant.conf'", + ] + ) return self.execute_device_cmd(device_id, write_psk_command) def kill_existing_wpa_supplicant(self, device_id): - self.logger.info("device: {}: kill existing wpa_supplicant" - .format(self.get_device_pretty_id(device_id))) + self.logger.info( + "device: {}: kill existing wpa_supplicant".format( + self.get_device_pretty_id(device_id) + ) + ) - kill_wpa_supplicant_command = 'killall wpa_supplicant' + kill_wpa_supplicant_command = "killall wpa_supplicant" return self.execute_device_cmd(device_id, kill_wpa_supplicant_command) def get_device_pretty_name(self, device_id): device_obj = self.device_config.get(device_id, None) if device_obj is not None: - return device_obj['type'] + return device_obj["type"] return "" def get_device_pretty_id(self, device_id): @@ -424,4 +498,6 @@ def get_device_pretty_id(self, device_id): @property def default_base_image(cls): - return os.environ.get("CHIP_CIRQUE_BASE_IMAGE", "project-chip/chip-cirque-device-base") + return os.environ.get( + "CHIP_CIRQUE_BASE_IMAGE", "project-chip/chip-cirque-device-base" + ) From 4f1f19652e7e5f3604121af52802ab4f7e66b78d Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Tue, 8 Oct 2024 16:23:01 -0400 Subject: [PATCH 23/26] Revert file change that I did not mean to change --- .../linux-cirque/helper/CHIPTestBase.py | 278 +++++++----------- 1 file changed, 101 insertions(+), 177 deletions(-) diff --git a/src/test_driver/linux-cirque/helper/CHIPTestBase.py b/src/test_driver/linux-cirque/helper/CHIPTestBase.py index 74c69bc441f969..ea45a62221e7e5 100644 --- a/src/test_driver/linux-cirque/helper/CHIPTestBase.py +++ b/src/test_driver/linux-cirque/helper/CHIPTestBase.py @@ -35,13 +35,13 @@ class TestResult(IntEnum): SYSTEM_FAILURE = 2 -""" +''' CHIPVirtualHome is a base class for single home tests child classes should implement: - setup() - test_routine() - tear_down() -""" +''' class CHIPVirtualHome: @@ -95,31 +95,31 @@ def query_api(self, end_point, args=[], binary=False): def execute_device_cmd(self, device_id, cmd, stream=False): self.logger.info( - "device: {} exec: {}".format(self.get_device_pretty_id(device_id), cmd) - ) - ret = requests.get( - self._build_request_url("device_cmd", [self.home_id, device_id, cmd]), - params={"stream": stream}, - stream=stream, - ) + "device: {} exec: {}".format(self.get_device_pretty_id(device_id), cmd)) + ret = requests.get(self._build_request_url('device_cmd', [self.home_id, device_id, cmd]), + params={'stream': stream}, + stream=stream) if stream: return ret ret_struct = ret.json() - command_ret_code = ret_struct.get("return_code", None) + command_ret_code = ret_struct.get('return_code', None) if command_ret_code is None: # could be 0 self.logger.error("cannot get command return code") raise Exception("cannot get command return code") self.logger.info( - "command return code: {}".format(ret_struct.get("return_code", "Unknown")) + "command return code: {}".format( + ret_struct.get('return_code', 'Unknown')) ) - command_output = ret_struct.get("output", None) + command_output = ret_struct.get('output', None) if command_output is None: # could be empty string self.logger.error("cannot get command output") raise Exception("cannot get command output") - self.logger.info("command output: \n{}".format(ret_struct.get("output", ""))) + self.logger.info( + "command output: \n{}".format(ret_struct.get('output', '')) + ) return ret_struct def sequenceMatch(self, string, patterns): @@ -128,7 +128,7 @@ def sequenceMatch(self, string, patterns): self.logger.info('Finding string: "{}"'.format(s)) this_find = string.find(s, last_find) if this_find < 0: - self.logger.info("Not found") + self.logger.info('Not found') return False self.logger.info("Found at index={}".format(this_find)) last_find = this_find + len(s) @@ -142,66 +142,49 @@ def reset_thread_devices(self, devices: Union[List[str], str]): devices = [devices] for device_id in devices: # Wait for otbr-agent and CHIP server start - self.assertTrue( - self.wait_for_device_output( - device_id, "Thread Border Router started on AIL", 10 - ) - ) - self.assertTrue( - self.wait_for_device_output(device_id, "[SVR] Server Listening...", 15) - ) + self.assertTrue(self.wait_for_device_output( + device_id, "Thread Border Router started on AIL", 10)) + self.assertTrue(self.wait_for_device_output( + device_id, "[SVR] Server Listening...", 15)) # Clear default Thread network commissioning data - self.logger.info( - "Resetting thread network on {}".format( - self.get_device_pretty_id(device_id) - ) - ) - self.execute_device_cmd(device_id, "ot-ctl factoryreset") + self.logger.info("Resetting thread network on {}".format( + self.get_device_pretty_id(device_id))) + self.execute_device_cmd(device_id, 'ot-ctl factoryreset') self.check_device_thread_state( - device_id=device_id, expected_role="disabled", timeout=10 - ) + device_id=device_id, expected_role="disabled", timeout=10) def check_device_thread_state(self, device_id, expected_role, timeout): if isinstance(expected_role, str): expected_role = [expected_role] self.logger.info( - f"Waiting for expected role. {self.get_device_pretty_id(device_id)}: {expected_role}" - ) + f"Waiting for expected role. {self.get_device_pretty_id(device_id)}: {expected_role}") start = time.time() while time.time() < (start + timeout): - reply = self.execute_device_cmd(device_id, "ot-ctl state") - if reply["output"].split()[0] in expected_role: + reply = self.execute_device_cmd(device_id, 'ot-ctl state') + if reply['output'].split()[0] in expected_role: return time.sleep(0.5) self.logger.error( - f"Device {self.get_device_pretty_id(device_id)} does not reach expected role" - ) + f"Device {self.get_device_pretty_id(device_id)} does not reach expected role") raise AssertionError - def form_thread_network( - self, - device_id: str, - expected_role: Union[str, List[str]], - timeout: int = 15, - dataset: str = "", - ): + def form_thread_network(self, device_id: str, expected_role: Union[str, List[str]], timeout: int = 15, + dataset: str = ""): """ Start Thread Network with provided dataset. If dataset is not provided then default will be set. Function that will be also verifying if device start in expected role. """ if not dataset: - dataset = ( - "0e080000000000010000" - + "000300000c" - + "35060004001fffe0" - + "0208fedcba9876543210" - + "0708fd00000000001234" - + "0510ffeeddccbbaa99887766554433221100" - + "030e54657374696e674e6574776f726b" - + "0102d252" - + "041081cb3b2efa781cc778397497ff520fa50c0302a0ff" - ) + dataset = "0e080000000000010000" + \ + "000300000c" + \ + "35060004001fffe0" + \ + "0208fedcba9876543210" + \ + "0708fd00000000001234" + \ + "0510ffeeddccbbaa99887766554433221100" + \ + "030e54657374696e674e6574776f726b" + \ + "0102d252" + \ + "041081cb3b2efa781cc778397497ff520fa50c0302a0ff" ot_init_commands = [ "ot-ctl thread stop", @@ -212,33 +195,30 @@ def form_thread_network( "ot-ctl dataset active", ] self.logger.info( - f"Setting Thread dataset for {self.get_device_pretty_id(device_id)}: {dataset}" - ) + f"Setting Thread dataset for {self.get_device_pretty_id(device_id)}: {dataset}") for cmd in ot_init_commands: self.execute_device_cmd(device_id, cmd) self.check_device_thread_state( - device_id=device_id, expected_role=expected_role, timeout=timeout - ) + device_id=device_id, expected_role=expected_role, timeout=timeout) def connect_to_thread_network(self): - """ + ''' The dataset in this function is used to replace the default dataset generated by openthread. When the test writer is calling this function to setup a thread network, it means they just want a working IPv6 network or a working thread network and don't care about the detail of this network. - """ + ''' self.logger.info("Running commands to form default Thread network") for device in self.thread_devices: - self.wait_for_device_output(device["id"], "Border router agent started.", 5) + self.wait_for_device_output( + device['id'], "Border router agent started.", 5) otInitCommands = [ "ot-ctl thread stop", "ot-ctl ifconfig down", - ( - "ot-ctl dataset set active 0e080000000000010000000300000d35060004001fffe00208d" - "ead00beef00cafe0708fd01234567890abc051000112233445566778899aabbccddeeff030a4f" - "70656e546872656164010212340410ad463152f9622c7297ec6c6c543a63e70c0302a0ff" - ), + ("ot-ctl dataset set active 0e080000000000010000000300000d35060004001fffe00208d" + "ead00beef00cafe0708fd01234567890abc051000112233445566778899aabbccddeeff030a4f" + "70656e546872656164010212340410ad463152f9622c7297ec6c6c543a63e70c0302a0ff"), "ot-ctl ifconfig up", "ot-ctl thread start", "ot-ctl dataset active", # Emit @@ -246,19 +226,17 @@ def connect_to_thread_network(self): for device in self.thread_devices: # Set default openthread provisioning for cmd in otInitCommands: - self.execute_device_cmd(device["id"], cmd) + self.execute_device_cmd(device['id'], cmd) self.logger.info("Waiting for Thread network to be formed...") threadNetworkFormed = False for i in range(30): roles = list() for device in self.thread_devices: # We can only check the status of ot-agent by query its state. - reply = self.execute_device_cmd(device["id"], "ot-ctl state") - roles.append(reply["output"].split()[0]) - threadNetworkFormed = (roles.count("leader") == 1) and ( - roles.count("leader") + roles.count("router") + roles.count("child") - == len(self.thread_devices) - ) + reply = self.execute_device_cmd(device['id'], 'ot-ctl state') + roles.append(reply['output'].split()[0]) + threadNetworkFormed = (roles.count('leader') == 1) and (roles.count( + 'leader') + roles.count('router') + roles.count('child') == len(self.thread_devices)) if threadNetworkFormed: break time.sleep(1) @@ -266,46 +244,33 @@ def connect_to_thread_network(self): self.logger.info("Thread network formed") def enable_wifi_on_device(self): - ssid, psk = self.query_api("wifi_ssid_psk", [self.home_id]) + ssid, psk = self.query_api('wifi_ssid_psk', [self.home_id]) self.logger.info("wifi ap ssid: {}, psk: {}".format(ssid, psk)) for device in self.non_ap_devices: self.logger.info( "device: {} connecting to desired ssid: {}".format( - self.get_device_pretty_id(device["id"]), ssid - ) - ) - self.write_psk_to_wpa_supplicant_config(device["id"], ssid, psk) - self.kill_existing_wpa_supplicant(device["id"]) - self.start_wpa_supplicant(device["id"]) + self.get_device_pretty_id(device['id']), ssid)) + self.write_psk_to_wpa_supplicant_config(device['id'], ssid, psk) + self.kill_existing_wpa_supplicant(device['id']) + self.start_wpa_supplicant(device['id']) time.sleep(5) def get_device_thread_ip(self, device_id): - ret = self.execute_device_cmd(device_id, "ot-ctl ipaddr") + ret = self.execute_device_cmd(device_id, 'ot-ctl ipaddr') ipaddr_list = ret["output"].splitlines() for ipstr in ipaddr_list: try: self.logger.info( - "device: {} thread ip: {}".format( - self.get_device_pretty_id(device_id), ipstr - ) - ) + "device: {} thread ip: {}".format(self.get_device_pretty_id(device_id), ipstr)) ipaddr = ipaddress.ip_address(ipstr) if ipaddr.is_link_local: continue if not ipaddr.is_private: continue - if ( - re.match( - ( - "fd[0-9a-f]{2}:[0-9a-f]{4}:[0-9a-f]" - "{4}:[0-9a-f]{4}:0000:00ff:fe00:[0-9a-f]{4}" - ), - ipaddr.exploded, - ) - is not None - ): + if re.match(("fd[0-9a-f]{2}:[0-9a-f]{4}:[0-9a-f]" + "{4}:[0-9a-f]{4}:0000:00ff:fe00:[0-9a-f]{4}"), ipaddr.exploded) is not None: continue self.logger.info("Get Mesh-Local EID: {}".format(ipstr)) return str(ipaddr) @@ -315,17 +280,12 @@ def get_device_thread_ip(self, device_id): return None def get_device_log(self, device_id): - return self.query_api("device_log", [self.home_id, device_id], binary=True) + return self.query_api('device_log', [self.home_id, device_id], binary=True) def wait_for_device_output(self, device_id, pattern, timeout=1): due = time.time() + timeout while True: - if self.sequenceMatch( - self.get_device_log(device_id).decode(), - [ - pattern, - ], - ): + if self.sequenceMatch(self.get_device_log(device_id).decode(), [pattern, ]): return True if time.time() < due: time.sleep(1) @@ -334,11 +294,11 @@ def wait_for_device_output(self, device_id, pattern, timeout=1): return False def assertTrue(self, exp, note=None): - """ + ''' assert{True|False} assert(Not)Equal python unittest style functions that raise exceptions when condition not met - """ + ''' if exp is not True: if note: self.logger.error(note) @@ -365,69 +325,49 @@ def assertNotEqual(self, val1, val2, note=None): def _build_request_url(self, end_point, args=[]): if len(args) == 0: return urljoin(self.cirque_url, end_point) - return urljoin( - self.cirque_url, - "{}/{}".format(end_point, "/".join([str(argv) for argv in args])), - ) + return urljoin(self.cirque_url, "{}/{}".format(end_point, '/'.join([str(argv) for argv in args]))) def destroy_home(self): self.logger.info("destroying home: {}".format(self.home_id)) - self.query_api("destroy_home", [self.home_id]) + self.query_api('destroy_home', [self.home_id]) def initialize_home(self): home_id = requests.post( - self._build_request_url("create_home"), json=self.device_config - ).json() + self._build_request_url('create_home'), json=self.device_config).json() self.logger.info("home id: {} created!".format(home_id)) - self.assertTrue( - home_id in list(self.query_api("get_homes")), - "created home_id did not match id from get_homes!!", - ) + self.assertTrue(home_id in + list(self.query_api('get_homes')), + "created home_id did not match id from get_homes!!") self.home_id = home_id device_types = set() - created_devices = self.query_api("home_devices", [home_id]) + created_devices = self.query_api('home_devices', [home_id]) - self.logger.info( - "home id: {} devices: {}".format( - home_id, json.dumps(created_devices, indent=4, sort_keys=True) - ) - ) + self.logger.info("home id: {} devices: {}".format( + home_id, json.dumps(created_devices, indent=4, sort_keys=True))) for device in created_devices.values(): - device_types.add(device["type"]) + device_types.add(device['type']) wanted_device_types = set() for device in self.device_config.values(): - wanted_device_types.add(device["type"]) + wanted_device_types.add(device['type']) - self.assertEqual( - device_types, - wanted_device_types, - "created device does not match to device config!!", - ) + self.assertEqual(device_types, wanted_device_types, + "created device does not match to device config!!") self.device_config = created_devices self.device_ids = [device_id for device_id in self.device_config] - self.non_ap_devices = [ - device - for device in self.device_config.values() - if device["type"] != "wifi_ap" - ] - self.thread_devices = [ - device - for device in self.device_config.values() - if device["capability"].get("Thread", None) is not None - ] - self.ap_devices = [ - device - for device in self.device_config.values() - if device["type"] == "wifi_ap" - ] + self.non_ap_devices = [device for device in self.device_config.values() + if device['type'] != 'wifi_ap'] + self.thread_devices = [device for device in self.device_config.values() + if device['capability'].get('Thread', None) is not None] + self.ap_devices = [device for device in self.device_config.values() + if device['type'] == 'wifi_ap'] def save_device_logs(self): timestamp = int(time.time()) @@ -436,61 +376,47 @@ def save_device_logs(self): os.makedirs("logs") for device in self.non_ap_devices: - ret_log = self.get_device_log(device["id"]) + ret_log = self.get_device_log(device['id']) # Use this format for easier sort - f_name = "{}-{}-{}.log".format(device["type"], timestamp, device["id"][:8]) + f_name = '{}-{}-{}.log'.format(device['type'], + timestamp, device['id'][:8]) self.logger.debug("device log name: \n{}".format(f_name)) - with open(os.path.join(log_dir, f_name), "wb") as fp: + with open(os.path.join(log_dir, f_name), 'wb') as fp: fp.write(ret_log) def start_wpa_supplicant(self, device_id): - self.logger.info( - "device: {}: starting wpa_supplicant on device".format( - self.get_device_pretty_id(device_id) - ) - ) + self.logger.info("device: {}: starting wpa_supplicant on device" + .format(self.get_device_pretty_id(device_id))) start_wpa_supplicant_command = "".join( - [ - "wpa_supplicant -B -i wlan0 ", - "-c /etc/wpa_supplicant/wpa_supplicant.conf ", - "-f /var/log/wpa_supplicant.log -t -dd", - ] - ) + ["wpa_supplicant -B -i wlan0 ", + "-c /etc/wpa_supplicant/wpa_supplicant.conf ", + "-f /var/log/wpa_supplicant.log -t -dd"]) return self.execute_device_cmd(device_id, start_wpa_supplicant_command) def write_psk_to_wpa_supplicant_config(self, device_id, ssid, psk): - self.logger.info( - "device: {}: writing ssid, psk to wpa_supplicant config".format( - self.get_device_pretty_id(device_id) - ) - ) + self.logger.info("device: {}: writing ssid, psk to wpa_supplicant config" + .format(self.get_device_pretty_id(device_id))) write_psk_command = "".join( - [ - "sh -c 'wpa_passphrase {} {} >> ".format(ssid, psk), - "/etc/wpa_supplicant/wpa_supplicant.conf'", - ] - ) + ["sh -c 'wpa_passphrase {} {} >> ".format(ssid, psk), + "/etc/wpa_supplicant/wpa_supplicant.conf'"]) return self.execute_device_cmd(device_id, write_psk_command) def kill_existing_wpa_supplicant(self, device_id): - self.logger.info( - "device: {}: kill existing wpa_supplicant".format( - self.get_device_pretty_id(device_id) - ) - ) + self.logger.info("device: {}: kill existing wpa_supplicant" + .format(self.get_device_pretty_id(device_id))) - kill_wpa_supplicant_command = "killall wpa_supplicant" + kill_wpa_supplicant_command = 'killall wpa_supplicant' return self.execute_device_cmd(device_id, kill_wpa_supplicant_command) def get_device_pretty_name(self, device_id): device_obj = self.device_config.get(device_id, None) if device_obj is not None: - return device_obj["type"] + return device_obj['type'] return "" def get_device_pretty_id(self, device_id): @@ -498,6 +424,4 @@ def get_device_pretty_id(self, device_id): @property def default_base_image(cls): - return os.environ.get( - "CHIP_CIRQUE_BASE_IMAGE", "project-chip/chip-cirque-device-base" - ) + return os.environ.get("CHIP_CIRQUE_BASE_IMAGE", "project-chip/chip-cirque-device-base") From bab965a5fc8cc1f728f9108e5606377adfb63b1d Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 9 Oct 2024 09:07:20 -0400 Subject: [PATCH 24/26] Update src/app/CommandHandlerImpl.cpp Co-authored-by: Boris Zbarsky --- src/app/CommandHandlerImpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index a57d9395968174..8c875c8924ecd4 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -524,7 +524,7 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); if (preCheckStatus != Status::Success) { - // Command failed for a specific path, keep trying the rest of the commands still + // Command failed for a specific path, but keep trying the rest of the paths. continue; } } From 38e3bdb6baf5fdd421bc8e0203b8e6d10138e277 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Wed, 9 Oct 2024 09:11:54 -0400 Subject: [PATCH 25/26] Remove separate member for accessing fabric index --- src/app/CommandHandlerImpl.cpp | 2 -- src/app/InteractionModelEngine.cpp | 2 +- src/app/data-model-provider/OperationTypes.h | 9 ++++++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index 8c875c8924ecd4..2d8e76441bb083 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -395,7 +395,6 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand request.path = concretePath; request.subjectDescriptor = GetSubjectDescriptor(); - request.accessingFabricIndex = GetAccessingFabricIndex(); request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); @@ -518,7 +517,6 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo request.path = concretePath; request.subjectDescriptor = GetSubjectDescriptor(); - request.accessingFabricIndex = GetAccessingFabricIndex(); request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 770f5386eabe70..5f0e392c26177a 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -1763,7 +1763,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::CheckCommandFlags(co // Fabric-scoped commands are not allowed before a specific accessing fabric is available. // This is mostly just during a PASE session before AddNOC. - if (aRequest.accessingFabricIndex == kUndefinedFabricIndex) + if (aRequest.GetAccessingFabricIndex() == kUndefinedFabricIndex) { return Status::UnsupportedAccess; } diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h index b13ec8f224b4fe..bef7b6f3f3281c 100644 --- a/src/app/data-model-provider/OperationTypes.h +++ b/src/app/data-model-provider/OperationTypes.h @@ -56,6 +56,14 @@ struct OperationRequest /// /// NOTE: once kInternal flag is removed, this will become non-optional std::optional subjectDescriptor; + + /// Accessing fabric index is the subjectDescriptor fabric index (if any). + /// This is a readability convenience function. + /// + /// Returns kUndefinedFabricIndex if no subject descriptor is available + FabricIndex GetAccessingFabricIndex() const { + return subjectDescriptor.has_value() ? subjectDescriptor->fabricIndex : kUndefinedFabricIndex; + } }; enum class ReadFlags : uint32_t @@ -100,7 +108,6 @@ struct InvokeRequest : OperationRequest { ConcreteCommandPath path; BitFlags invokeFlags; - FabricIndex accessingFabricIndex = kUndefinedFabricIndex; }; } // namespace DataModel From e497eecafa3286b239b817afe25e80d50e1be46c Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Wed, 9 Oct 2024 13:12:31 +0000 Subject: [PATCH 26/26] Restyled by clang-format --- src/app/CommandHandlerImpl.cpp | 8 ++++---- src/app/data-model-provider/OperationTypes.h | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/app/CommandHandlerImpl.cpp b/src/app/CommandHandlerImpl.cpp index 2d8e76441bb083..2142af63494348 100644 --- a/src/app/CommandHandlerImpl.cpp +++ b/src/app/CommandHandlerImpl.cpp @@ -393,8 +393,8 @@ Status CommandHandlerImpl::ProcessCommandDataIB(CommandDataIB::Parser & aCommand { DataModel::InvokeRequest request; - request.path = concretePath; - request.subjectDescriptor = GetSubjectDescriptor(); + request.path = concretePath; + request.subjectDescriptor = GetSubjectDescriptor(); request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); @@ -515,8 +515,8 @@ Status CommandHandlerImpl::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo { DataModel::InvokeRequest request; - request.path = concretePath; - request.subjectDescriptor = GetSubjectDescriptor(); + request.path = concretePath; + request.subjectDescriptor = GetSubjectDescriptor(); request.invokeFlags.Set(DataModel::InvokeFlags::kTimed, IsTimedInvoke()); Status preCheckStatus = mpCallback->ValidateCommandCanBeDispatched(request); diff --git a/src/app/data-model-provider/OperationTypes.h b/src/app/data-model-provider/OperationTypes.h index bef7b6f3f3281c..6bfffccc655f00 100644 --- a/src/app/data-model-provider/OperationTypes.h +++ b/src/app/data-model-provider/OperationTypes.h @@ -61,7 +61,8 @@ struct OperationRequest /// This is a readability convenience function. /// /// Returns kUndefinedFabricIndex if no subject descriptor is available - FabricIndex GetAccessingFabricIndex() const { + FabricIndex GetAccessingFabricIndex() const + { return subjectDescriptor.has_value() ? subjectDescriptor->fabricIndex : kUndefinedFabricIndex; } };