Generate read attribute #11650
Merged
Generate read attribute #11650
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
…cts on the ui next
JasonLiuZhuoCheng
force-pushed
the
generate-read-attribute
branch
from
63a30b9 to
a062072
Compare
pullapprove
bot
requested review from
anush-apple,
balducci-apple,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chulspro,
Damian-Nordic,
electrocucaracha,
erjiaqing and
franck-apple
pullapprove
bot
requested review from
sagar-apple,
saurabhst,
selissia,
tecimovic,
vivien-apple,
wbschiller,
woody-apple,
yufengwangca and
yunhanw-google
JasonLiuZhuoCheng
force-pushed
the
generate-read-attribute
branch
2 times, most recently
from
d1f6b2c to
39ab841
Compare
|
PR #11650: Size comparison from 0528ce0 to 389ccc2 Increases above 0.2%:
Increases (2 builds for esp32, linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
JasonLiuZhuoCheng
force-pushed
the
generate-read-attribute
branch
from
389ccc2 to
f01bced
Compare
|
PR #11650: Size comparison from 9964cdb to bbf2d37 Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
woody-apple
approved these changes
Nov 11, 2021
|
Fast tracking, given platform changes by a platform maintainer. |
|
/rebase |
austinh0
reviewed
Nov 11, 2021
...main/java/com/google/chip/chiptool/clusterclient/clusterinteraction/ClusterDetailFragment.kt
Show resolved
Hide resolved
...main/java/com/google/chip/chiptool/clusterclient/clusterinteraction/ClusterDetailFragment.kt
Show resolved
Hide resolved
...main/java/com/google/chip/chiptool/clusterclient/clusterinteraction/ClusterDetailFragment.kt
Show resolved
Hide resolved
...main/java/com/google/chip/chiptool/clusterclient/clusterinteraction/ClusterDetailFragment.kt
Show resolved
Hide resolved
| // read attribute | ||
| {{#chip_server_cluster_attributes}} | ||
| Map<String, CommandParameterInfo> read{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}CommandParams = new LinkedHashMap<String, CommandParameterInfo>(); | ||
| CommandInfo read{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}AttributeCommandInfo = new CommandInfo( |
There was a problem hiding this comment.
Tracking reads using CommandInfo makes sense from a code use perspective, but the naming is a bit weird (since reads are different than commands). Let's discuss an alternative offline
There was a problem hiding this comment.
Name CommandInfo to InteractionInfo
Comment on lines
+252
to
+254
| getCommandMap(clusterMap); | ||
| getReadAttributeMap(clusterMap); | ||
| return clusterMap; |
There was a problem hiding this comment.
I think this design is cleaner:
Map<String, ClusterInfo> clusterMap = new HashMap<>();
Map<> commandMap = getCommandMap();
Map<> readMap = getReadAttributeMap();
combineCommands(clusterMap, commandMap);
combineCommands(clusterMap, readMap);
void combineCommands(Map<String, ClusterInfo> dest, Map<String, ClusterInfo> source) {
// For every key-value pair, take the ClusterInfo commands from source and add them to the ClusterInfo
// commands in dest.
}
Since it moves the combine logic out of the individual functions.
There was a problem hiding this comment.
one problem is that in the getReadAttributeMap function, I will have to generate ClusterInfo, which is redundant. Here, what I did was add each command into the already generated ClusterInfo.
austinh0
reviewed
Nov 12, 2021
andy31415
pushed a commit
that referenced
this pull request
Nov 15, 2021
* resolve comments * Restyled by google-java-format * Restyled by gn * update zap generated code * fix comments * merge with master * fix unwanted format changes * modify clusterinfo mapping to resolve comment * final fix for comments * Delete misc.xml Co-authored-by: Restyled.io <[email protected]>
PSONALl
pushed a commit
to PSONALl/connectedhomeip
that referenced
this pull request
Dec 3, 2021
* try to find a state where m5stack doesn't bootloop * add back class description * select different cluster, command will remove previous displayed parameter * add responseValueInfo class instead of string split * remove unused variable * Restyled by whitespace * Restyled by google-java-format * Restyled by gn * fix parameter response ui alignment issue * resolve comments * fix format * regenerate clusterInfoMapping.java * resolve comments * merge with master to get pass the check * finish read attribute code generation, need to handle displaying objects on the ui next * able to display complex data on the ui * skip merge with master * need outer loop variable to store the deserialized string representation of the object * Update ClusterInteractionFragment.kt Co-authored-by: Restyled.io <[email protected]>
PSONALl
pushed a commit
to PSONALl/connectedhomeip
that referenced
this pull request
Dec 3, 2021
* resolve comments * Restyled by google-java-format * Restyled by gn * update zap generated code * fix comments * merge with master * fix unwanted format changes * modify clusterinfo mapping to resolve comment * final fix for comments * Delete misc.xml Co-authored-by: Restyled.io <[email protected]>
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.
Change overview
Testing
Manually tested.