Declare a "server cluster interface" that is intended to combine "attribute access interface" and "command handler interface" #37541
+1,554
−28
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 |
github-actions
bot
added
scripts
lib
app
core
documentation
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.
andy31415
force-pushed
the
server-cluster-interface
branch
from
c7ccbb3 to
4eab278
Compare
|
PR #37541: Size comparison from 522876c to 4eab278 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37541: Size comparison from 522876c to 8f7a492 Full report (3 builds for cc32xx, stm32)
|
… re-ordering, so we can provide stable ordering for clusters
|
PR #37541: Size comparison from 522876c to 576c374 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…ctedhomeip into server-cluster-interface
|
PR #37541: Size comparison from 08535fd to 0785397 Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
|
PR #37541: Size comparison from 08535fd to b2b80a1 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37541: Size comparison from 5621ff6 to c65e9a3 Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
|
PR #37541: Size comparison from 5621ff6 to 33aaf5c Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37541: Size comparison from 00c6f4e to 265ed70 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #37541: Size comparison from 64ae4d8 to b7f8e79 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…istry as this seems very useful
|
PR #37541: Size comparison from 64ae4d8 to 75c7879 Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
|
PR #37541: Size comparison from 64ae4d8 to 28a21f7 Full report (73 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.
This defines a
ServerClusterInterfaceclass along with a registry. Also slight update tobuild_coverageto run on more (specifically my) machines without errors.Implementation notes:
InvoketoInvokeCommandas per review feedback. Made this new name consistent across the new interface andDataModel::ProviderThis is currently unused. I expect usages to add flash & RAM cost, that will be slowly offset (especially RAM) as we move clusters around:
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.
Testing
Unit tests created. Got 100% coverage (including testing trivial "return error" functions to get that). The registry includes a randomized stress test.
Performance for the stress test seems ok enough (expect to be sub-second even on slower machines, however we may choose to test more). This was on a Ryzen 5900X:
A Xeon 6154 takes around 32ms.