Rafactor handling of TSNs in ZCL to allow the application using it.

This patch separates the transactionSequenceNumber used for sending from the
one received. According to the specification, the transactionSequenceNumber is
not meant to equally increase on both ends, but instead really just be a "random"
number which allows to match a reply to a request. Syncing them on both ends
has the outcome to increase the likelyhood of collisions if a device sends
a notification at the same time we send a request and thus even may wrongly
interpret that incoming command as a reply to the request. In fact, ideally TSNs for
outgoing messages would stay away as far as possible from incoming ones.

The old code additionally had the problem that it would re-use the last received
TSN for outgoing requests, given it used a post-increment when reading
m_transactionSequenceNumber after setting it to the last received TSN.

The new code will use a single static upcounting TSN for all outgoing requests
but will still allow overriding it with a custom TSN of for some reason a certain
device requires a specific TSN (apparently those exist).
It will not do anything with incoming TSNs but forward them now to the application
layer which may decide to use it match its own transactions or to deduplicate packets.

This allows fixing the issue in nymea that remote controls sometimes produce duplicate
pressed events (seen most often with the Tradfri Symfonisk) by discarding commands that
didn't increase the TSN.

libnymea-zigbee/zcl/closures/zigbeeclusterdoorlock.cpp

@@ -131,9 +131,6 @@ void ZigbeeClusterDoorLock::processDataIndication(ZigbeeClusterLibrary::Frame fr
 {
     qCDebug(dcZigbeeCluster()) << "Processing cluster frame" << m_node << m_endpoint << this << frame;
 
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (m_direction) {
     case Client:
         // If the client cluster sends data to a server cluster (independent which), the command was executed on the device like button pressed

libnymea-zigbee/zcl/closures/zigbeeclusterdoorlock.h

@@ -227,8 +227,8 @@ protected:
     void processDataIndication(ZigbeeClusterLibrary::Frame frame) override;
 
 signals:
-    void lockStateChanged(LockState lockState);
+    void lockStateChanged(ZigbeeClusterDoorLock::LockState lockState);
-    void doorStateChanged(DoorState doorState);
+    void doorStateChanged(ZigbeeClusterDoorLock::DoorState doorState);
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS(ZigbeeClusterDoorLock::SupportedOperationModes)

libnymea-zigbee/zcl/general/zigbeeclusteridentify.cpp

@@ -72,9 +72,6 @@ void ZigbeeClusterIdentify::processDataIndication(ZigbeeClusterLibrary::Frame fr
     Command command = static_cast<Command>(frame.header.command);
     qCDebug(dcZigbeeCluster()) << "Processing cluster frame" << m_node << m_endpoint << this << frame << command;
 
-    // Increase the tsn for continuouse id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (command) {
     case CommandIdentifyQuery:
         // We are not identifying, we can ignore the command according to the specs

libnymea-zigbee/zcl/general/zigbeeclusterlevelcontrol.cpp

@@ -129,16 +129,13 @@ void ZigbeeClusterLevelControl::setAttribute(const ZigbeeClusterAttribute &attri
 
 void ZigbeeClusterLevelControl::processDataIndication(ZigbeeClusterLibrary::Frame frame)
 {
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (m_direction) {
     case Client:
         // If the client cluster sends data to a server cluster (independent which), the command was executed on the device like button pressed
         if (frame.header.frameControl.direction == ZigbeeClusterLibrary::DirectionClientToServer) {
             // Read the payload which is
             Command command = static_cast<Command>(frame.header.command);
-            emit commandSent(command, frame.payload);
+            emit commandSent(command, frame.payload, frame.header.transactionSequenceNumber);
 
             bool withOnOff = false;
             switch (command) {
@@ -151,7 +148,7 @@ void ZigbeeClusterLevelControl::processDataIndication(ZigbeeClusterLibrary::Fram
                 payloadStream >> level >> transitionTime;
                 withOnOff = command == CommandMoveToLevelWithOnOff;
                 qCDebug(dcZigbeeCluster()).noquote().nospace() << "Command received from " << m_node << " " << m_endpoint << " " << this << " " << command << " withOnOff: " << withOnOff << " level: 0x" << QString::number(level, 16) << " transitionTime: 0x" << QString::number(transitionTime, 16);
-                emit commandMoveToLevelSent(withOnOff, level, transitionTime);
+                emit commandMoveToLevelSent(withOnOff, level, transitionTime, frame.header.transactionSequenceNumber);
                 break;
             }
             case CommandStepWithOnOff:
@@ -163,7 +160,7 @@ void ZigbeeClusterLevelControl::processDataIndication(ZigbeeClusterLibrary::Fram
                 payloadStream >> stepModeValue >> stepSize >> transitionTime;
                 withOnOff = command == CommandMoveToLevelWithOnOff;
                 qCDebug(dcZigbeeCluster()).noquote().nospace() << "Command received from " << m_node << " " << m_endpoint << " " << this << " " << command << " withOnOff: " << withOnOff << " stepModeValue: 0x" << QString::number(stepModeValue, 16) << " stepSize: 0x" << QString::number(stepSize, 16) << " transitionTime: 0x" << QString::number(transitionTime, 16);
-                emit commandStepSent(withOnOff, static_cast<StepMode>(stepModeValue), stepSize, transitionTime);
+                emit commandStepSent(withOnOff, static_cast<StepMode>(stepModeValue), stepSize, transitionTime, frame.header.transactionSequenceNumber);
                 break;
             }
             case CommandMoveWithOnOff:
@@ -175,9 +172,14 @@ void ZigbeeClusterLevelControl::processDataIndication(ZigbeeClusterLibrary::Fram
                 payloadStream >> moveModeValue >> rate;
                 withOnOff = command == CommandMoveToLevelWithOnOff;
                 qCDebug(dcZigbeeCluster()).noquote().nospace() << "Command received from " << m_node << " " << m_endpoint << " " << this << " " << command << " withOnOff:" << withOnOff <<  " moveModeValue: 0x" << QString::number(moveModeValue, 16) << " rate: 0x" << QString::number(rate, 16);
-                emit commandMoveSent(withOnOff, static_cast<MoveMode>(moveModeValue), rate);
+                emit commandMoveSent(withOnOff, static_cast<MoveMode>(moveModeValue), rate, frame.header.transactionSequenceNumber);
                 break;
             }
+            case CommandStopWithOnOff:
+            case CommandStop:
+                withOnOff = command == CommandMoveToLevelWithOnOff;
+                emit commandStopSent(withOnOff, frame.header.transactionSequenceNumber);
+                break;
             default:
                 qCDebug(dcZigbeeCluster()).noquote().nospace() << "Command received from " << m_node << " " << m_endpoint << " " << this << " " << command << " payload: 0x" << ZigbeeUtils::convertByteArrayToHexString(frame.payload);
                 break;

libnymea-zigbee/zcl/general/zigbeeclusterlevelcontrol.h

@@ -106,11 +106,11 @@ protected:
 
 signals:
     void currentLevelChanged(quint8 level);
-    void commandSent(ZigbeeClusterLevelControl::Command command, const QByteArray &parameter = QByteArray());
+    void commandSent(ZigbeeClusterLevelControl::Command command, const QByteArray &parameter, quint8 transactionSequenceNumber);
-    void commandMoveToLevelSent(bool withOnOff, quint8 level, quint16 transitionTime);
+    void commandMoveToLevelSent(bool withOnOff, quint8 level, quint16 transitionTime, quint8 transactionSequenceNumber);
-    void commandMoveSent(bool withOnOff, MoveMode moveMode, quint8 rate = 0xff);
+    void commandMoveSent(bool withOnOff, MoveMode moveMode, quint8 rate, quint8 transactionSeqenceNumber);
-    void commandStepSent(bool withOnOff, StepMode stepMode, quint8 stepSize, quint16 transitionTime);
+    void commandStepSent(bool withOnOff, StepMode stepMode, quint8 stepSize, quint16 transitionTime, quint8 transactionSequenceNumber);
-    void commandStopSent();
+    void commandStopSent(bool withOnOff, quint8 transactionSequenceNumber);
 
 };
 

libnymea-zigbee/zcl/general/zigbeeclusteronoff.cpp

@@ -105,16 +105,13 @@ void ZigbeeClusterOnOff::processDataIndication(ZigbeeClusterLibrary::Frame frame
 {
     qCDebug(dcZigbeeCluster()) << "Processing cluster frame" << m_node << m_endpoint << this << frame;
 
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (m_direction) {
     case Client:
         // If the client cluster sends data to a server cluster (independent which), the command was executed on the device like button pressed
         if (frame.header.frameControl.direction == ZigbeeClusterLibrary::DirectionClientToServer) {
             // Read the payload which is
             Command command = static_cast<Command>(frame.header.command);
-            emit commandSent(command, frame.payload);
+            emit commandSent(command, frame.payload, frame.header.transactionSequenceNumber);
             switch (command) {
             case CommandOffWithEffect: {
                 QByteArray payload = frame.payload;
@@ -123,7 +120,7 @@ void ZigbeeClusterOnOff::processDataIndication(ZigbeeClusterLibrary::Frame frame
                 quint8 effectValue = 0; quint16 effectVariant;
                 payloadStream >> effectValue >> effectVariant;
                 qCDebug(dcZigbeeCluster()) << "Command received from" << m_node << m_endpoint << this << command << "effect:" << effectValue << "effectVariant:" << effectVariant;
-                emit commandOffWithEffectSent(static_cast<Effect>(effectValue), effectVariant);
+                emit commandOffWithEffectSent(static_cast<Effect>(effectValue), effectVariant, frame.header.transactionSequenceNumber);
                 break;
             }
             case CommandOnWithTimedOff: {
@@ -133,7 +130,7 @@ void ZigbeeClusterOnOff::processDataIndication(ZigbeeClusterLibrary::Frame frame
                 quint8 acceptOnlyWhenOnInt = 0; quint16 onTime; quint16 offTime;
                 payloadStream >> acceptOnlyWhenOnInt >> onTime >> offTime;
                 qCDebug(dcZigbeeCluster()) << "Command received from" << m_node << m_endpoint << this << command << "accentOnlyWhenOnInt:" << acceptOnlyWhenOnInt << "onTime:" << onTime << "offTime:" << offTime;
-                emit commandOnWithTimedOffSent(static_cast<bool>(acceptOnlyWhenOnInt), onTime, offTime);
+                emit commandOnWithTimedOffSent(static_cast<bool>(acceptOnlyWhenOnInt), onTime, offTime, frame.header.transactionSequenceNumber);
                 break;
             }
             default:

libnymea-zigbee/zcl/general/zigbeeclusteronoff.h

@@ -94,12 +94,12 @@ signals:
     void powerChanged(bool power);
 
     // Client cluster signals
-    void commandSent(Command command, const QByteArray &parameters = QByteArray());
+    void commandSent(ZigbeeClusterOnOff::Command command, const QByteArray &parameters, quint8 transactionSequenceNumber);
     // On and off time is in 1/10 seconds
-    void commandOnWithTimedOffSent(bool acceptOnlyWhenOn, quint16 onTime, quint16 offTime);
+    void commandOnWithTimedOffSent(bool acceptOnlyWhenOn, quint16 onTime, quint16 offTime, quint8 transactionSequenceNumber);
 
     // On and off time is in 1/10 seconds
-    void commandOffWithEffectSent(Effect effect, quint8 effectVariant);
+    void commandOffWithEffectSent(ZigbeeClusterOnOff::Effect effect, quint8 effectVariant, quint8 transactionSequenceNumber);
 };
 
 #endif // ZIGBEECLUSTERONOFF_H

libnymea-zigbee/zcl/general/zigbeeclusterscenes.cpp

@@ -45,9 +45,6 @@ void ZigbeeClusterScenes::setAttribute(const ZigbeeClusterAttribute &attribute)
 
 void ZigbeeClusterScenes::processDataIndication(ZigbeeClusterLibrary::Frame frame)
 {
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (m_direction) {
     case Client: {
         // Read the payload which is
@@ -58,7 +55,7 @@ void ZigbeeClusterScenes::processDataIndication(ZigbeeClusterLibrary::Frame fram
         quint16 groupId = 0; quint8 sceneId;
         payloadStream >> groupId >> sceneId;
         qCDebug(dcZigbeeCluster()).noquote() << "Received" << command << "for group" << "0x" + QString::number(groupId, 16) << "and scene" << sceneId << "from" << m_node << m_endpoint << this;
-        emit commandSent(command, groupId, sceneId);
+        emit commandSent(command, groupId, sceneId, frame.header.transactionSequenceNumber);
         break;
     }
     case Server:

libnymea-zigbee/zcl/general/zigbeeclusterscenes.h

@@ -63,7 +63,7 @@ public:
     explicit ZigbeeClusterScenes(ZigbeeNetwork *network, ZigbeeNode *node, ZigbeeNodeEndpoint *endpoint, Direction direction, QObject *parent = nullptr);
 
 signals:
-    void commandSent(Command command, quint16 groupId, quint8 sceneId);
+    void commandSent(ZigbeeClusterScenes::Command command, quint16 groupId, quint8 sceneId, quint8 transactionSequenceNumber);
 
 private:
     void setAttribute(const ZigbeeClusterAttribute &attribute) override;

libnymea-zigbee/zcl/hvac/zigbeeclusterthermostat.cpp

@@ -41,9 +41,6 @@ void ZigbeeClusterThermostat::processDataIndication(ZigbeeClusterLibrary::Frame
 {
     qCDebug(dcZigbeeCluster()) << "Processing cluster frame" << m_node << m_endpoint << this << frame;
 
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
 //    switch (m_direction) {
 //    case Client:
 //        if (frame.header.frameControl.direction == ZigbeeClusterLibrary::DirectionClientToServer) {

libnymea-zigbee/zcl/lighting/zigbeeclustercolorcontrol.cpp

@@ -258,12 +258,3 @@ void ZigbeeClusterColorControl::setAttribute(const ZigbeeClusterAttribute &attri
     }
 }
 
-void ZigbeeClusterColorControl::processDataIndication(ZigbeeClusterLibrary::Frame frame)
-{
-    qCDebug(dcZigbeeCluster()) << "Processing cluster frame" << m_node << m_endpoint << this << frame;
-
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-}
-
-

libnymea-zigbee/zcl/lighting/zigbeeclustercolorcontrol.h

@@ -244,9 +244,6 @@ private:
 
     void setAttribute(const ZigbeeClusterAttribute &attribute) override;
 
-protected:
-    void processDataIndication(ZigbeeClusterLibrary::Frame frame) override;
-
 };
 
 #endif // ZIGBEECLUSTERCOLORCONTROL_H

libnymea-zigbee/zcl/manufacturerspecific/philips/zigbeeclustermanufacturerspecificphilips.cpp

@@ -43,9 +43,6 @@ void ZigbeeClusterManufacturerSpecificPhilips::processDataIndication(ZigbeeClust
 {
     qCDebug(dcZigbeeCluster()) << "Processing manufacturer specific (Philips) cluster frame" << m_node << m_endpoint << this << frame;
 
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (m_direction) {
     case Client:
         qCWarning(dcZigbeeCluster()) << "Unhandled ZCL indication in" << m_node << m_endpoint << this << frame;
@@ -60,7 +57,7 @@ void ZigbeeClusterManufacturerSpecificPhilips::processDataIndication(ZigbeeClust
                 quint8 button; quint16 unknown1; quint8 unknown2; quint8 operation;
                 payloadStream >> button >> unknown1 >> unknown2 >> operation;
                 qCDebug(dcZigbeeCluster()) << "Received manufacturer specific (Philips) button press. Button:" << button << "Operation:" << operation;
-                emit buttonPressed(button, Operation(operation));
+                emit buttonPressed(button, Operation(operation), frame.header.transactionSequenceNumber);
                 break;
             }
             default:

libnymea-zigbee/zcl/manufacturerspecific/philips/zigbeeclustermanufacturerspecificphilips.h

@@ -63,7 +63,7 @@ public:
 
 signals:
     // Server cluster signals
-    void buttonPressed(quint8 button, Operation operation);
+    void buttonPressed(quint8 button, ZigbeeClusterManufacturerSpecificPhilips::Operation operation, quint8 transactionSequenceNumber);
 
 protected:
     void processDataIndication(ZigbeeClusterLibrary::Frame frame) override;

libnymea-zigbee/zcl/ota/zigbeeclusterota.cpp

@@ -59,9 +59,6 @@ void ZigbeeClusterOta::processDataIndication(ZigbeeClusterLibrary::Frame frame)
 {
     qCDebug(dcZigbeeCluster()) << "Processing cluster frame" << m_node << m_endpoint << this << frame;
 
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (m_direction) {
     case Client:
         if (frame.header.frameControl.direction == ZigbeeClusterLibrary::DirectionClientToServer) {

libnymea-zigbee/zcl/security/zigbeeclusteriaszone.cpp

@@ -113,9 +113,6 @@ void ZigbeeClusterIasZone::processDataIndication(ZigbeeClusterLibrary::Frame fra
 {
     qCDebug(dcZigbeeCluster()) << "Processing cluster frame" << m_node << m_endpoint << this << frame;
 
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     switch (m_direction) {
     case Client:
         // TODO: handle client frames
@@ -161,7 +158,7 @@ void ZigbeeClusterIasZone::processDataIndication(ZigbeeClusterLibrary::Frame fra
                                            << zoneType << "Manufacturer code:" << ZigbeeUtils::convertUint16ToHexString(manufacturerCode);
                 // Update the ZoneState attribute
                 setAttribute(ZigbeeClusterAttribute(AttributeZoneType, ZigbeeDataType(Zigbee::Enum16, frame.payload.left(2))));
-                emit zoneEnrollRequest(zoneType, manufacturerCode);
+                emit zoneEnrollRequest(zoneType, manufacturerCode, frame.header.transactionSequenceNumber);
 
                 // Respond with default response if enabled
                 if (!frame.header.frameControl.disableDefaultResponse) {

libnymea-zigbee/zcl/security/zigbeeclusteriaszone.h

@@ -145,7 +145,7 @@ protected:
 
 signals:
     void zoneStatusChanged(ZoneStatusFlags zoneStatus, quint8 extendedStatus, quint8 zoneId, quint16 delay);
-    void zoneEnrollRequest(ZoneType zoneType, quint16 manufacturerCode);
+    void zoneEnrollRequest(ZoneType zoneType, quint16 manufacturerCode, quint8 transactionSequenceNumber);
 
 };
 

libnymea-zigbee/zcl/zigbeecluster.cpp

@@ -135,7 +135,7 @@ ZigbeeClusterReply *ZigbeeCluster::configureReporting(QList<ZigbeeClusterLibrary
 }
 
 
-ZigbeeClusterReply *ZigbeeCluster::executeGlobalCommand(quint8 command, const QByteArray &payload, quint16 manufacturerCode)
+ZigbeeClusterReply *ZigbeeCluster::executeGlobalCommand(quint8 command, const QByteArray &payload, quint16 manufacturerCode, quint8 transactionSequenceNumber)
 {
     // Build the request
     ZigbeeNetworkRequest request = createGeneralRequest();
@@ -165,7 +165,7 @@ ZigbeeClusterReply *ZigbeeCluster::executeGlobalCommand(quint8 command, const QB
     if (manufacturerCode != 0) {
         header.manufacturerCode = manufacturerCode;
     }
-    header.transactionSequenceNumber = m_transactionSequenceNumber++;
+    header.transactionSequenceNumber = transactionSequenceNumber;
 
     // Put them together
     ZigbeeClusterLibrary::Frame frame;
@@ -218,7 +218,7 @@ ZigbeeClusterReply *ZigbeeCluster::executeClusterCommand(quint8 command, const Q
     ZigbeeClusterLibrary::Header header;
     header.frameControl = frameControl;
     header.command = command;
-    header.transactionSequenceNumber = m_transactionSequenceNumber++;
+    header.transactionSequenceNumber = newTransactionSequenceNumber();
 
     // Build ZCL frame
     ZigbeeClusterLibrary::Frame frame;
@@ -406,9 +406,6 @@ void ZigbeeCluster::finishZclReply(ZigbeeClusterReply *zclReply)
 
 void ZigbeeCluster::processDataIndication(ZigbeeClusterLibrary::Frame frame)
 {
-    // Increase the tsn for continuous id increasing on both sides
-    m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
-
     // Warn about the unhandled cluster indication, you can override this method in cluster implementations
     qCWarning(dcZigbeeCluster()) << "Unhandled ZCL indication in" << m_node << m_endpoint << this << frame;
 }
@@ -424,6 +421,12 @@ void ZigbeeCluster::updateOrAddAttribute(const ZigbeeClusterAttribute &attribute
     }
 }
 
+quint8 ZigbeeCluster::newTransactionSequenceNumber()
+{
+    static quint8 tsn = 1;
+    return tsn++;
+}
+
 void ZigbeeCluster::processApsDataIndication(const QByteArray &asdu, const ZigbeeClusterLibrary::Frame &frame)
 {
     // Check if this indication is for a pending reply
@@ -452,8 +455,6 @@ void ZigbeeCluster::processApsDataIndication(const QByteArray &asdu, const Zigbe
             }
         }
 
-        // Increase the tsn for continuous id increasing on both sides
-        m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
         return;
     }
 
@@ -472,8 +473,6 @@ void ZigbeeCluster::processApsDataIndication(const QByteArray &asdu, const Zigbe
                 setAttribute(ZigbeeClusterAttribute(attributeId, dataType));
             }
 
-            // Increase the tsn for continuous id increasing on both sides
-            m_transactionSequenceNumber = frame.header.transactionSequenceNumber;
             return;
         }
     }

libnymea-zigbee/zcl/zigbeecluster.h

@@ -105,12 +105,10 @@ protected:
 
     // Helper methods for sending cluster specific commands
     ZigbeeNetworkRequest createGeneralRequest();
-    quint8 m_transactionSequenceNumber = 0;
     QHash<quint8, ZigbeeClusterReply *> m_pendingReplies;
 
     // Global commands
-    ZigbeeClusterReply *executeGlobalCommand(quint8 command, const QByteArray &payload = QByteArray(), quint16 manufacturerCode = 0x0000);
+    ZigbeeClusterReply *executeGlobalCommand(quint8 command, const QByteArray &payload = QByteArray(), quint16 manufacturerCode = 0x0000, quint8 transactionSequenceNumber = newTransactionSequenceNumber());
-    ZigbeeClusterReply *executeManufacturerSpecificGlobalCommand(quint8 command, quint16 manufacturerCode, const QByteArray &payload = QByteArray());
 
     // Cluster specific
     ZigbeeClusterReply *createClusterReply(const ZigbeeNetworkRequest &request, ZigbeeClusterLibrary::Frame frame);
@@ -126,6 +124,8 @@ protected:
 
     void updateOrAddAttribute(const ZigbeeClusterAttribute &attribute);
 
+    static quint8 newTransactionSequenceNumber();
+
 private:
     virtual void setAttribute(const ZigbeeClusterAttribute &attribute);