Add support for typedef

[gmarcosb]

Adds support for typedef in zap, supporting things like defining a VideoStreamID as a uint16, see the camera spec: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/10004

See also Matter SDK template changes @ https://github.com/project-chip/connectedhomeip/pull/36124

This should also eliminate the need for many of the things in chip-types.xml, which can now be simply defined in the XML files as a simple typedef

See also discussion with @bzbarsky-apple: https://github.com/project-chip/connectedhomeip/pull/35773#discussion_r1775652745

[gmarcosb]

@brdandu or @tecimovic could I get one of you to review? Thank you!

[brdandu]

I am wondering if this is very similar to base type implementation in https://github.com/project-chip/zap/pull/1472 . Do you think that can work the same way as your implementation?

[gmarcosb]

I am wondering if this is very similar to base type implementation in #1472 . Do you think that can work the same way as your implementation?

This seems to be a superset of that - in fact, this new typedef should probably supercede many of the base types - for example, in src/app/zap-templates/zcl/data-model/chip/chip-types.xml, cluster_id, field_id could instead use the typedef here

Not only does this PR allow for block helpers, it also allows for dynamic specifying of simple types alongside structs/etc. instead of having to have a definition in chip-types.xml as well as the code definitions - it's a lot more elegant

[brdandu]

I am wondering if this is very similar to base type implementation in #1472 . Do you think that can work the same way as your implementation?

This seems to be a superset of that - in fact, this new typedef should probably supercede many of the base types - for example, in src/app/zap-templates/zcl/data-model/chip/chip-types.xml, cluster_id, field_id could instead use the typedef here

Not only does this PR allow for block helpers, it also allows for dynamic specifying of simple types alongside structs/etc. instead of having to have a definition in chip-types.xml as well as the code definitions - it's a lot more elegant

I do see some of the benefits of having a typedef implementation for better spec readability. Here a couple of things I am thinking from the top of my head and worried about:

• The atomic identifier won't be associated with these types. Is that fine?
• Also as of today each type is made unique as per one of the types i.e. enum, bitmap, struct or number(discriminator). So when type def comes in then does it also mean it will be a typedef and something else?
• If the attribute types and command argument types start using the typedefs then do our existing helpers need to take that into account?

Should we have a corresponding chip repo PR which uses this to validate some of this behavior? @bzbarsky-apple @andy31415 do you guys have any input here?

[gmarcosb]

The atomic identifier won't be associated with these types. Is that fine:

I think so? I think we'd want to move some of the types currently defined in chip-types.xml to just be simple typedefs, so that it's not split between XML + code; but I may not be understanding what the detriment would be of not being associated with the atomic identifier

So when type def comes in then does it also mean it will be a typedef and something else?

Assuming I understand your question: typedef is treated exactly like enum & struct, i.e. the name is added to its own SQL table & it is checked for uniqueness against all types

do our existing helpers need to take that into account?

Yes, although in general there are shared functions - for example, when we lookup the type to output in code, we just do a redirect to the underlying type

[brdandu]

For reference, corresponding chip repo PR: https://github.com/project-chip/connectedhomeip/pull/36124

[brdandu]

I think so? I think we'd want to move some of the types currently defined in chip-types.xml to just be simple typedefs, so that it's not split between XML + code; but I may not be understanding what the detriment would be of not being associated with the atomic identifier

I could be wrong here but I believe we pass the atomic identifier in our zcl read and write attribute calls if I am not wrong. It is worth checking that our generated code continues to extract the right atomic identifier here when the typedef is introduced to the attribute types and this continues to work the same way: Eg for Test: Turn attribute type to a type def in xml. Then in you built application try to read and write this attribute to make sure this is working.

Assuming I understand your question: typedef is treated exactly like enum & struct, i.e. the name is added to its own SQL table & it is checked for uniqueness against all types

In our templates we very frequently pass the attribute type to a bunch of helpers for processing. What happens to the those helpers when the typedef is passed? Eg for test: Turn attribute type to a type def in xml. Now in the .zapt templates look for helpers like as_underlying_zcl_type/asUnderlyingZclType or as_underlying_type/asUnderlyingType which takes in the attribute type and gives a certain type for it. Check if this is behaving appropriately for typedefs

Yes, although in general there are shared functions - for example, when we lookup the type to output in code, we just do a redirect to the underlying type

Can this be verified by adding more unit tests here based on the above example?

[gmarcosb]

I added tests such that they are 1:1 with the enum & struct tests

Unfortunately, the tests are somewhat split between here & the matter SDK repo

Here, the tests just check for the basic functionality: i.e. if we find a typedef in a template, does it generate the right output?

Those tests here are updated to handle typedefs & pass

Then, in the Matter SDK repo, there are numerous other tests around the zap templates there (including the compiler itself - i.e. if the generated files compile)

Those tests are not yet complete, but are more testing the templates rather than ZAP itself (I am working on those now, though some are complete)

[gmarcosb]

Looping back around to this - as this causes no regressions (it's new functionality & a no-op to existing functionality), is it reasonable to get this in?