Fix updating transaction state for (near to) fragmented frames (#940)

Signed-off-by: Witold Sowa <[email protected]>

Author: wsowa

com.zsmartsystems.zigbee/src/main/java/com/zsmartsystems/zigbee/ZigBeeNetworkManager.java

@@ -1901,9 +1901,9 @@ public Future<CommandResult> sendTransaction(ZigBeeCommand command, ZigBeeTransa
     @Override
     public void receiveCommandState(int msgTag, ZigBeeTransportProgressState state) {
         logger.debug("RX STA: msgTag={} state={}", String.format("%02X", msgTag), state);
-        if (apsDataEntity.receiveCommandState(msgTag, state) == false) {
-            return;
+        if (apsDataEntity.receiveCommandState(msgTag, state)) {
+            // Pass the update to the transaction if this is NACK or ACK for last/only fragment
+            transactionManager.receiveCommandState(msgTag, state);
         }
-        transactionManager.receiveCommandState(msgTag, state);
     }
 }

com.zsmartsystems.zigbee/src/main/java/com/zsmartsystems/zigbee/aps/ApsDataEntity.java

@@ -164,7 +164,7 @@ public synchronized boolean send(final int msgTag, final ZigBeeApsFrame apsFrame
 
         // Check that we have fragmentation enabled and that this frame requires fragmenting
         // TODO: Don't fragment unicast or broadcast
-        if (apsFrame.getPayload().length < fragmentationLength || fragmentationWindow == 0) {
+        if (apsFrame.getPayload().length <= fragmentationLength || fragmentationWindow == 0) {
             transport.sendCommand(msgTag, apsFrame);
             return true;
         }
@@ -225,7 +225,7 @@ private void sendNextFragments(final ZigBeeApsFrame apsFrame) {
 
             fragment.setPayload(Arrays.copyOfRange(apsFrame.getPayload(), offset, end));
 
-            apsFrame.setFragmentOutstanding(apsFrame.getFragmentOutstanding() + 1);
+            apsFrame.oneFragmentSent();
             logger.debug("Sending APS Frame Fragment: outstanding={} {}", apsFrame.getFragmentOutstanding(), fragment);
 
             transport.sendCommand(apsFrame.getMsgTag(), fragment);
@@ -239,7 +239,7 @@ private void sendNextFragments(final ZigBeeApsFrame apsFrame) {
      *
      * @param msgTag the message tag whose state is updated
      * @param state the updated ZigBeeTransportProgressState for the transaction
-     * @return false if this frame is part of a fragmented packet and this is not the last frame. true otherwise.
+     * @return true if this last frame in packet or not fragmented frame. false otherwise.
      */
     public boolean receiveCommandState(int msgTag, ZigBeeTransportProgressState state) {
         ZigBeeApsFrame outgoingFrame = getFragmentedFrameFromQueue(msgTag);
@@ -258,8 +258,17 @@ public boolean receiveCommandState(int msgTag, ZigBeeTransportProgressState stat
         logger.debug("receiveCommandState tag={}, fragmentBase={}, fragmentOutstanding={}", msgTag,
                 outgoingFrame.getFragmentBase(), outgoingFrame.getFragmentOutstanding());
 
-        outgoingFrame.setFragmentBase(outgoingFrame.getFragmentBase() + 1);
-        outgoingFrame.setFragmentOutstanding(outgoingFrame.getFragmentOutstanding() - 1);
+        // TX ACK doesn't mean the frame was delivered so it should be ignored.
+        if (state == ZigBeeTransportProgressState.RX_ACK) {
+            outgoingFrame.oneFragmentCompleted();
+        }
+
+        if (state == ZigBeeTransportProgressState.TX_ACK &&
+                outgoingFrame.getFragmentBase() + 1 == outgoingFrame.getFragmentTotal()) {
+            logger.debug("TX ACKed for last block in frame: {}", outgoingFrame);
+            return true;
+        }
+
         if (outgoingFrame.getFragmentBase() == outgoingFrame.getFragmentTotal()) {
             logger.debug("Completed Sending Fragment APS Frame: {}", outgoingFrame);
             return true;

com.zsmartsystems.zigbee/src/main/java/com/zsmartsystems/zigbee/aps/ZigBeeApsFrame.java

@@ -426,4 +426,23 @@ public String toString() {
         return builder.toString();
     }
 
+    /**
+     * Calling this method indicates that transmission of a fragment has completed.
+     * It moves the fragment base and decrease outstanding fragments counter.
+     */
+    public void oneFragmentCompleted() {
+        fragmentBase++;
+        if (fragmentOutstanding > 0) {
+            fragmentOutstanding--;
+        }
+    }
+
+    /**
+     * Calling this method indicates that a fragment has been sent. It increases outstanding fragments counter.
+     */
+    public void oneFragmentSent() {
+        if (fragmentOutstanding <= fragmentTotal) {
+            this.fragmentOutstanding++;
+        }
+    }
 }

com.zsmartsystems.zigbee/src/test/java/com/zsmartsystems/zigbee/aps/ApsDataEntityTest.java

@@ -27,6 +27,7 @@
  *
  */
 public class ApsDataEntityTest {
+    public static final int FRAGMENTATION_LENGTH = 65;
     private static int TIMEOUT = 5000;
 
     @Test
@@ -226,6 +227,83 @@ public void receiveCommandState() {
         assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
     }
 
+    @Test
+    public void shouldExpectOneFrameReceivedForPayloadShorterThanFragmentationLength() {
+        ZigBeeTransportTransmit transport = Mockito.mock(ZigBeeTransportTransmit.class);
+        ApsDataEntity aps = new ApsDataEntity(transport);
+
+        aps.setFragmentationLength(FRAGMENTATION_LENGTH);
+
+        ZigBeeApsFrame apsFrame = new ZigBeeApsFrame();
+        apsFrame.setApsCounter(1);
+        apsFrame.setPayload(createData(0, FRAGMENTATION_LENGTH-1));
+
+        aps.send(0, apsFrame);
+
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.TX_ACK));
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
+    }
+
+    @Test
+    public void shouldExpectOneFrameReceivedForPayloadEqualToFragmentationLength() {
+        ZigBeeTransportTransmit transport = Mockito.mock(ZigBeeTransportTransmit.class);
+        ApsDataEntity aps = new ApsDataEntity(transport);
+
+        aps.setFragmentationLength(FRAGMENTATION_LENGTH);
+
+        ZigBeeApsFrame apsFrame = new ZigBeeApsFrame();
+        apsFrame.setApsCounter(1);
+        apsFrame.setPayload(createData(0, FRAGMENTATION_LENGTH));
+
+        aps.send(0, apsFrame);
+
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.TX_ACK));
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
+    }
+
+    @Test
+    public void shouldExpectTwoFramesReceivedForPayloadFittingInTwoFragments() {
+        ZigBeeTransportTransmit transport = Mockito.mock(ZigBeeTransportTransmit.class);
+        ApsDataEntity aps = new ApsDataEntity(transport);
+
+        aps.setFragmentationLength(FRAGMENTATION_LENGTH);
+
+        ZigBeeApsFrame apsFrame = new ZigBeeApsFrame();
+        apsFrame.setApsCounter(1);
+        apsFrame.setPayload(createData(0, FRAGMENTATION_LENGTH+1));
+
+        aps.send(0, apsFrame);
+
+        assertFalse(aps.receiveCommandState(0, ZigBeeTransportProgressState.TX_ACK));
+        assertFalse(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
+
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.TX_ACK));
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
+    }
+
+    @Test
+    public void shouldExpectThreeFramesReceivedForPayloadFittingInThreeFragments() {
+        ZigBeeTransportTransmit transport = Mockito.mock(ZigBeeTransportTransmit.class);
+        ApsDataEntity aps = new ApsDataEntity(transport);
+
+        aps.setFragmentationLength(FRAGMENTATION_LENGTH);
+
+        ZigBeeApsFrame apsFrame = new ZigBeeApsFrame();
+        apsFrame.setApsCounter(1);
+        apsFrame.setPayload(createData(0, FRAGMENTATION_LENGTH*2+1));
+
+        aps.send(0, apsFrame);
+
+        assertFalse(aps.receiveCommandState(0, ZigBeeTransportProgressState.TX_ACK));
+        assertFalse(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
+
+        assertFalse(aps.receiveCommandState(0, ZigBeeTransportProgressState.TX_ACK));
+        assertFalse(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
+
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.TX_ACK));
+        assertTrue(aps.receiveCommandState(0, ZigBeeTransportProgressState.RX_ACK));
+    }
+
     private int[] createData(int start, int length) {
         int[] data = new int[length];