diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 600499eefdbf97..476a080c1c801e 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -28,6 +28,7 @@ #include "messaging/ExchangeContext.h" #include +#include #include #include #include @@ -257,7 +258,7 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand VerifyOrExit(mpExchangeCtx != nullptr && mpExchangeCtx->HasSessionHandle(), err = CHIP_ERROR_INCORRECT_STATE); { - Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle()->GetSubjectDescriptor(); + Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); @@ -273,11 +274,18 @@ CHIP_ERROR CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommand { return AddStatus(concretePath, Protocols::InteractionModel::Status::Failure); } - // TODO: when wildcard/group invokes are supported, handle them to discard rather than fail with status + // TODO: when wildcard invokes are supported, handle them to discard rather than fail with status return AddStatus(concretePath, Protocols::InteractionModel::Status::UnsupportedAccess); } } + if (CommandNeedsTimedInvoke(concretePath.mClusterId, concretePath.mCommandId) && !IsTimedInvoke()) + { + // TODO: when wildcard invokes are supported, discard a + // wildcard-expanded path instead of returning a status. + return AddStatus(concretePath, Protocols::InteractionModel::Status::NeedsTimedInteraction); + } + err = aCommandElement.GetData(&commandDataReader); if (CHIP_END_OF_TLV == err) { @@ -359,6 +367,17 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo } SuccessOrExit(err); + // Per spec, we do the "is this a timed command?" check for every path, but + // since all paths that fail it just get silently discarded we can do it + // 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 (CommandNeedsTimedInvoke(clusterId, commandId)) + { + // Group commands are never timed. + ExitNow(); + } + iterator = groupDataProvider->IterateEndpoints(fabric); VerifyOrExit(iterator != nullptr, err = CHIP_ERROR_NO_MEMORY); @@ -384,7 +403,7 @@ CHIP_ERROR CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aCo } { - Access::SubjectDescriptor subjectDescriptor = mpExchangeCtx->GetSessionHandle()->GetSubjectDescriptor(); + Access::SubjectDescriptor subjectDescriptor = GetSubjectDescriptor(); Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId }; Access::Privilege requestPrivilege = RequiredPrivilege::ForInvokeCommand(concretePath); err = Access::GetAccessControl().Check(subjectDescriptor, requestPath, requestPrivilege); diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 6ca978990ae72f..c283a5177cffab 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -581,9 +581,6 @@ void InteractionModelEngine::DispatchCommand(CommandHandler & apCommandObj, cons if (handler) { - // TODO: Figure out who is responsible for handling checking - // apCommandObj->IsTimedInvoke() for commands that require a timed - // invoke and have a CommandHandlerInterface handling them. CommandHandlerInterface::HandlerContext context(apCommandObj, aCommandPath, apPayload); handler->InvokeCommand(context); diff --git a/src/app/zap-templates/partials/im_command_handler_cluster_commands.zapt b/src/app/zap-templates/partials/im_command_handler_cluster_commands.zapt index 979f01f9d06235..9053c1768b7de4 100644 --- a/src/app/zap-templates/partials/im_command_handler_cluster_commands.zapt +++ b/src/app/zap-templates/partials/im_command_handler_cluster_commands.zapt @@ -1,10 +1,4 @@ {{#if (isServer parent.clusterSide)}} -{{#if mustUseTimedInvoke}} -if (!apCommandObj->IsTimedInvoke()) { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; -} -{{/if}} Commands::{{asUpperCamelCase commandName}}::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) { diff --git a/src/app/zap-templates/templates/app/cluster-objects-src.zapt b/src/app/zap-templates/templates/app/cluster-objects-src.zapt index 0e61078026453d..f966be83caa0c3 100644 --- a/src/app/zap-templates/templates/app/cluster-objects-src.zapt +++ b/src/app/zap-templates/templates/app/cluster-objects-src.zapt @@ -86,7 +86,7 @@ CHIP_ERROR TypeInfo::DecodableType::Decode(TLV::TLVReader &reader, const Concret return CHIP_NO_ERROR; } -} +} // namespace Attributes namespace Events { {{#zcl_events}} @@ -126,11 +126,42 @@ CHIP_ERROR DecodableType::Decode(TLV::TLVReader &reader) { } } // namespace {{asUpperCamelCase name}}. {{/zcl_events}} -} // namespace Commands +} // namespace Events } // namespace {{asUpperCamelCase name}} {{/zcl_clusters}} } // namespace Clusters + +bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand) +{ + // Maybe it would be smaller code to codegen a table and walk over it? + // Not sure. + switch (aCluster) + { + {{#zcl_clusters}} + {{#zcl_commands_that_need_timed_invoke}} + {{#first}} + case Clusters::{{asUpperCamelCase parent.name}}::Id: + { + switch (aCommand) { + {{/first}} + case Clusters::{{asUpperCamelCase parent.name}}::Commands::{{asUpperCamelCase name}}::Id: + {{#last}} + return true; + default: + return false; + } + } + {{/last}} + {{/zcl_commands_that_need_timed_invoke}} + {{/zcl_clusters}} + default: + break; + } + + return false; +} + } // namespace app } // namespace chip diff --git a/src/app/zap-templates/templates/app/cluster-objects.zapt b/src/app/zap-templates/templates/app/cluster-objects.zapt index 5e1bd2be360b96..83be4395d76640 100644 --- a/src/app/zap-templates/templates/app/cluster-objects.zapt +++ b/src/app/zap-templates/templates/app/cluster-objects.zapt @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -244,5 +245,8 @@ public: {{/zcl_clusters}} } // namespace Clusters + +bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand); + } // namespace app } // namespace chip diff --git a/src/app/zap-templates/templates/app/helper.js b/src/app/zap-templates/templates/app/helper.js index a22f1dd73a9de9..8ae85a45b39867 100644 --- a/src/app/zap-templates/templates/app/helper.js +++ b/src/app/zap-templates/templates/app/helper.js @@ -823,6 +823,16 @@ async function zcl_events_fields_by_event_name(name, options) return templateUtil.templatePromise(this.global, promise) } +// Must be used inside zcl_clusters +async function zcl_commands_that_need_timed_invoke(options) +{ + const { db } = this.global; + let packageId = await templateUtil.ensureZclPackageId(this); + let commands = await queryCommand.selectCommandsByClusterId(db, this.id, packageId); + commands = commands.filter(cmd => cmd.mustUseTimedInvoke); + return templateUtil.collectBlocks(commands, options, this); +} + // // Module exports // @@ -846,3 +856,4 @@ exports.isWeaklyTypedEnum = isWeaklyTypedEnum; exports.getPythonFieldDefault = getPythonFieldDefault; exports.incrementDepth = incrementDepth; exports.zcl_events_fields_by_event_name = zcl_events_fields_by_event_name; +exports.zcl_commands_that_need_timed_invoke = zcl_commands_that_need_timed_invoke; diff --git a/zzz_generated/all-clusters-app/zap-generated/IMClusterCommandHandler.cpp b/zzz_generated/all-clusters-app/zap-generated/IMClusterCommandHandler.cpp index d632c304e0a9f0..b75c6ce3372fb7 100644 --- a/zzz_generated/all-clusters-app/zap-generated/IMClusterCommandHandler.cpp +++ b/zzz_generated/all-clusters-app/zap-generated/IMClusterCommandHandler.cpp @@ -51,11 +51,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP switch (aCommandPath.mCommandId) { case Commands::OpenBasicCommissioningWindow::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::OpenBasicCommissioningWindow::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -66,11 +61,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::OpenCommissioningWindow::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::OpenCommissioningWindow::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -81,11 +71,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::RevokeCommissioning::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::RevokeCommissioning::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -453,11 +438,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP switch (aCommandPath.mCommandId) { case Commands::ClearCredential::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::ClearCredential::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -467,11 +447,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::ClearUser::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::ClearUser::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -499,11 +474,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::LockDoor::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::LockDoor::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -513,11 +483,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::SetCredential::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::SetCredential::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -527,11 +492,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::SetUser::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::SetUser::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -541,11 +501,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::UnlockDoor::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::UnlockDoor::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) @@ -1692,11 +1647,6 @@ void DispatchServerCommand(CommandHandler * apCommandObj, const ConcreteCommandP break; } case Commands::TimedInvokeRequest::Id: { - if (!apCommandObj->IsTimedInvoke()) - { - apCommandObj->AddStatus(aCommandPath, Protocols::InteractionModel::Status::NeedsTimedInteraction); - return; - } Commands::TimedInvokeRequest::DecodableType commandData; TLVError = DataModel::Decode(aDataTlv, commandData); if (TLVError == CHIP_NO_ERROR) diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp index ac0358cc04db15..d93e33b27636fd 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.cpp @@ -26193,5 +26193,71 @@ namespace Events { } // namespace ElectricalMeasurement } // namespace Clusters + +bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand) +{ + // Maybe it would be smaller code to codegen a table and walk over it? + // Not sure. + switch (aCluster) + { + case Clusters::AdministratorCommissioning::Id: { + switch (aCommand) + { + case Clusters::AdministratorCommissioning::Commands::OpenCommissioningWindow::Id: + case Clusters::AdministratorCommissioning::Commands::OpenBasicCommissioningWindow::Id: + case Clusters::AdministratorCommissioning::Commands::RevokeCommissioning::Id: + return true; + default: + return false; + } + } + case Clusters::DoorLock::Id: { + switch (aCommand) + { + case Clusters::DoorLock::Commands::LockDoor::Id: + case Clusters::DoorLock::Commands::UnlockDoor::Id: + case Clusters::DoorLock::Commands::UnlockWithTimeout::Id: + case Clusters::DoorLock::Commands::SetPINCode::Id: + case Clusters::DoorLock::Commands::ClearPINCode::Id: + case Clusters::DoorLock::Commands::ClearAllPINCodes::Id: + case Clusters::DoorLock::Commands::SetRFIDCode::Id: + case Clusters::DoorLock::Commands::ClearRFIDCode::Id: + case Clusters::DoorLock::Commands::ClearAllRFIDCodes::Id: + case Clusters::DoorLock::Commands::SetUser::Id: + case Clusters::DoorLock::Commands::ClearUser::Id: + case Clusters::DoorLock::Commands::SetCredential::Id: + case Clusters::DoorLock::Commands::ClearCredential::Id: + return true; + default: + return false; + } + } + case Clusters::AccountLogin::Id: { + switch (aCommand) + { + case Clusters::AccountLogin::Commands::GetSetupPINRequest::Id: + case Clusters::AccountLogin::Commands::LoginRequest::Id: + case Clusters::AccountLogin::Commands::LogoutRequest::Id: + return true; + default: + return false; + } + } + case Clusters::TestCluster::Id: { + switch (aCommand) + { + case Clusters::TestCluster::Commands::TimedInvokeRequest::Id: + return true; + default: + return false; + } + } + default: + break; + } + + return false; +} + } // namespace app } // namespace chip diff --git a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h index 33a6c8e9f14bd4..fbda8bc997e4a1 100644 --- a/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h +++ b/zzz_generated/app-common/app-common/zap-generated/cluster-objects.h @@ -24,6 +24,8 @@ #include #include #include +#include +#include #include #include #include @@ -43269,5 +43271,8 @@ struct TypeInfo } // namespace ElectricalMeasurement } // namespace Clusters + +bool CommandNeedsTimedInvoke(ClusterId aCluster, CommandId aCommand); + } // namespace app } // namespace chip