Button events check for duplicated sequence number (IKEA, Tuya and LIDL)

[manup]

• Limited to IKEA and Tuya devices (also LIDL) to discard duplicated commands;
• For all other devices a warning is printed so we can track the issue;
• Sensor.previousSequenceNumber changed to uint16_t with initial value 0xffff to not accidantly discard valid commands.

This PR is successor for #4265 but only addresses the duplicate switch events.

The whole ZCL Default Response needs to be fixed at a higher level for all handlers in a separate PR.

[ebaauw]

Please limit this to Tuya devices.

For IKEA, I've only seen the issue when they send both groupcast and unicast messages, because of the binding of On/Off (and other client clusters) to the group, and of Power Configuration to the coordinator. Fixed that in #4627 by ignoring the unicast (seems safer than relying on sequence numbers which are recycled after 255 messages).

[manup]

Please limit this to Tuya devices.

For IKEA, I've only seen the issue when they send both groupcast and unicast messages, because of the binding of On/Off (and other client clusters) to the group, and of Power Configuration to the coordinator. Fixed that in #4627 by ignoring the unicast (seems safer than relying on sequence numbers which are recycled after 255 messages).

Ok I'll limit this to Tuya (not includes LIDL remote but it already had this handling).

Does #4627 also fix the issue for the IKEA shortcut button? In #4227 it is mentioned to have this problem too.

[ebaauw]

For all of these:

deconz-rest-plugin/de_web_plugin.cpp

Lines 4054 to 4076 in 2512867

	else if (sensor->modelId().startsWith(QLatin1String("TRADFRI on/off switch")) ||
	sensor->modelId().startsWith(QLatin1String("TRADFRI SHORTCUT Button")) ||
	sensor->modelId().startsWith(QLatin1String("Remote Control N2")) ||
	sensor->modelId().startsWith(QLatin1String("TRADFRI open/close remote")) ||
	// sensor->modelId().startsWith(QLatin1String("SYMFONISK")) ||
	sensor->modelId().startsWith(QLatin1String("TRADFRI motion sensor")))
	{
	checkReporting = true;
	if (ind.dstAddressMode() == deCONZ::ApsGroupAddress && ind.dstAddress().group() == 0)
	{
	checkClientCluster = true;
	ResourceItem *item = sensor->item(RConfigGroup);
	if (!item || (item && (item->toString() == QLatin1String("0") || item->toString().isEmpty())))
	{
	// still default group, create unique group and binding
	checkSensorGroup(sensor);
	}
	}
	if (ind.dstAddressMode() == deCONZ::ApsNwkAddress)
	{
	return;
	}
	}

[manup]

For all of these:

Perfect, have removed them now. And noticed I may have introduced a regression :)

For the HG06323 remote the original manufacturer name is replaced with "LIDL Livarno Lux" and here the isTuyaManufacturerName() doesn't trigger anymore.

I think the check needs to be changed to:

    if (zclFrame.sequenceNumber() == sensor->previousSequenceNumber)
    {
        // useful in general but limit scope to known problematic devices
        if (isTuyaManufacturerName(sensor->manufacturer()) || sensor->modelId() == QLatin1String("HG06323"))
        {
            // deCONZ doesn't always send ZCL Default Response to unicast commands, or they can get lost.
            // in this case some devices re-send the command multiple times
            DBG_Printf(DBG_INFO, "Discard duplicated zcl.cmd: 0x%02X, cluster: 0x%04X with zcl.seq: %u for %s / %s\n",
                       zclFrame.commandId(), ind.clusterId(), zclFrame.sequenceNumber(), qPrintable(sensor->manufacturer()), qPrintable(sensor->modelId()));
            return;
        }
        else // warn only
        {
            DBG_Printf(DBG_INFO, "Warning duplicated zcl.cmd: 0x%02X, cluster: 0x%04X with zcl.seq: %u for %s / %s\n",
                       zclFrame.commandId(), ind.clusterId(), zclFrame.sequenceNumber(), qPrintable(sensor->manufacturer()), qPrintable(sensor->modelId()));
        }
    }

[ebaauw]

For the HG06323 remote the original manufacturer name is replaced with "LIDL Livarno Lux" and here the isTuyaManufacturerName() doesn't trigger anymore.

Has anyone ever reported duplicate buttonevents for this device? Note that despite the name, this is a regular device, like all LIDL devices I've seen. It doesn't carry the Tuya cluster, and uses regular client clusters with group bindings (and without the bug in some of the IKEA firmwares).

[manup]

Hard to tell, the double sequence number check was already there (from the beginning?) so even if this was the case it wouldn't be noticed by users. I don't know if the check was only there because of copy paste or if the problem actually occured. If the remote only sends group casts the ZCL Default Response issue shouldn't be a problem.

To be on the safe side I'd add the check as shown above, with a comment to remove it after testing. I have access to the remote next week.

[Smanar]

The whole ZCL Default Response needs to be fixed at a higher level for all handlers in a separate PR.

From my memory the defaut response can solve the issue too.

[Smanar]

Lol, so I don't found the "approval" button.
But for me it s fine, Tuya/lidl are less sensible than ikea, and realy need this patch, time for the ZCL defaut response be fonctionnal, too much issues compared to the 255 looping message risk.

[manup]

This PR is meant to only address one thing, the duplicated commands. I'd like to fix the ZCL Default Response for good and in general (not device specific) in a separate issue. For this all other handlers need to be taken into account which needs a slight refactor.

[Smanar]

And logs can be usefull too, to reconize this issue for other brands/devices

[manup]

Note that the ZCL Default Response even then fixed can fail for all kinds of reasons: Like the APS queue is full, or the command gets lost on the way back — as we sometimes have routes which only work in one direction. Therefore having the duplicate check is not too bad in general.

[ebaauw]

Therefore having the duplicate check is not too bad in general.

No, but it should be at the level of receiving the APS-DATA.indication, not when handling the buttonevents.

[manup]

No, but it should be at the level of receiving the APS-DATA.indication, not when handling the buttonevents.

I hope so but would like to test this more before rolling out for all devices. I'm afraid of some wonky devices which are lazy to increase the sequence number.

[Smanar]

Seriously there is some devices that not increase the sequence number ?

[manup]

Seriously there is some devices that not increase the sequence number ?

I hope not but wouldn't be surprised :D