Add idea for handling Q quality #34522
Draft
+121
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Comment on lines
77
to
78
| CHIP_IM_STATUS_CODE(SuppressReport , SUPPRESS_REPORT , 0xcd) | ||
| CHIP_IM_STATUS_CODE(ForceReport , FORCE_REPORT , 0xce) |
There was a problem hiding this comment.
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.
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.
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.
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.
rbultman
commented
Jul 26, 2024
mhazley
reviewed
Jul 29, 2024
| else | ||
| { | ||
| SomeTimeType currentTime = GetTimeNowSomehow(); | ||
| if (currentTime - percentCurrentQTracking[ep].lastUpdateTime >= 1) { |
There was a problem hiding this comment.
Magic number, worth defining somewhere
mhazley
reviewed
Jul 29, 2024
| bool moveStarted; | ||
| DataModel::Nullable<chip::Percent> startPercent; | ||
| DataModel::Nullable<chip::Percent> endPercent; | ||
| auto lastUpdateTime; |
There was a problem hiding this comment.
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.
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)
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.