Configure Reporting Response Command

ebaauw commented 5 years ago

The ZCL spec states (in 2.5.8.1) that the ZCL payload of the Configure Reporting Response command should contain a series of (Status, Direction, Attribute Identifier).

I found during sniffing that the Eurotronic Spirit Zigbee sends Configure Reporting Response Commands with only a single byte payload, presumably the Status (see https://github.com/dresden-elektronik/deconz-rest-plugin/issues/1098#issuecomment-461608989). Wireshark reports an error, and I though I also saw an error in the deCONZ log.

While integration another iCasa dimmer yesterday, I found that it too sent a Configure Reporting Response response with only the Status. Yet deCONZ, configured the reporting for 0x0006/0x0000 and 0x0008/0x0000 nonetheless.

IEEE 802.15.4 Data, Dst: 0x1e16, Src: 0x90e4
ZigBee Network Layer Data, Dst: 0x0000, Src: 0x90e4
    Frame Control Field: 0x0248, Frame Type: Data, Discover Route: Enable, Security Data
    Destination: 0x0000
    Source: 0x90e4
    Radius: 30
    Sequence Number: 178
    [Extended Source: Ember_00:10:65:6f:f1 (00:0d:6f:00:10:65:6f:f1)]
    [Origin: 1]
    ZigBee Security Header
ZigBee Application Support Layer Data, Dst Endpt: 1, Src Endpt: 1
    Frame Control Field: Data (0x40)
    Destination Endpoint: 1
    Cluster: On/Off (0x0006)
    Profile: Home Automation (0x0104)
    Source Endpoint: 1
    Counter: 170
ZigBee Cluster Library Frame, Command: Configure Reporting Response, Seq: 152
    Frame Control Field: Profile-wide (0x08)
    Sequence Number: 152
    Command: Configure Reporting Response (0x07)
[Malformed Packet: ZigBee ZCL]
    [Expert Info (Error/Malformed): Malformed Packet (Exception occurred)]

Looking at the code, the REST API plugin seems to handle this case, but doesn't mark the reporting as being successfully configured. https://github.com/dresden-elektronik/deconz-rest-plugin/blob/72006cccc7d79862dbbe9bb857838b4e8c23269c/bindings.cpp#L458-L462

I really want to overwrite the factory default reporting for the Eurotronic Spirit (as currently in the code but commented out, see https://github.com/dresden-elektronik/deconz-rest-plugin/issues/1098#issuecomment-461936332), but I'm wondering whether it's safe to do so without the successfully configured mark. There's also some factory default periodic reporting for attributes 0x0012 and 0x0014, which I want to disable, as the REST API plugin doesn't use these (and 0x0014 isn't even functional).

The Eurotronic has Jennic all over it: mac prefix 0x00158d and manufacturer code 0x1037; the iCasa dimmer is a bit schizophrenic: mac prefix 0x000d6f (Ember) and manufacturer code 0x100b (Philips?!). Is that the same Zigbee stack, or are multiple manufacturers taking the same liberties interpreting the standard?

manup commented 5 years ago

The response seems to be valid and is constructed as described in ZCL spec 2.5.7.3 Effect on Receipt

When all attribute reporting configuration records have been processed, the device SHALL generate the constructed Configure Reporting Response command. If there are no attribute status records in the constructed command, indicating that all attributes were configured successfully, a single attribute status record SHALL be included in the command, with the status field set to SUCCESS and the direction and attribute identifier fields omitted.

So we need to check for SUCCESS and mark the reporting as successfully configured.

ebaauw commented 5 years ago

The response seems to be valid and is constructed as described in ZCL spec 2.5.7.3 Effect on Receipt

Indeed. Why don't they mention it under 2.5.8?

Do you know how to issue a change request/bug report to Wireshark?

So we need to check for SUCCESS and mark the reporting as successfully configured.

That would involve storing the request and matching the response to the request? It would seem there's already a runningTasks mechanism for that in de_web_plugin.cpp, but I'm not sure how to use that in bindings.cpp (or if it's even usable for sensor nodes).

I think I found another issue: sendConfigureReportingRequest() combines the reporting configuration for multiple attributes into a single request. Wouldn't that only work when either all attributes are standard, or all attributes are manufacturer specific? I think I need to call this twice for the Thermostat cluster, as the Eurotronic Spirit needs reporting for both standard and specific attributes.

manup commented 5 years ago

Do you know how to issue a change request/bug report to Wireshark?

Nope haven't done this yet, I guess they have a issue tracker perhaps on Github too.

That would involve storing the request and matching the response to the request? It would seem there's already a runningTasks mechanism for that in de_web_plugin.cpp, but I'm not sure how to use that in bindings.cpp (or if it's even usable for sensor nodes).

We already have the related ZCL sequence number NodeValue::zclSeqNum this can be used to match the respective cluster attribute report response.

Likely in a loop over all NodeValue attributes.

I think I found another issue: sendConfigureReportingRequest() combines the reporting configuration for multiple attributes into a single request. Wouldn't that only work when either all attributes are standard, or all attributes are manufacturer specific? I think I need to call this twice for the Thermostat cluster, as the Eurotronic Spirit needs reporting for both standard and specific attributes.

Good catch, yes I think manufacturer specific configuration must be separated.

ebaauw commented 5 years ago

We already have the related ZCL sequence number NodeValue::zclSeqNum this can be used to match the respective cluster attribute report response.

Likely in a loop over all NodeValue attributes.

That code seems already to be there: https://github.com/dresden-elektronik/deconz-rest-plugin/blob/72006cccc7d79862dbbe9bb857838b4e8c23269c/bindings.cpp#L468-L483

I do see a couple of issues:

Status isn't checked;
The loop over all NodeValue attributes seems to be executed for each attribute in the reponse, instead of checking the attrId from the response. In fact, this looks like this is the code that should be executed if the response only contains a single status for all attributes.
In sendConfigureReportingRequest(BindingTask &bt) the NodeValue's zclSeqNum (and minValue and maxValue) is only set for a couple of clusters/attributes. Does it make sense to handle that in sendConfigureReportingRequest(BindingTask &bt, const std::vector<ConfigureReportingRequest> &requests) instead? We already check the NodeValue there to see if reporting is already active.

Another question: why the check for minInterval == 0 && maxInterval == 0? If I understand the ZCL spec, to reset to factory default settings, they should be 0 and 65535. What values should be used to disable reporting altogether?

ebaauw commented 5 years ago

Commit should handle the case where the response only contains a single status field. It seems to be working in my test network, for both an IKEA colour light and an OSRAM colour temperature light.

Somehow, the Level cluster attribute 0x0000 doesn't get configured, since the value hasn't yet been created. But as long as the light is off, the level won't be polled.

Haven't yet tested the case where response contains a status per attribute.

dresden-elektronik / deconz-rest-plugin

Configure Reporting Response Command #1331