[WIP] Add ListNotifications on DataModelProvider #37526
Open
+46
−4
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
|
Review changes with |
|
PR #37526: Size comparison from 9c8cb33 to 5d233a2 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Comment on lines
79
to
80
| kListWriteEnd, | ||
| kListWriteEndFinal |
There was a problem hiding this comment.
I have no idea what the difference between those values is. Needs documentation.
There was a problem hiding this comment.
if the difference if "writeWasSuccessful" variable, ListWriteSuccess/ListWriteFailure seems better. However then we need to describe what "failure" and "success" mean (unsure if the previous API really defined them either...)
There was a problem hiding this comment.
Yes, the "aWriteWasSuccessful" was the only difference since I wanted to account for that option, otherwise we could have only ListWriteBegin and ListWriteEnd. The current documentation in AAI is not really clear about the meaning of that variable.
| @@ -79,6 +79,14 @@ class Provider : public ProviderMetadataTree | |||
| /// - validation of ACL/timed interaction flags/writability, if those checks are desired. | |||
| virtual ActionReturnStatus WriteAttribute(const WriteAttributeRequest & request, AttributeValueDecoder & decoder) = 0; | |||
|
|
|||
| /// Indicates the start/end of a series of list operations. This function will be called either before the first | |||
| /// Write operation or after the last one of a series of consequence attribute data of the same attribute. | |||
There was a problem hiding this comment.
"consequence" seems like the wrong word here.
There was a problem hiding this comment.
Yeah, I wasn't sure about that word either but since it was in the previous documentation from AAI I just took it from there. I think that rewriting the documentation for this function will be better.
| /// Indicates the start/end of a series of list operations. This function will be called either before the first | ||
| /// Write operation or after the last one of a series of consequence attribute data of the same attribute. | ||
| /// | ||
| /// 1) This function will be called if the client tries to set a nullable list attribute to null. |
There was a problem hiding this comment.
This seems a mistake from my side, it was supposed to be "once" per write begin and once per write end. So for a whole write operation would be two calls.
| /// Write operation or after the last one of a series of consequence attribute data of the same attribute. | ||
| /// | ||
| /// 1) This function will be called if the client tries to set a nullable list attribute to null. | ||
| /// 2) This function will only be called once for a series of consequent attribute data (regardless the kind of list |
There was a problem hiding this comment.
Again, "consequent" does not seem like the right word, but I can't tell what this is actually trying to say.
There was a problem hiding this comment.
Changed the description, from what I understand those are "consecutive" writes.
src/app/WriteHandler.cpp
Outdated
Comment on lines
272
to
273
| writeWasSuccessful ? DataModel::ListWriteOperation::kListWriteEndFinal | ||
| : DataModel::ListWriteOperation::kListWriteEnd); |
There was a problem hiding this comment.
This does not accurately capture what happens now with the success/fail arg, right?
There was a problem hiding this comment.
I think if we replace s/kListWriteEndFinal/kListWriteSuccess and s/kListWriteEnd/kListWriteFailure we may get similar functionality in a bit of a more obvious ways. Then we have:
- kListWriteBegin - this would be the old "WriteBegin"
- kListWriteSuccess - this would be the old "WriteEnd(path, true)"
- kListWriteFailure - this would be the old "WriteEnd(path, false)"
However I would love to get documentation built on when these are called and how. We should transfer over the existing documentation (and adapt as needed with examples/explanation):
There was a problem hiding this comment.
Existing docs does not explain what aWriteWasSuccessful means ... does this mean that the WriteAttribute succeeded? Why is that important and why can't that be tracked by the AAI if it needs it?
There was a problem hiding this comment.
Digging a little in the past issues it seems that the intention of "aWriteWasSuccessful" is indeed to notify if the write went as intended (true) or if there were some issues with the operation (false) like timeouts or some other "abort" reasons.
andy31415
added a commit
to andy31415/connectedhomeip
that referenced
this pull request
Feb 12, 2025
This defines a ServerClusterInterface class along with a registry. Also slight update to build_coverage to run on more (specifically my) machines without errors. Implementation notes: - Registry does NOT allow anymore to have "wildcard register" on endpoints. This is because we expect to have attribute members (no more "free/implicit" storage from ember buffers). - Interface now includes metadata for attributes (AAI does not) and maintains the cluster version. - This does not yet include changes in project-chip#37526 (so no AAI replacement for list begin/end) as that interface gets finalized. It will have it once that PR is finalized and merged. - Registry has no "cache" but uses a "move found item to front of list". This is ok for individual requests however for full iterations it would be slower (about 2x if we always iterate over all clusters ... since then average search would be N instead of N/2 for a list that never changes). We can revisit this approach at any time This is currently unused. I expect usages to add flash & RAM cost, that will be slowly offset (especially RAM) as we move clusters around: - RAM for linked list will be one list instead of 2 (AAI to CHI) so any cluster moved that uses both would save 1 pointer metadata will increase slightly, can be reduced if we get ember to stop generating metadata for included clusters - RAM can decrease by FeatureMap and ClusterRevision if we stop ember from allocating space for it (however we will offset this by flash "encode const value"). - RAM usage increases slightly during conversion for "store version data" which ember currently allocates for all clusters. - Very long term: if we replace all clusters, we can drop AAI/CHI and that would save some flash. Still expect no savings because new interface does all of AAI and CHI and a bit more. TLDR on resourcing: probably ok on RAM over time, there is a flash overhead for this, claiming this is important for testable and maintainable code in the future.
|
PR #37526: Size comparison from 9c8cb33 to 1a61b04 Increases above 0.2%:
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, 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.
Fixes #37307
The intention is to remove direct calls to AAI from WriteHandler, instead doing it through calls to DataModel::Provider. This to allow a more complete decoupling between these layers.
Testing
Tested with Unit Test suites.