[ZAP] Update endpoint_config.zapt template

[vivien-apple]

Problem

#9246 is trying to use the FeatureMap global attribute for the WindowCovering cluster. It ends up into some endianness issues with the `GENERATED_DEFAULTS.

I tried to upgrade the endpoint_config.zapt template to use the new syntax from upstream ZAP, and eventually fix the endianness issue.

I have kept this PR as a drag because big and little endian properly generates different defaults but it seems like the global attributes are not taken into account in the sql query at https://github.com/project-chip/zap/blob/bd655586081b4ea444d37fd455d5a817e417bd1c/src-electron/db/query-attribute.js#L407

For the thermostat cluster, instead of generating values into GENERATED_DEFAULTS, the value is put directly into GENERATED_ATTRIBUTES as a ptrDefautValue:

{                                                                                                                      \
                0xFFFC, ZCL_BITMAP32_ATTRIBUTE_TYPE, 4, (0x00), { (uint8_t *) 0x0000000b }                                         \
            }, /* 225 Cluster: Thermostat, Attribute: FeatureMap, Side: server*/  

If you try to run chip-all-clusters-app locally it crashes:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xb)
  * frame #0: 0x00007fff203ce0c2 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 194
    frame #1: 0x0000000100007488 chip-all-clusters-app`typeSensitiveMemCopy(clusterId=513, dest="", src="", am=0x0000000100111848, write=true, readLength=0, index=-1) at attribute-storage.cpp:514:13
    frame #2: 0x0000000100006ebb chip-all-clusters-app`emAfReadOrWriteAttribute(attRecord=0x00007ffeefbff220, metadata=0x0000000000000000, buffer="", readLength=0, write=true, index=-1) at attribute-storage.cpp:683:47
    frame #3: 0x0000000100007eb6 chip-all-clusters-app`emAfLoadAttributeDefaults(endpoint=255, writeTokens=false) at attribute-storage.cpp:1232:21
    frame #4: 0x0000000100007c77 chip-all-clusters-app`emberAfInitializeAttributes(endpoint=255) at attribute-storage.cpp:1140:5
    frame #5: 0x000000010000e2f2 chip-all-clusters-app`emberAfInit(exchangeMgr=0x00000001001224f0) at util.cpp:287:9
    frame #6: 0x0000000100003f92 chip-all-clusters-app`InitDataModelHandler(exchangeManager=0x00000001001224f0) at DataModelHandler.cpp:46:5
    frame #7: 0x00000001000ff53c chip-all-clusters-app`chip::Server::Init(this=0x0000000100120d30, delegate=0x0000000000000000, secureServicePort=5540, unsecureServicePort=5550) at Server.cpp:86:5
    frame #8: 0x000000010004d742 chip-all-clusters-app`ChipLinuxAppMainLoop() at AppMain.cpp:278:33
    frame #9: 0x0000000100001954 chip-all-clusters-app`main(argc=1, argv=0x00007ffeefbff738) at main.cpp:58:5
    frame #10: 0x00007fff203a6f5d libdyld.dylib`start + 1
    frame #11: 0x00007fff203a6f5d libdyld.dylib`start + 1
(lldb) up
frame #1: 0x0000000100007488 chip-all-clusters-app`typeSensitiveMemCopy(clusterId=513, dest="", src="", am=0x0000000100111848, write=true, readLength=0, index=-1) at attribute-storage.cpp:514:13
   511 	        }
   512 	        else
   513 	        {
-> 514 	            memmove(dest, src, am->size);
   515 	        }
   516 	    }
   517 	    return EMBER_ZCL_STATUS_SUCCESS;

There is an other issue with the size of ARRAY/List not properly taken into account too into the SQL request from the ZAP repo, but I have a local fix for this one. Getting global attributes into the sql query result is an other story looking at the size fo the query...

Change overview

• Update endpoint_config.zapt template

Testing

[vivien-apple]

For what it worth I have confirmed that it happens for global attributes and not the other by trying to change the value of an other bitmap32 attribute that is part of a cluster. The GENERATED_DEFAULTS were properly generated.

[brdandu]

I see that we did not have global attributes of size greater than 2 bytes before and did not account for the use case where global attributes made it into the Generated Defaults. However FeatureMap is a global attribute which has a size of 4 bytes and needs go into generated defaults in order to save space. Is the app failing to work because of no generated default for FeatureMap? Isn't the FeatureMap attribute default value still generated under GENERATED_ATTRIBUTES directly?
I can take a look at what can be done to include global attribute default values in generated defaults

Problem

#9246 is trying to use the FeatureMap global attribute for the WindowCovering cluster. It ends up into some endianness issues with the `GENERATED_DEFAULTS.

I tried to upgrade the endpoint_config.zapt template to use the new syntax from upstream ZAP, and eventually fix the endianness issue.

I have kept this PR as a drag because big and little endian properly generates different defaults but it seems like the global attributes are not taken into account in the sql query at https://github.com/project-chip/zap/blob/bd655586081b4ea444d37fd455d5a817e417bd1c/src-electron/db/query-attribute.js#L407

For the thermostat cluster, instead of generating values into GENERATED_DEFAULTS, the value is put directly into GENERATED_ATTRIBUTES as a ptrDefautValue:

{                                                                                                                      \
                0xFFFC, ZCL_BITMAP32_ATTRIBUTE_TYPE, 4, (0x00), { (uint8_t *) 0x0000000b }                                         \
            }, /* 225 Cluster: Thermostat, Attribute: FeatureMap, Side: server*/  

If you try to run chip-all-clusters-app locally it crashes:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xb)
  * frame #0: 0x00007fff203ce0c2 libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 194
    frame #1: 0x0000000100007488 chip-all-clusters-app`typeSensitiveMemCopy(clusterId=513, dest="", src="", am=0x0000000100111848, write=true, readLength=0, index=-1) at attribute-storage.cpp:514:13
    frame #2: 0x0000000100006ebb chip-all-clusters-app`emAfReadOrWriteAttribute(attRecord=0x00007ffeefbff220, metadata=0x0000000000000000, buffer="", readLength=0, write=true, index=-1) at attribute-storage.cpp:683:47
    frame #3: 0x0000000100007eb6 chip-all-clusters-app`emAfLoadAttributeDefaults(endpoint=255, writeTokens=false) at attribute-storage.cpp:1232:21
    frame #4: 0x0000000100007c77 chip-all-clusters-app`emberAfInitializeAttributes(endpoint=255) at attribute-storage.cpp:1140:5
    frame #5: 0x000000010000e2f2 chip-all-clusters-app`emberAfInit(exchangeMgr=0x00000001001224f0) at util.cpp:287:9
    frame #6: 0x0000000100003f92 chip-all-clusters-app`InitDataModelHandler(exchangeManager=0x00000001001224f0) at DataModelHandler.cpp:46:5
    frame #7: 0x00000001000ff53c chip-all-clusters-app`chip::Server::Init(this=0x0000000100120d30, delegate=0x0000000000000000, secureServicePort=5540, unsecureServicePort=5550) at Server.cpp:86:5
    frame #8: 0x000000010004d742 chip-all-clusters-app`ChipLinuxAppMainLoop() at AppMain.cpp:278:33
    frame #9: 0x0000000100001954 chip-all-clusters-app`main(argc=1, argv=0x00007ffeefbff738) at main.cpp:58:5
    frame #10: 0x00007fff203a6f5d libdyld.dylib`start + 1
    frame #11: 0x00007fff203a6f5d libdyld.dylib`start + 1
(lldb) up
frame #1: 0x0000000100007488 chip-all-clusters-app`typeSensitiveMemCopy(clusterId=513, dest="", src="", am=0x0000000100111848, write=true, readLength=0, index=-1) at attribute-storage.cpp:514:13
   511 	        }
   512 	        else
   513 	        {
-> 514 	            memmove(dest, src, am->size);
   515 	        }
   516 	    }
   517 	    return EMBER_ZCL_STATUS_SUCCESS;

There is an other issue with the size of ARRAY/List not properly taken into account too into the SQL request from the ZAP repo, but I have a local fix for this one. Getting global attributes into the sql query result is an other story looking at the size fo the query...

Change overview

• Update endpoint_config.zapt template

Testing

[vivien-apple]

Is the app failing to work because of no generated default for FeatureMap? Isn't the FeatureMap attribute default value still generated under GENERATED_ATTRIBUTES directly?

FeatureMap is generated under GENERATED_ATTRIBUTES, but since the attribute size is > 2 it goes under the https://github.com/project-chip/zap/blob/0e15f400de8cffccaeb001e0354f0c2e55037454/test/gen-template/zigbee/zap-config-version-2.zapt#L35 case.
This template path use the generated_defaults_index helper and since it is not part of GENERATED_DEFAULTS it hit this codepath: https://github.com/project-chip/zap/blob/bd655586081b4ea444d37fd455d5a817e417bd1c/src-electron/generator/helper-session.js#L1062

Casting to (uint8_t*) make the stack thing is it a ptrToDefaultValue and not a real value. See

connectedhomeip/src/app/util/af-types.h

Line 103 in 53dd583

constexpr EmberAfDefaultAttributeValue(uint8_t * ptr) : ptrToDefaultValue(ptr) {}

In any case storing it as a real value would not work since defaultValue are stored as a 2 bytes value in

connectedhomeip/src/app/util/af-types.h

Line 116 in 53dd583

uint16_t defaultValue;

Because the stack think it is a ptrToDefaultValue and the attribute size in bytes is > 2 it uses the value (0x0000000b in this case) as a pointer (in

connectedhomeip/src/app/util/attribute-storage.cpp

Line 1216 in 1ae045e

ptr = (uint8_t *) am->defaultValue.ptrToDefaultValue;

) and it ends up crashing calling memmove with this address as src in

connectedhomeip/src/app/util/attribute-storage.cpp

Line 514 in 1ae045e

memmove(dest, src, am->size);

Here is the generated code in GENERATED_ATTRIBUTES:

{                                                                                                                      \
                0xFFFC, ZCL_BITMAP32_ATTRIBUTE_TYPE, 4, (0x00), { (uint8_t *) 0x0000000b }                                         \
            }, /* 225 Cluster: Thermostat, Attribute: FeatureMap, Side: server*/  

[brdandu]

Is the app failing to work because of no generated default for FeatureMap? Isn't the FeatureMap attribute default value still generated under GENERATED_ATTRIBUTES directly?

FeatureMap is generated under GENERATED_ATTRIBUTES, but since the attribute size is > 2 it goes under the https://github.com/project-chip/zap/blob/0e15f400de8cffccaeb001e0354f0c2e55037454/test/gen-template/zigbee/zap-config-version-2.zapt#L35 case.
This template path use the generated_defaults_index helper and since it is not part of GENERATED_DEFAULTS it hit this codepath: https://github.com/project-chip/zap/blob/bd655586081b4ea444d37fd455d5a817e417bd1c/src-electron/generator/helper-session.js#L1062

Casting to (uint8_t*) make the stack thing is it a ptrToDefaultValue and not a real value. See

connectedhomeip/src/app/util/af-types.h

Line 103 in 53dd583

constexpr EmberAfDefaultAttributeValue(uint8_t * ptr) : ptrToDefaultValue(ptr) {}

In any case storing it as a real value would not work since defaultValue are stored as a 2 bytes value in

connectedhomeip/src/app/util/af-types.h

Line 116 in 53dd583

uint16_t defaultValue;

Because the stack think it is a ptrToDefaultValue and the attribute size in bytes is > 2 it uses the value (0x0000000b in this case) as a pointer (in

connectedhomeip/src/app/util/attribute-storage.cpp

Line 1216 in 1ae045e

ptr = (uint8_t *) am->defaultValue.ptrToDefaultValue;

) and it ends up crashing calling memmove with this address as src in

connectedhomeip/src/app/util/attribute-storage.cpp

Line 514 in 1ae045e

memmove(dest, src, am->size);

Here is the generated code in GENERATED_ATTRIBUTES:

{                                                                                                                      \
                0xFFFC, ZCL_BITMAP32_ATTRIBUTE_TYPE, 4, (0x00), { (uint8_t *) 0x0000000b }                                         \
            }, /* 225 Cluster: Thermostat, Attribute: FeatureMap, Side: server*/  

I see. Looking into this currently. Will update once I have pushed a fix.

[jmeg-sfy]

Hi Hope you can advance on this topic soon; let us know

[vivien-apple]

I have pushed an updated version that generates content using the fix that has been introduced into ZAP for global attributes not beeing generated properly. But, this branch still needs an other ZAP update once project-chip/zap#233 lands and it also needs #4369 to be fixed in some ways.

[vivien-apple]

I have just pushed an updated version that does not requires #4369 to be fixed. In fact fixing #4369 may requires a decent amount of code refactoring that are not part of this PR.

I would expect the ZAP CI to fails as there still a need for a ZAP update.

[woody-apple]

@vivien-apple ?

[jmeg-sfy]

@woody-apple @vivien-apple can we merge this or ZAP is still required to be updated ?

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[jmeg-sfy]

not stale

[woody-apple]

@vivien-apple ?

[vivien-apple]

@woody-apple @vivien-apple can we merge this or ZAP is still required to be updated ?

There is still some issues with ZAP. Nothing that is quick to fix. I assume I will have to do a different patch to fix the endianness problem you are interested in.

[bzbarsky-apple]

Fwiw, the endianness problem was fixed in #11451

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[jonsmirl]

Would the Linux kernel solution help out here? Check out include/linux/byteorder/genric.h in Linux kernel.
https://elixir.bootlin.com/linux/latest/source/include/linux/byteorder/generic.h

Basically you use these macros when generating code:

• ntohl(__u32 x)
• ntohs(__u16 x)
• htonl(__u32 x)
• htons(__u16 x)

Those say host to network order or network order to host. Then the h file does the magic needed to avoid the endianess problems. So if the endianess matches, these macros disappear. Otherwise they do the necessary byte swaps. There are fancier inlines lower down in the file for dealing with arrays.

You can also use C++ features to hide these conversions.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[woody-apple]

/rebase

[vivien-apple]

Closing as the endianness issue has been fixed by #11451 and will likely revisit this later in time.