Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ReadHandler] Rename and comment #27269

Merged
merged 8 commits into from
Jun 19, 2023
Merged

[ReadHandler] Rename and comment #27269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc9fd1e
Updated function names and description related to the scheduling of s…
lpbeliveau-silabs Jun 14, 2023
7ef6dad
Apply suggestions from code review
lpbeliveau-silabs Jun 15, 2023
be49678
Fixed the syntax for the IsReportableNow description
lpbeliveau-silabs Jun 15, 2023
42e112f
Added missing parameter and return description in comments
lpbeliveau-silabs Jun 15, 2023
d9f617f
Apply suggestions from code review
lpbeliveau-silabs Jun 16, 2023
e963a90
Reverted name changes for flag set and clear, reworded comments as su…
lpbeliveau-silabs Jun 16, 2023
b72536d
Apply suggestions from code review
lpbeliveau-silabs Jun 19, 2023
7a37b57
Restyled by clang-format
restyled-commits Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions examples/all-clusters-app/nxp/mw320/main.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to elapse"

// 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
Loading