Skip to content

Commit

Permalink
[IM] Make CommandHandler::AddResponseData atomic (#15906)
Browse files Browse the repository at this point in the history
* Add encode failure tests

* [IM] Make CommandHandler::AddResponseData atomic

* Reset -> Rollback

* Rename: AddResponseData -> AddResponse

* Address comments

* Update cluster code

* 50 ms is too short on CI

* Fix

* Fix
erjiaqing authored and pull[bot] committed Nov 29, 2023
1 parent 088f01c commit 42aef61
Showing 17 changed files with 224 additions and 129 deletions.
Original file line number Diff line number Diff line change
@@ -282,8 +282,7 @@ CHIP_ERROR OTAProviderExample::SendQueryImageResponse(chip::app::CommandHandler
response.metadataForRequestor.Emplace(chip::ByteSpan());
}

ReturnErrorOnFailure(commandObj->AddResponseData(commandPath, response));

commandObj->AddResponse(commandPath, response);
return CHIP_NO_ERROR;
}

@@ -390,7 +389,7 @@ EmberAfStatus OTAProviderExample::HandleApplyUpdateRequest(chip::app::CommandHan
// Reset back to success case for subsequent uses
mUpdateAction = OTAApplyUpdateAction::kProceed;

VerifyOrReturnError(commandObj->AddResponseData(commandPath, response) == CHIP_NO_ERROR, EMBER_ZCL_STATUS_FAILURE);
commandObj->AddResponse(commandPath, response);

return EMBER_ZCL_STATUS_SUCCESS;
}
18 changes: 18 additions & 0 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
@@ -481,10 +481,13 @@ CHIP_ERROR CommandHandler::AddClusterSpecificFailure(const ConcreteCommandPath &
CHIP_ERROR CommandHandler::PrepareCommand(const ConcreteCommandPath & aCommandPath, bool aStartDataStruct)
{
ReturnErrorOnFailure(AllocateBuffer());

mInvokeResponseBuilder.Checkpoint(mBackupWriter);
//
// We must not be in the middle of preparing a command, or having prepared or sent one.
//
VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE);
MoveToState(State::Preparing);
InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses();
InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse();
ReturnErrorOnFailure(invokeResponses.GetError());
@@ -526,6 +529,7 @@ CHIP_ERROR CommandHandler::PrepareStatus(const ConcreteCommandPath & aCommandPat
// We must not be in the middle of preparing a command, or having prepared or sent one.
//
VerifyOrReturnError(mState == State::Idle, CHIP_ERROR_INCORRECT_STATE);
MoveToState(State::Preparing);
InvokeResponseIBs::Builder & invokeResponses = mInvokeResponseBuilder.GetInvokeResponses();
InvokeResponseIB::Builder & invokeResponse = invokeResponses.CreateInvokeResponse();
ReturnErrorOnFailure(invokeResponses.GetError());
@@ -550,6 +554,17 @@ CHIP_ERROR CommandHandler::FinishStatus()
return CHIP_NO_ERROR;
}

CHIP_ERROR CommandHandler::RollbackResponse()
{
VerifyOrReturnError(mState == State::Preparing || mState == State::AddingCommand, CHIP_ERROR_INCORRECT_STATE);
mInvokeResponseBuilder.Rollback(mBackupWriter);
mInvokeResponseBuilder.ResetError();
// Note: We only support one command per request, so we reset the state to Idle here, need to review the states when adding
// supports of having multiple requests in the same transaction.
MoveToState(State::Idle);
return CHIP_NO_ERROR;
}

TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter()
{
if (mState != State::AddingCommand)
@@ -607,6 +622,9 @@ const char * CommandHandler::GetStateStr() const
case State::Idle:
return "Idle";

case State::Preparing:
return "Preparing";

case State::AddingCommand:
return "AddingCommand";

68 changes: 62 additions & 6 deletions src/app/CommandHandler.h
Original file line number Diff line number Diff line change
@@ -175,13 +175,42 @@ class CommandHandler
template <typename CommandData>
CHIP_ERROR AddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() };
ReturnErrorOnFailure(PrepareCommand(path, false));
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)), aData));
// TryAddResponseData will ensure we are in the correct state when calling AddResponseData.
CHIP_ERROR err = TryAddResponseData(aRequestCommandPath, aData);
if (err != CHIP_NO_ERROR)
{
// The state guarantees that either we can rollback or we don't have to rollback the buffer, so we don't care about the
// return value of RollbackResponse.
RollbackResponse();
}
return err;
}

return FinishCommand(/* aEndDataStruct = */ false);
/**
* API for adding a response. This will try to encode a data response (response command), and if that fails will encode a a
* Protocols::InteractionModel::Status::Failure status response instead.
*
* The template parameter T is generally expected to be a ClusterName::Commands::CommandName::Type struct, but any object that
* can be encoded using the DataModel::Encode machinery and exposes the right command id will work.
*
* Since the function will call AddStatus when it fails to encode the data, it cannot send any response when it fails to encode
* a status code since another AddStatus call will also fail. The error from AddStatus will just be logged.
*
* @param [in] aRequestCommandPath the concrete path of the command we are
* responding to.
* @param [in] aData the data for the response.
*/
template <typename CommandData>
void AddResponse(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
if (CHIP_NO_ERROR != AddResponseData(aRequestCommandPath, aData))
{
CHIP_ERROR err = AddStatus(aRequestCommandPath, Protocols::InteractionModel::Status::Failure);
if (err != CHIP_NO_ERROR)
{
ChipLogError(DataManagement, "Failed to encode status: %" CHIP_ERROR_FORMAT, err.Format());
}
}
}

/**
@@ -207,6 +236,7 @@ class CommandHandler
enum class State
{
Idle, ///< Default state that the object starts out in, where no work has commenced
Preparing, ///< We are prepaing the command or status header.
AddingCommand, ///< In the process of adding a command.
AddedCommand, ///< A command has been completely encoded and is awaiting transmission.
CommandSent, ///< The command has been sent successfully.
@@ -216,6 +246,11 @@ class CommandHandler
void MoveToState(const State aTargetState);
const char * GetStateStr() const;

/**
* Rollback the state to before encoding the current ResponseData (before calling PrepareCommand / PrepareStatus)
*/
CHIP_ERROR RollbackResponse();

/*
* This forcibly closes the exchange context if a valid one is pointed to. Such a situation does
* not arise during normal message processing flows that all normally call Close() above. This can only
@@ -270,6 +305,26 @@ class CommandHandler
CHIP_ERROR AddStatusInternal(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
const Optional<ClusterStatus> & aClusterStatus);

/**
* If this function fails, it may leave our TLV buffer in an inconsistent state. Callers should snapshot as needed before
* calling this function, and roll back as needed afterward.
*
* @param [in] aRequestCommandPath the concrete path of the command we are
* responding to.
* @param [in] aData the data for the response.
*/
template <typename CommandData>
CHIP_ERROR TryAddResponseData(const ConcreteCommandPath & aRequestCommandPath, const CommandData & aData)
{
ConcreteCommandPath path = { aRequestCommandPath.mEndpointId, aRequestCommandPath.mClusterId, CommandData::GetCommandId() };
ReturnErrorOnFailure(PrepareCommand(path, false));
TLV::TLVWriter * writer = GetCommandDataIBTLVWriter();
VerifyOrReturnError(writer != nullptr, CHIP_ERROR_INCORRECT_STATE);
ReturnErrorOnFailure(DataModel::Encode(*writer, TLV::ContextTag(to_underlying(CommandDataIB::Tag::kData)), aData));

return FinishCommand(/* aEndDataStruct = */ false);
}

Messaging::ExchangeContext * mpExchangeCtx = nullptr;
Callback * mpCallback = nullptr;
InvokeResponseMessage::Builder mInvokeResponseBuilder;
@@ -280,6 +335,7 @@ class CommandHandler

State mState = State::Idle;
chip::System::PacketBufferTLVWriter mCommandMessageWriter;
TLV::TLVWriter mBackupWriter;
bool mBufferAllocated = false;
};

9 changes: 3 additions & 6 deletions src/app/CommandResponseHelper.h
Original file line number Diff line number Diff line change
@@ -36,12 +36,9 @@ class CommandResponseHelper

CHIP_ERROR Success(const CommandData & aResponse)
{
CHIP_ERROR err = mCommandHandler->AddResponseData(mCommandPath, aResponse);
if (err == CHIP_NO_ERROR)
{
mSentResponse = true;
}
return err;
mCommandHandler->AddResponse(mCommandPath, aResponse);
mSentResponse = true;
return CHIP_NO_ERROR;
};

CHIP_ERROR Success(ClusterStatus aClusterStatus)
Original file line number Diff line number Diff line change
@@ -45,7 +45,7 @@ void DiagnosticLogsCommandHandler::InvokeCommand(HandlerContext & handlerContext
if (mBuffer.IsEmpty())
{
response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kNoLogs;
handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response);
handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response);
break;
}

@@ -58,7 +58,7 @@ void DiagnosticLogsCommandHandler::InvokeCommand(HandlerContext & handlerContext
if (!buf)
{
response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kBusy;
handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response);
handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response);
break;
}

@@ -71,19 +71,19 @@ void DiagnosticLogsCommandHandler::InvokeCommand(HandlerContext & handlerContext
response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kSuccess;
response.content = chip::ByteSpan(buf.get() + sizeof(timeMs), logSize - sizeof(timeMs));
response.timeStamp = timeMs;
handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response);
handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response);
}
break;
case chip::app::Clusters::DiagnosticLogs::LogsIntent::kNetworkDiag: {
chip::app::Clusters::DiagnosticLogs::Commands::RetrieveLogsResponse::Type response;
response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kNoLogs;
handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response);
handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response);
}
break;
case chip::app::Clusters::DiagnosticLogs::LogsIntent::kCrashLogs: {
chip::app::Clusters::DiagnosticLogs::Commands::RetrieveLogsResponse::Type response;
response.status = chip::app::Clusters::DiagnosticLogs::LogsStatus::kNoLogs;
handlerContext.mCommandHandler.AddResponseData(handlerContext.mRequestPath, response);
handlerContext.mCommandHandler.AddResponse(handlerContext.mRequestPath, response);
}
break;
}
21 changes: 10 additions & 11 deletions src/app/clusters/door-lock-server/door-lock-server.cpp
Original file line number Diff line number Diff line change
@@ -2169,11 +2169,10 @@ DlStatus DoorLockServer::clearSchedules(chip::EndpointId endpointId, uint16_t us
return DlStatus::kSuccess;
}

CHIP_ERROR DoorLockServer::sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj,
const chip::app::ConcreteCommandPath & commandPath, uint8_t weekdayIndex,
uint16_t userIndex, DlStatus status, DlDaysMaskMap daysMask,
uint8_t startHour, uint8_t startMinute, uint8_t endHour,
uint8_t endMinute)
void DoorLockServer::sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj,
const chip::app::ConcreteCommandPath & commandPath, uint8_t weekdayIndex,
uint16_t userIndex, DlStatus status, DlDaysMaskMap daysMask, uint8_t startHour,
uint8_t startMinute, uint8_t endHour, uint8_t endMinute)
{
VerifyOrDie(nullptr != commandObj);

@@ -2190,7 +2189,7 @@ CHIP_ERROR DoorLockServer::sendGetWeekDayScheduleResponse(chip::app::CommandHand
response.endMinute = Optional<uint8_t>(endMinute);
}

return commandObj->AddResponseData(commandPath, response);
commandObj->AddResponse(commandPath, response);
}

bool DoorLockServer::yearDayIndexValid(chip::EndpointId endpointId, uint8_t yearDayIndex)
@@ -2242,10 +2241,10 @@ DlStatus DoorLockServer::clearYearDaySchedules(chip::EndpointId endpointId, uint
return DlStatus::kSuccess;
}

CHIP_ERROR DoorLockServer::sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj,
const chip::app::ConcreteCommandPath & commandPath, uint8_t yearDayIndex,
uint16_t userIndex, DlStatus status, uint32_t localStartTime,
uint32_t localEndTime)
void DoorLockServer::sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj,
const chip::app::ConcreteCommandPath & commandPath, uint8_t yearDayIndex,
uint16_t userIndex, DlStatus status, uint32_t localStartTime,
uint32_t localEndTime)
{
VerifyOrDie(nullptr != commandObj);

@@ -2259,7 +2258,7 @@ CHIP_ERROR DoorLockServer::sendGetYearDayScheduleResponse(chip::app::CommandHand
response.localEndTime = Optional<uint32_t>(localEndTime);
}

return commandObj->AddResponseData(commandPath, response);
commandObj->AddResponse(commandPath, response);
}

EmberAfStatus DoorLockServer::clearCredential(chip::EndpointId endpointId, chip::FabricIndex modifier, chip::NodeId sourceNodeId,
16 changes: 7 additions & 9 deletions src/app/clusters/door-lock-server/door-lock-server.h
Original file line number Diff line number Diff line change
@@ -238,21 +238,19 @@ class DoorLockServer
DlStatus clearWeekDaySchedules(chip::EndpointId endpointId, uint16_t userIndex);
DlStatus clearSchedules(chip::EndpointId endpointId, uint16_t userIndex);

CHIP_ERROR sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj,
const chip::app::ConcreteCommandPath & commandPath, uint8_t weekdayIndex,
uint16_t userIndex, DlStatus status, DlDaysMaskMap daysMask = DlDaysMaskMap(0),
uint8_t startHour = 0, uint8_t startMinute = 0, uint8_t endHour = 0,
uint8_t endMinute = 0);
void sendGetWeekDayScheduleResponse(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
uint8_t weekdayIndex, uint16_t userIndex, DlStatus status,
DlDaysMaskMap daysMask = DlDaysMaskMap(0), uint8_t startHour = 0, uint8_t startMinute = 0,
uint8_t endHour = 0, uint8_t endMinute = 0);

bool yearDayIndexValid(chip::EndpointId endpointId, uint8_t yearDayIndex);

DlStatus clearYearDaySchedule(chip::EndpointId endpointId, uint16_t userIndex, uint8_t weekDayIndex);
DlStatus clearYearDaySchedules(chip::EndpointId endpointId, uint16_t userIndex);

CHIP_ERROR sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj,
const chip::app::ConcreteCommandPath & commandPath, uint8_t yearDayIndex,
uint16_t userIndex, DlStatus status, uint32_t localStartTime = 0,
uint32_t localEndTime = 0);
void sendGetYearDayScheduleResponse(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
uint8_t yearDayIndex, uint16_t userIndex, DlStatus status, uint32_t localStartTime = 0,
uint32_t localEndTime = 0);

bool sendRemoteLockUserChange(chip::EndpointId endpointId, DlLockDataType dataType, DlDataOperationType operation,
chip::NodeId nodeId, chip::FabricIndex fabricIndex, uint16_t userIndex = 0,
Original file line number Diff line number Diff line change
@@ -151,12 +151,12 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
CheckSuccess(failSafeContext.ArmFailSafe(accessingFabricIndex, System::Clock::Seconds16(commandData.expiryLengthSeconds)),
Failure);
response.errorCode = CommissioningError::kOk;
CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure);
commandObj->AddResponse(commandPath, response);
}
else
{
response.errorCode = CommissioningError::kBusyWithOtherAdmin;
CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure);
commandObj->AddResponse(commandPath, response);
}

return true;
@@ -201,7 +201,7 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
}
}

CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure);
commandObj->AddResponse(commandPath, response);

return true;
}
@@ -218,7 +218,7 @@ bool emberAfGeneralCommissioningClusterSetRegulatoryConfigCallback(app::CommandH

Commands::SetRegulatoryConfigResponse::Type response;
response.errorCode = CommissioningError::kOk;
CheckSuccess(commandObj->AddResponseData(commandPath, response), Failure);
commandObj->AddResponse(commandPath, response);

return true;
}
13 changes: 2 additions & 11 deletions src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp
Original file line number Diff line number Diff line change
@@ -443,12 +443,7 @@ bool emberAfGroupKeyManagementClusterKeySetReadCallback(
}
response.groupKeySet.epochKey2.SetNull();

CHIP_ERROR err = commandObj->AddResponseData(commandPath, response);
if (CHIP_NO_ERROR != err)
{
ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetRead failed: %s", ErrorStr(err));
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
}
commandObj->AddResponse(commandPath, response);
return true;
}

@@ -535,11 +530,7 @@ bool emberAfGroupKeyManagementClusterKeySetReadAllIndicesCallback(
return true;
}

CHIP_ERROR err = commandObj->AddResponseData(commandPath, KeySetReadAllIndicesResponse(keysIt));
if (CHIP_NO_ERROR != err)
{
ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetReadAllIndices failed: %s", ErrorStr(err));
}
commandObj->AddResponse(commandPath, KeySetReadAllIndicesResponse(keysIt));
keysIt->Release();
return true;
}
28 changes: 5 additions & 23 deletions src/app/clusters/groups-server/groups-server.cpp
Original file line number Diff line number Diff line change
@@ -137,7 +137,6 @@ bool emberAfGroupsClusterAddGroupCallback(app::CommandHandler * commandObj, cons
{
auto fabricIndex = commandObj->GetAccessingFabricIndex();
Groups::Commands::AddGroupResponse::Type response;
CHIP_ERROR err = CHIP_NO_ERROR;

// For all networks, Add Group commands are only responded to when
// they are addressed to a single device.
@@ -148,12 +147,7 @@ bool emberAfGroupsClusterAddGroupCallback(app::CommandHandler * commandObj, cons

response.groupId = commandData.groupId;
response.status = GroupAdd(fabricIndex, commandPath.mEndpointId, commandData.groupId, commandData.groupName);
err = commandObj->AddResponseData(commandPath, response);
if (CHIP_NO_ERROR != err)
{
ChipLogDetail(Zcl, "GroupsCluster: AddGroup failed: %s", err.AsString());
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
}
commandObj->AddResponse(commandPath, response);
return true;
}

@@ -185,12 +179,7 @@ bool emberAfGroupsClusterViewGroupCallback(app::CommandHandler * commandObj, con
exit:
response.groupId = groupId;
response.status = status;
err = commandObj->AddResponseData(commandPath, response);
if (CHIP_NO_ERROR != err)
{
ChipLogDetail(Zcl, "GroupsCluster: ViewGroup failed: %s", err.AsString());
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
}
commandObj->AddResponse(commandPath, response);
return true;
}

@@ -284,14 +273,13 @@ bool emberAfGroupsClusterGetGroupMembershipCallback(app::CommandHandler * comman

{
GroupDataProvider::EndpointIterator * iter = nullptr;
CHIP_ERROR err = CHIP_NO_ERROR;

iter = provider->IterateEndpoints(fabricIndex);
VerifyOrExit(nullptr != iter, status = EMBER_ZCL_STATUS_FAILURE);

err = commandObj->AddResponseData(commandPath, GroupMembershipResponse(commandData, commandPath.mEndpointId, iter));
commandObj->AddResponse(commandPath, GroupMembershipResponse(commandData, commandPath.mEndpointId, iter));
iter->Release();
status = (CHIP_NO_ERROR == err) ? EMBER_ZCL_STATUS_SUCCESS : EMBER_ZCL_STATUS_FAILURE;
status = EMBER_ZCL_STATUS_SUCCESS;
}

exit:
@@ -308,7 +296,6 @@ bool emberAfGroupsClusterRemoveGroupCallback(app::CommandHandler * commandObj, c
{
auto fabricIndex = commandObj->GetAccessingFabricIndex();
Groups::Commands::RemoveGroupResponse::Type response;
CHIP_ERROR err = CHIP_NO_ERROR;

// For all networks, Remove Group commands are only responded to when
// they are addressed to a single device.
@@ -324,12 +311,7 @@ bool emberAfGroupsClusterRemoveGroupCallback(app::CommandHandler * commandObj, c
response.groupId = commandData.groupId;
response.status = GroupRemove(fabricIndex, commandPath.mEndpointId, commandData.groupId);

err = commandObj->AddResponseData(commandPath, response);
if (CHIP_NO_ERROR != err)
{
ChipLogDetail(Zcl, "GroupsCluster: RemoveGroup failed: %s", err.AsString());
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
}
commandObj->AddResponse(commandPath, response);
return true;
}

10 changes: 5 additions & 5 deletions src/app/clusters/network-commissioning/network-commissioning.cpp
Original file line number Diff line number Diff line change
@@ -291,23 +291,23 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands
MATTER_TRACE_EVENT_SCOPE("HandleAddOrUpdateWiFiNetwork", "NetworkCommissioning");
Commands::NetworkConfigResponse::Type response;
response.networkingStatus = ToClusterObjectEnum(mpDriver.Get<WiFiDriver *>()->AddOrUpdateNetwork(req.ssid, req.credentials));
ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response);
ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
}

void Instance::HandleAddOrUpdateThreadNetwork(HandlerContext & ctx, const Commands::AddOrUpdateThreadNetwork::DecodableType & req)
{
MATTER_TRACE_EVENT_SCOPE("HandleAddOrUpdateThreadNetwork", "NetworkCommissioning");
Commands::NetworkConfigResponse::Type response;
response.networkingStatus = ToClusterObjectEnum(mpDriver.Get<ThreadDriver *>()->AddOrUpdateNetwork(req.operationalDataset));
ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response);
ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
}

void Instance::HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveNetwork::DecodableType & req)
{
MATTER_TRACE_EVENT_SCOPE("HandleRemoveNetwork", "NetworkCommissioning");
Commands::NetworkConfigResponse::Type response;
response.networkingStatus = ToClusterObjectEnum(mpWirelessDriver->RemoveNetwork(req.networkID));
ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response);
ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
}

void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::ConnectNetwork::DecodableType & req)
@@ -331,7 +331,7 @@ void Instance::HandleReorderNetwork(HandlerContext & ctx, const Commands::Reorde
MATTER_TRACE_EVENT_SCOPE("HandleReorderNetwork", "NetworkCommissioning");
Commands::NetworkConfigResponse::Type response;
response.networkingStatus = ToClusterObjectEnum(mpWirelessDriver->ReorderNetwork(req.networkID, req.networkIndex));
ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, response);
ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
}

void Instance::OnResult(Status commissioningError, CharSpan errorText, int32_t interfaceStatus)
@@ -349,7 +349,7 @@ void Instance::OnResult(Status commissioningError, CharSpan errorText, int32_t i
response.networkingStatus = ToClusterObjectEnum(commissioningError);
response.debugText = errorText;
response.errorValue = interfaceStatus;
commandHandle->AddResponseData(mPath, response);
commandHandle->AddResponse(mPath, response);

mLastNetworkIDLen = mConnectingNetworkIDLen;
memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen);
Original file line number Diff line number Diff line change
@@ -61,8 +61,8 @@ using namespace chip::Protocols::InteractionModel;

namespace {

CHIP_ERROR SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status,
uint8_t index, const CharSpan & debug_text);
void SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status,
uint8_t index, const CharSpan & debug_text);
OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err);

constexpr uint8_t kDACCertificate = 1;
@@ -519,8 +519,8 @@ namespace {
// TODO: Manage ephemeral RCAC/ICAC/NOC storage to avoid a full FabricInfo being needed here.
FabricInfo gFabricBeingCommissioned;

CHIP_ERROR SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status,
uint8_t index, const CharSpan & debug_text)
void SendNOCResponse(app::CommandHandler * commandObj, const ConcreteCommandPath & path, OperationalCertStatus status,
uint8_t index, const CharSpan & debug_text)
{
Commands::NOCResponse::Type payload;
payload.statusCode = status;
@@ -535,7 +535,7 @@ CHIP_ERROR SendNOCResponse(app::CommandHandler * commandObj, const ConcreteComma
payload.debugText.Emplace(to_send);
}

return commandObj->AddResponseData(path, payload);
commandObj->AddResponse(path, payload);
}

OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err)
@@ -774,7 +774,7 @@ bool emberAfOperationalCredentialsClusterCertificateChainRequestCallback(
}

response.certificate = derBufSpan;
SuccessOrExit(err = commandObj->AddResponseData(commandPath, response));
commandObj->AddResponse(commandPath, response);

exit:
if (err != CHIP_NO_ERROR)
@@ -834,7 +834,7 @@ bool emberAfOperationalCredentialsClusterAttestationRequestCallback(app::Command

response.attestationElements = attestationElementsSpan;
response.signature = signatureSpan;
SuccessOrExit(err = commandObj->AddResponseData(commandPath, response));
commandObj->AddResponse(commandPath, response);
}

exit:
@@ -922,7 +922,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler

response.NOCSRElements = nocsrElementsSpan;
response.attestationSignature = signatureSpan;
SuccessOrExit(err = commandObj->AddResponseData(commandPath, response));
commandObj->AddResponse(commandPath, response);
}

exit:
44 changes: 10 additions & 34 deletions src/app/clusters/test-cluster-server/test-cluster-server.cpp
Original file line number Diff line number Diff line change
@@ -714,11 +714,7 @@ bool emberAfTestClusterClusterTestSpecificCallback(CommandHandler * apCommandObj
{
TestSpecificResponse::Type responseData;
responseData.returnValue = 7;
CHIP_ERROR err = apCommandObj->AddResponseData(commandPath, responseData);
if (CHIP_NO_ERROR != err)
{
ChipLogError(Zcl, "Test Cluster: failed to send TestSpecific response: %" CHIP_ERROR_FORMAT, err.Format());
}
apCommandObj->AddResponse(commandPath, responseData);
return true;
}

@@ -738,23 +734,15 @@ bool emberAfTestClusterClusterTestAddArgumentsCallback(CommandHandler * apComman

TestAddArgumentsResponse::Type responseData;
responseData.returnValue = static_cast<uint8_t>(commandData.arg1 + commandData.arg2);
CHIP_ERROR err = apCommandObj->AddResponseData(commandPath, responseData);
if (CHIP_NO_ERROR != err)
{
ChipLogError(Zcl, "Test Cluster: failed to send TestAddArguments response: %" CHIP_ERROR_FORMAT, err.Format());
}
apCommandObj->AddResponse(commandPath, responseData);
return true;
}

static bool SendBooleanResponse(CommandHandler * commandObj, const ConcreteCommandPath & commandPath, bool value)
{
Commands::BooleanResponse::Type response;
response.value = value;
CHIP_ERROR err = commandObj->AddResponseData(commandPath, response);
if (err != CHIP_NO_ERROR)
{
commandObj->AddStatus(commandPath, Protocols::InteractionModel::Status::Failure);
}
commandObj->AddResponse(commandPath, response);
return true;
}

@@ -811,7 +799,7 @@ bool emberAfTestClusterClusterTestEmitTestEventRequestCallback(
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
return true;
}
commandObj->AddResponseData(commandPath, responseData);
commandObj->AddResponse(commandPath, responseData);
return true;
}

@@ -827,7 +815,7 @@ bool emberAfTestClusterClusterTestEmitTestFabricScopedEventRequestCallback(
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
return true;
}
commandObj->AddResponseData(commandPath, responseData);
commandObj->AddResponse(commandPath, responseData);
return true;
}

@@ -923,7 +911,7 @@ bool emberAfTestClusterClusterTestListInt8UReverseRequestCallback(
Commands::TestListInt8UReverseResponse::Type responseData;
if (count == 0)
{
SuccessOrExit(commandObj->AddResponseData(commandPath, responseData));
commandObj->AddResponse(commandPath, responseData);
return true;
}
size_t cur = count;
@@ -937,7 +925,7 @@ bool emberAfTestClusterClusterTestListInt8UReverseRequestCallback(
VerifyOrExit(cur == 0, );
VerifyOrExit(iter.GetStatus() == CHIP_NO_ERROR, );
responseData.arg1 = DataModel::List<uint8_t>(responseBuf.Get(), count);
SuccessOrExit(commandObj->AddResponseData(commandPath, responseData));
commandObj->AddResponse(commandPath, responseData);
return true;
}

@@ -953,11 +941,7 @@ bool emberAfTestClusterClusterTestEnumsRequestCallback(CommandHandler * commandO
response.arg1 = commandData.arg1;
response.arg2 = commandData.arg2;

CHIP_ERROR err = commandObj->AddResponseData(commandPath, response);
if (err != CHIP_NO_ERROR)
{
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
}
commandObj->AddResponse(commandPath, response);
return true;
}

@@ -979,11 +963,7 @@ bool emberAfTestClusterClusterTestNullableOptionalRequestCallback(
response.originalValue.Emplace(commandData.arg1.Value());
}

CHIP_ERROR err = commandObj->AddResponseData(commandPath, response);
if (err != CHIP_NO_ERROR)
{
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
}
commandObj->AddResponse(commandPath, response);
return true;
}

@@ -1000,11 +980,7 @@ bool emberAfTestClusterClusterSimpleStructEchoRequestCallback(CommandHandler * c
response.arg1.g = commandData.arg1.g;
response.arg1.h = commandData.arg1.h;

CHIP_ERROR err = commandObj->AddResponseData(commandPath, response);
if (err != CHIP_NO_ERROR)
{
emberAfSendImmediateDefaultResponse(EMBER_ZCL_STATUS_FAILURE);
}
commandObj->AddResponse(commandPath, response);
return true;
}

81 changes: 80 additions & 1 deletion src/app/tests/TestCommandInteraction.cpp
Original file line number Diff line number Diff line change
@@ -26,6 +26,7 @@

#include <app/AppBuildConfig.h>
#include <app/InteractionModelEngine.h>
#include <app/data-model/Encode.h>
#include <app/tests/AppTestContext.h>
#include <lib/core/CHIPCore.h>
#include <lib/core/CHIPTLV.h>
@@ -178,6 +179,8 @@ class TestCommandInteraction
static void TestCommandHandlerWithProcessReceivedNotExistCommand(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerWithSendSimpleCommandData(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerCommandEncodeExternalFailure(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerWithSendSimpleStatusCode(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerWithSendEmptyResponse(nlTestSuite * apSuite, void * apContext);
static void TestCommandHandlerWithProcessReceivedMsg(nlTestSuite * apSuite, void * apContext);
@@ -515,6 +518,23 @@ struct Fields
}
};

struct BadFields
{
static constexpr chip::CommandId GetCommandId() { return 4; }
CHIP_ERROR Encode(TLV::TLVWriter & aWriter, TLV::Tag aTag) const
{
TLV::TLVType outerContainerType;
uint8_t data[36] = { 0 };
ReturnErrorOnFailure(aWriter.StartContainer(aTag, TLV::kTLVType_Structure, outerContainerType));
// Just encode something bad to return a failure state here.
for (uint8_t i = 1; i < UINT8_MAX; i++)
{
ReturnErrorOnFailure(app::DataModel::Encode(aWriter, TLV::ContextTag(i), ByteSpan(data)));
}
return aWriter.EndContainer(outerContainerType);
}
};

void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
@@ -527,7 +547,64 @@ void TestCommandInteraction::TestCommandHandlerCommandDataEncoding(nlTestSuite *

auto path = MakeTestCommandPath();

err = commandHandler.AddResponseData(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), Fields());
commandHandler.AddResponse(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), Fields());
err = commandHandler.Finalize(commandPacket);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
chip::System::PacketBufferTLVReader reader;
InvokeResponseMessage::Parser invokeResponseMessageParser;
reader.Init(std::move(commandPacket));
err = invokeResponseMessageParser.Init(reader);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
err = invokeResponseMessageParser.CheckSchemaValidity();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
#endif
}

void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
CHIP_ERROR err = CHIP_NO_ERROR;
app::CommandHandler commandHandler(nullptr);
System::PacketBufferHandle commandPacket;

TestExchangeDelegate delegate;
commandHandler.mpExchangeCtx = ctx.NewExchangeToAlice(&delegate);

auto path = MakeTestCommandPath();

commandHandler.AddResponse(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), BadFields());
err = commandHandler.Finalize(commandPacket);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
chip::System::PacketBufferTLVReader reader;
InvokeResponseMessage::Parser invokeResponseMessageParser;
reader.Init(std::move(commandPacket));
err = invokeResponseMessageParser.Init(reader);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
err = invokeResponseMessageParser.CheckSchemaValidity();
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
#endif
}

void TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure(nlTestSuite * apSuite, void * apContext)
{
TestContext & ctx = *static_cast<TestContext *>(apContext);
CHIP_ERROR err = CHIP_NO_ERROR;
app::CommandHandler commandHandler(nullptr);
System::PacketBufferHandle commandPacket;

TestExchangeDelegate delegate;
commandHandler.mpExchangeCtx = ctx.NewExchangeToAlice(&delegate);

auto path = MakeTestCommandPath();

err = commandHandler.AddResponseData(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId), BadFields());
NL_TEST_ASSERT(apSuite, err != CHIP_NO_ERROR);
err = commandHandler.AddStatus(ConcreteCommandPath(path.mEndpointId, path.mClusterId, path.mCommandId),
Protocols::InteractionModel::Status::Failure);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
err = commandHandler.Finalize(commandPacket);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
@@ -826,6 +903,8 @@ const nlTest sTests[] =
NL_TEST_DEF("TestCommandSenderWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandSenderWithProcessReceivedMsg),
NL_TEST_DEF("TestCommandHandlerWithSendSimpleCommandData", chip::app::TestCommandInteraction::TestCommandHandlerWithSendSimpleCommandData),
NL_TEST_DEF("TestCommandHandlerCommandDataEncoding", chip::app::TestCommandInteraction::TestCommandHandlerCommandDataEncoding),
NL_TEST_DEF("TestCommandHandlerCommandEncodeFailure", chip::app::TestCommandInteraction::TestCommandHandlerCommandEncodeFailure),
NL_TEST_DEF("TestCommandHandlerCommandEncodeExternalFailure", chip::app::TestCommandInteraction::TestCommandHandlerCommandEncodeExternalFailure),
NL_TEST_DEF("TestCommandHandlerWithSendSimpleStatusCode", chip::app::TestCommandInteraction::TestCommandHandlerWithSendSimpleStatusCode),
NL_TEST_DEF("TestCommandHandlerWithProcessReceivedMsg", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedMsg),
NL_TEST_DEF("TestCommandHandlerWithProcessReceivedNotExistCommand", chip::app::TestCommandInteraction::TestCommandHandlerWithProcessReceivedNotExistCommand),
2 changes: 1 addition & 1 deletion src/app/tests/TestTimedHandler.cpp
Original file line number Diff line number Diff line change
@@ -111,7 +111,7 @@ void TestTimedHandler::TestFollowingMessageFastEnough(nlTestSuite * aSuite, void
TestContext & ctx = *static_cast<TestContext *>(aContext);

System::PacketBufferHandle payload;
GenerateTimedRequest(aSuite, 50, payload);
GenerateTimedRequest(aSuite, 500, payload);

TestExchangeDelegate delegate;
ExchangeContext * exchange = ctx.NewExchangeToAlice(&delegate);
2 changes: 1 addition & 1 deletion src/controller/tests/TestServerCommandDispatch.cpp
Original file line number Diff line number Diff line change
@@ -93,7 +93,7 @@ void TestClusterCommandHandler::InvokeCommand(chip::app::CommandHandlerInterface
dataResponse.arg1 = nestedStructList;
dataResponse.arg6 = true;

ctx.mCommandHandler.AddResponseData(ctx.mRequestPath, dataResponse);
ctx.mCommandHandler.AddResponse(ctx.mRequestPath, dataResponse);
}

return CHIP_NO_ERROR;
2 changes: 1 addition & 1 deletion src/controller/tests/data_model/TestCommands.cpp
Original file line number Diff line number Diff line change
@@ -106,7 +106,7 @@ void DispatchSingleClusterCommand(const ConcreteCommandPath & aCommandPath, chip
dataResponse.arg1 = nestedStructList;
dataResponse.arg6 = true;

apCommandObj->AddResponseData(aCommandPath, dataResponse);
apCommandObj->AddResponse(aCommandPath, dataResponse);
}
else if (responseDirective == kSendSuccessStatusCode)
{

0 comments on commit 42aef61

Please sign in to comment.