Skip to content

Commit

Permalink
[ReadHandler] Rename and comment (#27269)
Browse files Browse the repository at this point in the history 
* Updated function names and description related to the scheduling of subcription reports in the ReadHandler to explicit its functionnality

* Apply suggestions from code review

Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com>

* Fixed the syntax for the IsReportableNow description

* Added missing parameter and return description in comments

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Reverted name changes for flag set and clear, reworded comments as suggested

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

* Restyled by clang-format

---------

Co-authored-by: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Restyled.io <commits@restyled.io>
  • Loading branch information
@lpbeliveau-silabs @mkardous-silabs
@bzbarsky-apple @restyled-commits @pull
4 people authored and pull[bot] committed Oct 12, 2023
1 parent f8e0807 commit 3460097
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 92 deletions.
12 changes: 6 additions & 6 deletions examples/all-clusters-app/nxp/mw320/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
//#include "FreeRTOS.h"
//#include "task.h"
// #include "FreeRTOS.h"
// #include "task.h"

#include <lib/shell/Engine.h>

Expand All @@ -28,7 +28,7 @@
#include <lib/support/CHIPArgParser.hpp>
#include <lib/support/CodeUtils.h>

//#include <lib/support/RandUtils.h> //==> rm from TE7.5
// #include <lib/support/RandUtils.h> //==> rm from TE7.5
#include <app-common/zap-generated/attributes/Accessors.h>
#include <app-common/zap-generated/ids/Attributes.h>
#include <app-common/zap-generated/ids/Clusters.h>
Expand Down Expand Up @@ -59,9 +59,9 @@
#include "app/clusters/ota-requestor/DefaultOTARequestor.h"
#include "app/clusters/ota-requestor/DefaultOTARequestorDriver.h"
#include "app/clusters/ota-requestor/DefaultOTARequestorStorage.h"
//#include <app/clusters/ota-requestor/DefaultOTARequestorUserConsent.h>
// #include <app/clusters/ota-requestor/DefaultOTARequestorUserConsent.h>
#include "platform/nxp/mw320/OTAImageProcessorImpl.h"
//#include "app/clusters/ota-requestor/OTARequestorDriver.h"
// #include "app/clusters/ota-requestor/OTARequestorDriver.h"

// for ota module test
#include "mw320_ota.h"
Expand Down Expand Up @@ -1528,7 +1528,7 @@ static void OnSwitchAttributeChangeCallback(EndpointId endpointId, AttributeId a
ReadHandler * phandler = pimEngine->ActiveHandlerAt(i);
if (phandler->IsType(chip::app::ReadHandler::InteractionType::Subscribe) &&
(phandler->IsGeneratingReports() || phandler->IsAwaitingReportResponse())) {
phandler->UnblockUrgentEventDelivery();
phandler->ForceDirtyState();
do_sendrpt = true;
break;
}
Expand Down
52 changes: 26 additions & 26 deletions src/app/ReadHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,10 +124,10 @@ ReadHandler::~ReadHandler()
if (IsType(InteractionType::Subscribe))
{
InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer(
OnUnblockHoldReportCallback, this);
MinIntervalExpiredCallback, this);

InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer(
OnRefreshSubscribeTimerSyncCallback, this);
MaxIntervalExpiredCallback, this);
}

if (IsAwaitingReportResponse())
Expand Down Expand Up @@ -248,7 +248,7 @@ CHIP_ERROR ReadHandler::OnStatusResponse(Messaging::ExchangeContext * apExchange

CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aStatus)
{
VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnLogError(IsReportableNow(), CHIP_ERROR_INCORRECT_STATE);
if (IsPriming() || IsChunkedReport())
{
mSessionHandle.Grab(mExchangeCtx->GetSessionHandle());
Expand All @@ -272,7 +272,7 @@ CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aSt

CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks)
{
VerifyOrReturnLogError(IsReportable(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnLogError(IsReportableNow(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrDie(!IsAwaitingReportResponse()); // Should not be reportable!
if (IsPriming() || IsChunkedReport())
{
Expand Down Expand Up @@ -319,10 +319,10 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b

if (IsType(InteractionType::Subscribe) && !IsPriming())
{
// Ignore the error from RefreshSubscribeSyncTimer. If we've
// Ignore the error from UpdateReportTimer. If we've
// successfully sent the message, we need to return success from
// this method.
RefreshSubscribeSyncTimer();
UpdateReportTimer();
}
}
if (!aMoreChunks)
Expand Down Expand Up @@ -591,7 +591,7 @@ void ReadHandler::MoveToState(const HandlerState aTargetState)
// If we just unblocked sending reports, let's go ahead and schedule the reporting
// engine to run to kick that off.
//
if (aTargetState == HandlerState::GeneratingReports && IsReportable())
if (aTargetState == HandlerState::GeneratingReports && IsReportableNow())
{
InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
}
Expand Down Expand Up @@ -634,7 +634,7 @@ CHIP_ERROR ReadHandler::SendSubscribeResponse()
ReturnErrorOnFailure(writer.Finalize(&packet));
VerifyOrReturnLogError(mExchangeCtx, CHIP_ERROR_INCORRECT_STATE);

ReturnErrorOnFailure(RefreshSubscribeSyncTimer());
ReturnErrorOnFailure(UpdateReportTimer());

ClearStateFlag(ReadHandlerFlags::PrimingReports);
return mExchangeCtx->SendMessage(Protocols::InteractionModel::MsgType::SubscribeResponse, std::move(packet));
Expand Down Expand Up @@ -753,42 +753,42 @@ void ReadHandler::PersistSubscription()
}
}

void ReadHandler::OnUnblockHoldReportCallback(System::Layer * apSystemLayer, void * apAppState)
void ReadHandler::MinIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState)
{
VerifyOrReturn(apAppState != nullptr);
ReadHandler * readHandler = static_cast<ReadHandler *>(apAppState);
ChipLogDetail(DataManagement, "Unblock report hold after min %d seconds", readHandler->mMinIntervalFloorSeconds);
readHandler->ClearStateFlag(ReadHandlerFlags::HoldReport);
readHandler->ClearStateFlag(ReadHandlerFlags::WaitingUntilMinInterval);
InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer(
System::Clock::Seconds16(readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds),
OnRefreshSubscribeTimerSyncCallback, readHandler);
System::Clock::Seconds16(readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds), MaxIntervalExpiredCallback,
readHandler);
}

void ReadHandler::OnRefreshSubscribeTimerSyncCallback(System::Layer * apSystemLayer, void * apAppState)
void ReadHandler::MaxIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState)
{
VerifyOrReturn(apAppState != nullptr);
ReadHandler * readHandler = static_cast<ReadHandler *>(apAppState);
readHandler->ClearStateFlag(ReadHandlerFlags::HoldSync);
readHandler->ClearStateFlag(ReadHandlerFlags::WaitingUntilMaxInterval);
ChipLogProgress(DataManagement, "Refresh subscribe timer sync after %d seconds",
readHandler->mMaxInterval - readHandler->mMinIntervalFloorSeconds);
}

CHIP_ERROR ReadHandler::RefreshSubscribeSyncTimer()
CHIP_ERROR ReadHandler::UpdateReportTimer()
{
InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer(
OnUnblockHoldReportCallback, this);
MinIntervalExpiredCallback, this);
InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->CancelTimer(
OnRefreshSubscribeTimerSyncCallback, this);
MaxIntervalExpiredCallback, this);

if (!IsChunkedReport())
{
ChipLogProgress(DataManagement, "Refresh Subscribe Sync Timer with min %d seconds and max %d seconds",
mMinIntervalFloorSeconds, mMaxInterval);
SetStateFlag(ReadHandlerFlags::HoldReport);
SetStateFlag(ReadHandlerFlags::HoldSync);
SetStateFlag(ReadHandlerFlags::WaitingUntilMinInterval);
SetStateFlag(ReadHandlerFlags::WaitingUntilMaxInterval);
ReturnErrorOnFailure(
InteractionModelEngine::GetInstance()->GetExchangeManager()->GetSessionManager()->SystemLayer()->StartTimer(
System::Clock::Seconds16(mMinIntervalFloorSeconds), OnUnblockHoldReportCallback, this));
System::Clock::Seconds16(mMinIntervalFloorSeconds), MinIntervalExpiredCallback, this));
}

return CHIP_NO_ERROR;
Expand All @@ -800,13 +800,13 @@ void ReadHandler::ResetPathIterator()
mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState();
}

void ReadHandler::SetDirty(const AttributePathParams & aAttributeChanged)
void ReadHandler::AttributePathIsDirty(const AttributePathParams & aAttributeChanged)
{
ConcreteAttributePath path;

mDirtyGeneration = InteractionModelEngine::GetInstance()->GetReportingEngine().GetDirtySetGeneration();

// We won't reset the path iterator for every SetDirty call to reduce the number of full data reports.
// We won't reset the path iterator for every AttributePathIsDirty call to reduce the number of full data reports.
// The iterator will be reset after finishing each report session.
//
// Here we just reset the iterator to the beginning of the current cluster, if the dirty path affects it.
Expand All @@ -829,7 +829,7 @@ void ReadHandler::SetDirty(const AttributePathParams & aAttributeChanged)
mAttributeEncoderState = AttributeValueEncoder::AttributeEncodeState();
}

if (IsReportable())
if (IsReportableNow())
{
InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
}
Expand All @@ -844,17 +844,17 @@ Transport::SecureSession * ReadHandler::GetSession() const
return mSessionHandle->AsSecureSession();
}

void ReadHandler::UnblockUrgentEventDelivery()
void ReadHandler::ForceDirtyState()
{
SetStateFlag(ReadHandlerFlags::ForceDirty);
}

void ReadHandler::SetStateFlag(ReadHandlerFlags aFlag, bool aValue)
{
bool oldReportable = IsReportable();
bool oldReportable = IsReportableNow();
mFlags.Set(aFlag, aValue);
// If we became reportable, schedule a reporting run.
if (!oldReportable && IsReportable())
if (!oldReportable && IsReportableNow())
{
InteractionModelEngine::GetInstance()->GetReportingEngine().ScheduleRun();
}
Expand Down
60 changes: 38 additions & 22 deletions src/app/ReadHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,15 +212,15 @@ class ReadHandler : public Messaging::ExchangeDelegate

enum class ReadHandlerFlags : uint8_t
{
// mHoldReport is used to prevent subscription data delivery while we are
// WaitingUntilMinInterval is used to prevent subscription data delivery while we are
// waiting for the min reporting interval to elapse.
HoldReport = (1 << 0),
WaitingUntilMinInterval = (1 << 0),

// mHoldSync is used to prevent subscription empty report delivery while we
// are waiting for the max reporting interval to elaps. When mHoldSync
// WaitingUntilMaxInterval is used to prevent subscription empty report delivery while we
// are waiting for the max reporting interval to elaps. When WaitingUntilMaxInterval
// becomes false, we are allowed to send an empty report to keep the
// subscription alive on the client.
HoldSync = (1 << 1),
WaitingUntilMaxInterval = (1 << 1),

// The flag indicating we are in the middle of a series of chunked report messages, this flag will be cleared during
// sending last chunked message.
Expand Down Expand Up @@ -290,13 +290,16 @@ class ReadHandler : public Messaging::ExchangeDelegate
bool IsFromSubscriber(Messaging::ExchangeContext & apExchangeContext) const;

bool IsIdle() const { return mState == HandlerState::Idle; }
bool IsReportable() const

/// @brief Returns whether the ReadHandler is in a state where it can immediately send a report. This function
/// is used to determine whether a report generation should be scheduled for the handler.
bool IsReportableNow() const
{
// Important: Anything that changes the state IsReportable depends on in
// a way that causes IsReportable to become true must call ScheduleRun
// Important: Anything that changes the state IsReportableNow depends on in
// a way that causes IsReportableNow to become true must call ScheduleRun
// on the reporting engine.
return mState == HandlerState::GeneratingReports && !mFlags.Has(ReadHandlerFlags::HoldReport) &&
(IsDirty() || !mFlags.Has(ReadHandlerFlags::HoldSync));
return mState == HandlerState::GeneratingReports && !mFlags.Has(ReadHandlerFlags::WaitingUntilMinInterval) &&
(IsDirty() || !mFlags.Has(ReadHandlerFlags::WaitingUntilMaxInterval));
}
bool IsGeneratingReports() const { return mState == HandlerState::GeneratingReports; }
bool IsAwaitingReportResponse() const { return mState == HandlerState::AwaitingReportResponse; }
Expand All @@ -323,10 +326,10 @@ class ReadHandler : public Messaging::ExchangeDelegate
void GetSubscriptionId(SubscriptionId & aSubscriptionId) const { aSubscriptionId = mSubscriptionId; }
AttributePathExpandIterator * GetAttributePathExpandIterator() { return &mAttributePathExpandIterator; }

/**
* Notify the read handler that a set of attribute paths has been marked dirty.
*/
void SetDirty(const AttributePathParams & aAttributeChanged);
/// @brief Notifies the read handler that a set of attribute paths has been marked dirty. This will schedule a reporting engine
/// run if the change to the attribute path makes the ReadHandler reportable.
/// @param aAttributeChanged Path to the attribute that was changed.
void AttributePathIsDirty(const AttributePathParams & aAttributeChanged);
bool IsDirty() const
{
return (mDirtyGeneration > mPreviousReportsBeginGeneration) || mFlags.Has(ReadHandlerFlags::ForceDirty);
Expand All @@ -349,7 +352,10 @@ class ReadHandler : public Messaging::ExchangeDelegate

auto GetTransactionStartGeneration() const { return mTransactionStartGeneration; }

void UnblockUrgentEventDelivery();
/// @brief Forces the read handler into a dirty state, regardless of what's going on with attributes.
/// This can lead to scheduling of a reporting run immediately, if the min interval has been reached,
/// or after the min interval is reached if it has not yet been reached.
void ForceDirtyState();

const AttributeValueEncoder::AttributeEncodeState & GetAttributeEncodeState() const { return mAttributeEncoderState; }
void SetAttributeEncodeState(const AttributeValueEncoder::AttributeEncodeState & aState) { mAttributeEncoderState = aState; }
Expand Down Expand Up @@ -396,9 +402,12 @@ class ReadHandler : public Messaging::ExchangeDelegate
*/
void Close(CloseOptions options = CloseOptions::kDropPersistedSubscription);

static void OnUnblockHoldReportCallback(System::Layer * apSystemLayer, void * apAppState);
static void OnRefreshSubscribeTimerSyncCallback(System::Layer * apSystemLayer, void * apAppState);
CHIP_ERROR RefreshSubscribeSyncTimer();
/// @brief This function is called when the min interval timer has expired, it restarts the timer on a timeout equal to the
/// difference between the max interval and the min interval.
static void MinIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState);
static void MaxIntervalExpiredCallback(System::Layer * apSystemLayer, void * apAppState);
/// @brief This function is called when a report is sent and it restarts the min interval timer.
CHIP_ERROR UpdateReportTimer();
CHIP_ERROR SendSubscribeResponse();
CHIP_ERROR ProcessSubscribeRequest(System::PacketBufferHandle && aPayload);
CHIP_ERROR ProcessReadRequest(System::PacketBufferHandle && aPayload);
Expand All @@ -416,8 +425,15 @@ class ReadHandler : public Messaging::ExchangeDelegate

void PersistSubscription();

// Helpers for managing our state flags properly.
/// @brief Modifies a state flag in the read handler. If the read handler went from a
/// non-reportable state to a reportable state, schedules a reporting engine run.
/// @param aFlag Flag to set
/// @param aValue Flag new value
void SetStateFlag(ReadHandlerFlags aFlag, bool aValue = true);

/// @brief This function call SetStateFlag with the flag value set to false, thus possibly emitting a report
/// generation.
/// @param aFlag Flag to clear
void ClearStateFlag(ReadHandlerFlags aFlag);

// Helpers for continuing the subscription resumption
Expand All @@ -436,7 +452,7 @@ class ReadHandler : public Messaging::ExchangeDelegate
// current generation when we started sending the last set reports that we completed.
//
// This allows us to reset the iterator to the beginning of the current
// cluster instead of the beginning of the whole report in SetDirty, without
// cluster instead of the beginning of the whole report in AttributePathIsDirty, without
// permanently missing dirty any paths.
uint64_t mDirtyGeneration = 0;

Expand All @@ -450,14 +466,14 @@ class ReadHandler : public Messaging::ExchangeDelegate
/*
* (mDirtyGeneration = b > a, this is a dirty read handler)
* +- Start Report -> mCurrentReportsBeginGeneration = c
* | +- SetDirty (Attribute Y) -> mDirtyGeneration = d
* | +- AttributePathIsDirty (Attribute Y) -> mDirtyGeneration = d
* | | +- Last Chunk -> mPreviousReportsBeginGeneration = mCurrentReportsBeginGeneration = c
* | | | +- (mDirtyGeneration = d) > (mPreviousReportsBeginGeneration = c), this is a dirty read handler
* | | | | Attribute X has a dirty generation less than c, Attribute Y has a dirty generation larger than c
* | | | | So Y will be included in the report but X will not be inclued in this report.
* -a--b--c------d-----e---f---> Generation
* | |
* | +- SetDirty (Attribute X) (mDirtyGeneration = b)
* | +- AttributePathIsDirty (Attribute X) (mDirtyGeneration = b)
* +- mPreviousReportsBeginGeneration
* For read handler, if mDirtyGeneration > mPreviousReportsBeginGeneration, then we regard it as a dirty read handler, and it
* should generate report on timeout reached.
Expand Down

0 comments on commit 3460097

Please sign in to comment.