From 11021941e24d52c4ca56e5496d478432ea072f75 Mon Sep 17 00:00:00 2001 From: yunhanw-google Date: Sat, 20 Nov 2021 15:45:28 -0800 Subject: [PATCH] Fix status report ec nullout and suppress report during read (#11955) --- src/app/InteractionModelEngine.cpp | 2 +- src/app/ReadClient.cpp | 31 ++++++++++++--------------- src/app/ReadClient.h | 2 +- src/app/ReadHandler.cpp | 20 +++++++++++++---- src/app/reporting/Engine.cpp | 8 +++++++ src/app/tests/TestReadInteraction.cpp | 19 ++++++++-------- 6 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index b0d6ccb18444c9..c7c1428de3fdd2 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -376,7 +376,7 @@ CHIP_ERROR InteractionModelEngine::OnUnsolicitedReportData(Messaging::ExchangeCo for (auto & readClient : mReadClients) { - if (!readClient.IsSubscriptionTypeIdle()) + if (!readClient.IsSubscriptionIdle()) { continue; } diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index f71eb6e4da78cb..975cb780e2801b 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -291,7 +291,6 @@ CHIP_ERROR ReadClient::OnUnsolicitedReportData(Messaging::ExchangeContext * apEx { mpExchangeCtx = apExchangeContext; CHIP_ERROR err = ProcessReportData(std::move(aPayload)); - mpExchangeCtx = nullptr; if (err != CHIP_NO_ERROR) { ShutdownInternal(err); @@ -306,7 +305,7 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) bool isEventReportsPresent = false; bool isAttributeReportIBsPresent = false; - bool suppressResponse = false; + bool suppressResponse = true; uint64_t subscriptionId = 0; EventReports::Parser EventReports; AttributeReportIBs::Parser attributeReportIBs; @@ -326,7 +325,8 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) err = report.GetSuppressResponse(&suppressResponse); if (CHIP_END_OF_TLV == err) { - err = CHIP_NO_ERROR; + suppressResponse = false; + err = CHIP_NO_ERROR; } SuccessOrExit(err); @@ -399,12 +399,6 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) mpCallback->OnReportEnd(this); } - if (!suppressResponse) - { - // TODO: Add status report support and correspond handler in ReadHandler, particular for situation when there - // are multiple reports - } - exit: if (IsSubscriptionType()) { @@ -418,14 +412,19 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload) } } - StatusResponse::SendStatusResponse(err == CHIP_NO_ERROR ? Protocols::InteractionModel::Status::Success - : Protocols::InteractionModel::Status::InvalidSubscription, - mpExchangeCtx, IsAwaitingSubscribeResponse() || mPendingMoreChunks); - - if (!mInitialReport && !mPendingMoreChunks) + if (!suppressResponse) { - mpExchangeCtx = nullptr; + bool noResponseExpected = IsSubscriptionIdle() && !mPendingMoreChunks; + err = StatusResponse::SendStatusResponse(err == CHIP_NO_ERROR ? Protocols::InteractionModel::Status::Success + : Protocols::InteractionModel::Status::InvalidSubscription, + mpExchangeCtx, !noResponseExpected); + + if (noResponseExpected || (err != CHIP_NO_ERROR)) + { + mpExchangeCtx = nullptr; + } } + mInitialReport = false; return err; } @@ -449,10 +448,8 @@ CHIP_ERROR ReadClient::ProcessAttributePath(AttributePathIB::Parser & aAttribute // The ReportData must contain a concrete attribute path err = aAttributePath.GetEndpoint(&(aClusterInfo.mEndpointId)); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); - err = aAttributePath.GetCluster(&(aClusterInfo.mClusterId)); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); - err = aAttributePath.GetAttribute(&(aClusterInfo.mAttributeId)); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index e889e05754cb3c..0191f3e348a40a 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -250,7 +250,7 @@ class ReadClient : public Messaging::ExchangeDelegate * */ bool IsFree() const { return mState == ClientState::Uninitialized; } - bool IsSubscriptionTypeIdle() const { return mState == ClientState::SubscriptionActive; } + bool IsSubscriptionIdle() const { return mState == ClientState::SubscriptionActive; } bool IsAwaitingInitialReport() const { return mState == ClientState::AwaitingInitialReport; } bool IsAwaitingSubscribeResponse() const { return mState == ClientState::AwaitingSubscribeResponse; } diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 94e868dad6e1d7..0d079f0f59b031 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -203,10 +203,22 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b mpExchangeCtx->SetResponseTimeout(kImMessageTimeout); } VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE); - mIsChunkedReport = aMoreChunks; - MoveToState(HandlerState::AwaitingReportResponse); - CHIP_ERROR err = mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload), - Messaging::SendFlags(Messaging::SendMessageFlags::kExpectResponse)); + mIsChunkedReport = aMoreChunks; + bool noResponseExpected = IsReadType() && !mIsChunkedReport; + if (!noResponseExpected) + { + MoveToState(HandlerState::AwaitingReportResponse); + } + CHIP_ERROR err = + mpExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::ReportData, std::move(aPayload), + Messaging::SendFlags(noResponseExpected ? Messaging::SendMessageFlags::kNone + : Messaging::SendMessageFlags::kExpectResponse)); + if (err == CHIP_NO_ERROR && noResponseExpected) + { + mpExchangeCtx = nullptr; + InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm(); + } + if (err == CHIP_NO_ERROR) { if (IsSubscriptionType() && !IsInitialReport()) diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 21b4fa2986b5f3..13fd1f8bc30853 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -324,6 +324,10 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) { reportDataBuilder.MoreChunkedMessages(hasMoreChunks); } + else if (apReadHandler->IsReadType()) + { + reportDataBuilder.SuppressResponse(true); + } reportDataBuilder.EndOfReportDataMessage(); SuccessOrExit(err = reportDataBuilder.GetError()); @@ -361,6 +365,10 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) { apReadHandler->Shutdown(ReadHandler::ShutdownOptions::AbortCurrentExchange); } + else if (apReadHandler->IsReadType() && !hasMoreChunks) + { + apReadHandler->Shutdown(); + } return err; } diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 4ee5301381a8e9..086889fc2b1d7f 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -279,11 +279,11 @@ class TestReadInteraction private: static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedInvalidReport = false); + bool aNeedInvalidReport, bool aSuppressResponse); }; void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedInvalidReport) + bool aNeedInvalidReport, bool aSuppressResponse) { CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferTLVWriter writer; @@ -294,6 +294,9 @@ void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apCon err = reportDataMessageBuilder.Init(&writer); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + reportDataMessageBuilder.SuppressResponse(aSuppressResponse); + NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR); + AttributeReportIBs::Builder attributeReportIBsBuilder = reportDataMessageBuilder.CreateAttributeReportIBs(); NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR); @@ -346,9 +349,6 @@ void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apCon attributeReportIBsBuilder.EndOfAttributeReportIBs(); NL_TEST_ASSERT(apSuite, attributeReportIBsBuilder.GetError() == CHIP_NO_ERROR); - reportDataMessageBuilder.SuppressResponse(true); - NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR); - reportDataMessageBuilder.MoreChunkedMessages(false); NL_TEST_ASSERT(apSuite, reportDataMessageBuilder.GetError() == CHIP_NO_ERROR); @@ -372,8 +372,7 @@ void TestReadInteraction::TestReadClient(nlTestSuite * apSuite, void * apContext err = readClient.SendReadRequest(readPrepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - GenerateReportData(apSuite, apContext, buf); - + GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/); err = readClient.ProcessReportData(std::move(buf)); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); @@ -397,7 +396,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr); readHandler.Init(&ctx.GetExchangeManager(), nullptr, exchangeCtx, chip::app::ReadHandler::InteractionType::Read); - GenerateReportData(apSuite, apContext, reportDatabuf); + GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/); err = readHandler.SendReportData(std::move(reportDatabuf), false); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); @@ -500,7 +499,7 @@ void TestReadInteraction::TestReadClientInvalidReport(nlTestSuite * apSuite, voi err = readClient.SendReadRequest(readPrepareParams); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); - GenerateReportData(apSuite, apContext, buf, true /*aNeedInvalidReport*/); + GenerateReportData(apSuite, apContext, buf, true /*aNeedInvalidReport*/, true /* aSuppressResponse*/); err = readClient.ProcessReportData(std::move(buf)); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); @@ -526,7 +525,7 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu Messaging::ExchangeContext * exchangeCtx = ctx.NewExchangeToAlice(nullptr); readHandler.Init(&ctx.GetExchangeManager(), nullptr, exchangeCtx, chip::app::ReadHandler::InteractionType::Read); - GenerateReportData(apSuite, apContext, reportDatabuf); + GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/); err = readHandler.SendReportData(std::move(reportDatabuf), false); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE);