Skip to content

Commit

Permalink
More consistently set isArray to true for lists. (#11815)
Browse files Browse the repository at this point in the history
Before this PR lists in struct and command fields have isArray (and
often isList) set, while lists in attribute values have just isList
set.

This change tries to make the two cases look more similar by also
setting isARray for list-typed attributes, which will ease template
development going forward, due to not having to repeat the same logic
for isList and isArray in templates that are trying to work with a
value regardless of where it appears.

This requires some workarounds in places that use ZAP built-in
functions that sniff for "isArray" on the context object and started
incorrectly picking it up when working with elements in list-typed
attributes.  Those same templates would have been broken for
list-typed members of structs, but they mostly don't support structs
yet, so weren't exercising those codepaths.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Feb 3, 2022
1 parent 2ed1f20 commit 3923731
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 13 deletions.
5 changes: 3 additions & 2 deletions src/app/zap-templates/common/ClustersHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -288,8 +288,9 @@ function handleList(item, [ atomics, enums, bitmaps, structs ])
throw new Error(item.label, 'List[T] is missing type "T" information');
}

item.isList = true;
item.type = entryType;
item.isList = true;
item.isArray = true;
item.type = entryType;
enhancedItem(item, [ atomics, enums, bitmaps, structs ]);

return true;
Expand Down
18 changes: 12 additions & 6 deletions src/controller/java/templates/CHIPReadCallbacks-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,14 @@ void CHIP{{chipCallback.name}}AttributeCallback::CallbackFn(void * context, {{ch
{{#chip_server_cluster_attributes}}
{{#if isList}}

{{! NOTE: Some of our helpers rely on broken ZAP APIs that sniff for "isArray"
when we are just trying to work with the type of an array element. Fix
that by defining some inline partials that let us force isArray to false as
needed. }}
{{~#*inline "asUnboxedJniSignature"}}{{asJniSignature type false}}{{/inline}}
{{~#*inline "asUnboxedJniSignatureForEntry"}}{{> asUnboxedJniSignature isArray=false}}{{/inline~}}
{{~#*inline "asBoxedJavaBasicType"}}{{asJavaBasicTypeForZclType type true}}{{/inline~}}
{{~#*inline "asBoxedJavaBasicTypeForEntry"}}{{>asBoxedJavaBasicType isArray=false}}{{/inline~}}
CHIP{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}AttributeCallback::CHIP{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}AttributeCallback(jobject javaCallback) :
chip::Callback::Callback<{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}ListAttributeCallback>(CallbackFn, this)
{
Expand Down Expand Up @@ -217,9 +225,9 @@ void CHIP{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}AttributeCallb
jobject {{asLowerCamelCase name}} = nullptr;
if (!{{asLowerCamelCase name}}Null && {{asLowerCamelCase name}}HasValue) {
jclass {{asLowerCamelCase name}}EntryCls;
chip::JniReferences::GetInstance().GetClassRef(env, "java/lang/{{asJavaBasicTypeForZclType type true}}", {{asLowerCamelCase name}}EntryCls);
chip::JniReferences::GetInstance().GetClassRef(env, "java/lang/{{>asBoxedJavaBasicTypeForEntry}}", {{asLowerCamelCase name}}EntryCls);
chip::JniClass {{asLowerCamelCase name}}JniClass({{asLowerCamelCase name}}EntryCls);
jmethodID {{asLowerCamelCase name}}EntryTypeCtor = env->GetMethodID({{asLowerCamelCase name}}EntryCls, "<init>", "({{asJniSignature type false}})V");
jmethodID {{asLowerCamelCase name}}EntryTypeCtor = env->GetMethodID({{asLowerCamelCase name}}EntryCls, "<init>", "({{>asUnboxedJniSignatureForEntry}})V");
{{asLowerCamelCase name}} = env->NewObject({{asLowerCamelCase name}}EntryCls, {{asLowerCamelCase name}}EntryTypeCtor, {{asLowerCamelCase name}}Value);
}
{{/if}}
Expand Down Expand Up @@ -252,7 +260,6 @@ void CHIP{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}AttributeCallb
env->CallBooleanMethod(arrayListObj, arrayListAddMethod, attributeObj);
{{else}}
bool entryNull = false;
{{#unless isArray}}
{{#unless isStruct}}
{{#if isNullable}}
{{chipType}} entryValue;
Expand All @@ -264,7 +271,6 @@ void CHIP{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}AttributeCallb
{{chipType}} entryValue = entry;
{{/if}}
{{/unless}}
{{/unless}}

{{#if isStruct}}
{{! TODO: Add support for structs here }}
Expand All @@ -284,9 +290,9 @@ void CHIP{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}AttributeCallb
jobject entryObject = nullptr;
if (!entryNull) {
jclass entryTypeCls;
chip::JniReferences::GetInstance().GetClassRef(env, "java/lang/{{asJavaBasicTypeForZclType type true}}", entryTypeCls);
chip::JniReferences::GetInstance().GetClassRef(env, "java/lang/{{>asBoxedJavaBasicTypeForEntry}}", entryTypeCls);
chip::JniClass jniClass(entryTypeCls);
jmethodID entryTypeCtor = env->GetMethodID(entryTypeCls, "<init>", "({{asJniSignature type false}})V");
jmethodID entryTypeCtor = env->GetMethodID(entryTypeCls, "<init>", "({{>asUnboxedJniSignatureForEntry}})V");
entryObject = env->NewObject(entryTypeCls, entryTypeCtor, entryValue);
}
{{/if}}
Expand Down
6 changes: 5 additions & 1 deletion src/controller/java/templates/ChipClusters-java.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,11 @@ public class ChipClusters {
{{else if (isCharString type)}}
// Add String field here after ByteSpan is properly emitted in C++ layer
{{else}}
{{asJavaBasicTypeForZclType type true}}
{{! NOTE: asJavaBasicTypeForZclType ends up sniffing for isArray on the
context. Since we want the type of our _entry_, force isArray to
false. }}
{{~#*inline "asJavaBasicTypeForEntry"}}{{asJavaBasicTypeForZclType type true}}{{/inline~}}
{{> asJavaBasicTypeForEntry isArray=false}}
{{/if}}
{{#if isOptional}}
{{#unless isArray}}
Expand Down
14 changes: 12 additions & 2 deletions src/controller/java/templates/ClusterInfo-java.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,12 @@ public class ClusterInfoMapping {
{{else if (isCharString type)}}
// Add String field here after ByteSpan is properly emitted in C++ layer
{{else}}
{{asJavaBasicTypeForZclType type true}}
{{! NOTE: asJavaBasicTypeForZclType does not handle isArray well, so
add an inline partial to force isArray to false when we want the
types of list entries. }}
{{~#*inline "asBoxedJavaBasicType"}}{{asJavaBasicTypeForZclType type true}}{{/inline~}}
{{~#*inline "asBoxedJavaBasicTypeForEntry"}}{{> asBoxedJavaBasicType isArray=false}}{{/inline~}}
{{> asBoxedJavaBasicTypeForEntry}}
{{/if}}
{{/if}}
> valueList) {
Expand All @@ -230,7 +235,12 @@ public class ClusterInfoMapping {
{{else if (isCharString type)}}
// Add String field here after ByteSpan is properly emitted in C++ layer
{{else}}
CommandResponseInfo commandResponseInfo = new CommandResponseInfo("valueList", "List<{{asJavaBasicTypeForZclType type true}}>");
{{! NOTE: asJavaBasicTypeForZclType does not handle isArray well, so
add an inline partial to force isArray to false when we want the
types of list entries. }}
{{~#*inline "asBoxedJavaBasicType"}}{{asJavaBasicTypeForZclType type true}}{{/inline~}}
{{~#*inline "asBoxedJavaBasicTypeForEntry"}}{{> asBoxedJavaBasicType isArray=false}}{{/inline~}}
CommandResponseInfo commandResponseInfo = new CommandResponseInfo("valueList", "List<{{>asBoxedJavaBasicTypeForEntry}}>");
{{/if}}
{{/if}}

Expand Down
7 changes: 6 additions & 1 deletion src/controller/python/templates/python-CHIPClusters-cpp.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,12 @@ static void On{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}ListAttri
{{else if (isCharString type)}}
ChipLogProgress(Zcl, " %.*s,", static_cast<int>(entry.size()), entry.data());
{{else}}
ChipLogProgress(Zcl, " {{asPrintFormat type}},", entry);
{{~! NOTE: asPrintFormat does not handle the isArray case at
all right, but we want the print format for our _entries_
anyway. Indirect through a partial that will let us set
isArray to false. ~}}
{{~#*inline "asPrintFormatForElement"}}{{asPrintFormat type}}{{/inline~}}
ChipLogProgress(Zcl, " {{>asPrintFormatForElement isArray=false}},", entry);
{{/if}}
#endif // CHIP_PROGRESS_LOGGING
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
{{#chip_client_clusters}}
{{#chip_server_cluster_attributes}}
{{#if isList}}
{{#>CHIPCallbackBridge partial-type="List" }}{{asUpperCamelCase ../../name}}{{asUpperCamelCase ../name}}ListAttributeCallback{{/CHIPCallbackBridge}}
{{#>CHIPCallbackBridge partial-type="List" isArray=false}}{{asUpperCamelCase ../../name}}{{asUpperCamelCase ../name}}ListAttributeCallback{{/CHIPCallbackBridge}}
{{/if}}
{{/chip_server_cluster_attributes}}
{{/chip_client_clusters}}
Expand Down

0 comments on commit 3923731

Please sign in to comment.