Skip to content

Commit

Permalink
follow-up for pr9195 (#9251)
Browse files Browse the repository at this point in the history 
--Move OnReportConfirm in Shutdown for read handler and remove duplicate code for code optimization
--Remove request parameter from GenerateAttributePathList and GenerateEventPathList
--Rename reportError to readError
--Mark the first read report as initial
  • Loading branch information
@yunhanw-google @pull
yunhanw-google authored and pull[bot] committed Oct 4, 2021
1 parent c8d4ed2 commit 22d906d
Show file tree
Hide file tree
Showing 10 changed files with 95 additions and 90 deletions.
2 changes: 1 addition & 1 deletion src/app/InteractionModelDelegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class InteractionModelDelegate
* fail to process report data.
* @retval # CHIP_ERROR_NOT_IMPLEMENTED if not implemented
*/
virtual CHIP_ERROR ReportError(const ReadClient * apReadClient, CHIP_ERROR aError) { return CHIP_ERROR_NOT_IMPLEMENTED; }
virtual CHIP_ERROR ReadError(const ReadClient * apReadClient, CHIP_ERROR aError) { return CHIP_ERROR_NOT_IMPLEMENTED; }

/**
* Notification that a Command Send has received an Invoke Command Response containing a status code.
Expand Down
69 changes: 38 additions & 31 deletions src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,22 @@ CHIP_ERROR ReadClient::SendReadRequest(NodeId aNodeId, FabricIndex aFabricIndex,

if (aEventPathParamsListSize != 0 && apEventPathParamsList != nullptr)
{
err = GenerateEventPathList(request, apEventPathParamsList, aEventPathParamsListSize, aEventNumber);
EventPathList::Builder & eventPathListBuilder = request.CreateEventPathListBuilder();
SuccessOrExit(err = eventPathListBuilder.GetError());
err = GenerateEventPathList(eventPathListBuilder, apEventPathParamsList, aEventPathParamsListSize);
SuccessOrExit(err);
if (aEventNumber != 0)
{
// EventNumber is optional
request.EventNumber(aEventNumber);
}
}

if (aAttributePathParamsListSize != 0 && apAttributePathParamsList != nullptr)
{
err = GenerateAttributePathList(request, apAttributePathParamsList, aAttributePathParamsListSize);
AttributePathList::Builder attributePathListBuilder = request.CreateAttributePathListBuilder();
SuccessOrExit(err = attributePathListBuilder.GetError());
err = GenerateAttributePathList(attributePathListBuilder, apAttributePathParamsList, aAttributePathParamsListSize);
SuccessOrExit(err);
}

Expand Down Expand Up @@ -200,14 +209,14 @@ CHIP_ERROR ReadClient::SendStatusReport(CHIP_ERROR aError, bool aExpectResponse)
return CHIP_NO_ERROR;
}

CHIP_ERROR ReadClient::GenerateEventPathList(ReadRequest::Builder & aRequest, EventPathParams * apEventPathParamsList,
size_t aEventPathParamsListSize, EventNumber & aEventNumber)
CHIP_ERROR ReadClient::GenerateEventPathList(EventPathList::Builder & aEventPathListBuilder,
EventPathParams * apEventPathParamsList, size_t aEventPathParamsListSize)
{
CHIP_ERROR err = CHIP_NO_ERROR;
EventPathList::Builder & eventPathListBuilder = aRequest.CreateEventPathListBuilder();
CHIP_ERROR err = CHIP_NO_ERROR;

for (size_t eventIndex = 0; eventIndex < aEventPathParamsListSize; ++eventIndex)
{
EventPath::Builder eventPathBuilder = eventPathListBuilder.CreateEventPathBuilder();
EventPath::Builder eventPathBuilder = aEventPathListBuilder.CreateEventPathBuilder();
EventPathParams eventPath = apEventPathParamsList[eventIndex];
eventPathBuilder.NodeId(eventPath.mNodeId)
.EventId(eventPath.mEventId)
Expand All @@ -217,28 +226,21 @@ CHIP_ERROR ReadClient::GenerateEventPathList(ReadRequest::Builder & aRequest, Ev
SuccessOrExit(err = eventPathBuilder.GetError());
}

eventPathListBuilder.EndOfEventPathList();
SuccessOrExit(err = eventPathListBuilder.GetError());

if (aEventNumber != 0)
{
// EventNumber is optional
aRequest.EventNumber(aEventNumber);
}
aEventPathListBuilder.EndOfEventPathList();
SuccessOrExit(err = aEventPathListBuilder.GetError());

exit:
ChipLogFunctError(err);
return err;
}

CHIP_ERROR ReadClient::GenerateAttributePathList(ReadRequest::Builder & aRequest, AttributePathParams * apAttributePathParamsList,
CHIP_ERROR ReadClient::GenerateAttributePathList(AttributePathList::Builder & aAttributePathListBuilder,
AttributePathParams * apAttributePathParamsList,
size_t aAttributePathParamsListSize)
{
AttributePathList::Builder attributePathListBuilder = aRequest.CreateAttributePathListBuilder();
ReturnErrorOnFailure(attributePathListBuilder.GetError());
for (size_t index = 0; index < aAttributePathParamsListSize; index++)
{
AttributePath::Builder attributePathBuilder = attributePathListBuilder.CreateAttributePathBuilder();
AttributePath::Builder attributePathBuilder = aAttributePathListBuilder.CreateAttributePathBuilder();
attributePathBuilder.NodeId(apAttributePathParamsList[index].mNodeId)
.EndpointId(apAttributePathParamsList[index].mEndpointId)
.ClusterId(apAttributePathParamsList[index].mClusterId);
Expand All @@ -257,8 +259,8 @@ CHIP_ERROR ReadClient::GenerateAttributePathList(ReadRequest::Builder & aRequest
attributePathBuilder.EndOfAttributePath();
ReturnErrorOnFailure(attributePathBuilder.GetError());
}
attributePathListBuilder.EndOfAttributePathList();
return attributePathListBuilder.GetError();
aAttributePathListBuilder.EndOfAttributePathList();
return aAttributePathListBuilder.GetError();
}

CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
Expand All @@ -267,22 +269,27 @@ CHIP_ERROR ReadClient::OnMessageReceived(Messaging::ExchangeContext * apExchange
CHIP_ERROR err = CHIP_NO_ERROR;
VerifyOrExit(!IsFree(), err = CHIP_ERROR_INCORRECT_STATE);
VerifyOrExit(mpDelegate != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
VerifyOrExit(aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData),
err = CHIP_ERROR_INVALID_MESSAGE_TYPE);
err = ProcessReportData(std::move(aPayload));
if (aPayloadHeader.HasMessageType(Protocols::InteractionModel::MsgType::ReportData))
{
err = ProcessReportData(std::move(aPayload));
SuccessOrExit(err);
mpDelegate->ReportProcessed(this);
}
else
{
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
}

SuccessOrExit(err);
err = SendStatusReport(err, false);

exit:
ChipLogFunctError(err);
if (err != CHIP_NO_ERROR)
{
mpDelegate->ReportError(this, err);
}
else
{
mpDelegate->ReportProcessed(this);
mpDelegate->ReadError(this, err);
}
ChipLogFunctError(err);

ShutdownInternal(err);

return err;
Expand Down Expand Up @@ -387,7 +394,7 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex
{
if (ClientState::AwaitingResponse == mState)
{
mpDelegate->ReportError(this, CHIP_ERROR_TIMEOUT);
mpDelegate->ReadError(this, CHIP_ERROR_TIMEOUT);
}
}
ShutdownInternal(CHIP_ERROR_TIMEOUT);
Expand Down
15 changes: 9 additions & 6 deletions src/app/ReadClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ class ReadClient : public Messaging::ExchangeDelegate
/**
* Send a Read Request. There can be one Read Request outstanding on a given ReadClient.
* If SendReadRequest returns success, no more Read Requests can be sent on this ReadClient
* until the corresponding InteractionModelDelegate::ReportProcessed or InteractionModelDelegate::ReportError
* until the corresponding InteractionModelDelegate::ReportProcessed or InteractionModelDelegate::ReadError
* call happens with guarantee.
*
* Client can specify the maximum time to wait for response (in milliseconds) via timeout parameter.
Expand Down Expand Up @@ -120,14 +120,17 @@ class ReadClient : public Messaging::ExchangeDelegate
* Check if current read client is being used
*
*/
bool IsFree() const { return mState == ClientState::Uninitialized; };
bool IsFree() const { return mState == ClientState::Uninitialized; }

CHIP_ERROR GenerateEventPathList(ReadRequest::Builder & aRequest, EventPathParams * apEventPathParamsList,
size_t aEventPathParamsListSize, EventNumber & aEventNumber);
CHIP_ERROR GenerateAttributePathList(ReadRequest::Builder & aRequest, AttributePathParams * apAttributePathParamsList,
size_t aAttributePathParamsListSize);
CHIP_ERROR GenerateEventPathList(EventPathList::Builder & aEventPathListBuilder, EventPathParams * apEventPathParamsList,
size_t aEventPathParamsListSize);
CHIP_ERROR GenerateAttributePathList(AttributePathList::Builder & aAttributeathListBuilder,
AttributePathParams * apAttributePathParamsList, size_t aAttributePathParamsListSize);
CHIP_ERROR ProcessAttributeDataList(TLV::TLVReader & aAttributeDataListReader);

void SetExchangeContext(Messaging::ExchangeContext * apExchangeContext) { mpExchangeCtx = apExchangeContext; }
void ClearExchangeContext() { mpExchangeCtx = nullptr; }

void MoveToState(const ClientState aTargetState);
CHIP_ERROR ProcessReportData(System::PacketBufferHandle && aPayload);
CHIP_ERROR AbortExistingExchangeContext();
Expand Down
41 changes: 11 additions & 30 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ CHIP_ERROR ReadHandler::Init(InteractionModelDelegate * apDelegate, Messaging::E

void ReadHandler::Shutdown()
{
if (IsReporting())
{
InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
}
InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpAttributeClusterInfoList);
InteractionModelEngine::GetInstance()->ReleaseClusterInfoList(mpEventClusterInfoList);
mpExchangeCtx = nullptr;
Expand Down Expand Up @@ -84,28 +88,12 @@ CHIP_ERROR ReadHandler::OnStatusReport(Messaging::ExchangeContext * apExchangeCo
Protocols::SecureChannel::StatusReport statusReport;
err = statusReport.Parse(std::move(aPayload));
SuccessOrExit(err);
ChipLogProgress(DataManagement, "Receive Status Report, protocol id is %" PRIu32 ", protocol code is %" PRIu16,
statusReport.GetProtocolId(), statusReport.GetProtocolCode());
if ((statusReport.GetProtocolId() == Protocols::InteractionModel::Id.ToFullyQualifiedSpecForm()) &&
(statusReport.GetProtocolCode() == to_underlying(Protocols::InteractionModel::ProtocolCode::Success)))
{
switch (mState)
{
case HandlerState::Reporting:
InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
break;
case HandlerState::Reportable:
case HandlerState::Initialized:
case HandlerState::Uninitialized:
default:
err = CHIP_ERROR_INCORRECT_STATE;
break;
}
}
else
{
err = CHIP_ERROR_INVALID_ARGUMENT;
}
ChipLogProgress(DataManagement, "in state %s, receive status report, protocol id is %" PRIu32 ", protocol code is %" PRIu16,
GetStateStr(), statusReport.GetProtocolId(), statusReport.GetProtocolCode());
VerifyOrExit((statusReport.GetProtocolId() == Protocols::InteractionModel::Id.ToFullyQualifiedSpecForm()) &&
(statusReport.GetProtocolCode() == to_underlying(Protocols::InteractionModel::ProtocolCode::Success)),
err = CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrExit(IsReporting(), err = CHIP_ERROR_INCORRECT_STATE);

exit:
Shutdown();
Expand Down Expand Up @@ -154,10 +142,6 @@ CHIP_ERROR ReadHandler::OnUnknownMsgType(Messaging::ExchangeContext * apExchange
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload)
{
ChipLogDetail(DataManagement, "Msg type %d not supported", aPayloadHeader.GetMessageType());
if (IsReporting())
{
InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
}
Shutdown();
return CHIP_ERROR_INVALID_MESSAGE_TYPE;
}
Expand All @@ -166,10 +150,6 @@ void ReadHandler::OnResponseTimeout(Messaging::ExchangeContext * apExchangeConte
{
ChipLogProgress(DataManagement, "Time out! failed to receive status response from Exchange: %d",
apExchangeContext->GetExchangeId());
if (IsReporting())
{
InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
}
Shutdown();
}

Expand Down Expand Up @@ -289,6 +269,7 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathList::Parser & aAt
err = InteractionModelEngine::GetInstance()->PushFront(mpAttributeClusterInfoList, clusterInfo);
SuccessOrExit(err);
mpAttributeClusterInfoList->SetDirty();
SetInitialReport();
}
// if we have exhausted this container
if (CHIP_END_OF_TLV == err)
Expand Down
5 changes: 5 additions & 0 deletions src/app/ReadHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,9 @@ class ReadHandler : public Messaging::ExchangeDelegate
// is larger than current self vended event number
void MoveToNextScheduledDirtyPriority();

bool IsInitialReport() { return mInitialReport; }
void ClearInitialReport() { mInitialReport = false; }

private:
enum class HandlerState
{
Expand All @@ -128,6 +131,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
void OnResponseTimeout(Messaging::ExchangeContext * apExchangeContext) override;
CHIP_ERROR OnUnknownMsgType(Messaging::ExchangeContext * apExchangeContext, const PacketHeader & aPacketHeader,
const PayloadHeader & aPayloadHeader, System::PacketBufferHandle && aPayload);
void SetInitialReport() { mInitialReport = true; }
void MoveToState(const HandlerState aTargetState);

const char * GetStateStr() const;
Expand All @@ -150,6 +154,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
// The last schedule event number snapshoted in the beginning when preparing to fill new events to reports
EventNumber mLastScheduledEventNumber[kNumPriorityLevel];
InteractionModelDelegate * mpDelegate = nullptr;
bool mInitialReport = false;
};
} // namespace app
} // namespace chip
26 changes: 16 additions & 10 deletions src/app/reporting/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ Engine::RetrieveClusterData(AttributeDataElement::Builder & aAttributeDataElemen
err = attributePathBuilder.GetError();
SuccessOrExit(err);

ChipLogDetail(DataManagement, "<RE:Run> Cluster %" PRIx32 ", Field %" PRIx32 " is dirty", aClusterInfo.mClusterId,
aClusterInfo.mFieldId);

err = ReadSingleClusterData(aClusterInfo, aAttributeDataElementBuilder.GetWriter(), nullptr /* data exists */);
SuccessOrExit(err);
aAttributeDataElementBuilder.MoreClusterData(false);
Expand Down Expand Up @@ -98,15 +101,16 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeDataList(ReportData::Builder &
if (clusterInfo->IsDirty())
{
AttributeDataElement::Builder attributeDataElementBuilder = attributeDataList.CreateAttributeDataElementBuilder();
ChipLogDetail(DataManagement, "<RE:Run> Cluster " ChipLogFormatMEI ", Field %" PRIx32 " is dirty",
ChipLogValueMEI(clusterInfo->mClusterId), clusterInfo->mFieldId);
// Retrieve data for this cluster instance and clear its dirty flag.
err = RetrieveClusterData(attributeDataElementBuilder, *clusterInfo);
VerifyOrExit(err == CHIP_NO_ERROR,
ChipLogError(DataManagement, "<RE:Run> Error retrieving data from cluster, aborting"));
attributeClean = false;
if (apReadHandler->IsInitialReport())
{
// Retrieve data for this cluster instance and clear its dirty flag.
err = RetrieveClusterData(attributeDataElementBuilder, *clusterInfo);
VerifyOrExit(err == CHIP_NO_ERROR,
ChipLogError(DataManagement, "<RE:Run> Error retrieving data from cluster, aborting"));
attributeClean = false;
}
clusterInfo->ClearDirty();
}

clusterInfo = clusterInfo->mpNext;
}
attributeDataList.EndOfAttributeDataList();
Expand Down Expand Up @@ -323,8 +327,10 @@ void Engine::Run()
if (readHandler->IsReportable())
{
CHIP_ERROR err = BuildAndSendSingleReportData(readHandler);
ChipLogFunctError(err);
return;
if (err != CHIP_NO_ERROR)
{
return;
}
}
numReadHandled++;
mCurReadHandlerIdx = (mCurReadHandlerIdx + 1) % CHIP_IM_MAX_NUM_READ_HANDLER;
Expand Down
19 changes: 11 additions & 8 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,8 @@ void TestReadInteraction::TestReadClientGenerateAttributePathList(nlTestSuite *
attributePathParams[0].mFlags.Set(AttributePathParams::Flags::kFieldIdValid);
attributePathParams[1].mFlags.Set(AttributePathParams::Flags::kFieldIdValid);
attributePathParams[1].mFlags.Set(AttributePathParams::Flags::kListIndexValid);
err = readClient.GenerateAttributePathList(request, attributePathParams, 2 /*aAttributePathParamsListSize*/);
AttributePathList::Builder & attributePathListBuilder = request.CreateAttributePathListBuilder();
err = readClient.GenerateAttributePathList(attributePathListBuilder, attributePathParams, 2 /*aAttributePathParamsListSize*/);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
}

Expand All @@ -383,7 +384,8 @@ void TestReadInteraction::TestReadClientGenerateInvalidAttributePathList(nlTestS
AttributePathParams attributePathParams[2];
attributePathParams[0].mFlags.Set(AttributePathParams::Flags::kFieldIdValid);
attributePathParams[1].mFlags.Set(AttributePathParams::Flags::kListIndexValid);
err = readClient.GenerateAttributePathList(request, attributePathParams, 2 /*aAttributePathParamsListSize*/);
AttributePathList::Builder & attributePathListBuilder = request.CreateAttributePathListBuilder();
err = readClient.GenerateAttributePathList(attributePathListBuilder, attributePathParams, 2 /*aAttributePathParamsListSize*/);
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH);
}

Expand Down Expand Up @@ -469,8 +471,7 @@ void TestReadInteraction::TestReadClientGenerateOneEventPathList(nlTestSuite * a
System::PacketBufferHandle msgBuf;
System::PacketBufferTLVWriter writer;
ReadRequest::Builder request;
chip::EventNumber eventNumber = 0;
msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
NL_TEST_ASSERT(apSuite, !msgBuf.IsNull());
writer.Init(std::move(msgBuf));
err = request.Init(&writer);
Expand All @@ -485,7 +486,8 @@ void TestReadInteraction::TestReadClientGenerateOneEventPathList(nlTestSuite * a
eventPathParams[0].mClusterId = 3;
eventPathParams[0].mEventId = 4;

err = readClient.GenerateEventPathList(request, eventPathParams, 1 /*aEventPathParamsListSize*/, eventNumber);
EventPathList::Builder & eventPathListBuilder = request.CreateEventPathListBuilder();
err = readClient.GenerateEventPathList(eventPathListBuilder, eventPathParams, 1 /*aEventPathParamsListSize*/);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

request.EndOfReadRequest();
Expand Down Expand Up @@ -518,8 +520,7 @@ void TestReadInteraction::TestReadClientGenerateTwoEventPathList(nlTestSuite * a
System::PacketBufferHandle msgBuf;
System::PacketBufferTLVWriter writer;
ReadRequest::Builder request;
chip::EventNumber eventNumber = 0;
msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes);
NL_TEST_ASSERT(apSuite, !msgBuf.IsNull());
writer.Init(std::move(msgBuf));
err = request.Init(&writer);
Expand All @@ -538,7 +539,9 @@ void TestReadInteraction::TestReadClientGenerateTwoEventPathList(nlTestSuite * a
eventPathParams[1].mEndpointId = 2;
eventPathParams[1].mClusterId = 3;
eventPathParams[1].mEventId = 5;
err = readClient.GenerateEventPathList(request, eventPathParams, 2 /*aEventPathParamsListSize*/, eventNumber);

EventPathList::Builder & eventPathListBuilder = request.CreateEventPathListBuilder();
err = readClient.GenerateEventPathList(eventPathListBuilder, eventPathParams, 2 /*aEventPathParamsListSize*/);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);

request.EndOfReadRequest();
Expand Down
Loading

0 comments on commit 22d906d

Please sign in to comment.