-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Add idea for handling Q quality #34522
base: master
Are you sure you want to change the base?
Conversation
CHIP_IM_STATUS_CODE(SuppressReport , SUPPRESS_REPORT , 0xcd) | ||
CHIP_IM_STATUS_CODE(ForceReport , FORCE_REPORT , 0xce) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the list of spec-defined status codes. We can't just randomly add things to it....
In particular, 0xCD and 0xCE are in fact defined in the 1.4 spec and just haven't been added here yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. Seems like this would be useful to other clusters having Q-quality attributes and not using the AttibuteAccessInterface?
case PercentCurrent::Id: { | ||
res = Status::Success; | ||
// ReportRequiredByWhatever - Something that get's triggered on edges to force reporting | ||
if (ReportRequiredByWhatever) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but where do we store the state to compute this value? And at that point, it seems like it should also just store the last-reported PercentCurrent value and cluster users should call a cluster API to set the attribute instead of directly poking the attribute store....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that would be the next step. I had similar thoughts about local storage. Wanted to see if the overall approach (enforce in the PreAttributeChangeCallback with support from the IM for the actual reporting or not) seemed sane.
else | ||
{ | ||
SomeTimeType currentTime = GetTimeNowSomehow(); | ||
if (currentTime - percentCurrentQTracking[ep].lastUpdateTime >= 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number, worth defining somewhere
bool moveStarted; | ||
DataModel::Nullable<chip::Percent> startPercent; | ||
DataModel::Nullable<chip::Percent> endPercent; | ||
auto lastUpdateTime; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth explicitly initialising to zero in case we get a current update without a setting update
{ | ||
// need a report at the start of a move. | ||
percentCurrentQTracking[ep].moveStarted = false; | ||
percentCurrentQTracking[ep].lastUpdateTime = GetTimeNowSomehow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to store this stuff out-of-band like this anyway, it's probably a good idea to use src/app/cluster-building-blocks/QuieterReporting.h bits to store a bunch of this state.
PR #34522: Size comparison from 4205cc3 to 0231948 Full report (25 builds for bl602, bl702, bl702l, cc13x4_26x4, efr32, nxp, qpg, stm32)
|
PR #34522: Size comparison from 4205cc3 to 586f291 Full report (75 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
No description provided.