Actions: Add generic implementation for the actions cluster. #35568
+1,613
−471
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
pullapprove
bot
requested review from
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple and
kiel-apple
|
PR #35568: Size comparison from 6e8676b to 2612675 Increases above 0.2%:
Full report (42 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink)
|
jadhavrohit924
force-pushed
the
actions
branch
from
2612675 to
bef53be
Compare
|
PR #35568: Size comparison from 6e8676b to bef53be Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
…ong with name buffer and added events
…of MatterActionsPluginServerInitCallback in example
jadhavrohit924
force-pushed
the
actions
branch
from
4cf19e5 to
e0f8801
Compare
|
PR #35568: Size comparison from dd0ce89 to 0a99a10 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)
|
Comment on lines
+40
to
+45
| Delegate * gDelegateTable[kActionsDelegateTableSize] = { nullptr }; | ||
|
|
||
| Delegate * GetDelegate(EndpointId aEndpoint) | ||
| { | ||
| return (aEndpoint >= kActionsDelegateTableSize ? nullptr : gDelegateTable[aEndpoint]); | ||
| } |
There was a problem hiding this comment.
This is a bad pattern to continue. We need to have a better state keeping pattern than constantly using global static arrays.
There was a problem hiding this comment.
The right solutions in general seem to be:
- One cluster instance per endpoint as opposed to having a singleton that handles all endpoint, or
- Some way that the cluster instance maps endpoints to delegates that can be configured sanely at compile time if we want to support size caps on it, or that just does dynamic memory (e.g. std::map) if that's OK for the cluster in question.
I suspect in most cases item 1 is a better idea....
There was a problem hiding this comment.
Even with item 1, we would need to map endpoints with the delegates in the server implementations?
There was a problem hiding this comment.
With item 1 each cluster instance is a separate object which just has a pointer to its delegate; there is nothing to "map".
|
PR #35568: Size comparison from dd0ce89 to 717440f 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.
Problem:
Changes:
Testing: