From 460ca44114509b7b8e3be95a5fd7c349825cdfab Mon Sep 17 00:00:00 2001 From: yunhanw Date: Thu, 11 Nov 2021 23:03:24 -0800 Subject: [PATCH] address comments --- src/app/CommandSender.cpp | 16 +++------------- src/app/InteractionModelEngine.cpp | 9 +++------ src/app/InteractionModelEngine.h | 11 +++++++++-- src/app/ReadClient.cpp | 13 ++----------- src/app/ReadHandler.cpp | 7 +++---- src/app/StatusResponse.cpp | 25 ++++++++++++++++++------- src/app/StatusResponse.h | 4 ++-- src/app/WriteClient.cpp | 13 ++----------- src/app/util/basic-types.h | 1 + 9 files changed, 43 insertions(+), 56 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index fd22c96462e8e0..9e3da0b7fae067 100644 --- a/src/app/CommandSender.cpp +++ b/src/app/CommandSender.cpp @@ -92,26 +92,18 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha System::PacketBufferHandle && aPayload) { CHIP_ERROR err = CHIP_NO_ERROR; + StatusIB status(Protocols::InteractionModel::Status::Failure); VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE); if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::InvokeCommandResponse)) { err = ProcessInvokeResponse(std::move(aPayload)); SuccessOrExit(err); + status.mStatus = Protocols::InteractionModel::Status::Success; } else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse)) { - Protocols::InteractionModel::Status status; - err = StatusResponse::ProcessStatusResponse(apExchangeContext, std::move(aPayload), status); - mpExchangeCtx = nullptr; + err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status); SuccessOrExit(err); - if (status == Protocols::InteractionModel::Status::ResourceExhausted) - { - err = CHIP_ERROR_NO_MEMORY; - } - else - { - err = CHIP_ERROR_INCORRECT_STATE; - } } else { @@ -123,8 +115,6 @@ CHIP_ERROR CommandSender::OnMessageReceived(Messaging::ExchangeContext * apExcha { if (err != CHIP_NO_ERROR) { - StatusIB status; - status.mStatus = Protocols::InteractionModel::Status::Failure; mpCallback->OnError(this, status, err); } } diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 2f1541a951bf20..b0d6ccb18444c9 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -402,7 +402,7 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReadRequest)) { SuccessOrExit(OnReadInitialRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), - ReadHandler::InteractionType::Read, status)); + ReadHandler::InteractionType::Read, status)); } else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::WriteRequest)) { @@ -411,11 +411,12 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::SubscribeRequest)) { SuccessOrExit(OnReadInitialRequest(apExchangeContext, aPayloadHeader, std::move(aPayload), - ReadHandler::InteractionType::Subscribe, status)); + ReadHandler::InteractionType::Subscribe, status)); } else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData)) { ReturnErrorOnFailure(OnUnsolicitedReportData(apExchangeContext, aPayloadHeader, std::move(aPayload))); + status = Protocols::InteractionModel::Status::Success; } else { @@ -426,10 +427,6 @@ CHIP_ERROR InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext if (status != Protocols::InteractionModel::Status::Success) { err = StatusResponse::SendStatusResponse(status, apExchangeContext, false /*aExpectResponse*/); - if (err != CHIP_NO_ERROR) - { - apExchangeContext->Abort(); - } } return err; } diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 6a738611f091b3..5810bae71d854b 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -200,6 +200,11 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman void OnDone(CommandHandler & apCommandObj) override; + /** + * Called when Interaction Model receives a Command Request message. Errors processing + * the Command Request are handled entirely within this function. The caller pre-sets status to failure and the callee is + * expected to set it to success if it does not want an automatic status response message to be sent. + */ CHIP_ERROR OnInvokeCommandRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload, Protocols::InteractionModel::Status & aStatus); CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, @@ -208,7 +213,8 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman /** * Called when Interaction Model receives a Read Request message. Errors processing - * the Read Request are handled entirely within this function. + * the Read Request are handled entirely within this function. The caller pre-sets status to failure and the callee is + * expected to set it to success if it does not want an automatic status response message to be sent. */ CHIP_ERROR OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, @@ -217,7 +223,8 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman /** * Called when Interaction Model receives a Write Request message. Errors processing - * the Write Request are handled entirely within this function. + * the Write Request are handled entirely within this function. The caller pre-sets status to failure and the callee is + * expected to set it to success if it does not want an automatic status response message to be sent. */ CHIP_ERROR OnWriteRequest(Messaging::ExchangeContext * apExchangeContext, const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload, Protocols::InteractionModel::Status & aStatus); diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 33ceea9995c6a4..341ba41d0c1d52 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -250,19 +250,10 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange } else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse)) { - Protocols::InteractionModel::Status status; + StatusIB status; VerifyOrExit(apExchangeContext == mpExchangeCtx, err = CHIP_ERROR_INCORRECT_STATE); - err = StatusResponse::ProcessStatusResponse(apExchangeContext, std::move(aPayload), status); - mpExchangeCtx = nullptr; + err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status); SuccessOrExit(err); - if (status == Protocols::InteractionModel::Status::ResourceExhausted) - { - err = CHIP_ERROR_NO_MEMORY; - } - else - { - err = CHIP_ERROR_INCORRECT_STATE; - } } else { diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index e7814b0faf9283..ad6fde6b545e01 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -134,10 +134,9 @@ CHIP_ERROR ReadHandler::OnReadInitialRequest(System::PacketBufferHandle && aPayl CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload) { CHIP_ERROR err = CHIP_NO_ERROR; - Protocols::InteractionModel::Status status; - err = StatusResponse::ProcessStatusResponse(apExchangeContext, std::move(aPayload), status); - VerifyOrExit((err == CHIP_NO_ERROR) && (status == Protocols::InteractionModel::Status::Success), - err = CHIP_ERROR_INVALID_ARGUMENT); + StatusIB status; + err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status); + SuccessOrExit(err); switch (mState) { case HandlerState::AwaitingReportResponse: diff --git a/src/app/StatusResponse.cpp b/src/app/StatusResponse.cpp index f35bc20292a08c..dbc5afc6ec2f2a 100644 --- a/src/app/StatusResponse.cpp +++ b/src/app/StatusResponse.cpp @@ -24,7 +24,6 @@ namespace app { CHIP_ERROR StatusResponse::SendStatusResponse(Protocols::InteractionModel::Status aStatus, Messaging::ExchangeContext * apExchangeContext, bool aExpectResponse) { - using Protocols::InteractionModel::Status; VerifyOrReturnError(apExchangeContext != nullptr, CHIP_ERROR_INCORRECT_STATE); System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); VerifyOrReturnError(!msgBuf.IsNull(), CHIP_ERROR_NO_MEMORY); @@ -43,10 +42,9 @@ CHIP_ERROR StatusResponse::SendStatusResponse(Protocols::InteractionModel::Statu return CHIP_NO_ERROR; } -CHIP_ERROR StatusResponse::ProcessStatusResponse(Messaging::ExchangeContext * apExchangeContext, - System::PacketBufferHandle && aPayload, - Protocols::InteractionModel::Status & aStatus) +CHIP_ERROR StatusResponse::ProcessStatusResponse(System::PacketBufferHandle && aPayload, StatusIB & aStatus) { + CHIP_ERROR err = CHIP_NO_ERROR; StatusResponseMessage::Parser response; System::PacketBufferTLVReader reader; reader.Init(std::move(aPayload)); @@ -55,9 +53,22 @@ CHIP_ERROR StatusResponse::ProcessStatusResponse(Messaging::ExchangeContext * ap #if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK ReturnErrorOnFailure(response.CheckSchemaValidity()); #endif - ReturnErrorOnFailure(response.GetStatus(aStatus)); - ChipLogProgress(InteractionModel, "Receive status response, status is %" PRIu16, to_underlying(aStatus)); - return CHIP_NO_ERROR; + ReturnErrorOnFailure(response.GetStatus(aStatus.mStatus)); + ChipLogProgress(InteractionModel, "Received status response, status is %" PRIu16, to_underlying(aStatus.mStatus)); + + if (aStatus.mStatus == Protocols::InteractionModel::Status::Success) + { + err = CHIP_NO_ERROR; + } + else if (aStatus.mStatus == Protocols::InteractionModel::Status::ResourceExhausted) + { + err = CHIP_ERROR_NO_MEMORY; + } + else + { + err = CHIP_ERROR_INCORRECT_STATE; + } + return err; } } // namespace app } // namespace chip diff --git a/src/app/StatusResponse.h b/src/app/StatusResponse.h index dde4270bcc0f62..03114def8f1da6 100644 --- a/src/app/StatusResponse.h +++ b/src/app/StatusResponse.h @@ -18,6 +18,7 @@ #pragma once +#include #include #include #include @@ -31,8 +32,7 @@ class StatusResponse public: static CHIP_ERROR SendStatusResponse(Protocols::InteractionModel::Status aStatus, Messaging::ExchangeContext * apExchangeContext, bool aExpectResponse); - static CHIP_ERROR ProcessStatusResponse(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload, - Protocols::InteractionModel::Status & aStatus); + static CHIP_ERROR ProcessStatusResponse(System::PacketBufferHandle && aPayload, StatusIB & aStatus); }; } // namespace app } // namespace chip diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index f6241bde189121..f743a45c7d61fe 100644 --- a/src/app/WriteClient.cpp +++ b/src/app/WriteClient.cpp @@ -286,18 +286,9 @@ CHIP_ERROR WriteClient::OnMessageReceived(Messaging::ExchangeContext * apExchang } else if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::StatusResponse)) { - Protocols::InteractionModel::Status status; - err = StatusResponse::ProcessStatusResponse(apExchangeContext, std::move(aPayload), status); - mpExchangeCtx = nullptr; + StatusIB status; + err = StatusResponse::ProcessStatusResponse(std::move(aPayload), status); SuccessOrExit(err); - if (status == Protocols::InteractionModel::Status::ResourceExhausted) - { - err = CHIP_ERROR_NO_MEMORY; - } - else - { - err = CHIP_ERROR_INCORRECT_STATE; - } } else { diff --git a/src/app/util/basic-types.h b/src/app/util/basic-types.h index 53256533ef0a3d..97aebe690423c0 100644 --- a/src/app/util/basic-types.h +++ b/src/app/util/basic-types.h @@ -29,6 +29,7 @@ #include namespace chip { + typedef uint8_t Percent; typedef uint16_t Percent100ths;