Skip to content

Commit

Permalink
Correctly call OnReportBegin/End. (#12071)
Browse files Browse the repository at this point in the history
With basic chunking support already in the tree, This fixes up the logic bug where
OnReportBegin/End were not being called at the right times on incoming
report processing.

This also fixes up some terminology confusion in ReadClient/Handler
where the use of `mIsInitialReport` meant two different things in those
two contexts: In ReadClient, it literally meant the first report in a
train of priming reports, while on the handler, it referred to the
entire priming phase of reporting.
  • Loading branch information
mrjerryjohns authored and pull[bot] committed May 18, 2022
1 parent 4f266ac commit 1788470
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 16 deletions.
14 changes: 12 additions & 2 deletions src/app/ReadClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -391,12 +391,18 @@ CHIP_ERROR ReadClient::ProcessReportData(System::PacketBufferHandle && aPayload)
TLV::TLVReader attributeReportIBsReader;
attributeReportIBs.GetReader(&attributeReportIBsReader);

mpCallback->OnReportBegin(this);
if (IsInitialReport())
{
mpCallback->OnReportBegin(this);
}

err = ProcessAttributeReportIBs(attributeReportIBsReader);
SuccessOrExit(err);

mpCallback->OnReportEnd(this);
if (!mPendingMoreChunks)
{
mpCallback->OnReportEnd(this);
}
}

exit:
Expand Down Expand Up @@ -500,6 +506,10 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo

ConcreteDataAttributePath attributePath(clusterInfo.mEndpointId, clusterInfo.mClusterId, clusterInfo.mAttributeId);

//
// TODO: Add support for correctly handling appends/updates whenever list chunking support
// on the server side is added.
//
if (dataReader.GetType() == TLV::kTLVType_Array)
{
attributePath.mListOp = ConcreteDataAttributePath::ListOperation::ReplaceAll;
Expand Down
14 changes: 7 additions & 7 deletions src/app/ReadHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ CHIP_ERROR ReadHandler::Init(Messaging::ExchangeManager * apExchangeMgr, Interac
mpAttributeClusterInfoList = nullptr;
mpEventClusterInfoList = nullptr;
mCurrentPriority = PriorityLevel::Invalid;
mInitialReport = true;
mIsPrimingReports = true;
MoveToState(HandlerState::Initialized);
mpDelegate = apDelegate;
mSubscriptionId = 0;
Expand Down Expand Up @@ -103,7 +103,7 @@ void ReadHandler::Shutdown(ShutdownOptions aOptions)
mpAttributeClusterInfoList = nullptr;
mpEventClusterInfoList = nullptr;
mCurrentPriority = PriorityLevel::Invalid;
mInitialReport = false;
mIsPrimingReports = false;
mpDelegate = nullptr;
mHoldReport = false;
mDirty = false;
Expand Down Expand Up @@ -156,7 +156,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
else if (IsSubscriptionType())
{
InteractionModelEngine::GetInstance()->GetReportingEngine().OnReportConfirm();
if (IsInitialReport())
if (IsPriming())
{
err = SendSubscribeResponse();
mpExchangeCtx = nullptr;
Expand Down Expand Up @@ -192,7 +192,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange
CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks)
{
VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE);
if (IsInitialReport() || IsChunkedReport())
if (IsPriming() || IsChunkedReport())
{
mSessionHandle.SetValue(mpExchangeCtx->GetSessionHandle());
}
Expand Down Expand Up @@ -221,7 +221,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b

if (err == CHIP_NO_ERROR)
{
if (IsSubscriptionType() && !IsInitialReport())
if (IsSubscriptionType() && !IsPriming())
{
err = RefreshSubscribeSyncTimer();
}
Expand Down Expand Up @@ -396,7 +396,7 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathIBs::Parser & aAtt
SuccessOrExit(err);
err = InteractionModelEngine::GetInstance()->PushFront(mpAttributeClusterInfoList, clusterInfo);
SuccessOrExit(err);
mInitialReport = true;
mIsPrimingReports = true;
}
// if we have exhausted this container
if (CHIP_END_OF_TLV == err)
Expand Down Expand Up @@ -539,7 +539,7 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse()
VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);

ReturnErrorOnFailure(RefreshSubscribeSyncTimer());
mInitialReport = false;
mIsPrimingReports = false;
MoveToState(HandlerState::GeneratingReports);
if (mpDelegate != nullptr)
{
Expand Down
16 changes: 10 additions & 6 deletions src/app/ReadHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
bool IsReadType() { return mInteractionType == InteractionType::Read; }
bool IsSubscriptionType() { return mInteractionType == InteractionType::Subscribe; }
bool IsChunkedReport() { return mIsChunkedReport; }
bool IsInitialReport() { return mInitialReport; }
bool IsPriming() { return mIsPrimingReports; }
bool IsActiveSubscription() const { return mActiveSubscription; }
CHIP_ERROR OnSubscribeRequest(Messaging::ExchangeContext * apExchangeContext, System::PacketBufferHandle && aPayload);
void GetSubscriptionId(uint64_t & aSubscriptionId) { aSubscriptionId = mSubscriptionId; }
Expand Down Expand Up @@ -193,11 +193,15 @@ class ReadHandler : public Messaging::ExchangeDelegate
EventNumber mLastScheduledEventNumber[kNumPriorityLevel];
Messaging::ExchangeManager * mpExchangeMgr = nullptr;
InteractionModelDelegate * mpDelegate = nullptr;
bool mInitialReport = false;
InteractionType mInteractionType = InteractionType::Read;
uint64_t mSubscriptionId = 0;
uint16_t mMinIntervalFloorSeconds = 0;
uint16_t mMaxIntervalCeilingSeconds = 0;

// Tracks whether we're in the initial phase of receiving priming
// reports, which is always true for reads and true for subscriptions
// prior to receiving a subscribe response.
bool mIsPrimingReports = false;
InteractionType mInteractionType = InteractionType::Read;
uint64_t mSubscriptionId = 0;
uint16_t mMinIntervalFloorSeconds = 0;
uint16_t mMaxIntervalCeilingSeconds = 0;
Optional<SessionHandle> mSessionHandle;
bool mHoldReport = false;
bool mDirty = false;
Expand Down
2 changes: 1 addition & 1 deletion src/app/reporting/Engine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu
for (; apReadHandler->GetAttributePathExpandIterator()->Get(readPath);
apReadHandler->GetAttributePathExpandIterator()->Next())
{
if (!apReadHandler->IsInitialReport())
if (!apReadHandler->IsPriming())
{
bool concretePathDirty = false;
// TODO: Optimize this implementation by making the iterator only emit intersected paths.
Expand Down

0 comments on commit 1788470

Please sign in to comment.