From 2510760c958c3e22c2602e889cd98c4b9948f5bb Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 15 Nov 2021 22:04:37 -0500 Subject: [PATCH] More consistently set isArray to true for lists. (#11815) 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. --- src/app/zap-templates/common/ClustersHelper.js | 5 +++-- .../java/templates/CHIPReadCallbacks-src.zapt | 18 ++++++++++++------ .../java/templates/ChipClusters-java.zapt | 6 +++++- .../java/templates/ClusterInfo-java.zapt | 14 ++++++++++++-- .../templates/python-CHIPClusters-cpp.zapt | 7 ++++++- .../CHIP/templates/CHIPCallbackBridge-src.zapt | 2 +- 6 files changed, 39 insertions(+), 13 deletions(-) diff --git a/src/app/zap-templates/common/ClustersHelper.js b/src/app/zap-templates/common/ClustersHelper.js index e800fab0a17ba5..45aaca3c038508 100644 --- a/src/app/zap-templates/common/ClustersHelper.js +++ b/src/app/zap-templates/common/ClustersHelper.js @@ -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; diff --git a/src/controller/java/templates/CHIPReadCallbacks-src.zapt b/src/controller/java/templates/CHIPReadCallbacks-src.zapt index 4c1931dba0c3fa..2f98c332bb62fe 100644 --- a/src/controller/java/templates/CHIPReadCallbacks-src.zapt +++ b/src/controller/java/templates/CHIPReadCallbacks-src.zapt @@ -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) { @@ -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, "", "({{asJniSignature type false}})V"); + jmethodID {{asLowerCamelCase name}}EntryTypeCtor = env->GetMethodID({{asLowerCamelCase name}}EntryCls, "", "({{>asUnboxedJniSignatureForEntry}})V"); {{asLowerCamelCase name}} = env->NewObject({{asLowerCamelCase name}}EntryCls, {{asLowerCamelCase name}}EntryTypeCtor, {{asLowerCamelCase name}}Value); } {{/if}} @@ -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; @@ -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 }} @@ -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, "", "({{asJniSignature type false}})V"); + jmethodID entryTypeCtor = env->GetMethodID(entryTypeCls, "", "({{>asUnboxedJniSignatureForEntry}})V"); entryObject = env->NewObject(entryTypeCls, entryTypeCtor, entryValue); } {{/if}} diff --git a/src/controller/java/templates/ChipClusters-java.zapt b/src/controller/java/templates/ChipClusters-java.zapt index 6949da07b66c68..db9d6f57e783d8 100644 --- a/src/controller/java/templates/ChipClusters-java.zapt +++ b/src/controller/java/templates/ChipClusters-java.zapt @@ -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}} diff --git a/src/controller/java/templates/ClusterInfo-java.zapt b/src/controller/java/templates/ClusterInfo-java.zapt index 252fc7528cd765..28c5f98bebdd4d 100644 --- a/src/controller/java/templates/ClusterInfo-java.zapt +++ b/src/controller/java/templates/ClusterInfo-java.zapt @@ -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) { @@ -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}} diff --git a/src/controller/python/templates/python-CHIPClusters-cpp.zapt b/src/controller/python/templates/python-CHIPClusters-cpp.zapt index dbae1f02d41a3d..e3ed162819402a 100644 --- a/src/controller/python/templates/python-CHIPClusters-cpp.zapt +++ b/src/controller/python/templates/python-CHIPClusters-cpp.zapt @@ -150,7 +150,12 @@ static void On{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}ListAttri {{else if (isCharString type)}} ChipLogProgress(Zcl, " %.*s,", static_cast(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 } diff --git a/src/darwin/Framework/CHIP/templates/CHIPCallbackBridge-src.zapt b/src/darwin/Framework/CHIP/templates/CHIPCallbackBridge-src.zapt index 1041a1b2e6838f..f66f3b15c3d2c1 100644 --- a/src/darwin/Framework/CHIP/templates/CHIPCallbackBridge-src.zapt +++ b/src/darwin/Framework/CHIP/templates/CHIPCallbackBridge-src.zapt @@ -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}}