Skip to content

Commit

Permalink
Fix status report ec nullout and suppress report during read (#11955)
Browse files Browse the repository at this point in the history
  • Loading branch information
@yunhanw-google @pull
yunhanw-google authored and pull[bot] committed May 18, 2022
1 parent 70ba1f4 commit 1102194
Show file tree
Hide file tree
Showing 6 changed files with 49 additions and 33 deletions.
2 changes: 1 addition & 1 deletion src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ CHIP_ERROR InteractionModelEngine::OnUnsolicitedReportData(Messaging::ExchangeCo

for (auto & readClient : mReadClients)
{
if (!readClient.IsSubscriptionTypeIdle())
if (!readClient.IsSubscriptionIdle())
{
continue;
}
Expand Down
31 changes: 14 additions & 17 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand All @@ -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);

Expand Down Expand Up @@ -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())
{
Expand All @@ -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;
}
Expand All @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion src/app/ReadClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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; }

Expand Down
20 changes: 16 additions & 4 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
8 changes: 8 additions & 0 deletions src/app/reporting/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -361,6 +365,10 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler)
{
apReadHandler->Shutdown(ReadHandler::ShutdownOptions::AbortCurrentExchange);
}
else if (apReadHandler->IsReadType() && !hasMoreChunks)
{
apReadHandler->Shutdown();
}
return err;
}

Expand Down
19 changes: 9 additions & 10 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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);

Expand Down

0 comments on commit 1102194

Please sign in to comment.