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

endpoint.configureReporting should use manufacturerCode from ZCL definition when available #844

Merged
merged 5 commits into from
Dec 27, 2023

Conversation

sjorge
Copy link
Contributor

@sjorge sjorge commented Dec 27, 2023

I ran into this when trying to update the Ubisys H1, unless you manually specify the options (the frontend does not), we try to configure it without specifying the manufacturerCode. In this case resulting in a weird INVALID_DATA_TYPE error.

When our ZCL Cluster definition has an attribute manufacturerCode override specified, we should use it.

I worked backwards from what I usually do... which means I probably did it in the correct order 😬
I wrote a test first that configures hvacThermostat.ubisysVacationMode and swap the device in the mock to one with the Ubisys manufacturerCode. And update the manufacturerCode, attributeID and attributeDataType in the expected data to match those of ubisysVacationMode.


 FAIL  test/controller.test.ts (24.601 s)
  ● Controller › Endpoint configure reporting for manufacturer specific attribute from definition

    expect(received).toStrictEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 2

    @@ -73,14 +73,14 @@
          "commandIdentifier": 6,
          "frameControl": Object {
            "direction": 0,
            "disableDefaultResponse": true,
            "frameType": 0,
    -       "manufacturerSpecific": true,
    +       "manufacturerSpecific": false,
            "reservedBits": 0,
          },
    -     "manufacturerCode": 4338,
    +     "manufacturerCode": null,
          "transactionSequenceNumber": 11,
        },
        "Payload": Array [
          Object {
            "attrId": 18,

      2752 |         expect(call[1]).toBe(129);
      2753 |         expect(call[2]).toBe(1)
    > 2754 |         expect({...deepClone(call[3]), Cluster: {}}).toStrictEqual({"Cluster":{},"Command":{"ID":6,"name":"configReport","parameters":[{"name":"direction","type":32},{"name":"attrId","type":33},{"conditions":[{"type":"directionEquals","value":0}],"name":"dataType","type":32},{"conditions":[{"type":"directionEquals","value":0}],"name":"minRepIntval","type":33},{"conditions":[{"type":"directionEquals","value":0}],"name":"maxRepIntval","type":33},{"conditions":[{"type":"directionEquals","value":0},{"type":"dataTypeValueTypeEquals","value":"ANALOG"}],"name":"repChange","type":1000},{"conditions":[{"type":"directionEquals","value":1}],"name":"timeout","type":33}],"response":7},"Header":{"commandIdentifier":6,"frameControl":{"direction":0,"disableDefaultResponse":true,"frameType":0,"manufacturerSpecific":true,"reservedBits":0},"manufacturerCode":4338,"transactionSequenceNumber":11},"Payload":[{"attrId":18,"dataType":16,"direction":0,"maxRepIntval":10,"minRepIntval":1,"repChange":1}]});
           |                                                      ^
      2755 |
      2756 |         expect(endpoint.configuredReportings.length).toBe(1);
      2757 |         expect({...endpoint.configuredReportings[0], cluster: undefined}).toStrictEqual({"attribute":{"ID":18,"type":16,"manufacturerCode":4338,"name":"ubisysVacationMode"},"minimumReportInterval":1,"maximumReportInterval":10,"reportableChange":1, "cluster": undefined});

      at toStrictEqual (test/controller.test.ts:2754:54)
      at call (test/controller.test.ts:2:1)
      at Generator.tryCatch (test/controller.test.ts:2:1)
      at Generator._invoke [as next] (test/controller.test.ts:2:1)
      at asyncGeneratorStep (test/controller.test.ts:2:1)
      at asyncGeneratorStep (test/controller.test.ts:2:1)

That test failed (as expected), after updating endpoint.configureReporting, the new test now passes without manually specifying the manufacturerCode option.

if (attribute.hasOwnProperty('manufacturerCode')) options.manufacturerCode = attribute.manufacturerCode; doesn't feel very TypeScript to me so there may be a cleaner way to write the same line of code ?

This closes #843, I had hoped to be able to add the new features to the H1 before end of year, but since I ran into this it probably going to be for early next year.

When defining a manufacturerCode in the ZCL cluster definition, it should be used when configuring reporting, even when not specified.
@sjorge sjorge force-pushed the reportingmanufacturerCode branch from b394e9d to b8f3f31 Compare December 27, 2023 08:59
@sjorge

This comment was marked as resolved.

@sjorge sjorge force-pushed the reportingmanufacturerCode branch from b8f3f31 to add92d7 Compare December 27, 2023 09:01
@@ -633,6 +633,7 @@ class Endpoint extends Entity {
const attribute = cluster.getAttribute(item.attribute);
dataType = attribute.type;
attrId = attribute.ID;
if (attribute.hasOwnProperty('manufacturerCode')) options.manufacturerCode = attribute.manufacturerCode;
Copy link
Owner

Choose a reason for hiding this comment

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

Could you try if just options.manufacturerCode = attribute.manufacturerCode; also works here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would override the one from

options = this.getOptionsWithDefaults(options, true, Zcl.Direction.CLIENT_TO_SERVER, cluster.manufacturerCode);
by undefined if the attribute doesn't have one set

@Koenkk Koenkk merged commit 9951d44 into Koenkk:master Dec 27, 2023
1 check passed
@Koenkk
Copy link
Owner

Koenkk commented Dec 27, 2023

Thanks again!

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.

endpoint.configureReporting does not take into account attribute specific manufacturerCode ?
2 participants