Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix command responses in the scenes cluster #1241

Merged
merged 4 commits into from
Nov 18, 2024
Merged

Conversation

canardos
Copy link
Contributor

View scene command

The viewRsp command for the genScenes cluster contains 6 properties. These match the ZCL specified fields, but error responses will only contain status, groupid, and sceneid. From the ZCL spec (rev.8):

If the status is SUCCESS, the Transition time, Scene Name and Extension field fields are copied from the corresponding fields in the table entry, otherwise they are omitted.

This results in a buffer overrun in the parsePayloadCluster function of the ZclFrame during parsing of the incoming response command when not all fields are present.

I am not overly familiar with the library, but looking at other clusters, it appears that the right solution is use of the conditions property of the ParameterDefinition:

viewRsp: {
    ID: 1,
    parameters: [
        {name: 'status', type: DataType.UINT8},
        {name: 'groupid', type: DataType.UINT16},
        {name: 'sceneid', type: DataType.UINT8},
        {name: 'transtime', type: DataType.UINT16, conditions: [{type: ParameterCondition.STATUS_EQUAL, value: Status.SUCCESS}]},
        {name: 'scenename', type: DataType.CHAR_STR, conditions: [{type: ParameterCondition.STATUS_EQUAL, value: Status.SUCCESS}]},
        {name: 'extensionfieldsets', type: BuffaloZclDataType.EXTENSION_FIELD_SETS, conditions: [{type: ParameterCondition.STATUS_EQUAL, value: Status.SUCCESS}]},
    ],
},

Devices not supporting scene names are required to return the null string so this doesn't need to be accounted for.

Get scene membership

As with the prior command, the ZCL (rev.8) states:

3.7.2.5.6.1 Get Scene Membership Response Command
...
If the status is not SUCCESS, then the Scene count and Scene list field are omitted

Adding parameter conditions to these fields:

getSceneMembershipRsp: {
    ID: 6,
    parameters: [
        {name: 'status', type: DataType.UINT8},
        {name: 'capacity', type: DataType.UINT8},
        {name: 'groupid', type: DataType.UINT16},
        {name: 'scenecount', type: DataType.UINT8, conditions: [{type: ParameterCondition.STATUS_EQUAL, value: Status.SUCCESS}]},
        {name: 'scenelist', type: BuffaloZclDataType.LIST_UINT8, conditions: [{type: ParameterCondition.STATUS_EQUAL, value: Status.SUCCESS}]},
    ],
},

Enhanced Add Scene command

3.7.2.5.7 Enhanced Add Scene Response Command
...
The payload of this command SHALL be formatted in the same way as the Add Scene Response command specified in the ZCL scenes cluster.

The current implementation has no parameters. Copying from add add scene response:

enhancedAddRsp: {
    ID: 64,
    parameters: [
        {name: 'status', type: DataType.UINT8},
        {name: 'groupId', type: DataType.UINT16},
        {name: 'sceneId', type: DataType.UINT8},
    ],
},

Enhanced View Scene command

3.7.2.5.8 Enhanced View Scene Response Command
...
The payload of this command SHALL be formatted in the same way as the View Scene Response command, with the following difference:
The Transition Time field SHALL be measured in tenths of a second rather than in seconds.

The same parameter conditions as for the View Scene command above are required.

Copy link
Collaborator

@Nerivec Nerivec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I noticed the attributes in genBasic are also a bit out of whack, will need to fix this too.

@Koenkk Koenkk merged commit 1d54cba into Koenkk:master Nov 18, 2024
1 check passed
@Koenkk
Copy link
Owner

Koenkk commented Nov 18, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants