Update the ZAP template to encode/decode commands

[vivien-apple]

Problem

There are a few issues with the current template.

1. {{isArray}} is not taken into account. It results into a mismatch between
   the signatures of some clusters methods
2. Instead of using raw types, such as uint8_t, uint16_t, the current template
   uses enum. It creates 2 types of issues:
   • The current template generates code that rely on sizeof(type) to move the
     pointer for the payload buffer. It does not work well with enum.
   • The signatures of methods are using raw types. So it ask for a change
     in all the cluster signatures.
3. The buffer start pointer is mispositioned: It uses 0 instead of cmd->payloadStartIndex
4. String are using uint8_t *, which is OK but the pointer to the payload buffer is not
   moved to take into account the additional bytes with the size of the string

I'm not fully happy with this PR. It shows the need to add a new helper to ZAP in order to make things easier here. But I don't want to wait for it to be ready before landing this PR since this is the last missing piece that gets me a working all-clusters-app application based on ZAP generated (#3464).

So I will open an issue to track that.

Furthermore there is still an issue with the client side of cluster. It does not matter to land this piece of code but I noticed that the templates does not use the presentIf field from the Silabs XML. It will be needed to implement properly the client side decoding.

[vivien-apple]

I have opened project-chip/zap#65 for the presentIf issue.

[vivien-apple]

I have updated the template so when the presentIf fix changes upstream, the template will be ready for it.

[bzbarsky-apple]

This is all sorts of broken, including missing length checks and endianness fail, but I guess once we move to DataModelReader this will be moot....

Maybe add a comment that includes the string "emberAfGetInt" in it so when I search for that I find this spot?

[vivien-apple]

Yeah...

The tricky thing is that {{asUnderlyingType}} is an async method and I have not found the right incantation that would have allowed me to pass it to a helper function.

Otherwise I could easily have written an {{#if}} case for all types, like:

{{#if (isStrEqual {{asunderlyingType type}} "uint8_t")}}
{{asSymbol label}} = emberAfGetInt8u(cmd->buffer, payloadOffset, cmd->bufLen);
payloadOffset += 1u;
{{else if (isStrEqual {{asunderlyingType type}} "uint16_t")}}
{{asSymbol label}} = emberAfGetInt16u(cmd->buffer, payloadOffset, cmd->bufLen);
payloadOffset += 2u;
{{ else if ...}}
{{/if}}
if (cmd->Buffer + payloadOffset >= cmd->bufLen)
{
    return EMBER_ZCL_STATUS_MALFORMED_COMMAND;
}

And maybe I could have used an accumulator (https://github.com/project-chip/zap/blob/72c4b75847d92d077e0be02cb8fba14a4900a830/test/gen-template/zigbee/accumulator.zapt#L8) at the beginning of the function to

But yes. In general this code is clearly unsafe.

[jepenven-silabs]

@vivien-apple I'm concern about the stack usage of your new code. The goal of using pointer to the data was to prevent unnecessary stack usage since all of those variables will be pass directly to the callback handler. Seems to me like this use more stack and is less scalable than the previous implementation.

[vivien-apple]

I'm not sure about the less scalable part but for sure it will use more stack. The current plan for this code is to be rewritten using DataModelReader (

connectedhomeip/src/app/message-reader.h

Line 35 in 84847dd

class DataModelReader

)

[bzbarsky-apple]

The goal of using pointer to the data was to prevent unnecessary stack usage since all of those variables will be pass directly to the callback handler

At the very least, the need to be swapped into native endianness, right?

But also, with the different underlying encoding for CHIP (TLV-based) this won't be viable in any case....

[bzbarsky-apple]

So default for strings is null, not "zero-length string"? That might end up changing....

[vivien-apple]

I think this is NULL because the string is not part of the bytes of the payload. By spec it is not supposed to be here.
I have not looked deeply but I imagine that "zero-length string" is potentially a valid string for some APIs.

[jepenven-silabs]

@vivien-apple I'm concern about the stack usage of your new code. The goal of using pointer to the data was to prevent unnecessary stack usage since all of those variables will be pass directly to the callback handler. Seems to me like this use more stack and is less scalable than the previous implementation.

[vivien-apple]

I have updated the template to uses some of the helpers from zap-core directly inside helper-chip.js. It should be much closer to the original code and it is much easier to maintain.

[vivien-apple]

For the record I have also updated .clang-format for JavaScript as it was completely messing up the .js style.