Merge pull request #59 from matchews/codex/implement-single-ack-packet-in-udpmessage

Use single ACK packet for OmniLogic UDP

Author: matchews

bundles/org.openhab.binding.haywardomnilogiclocal/src/main/java/org/openhab/binding/haywardomnilogiclocal/internal/net/AckHandler.java

@@ -19,7 +19,6 @@
 import java.net.InetAddress;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
-import org.openhab.binding.haywardomnilogiclocal.internal.HaywardMessageType;
 
 /**
  * Handles sending ACK responses back to the OmniLogic controller.
@@ -35,12 +34,6 @@ public AckHandler(InetAddress address, int port) {
     }
 
     public void sendAck(DatagramSocket socket, int messageId) throws IOException {
-        byte[] out = UdpMessage.encodeRequest(HaywardMessageType.ACK, "ACK", messageId);
-        DatagramPacket packet = new DatagramPacket(out, out.length, address, port);
-        socket.send(packet);
-
-        byte[] ackBytes = UdpMessage.encodeRequest(HaywardMessageType.ACK, "", messageId);
-        DatagramPacket ackPacket = new DatagramPacket(ackBytes, ackBytes.length, address, port);
-        socket.send(ackPacket);
+        socket.send(new DatagramPacket(UdpMessage.buildAck(messageId), UdpMessage.buildAck(messageId).length, address, port));
     }
 }

bundles/org.openhab.binding.haywardomnilogiclocal/src/main/java/org/openhab/binding/haywardomnilogiclocal/internal/net/UdpMessage.java

@@ -77,6 +77,13 @@ public static byte[] encodeRequest(HaywardMessageType msgType, String xml) throw
         return encodeRequest(msgType, xml, null);
     }
 
+    /**
+     * Builds a single ACK packet for the given message id.
+     */
+    public static byte[] buildAck(int messageId) throws UnsupportedEncodingException {
+        return encodeRequest(HaywardMessageType.ACK, "ACK", Integer.valueOf(messageId));
+    }
+
     /**
      * Decodes a response message received from the controller.
      */

bundles/org.openhab.binding.haywardomnilogiclocal/src/test/java/org/openhab/binding/haywardomnilogiclocal/internal/net/UdpClientTest.java

@@ -45,6 +45,7 @@ public void sendShouldIgnoreAckAndProcessSubsequentData() throws Exception {
         final int port = server.getLocalPort();
         final AtomicBoolean ackBeforeLead = new AtomicBoolean(false);
         final AtomicBoolean ackAfterLead = new AtomicBoolean(false);
+        final AtomicBoolean extraAckAfterLead = new AtomicBoolean(false);
         final String responseXml = "<Response>OK</Response>";
 
         Thread serverThread = new Thread(() -> {
@@ -86,12 +87,13 @@ public void sendShouldIgnoreAckAndProcessSubsequentData() throws Exception {
                     if (msgType == HaywardMessageType.ACK.getMsgInt()) {
                         ackAfterLead.set(true);
                     }
-                    // consume possible second ACK
+                    // ensure no extra ACK is sent
                     server.setSoTimeout(100);
                     try {
                         server.receive(new DatagramPacket(new byte[4096], 4096));
+                        extraAckAfterLead.set(true);
                     } catch (SocketTimeoutException ex) {
-                        // ignore
+                        // expected - no extra ACK
                     }
                 } catch (SocketTimeoutException e) {
                     // no ACK received
@@ -116,6 +118,7 @@ public void sendShouldIgnoreAckAndProcessSubsequentData() throws Exception {
         assertEquals(responseXml, response.getXml());
         assertFalse(ackBeforeLead.get());
         assertTrue(ackAfterLead.get());
+        assertFalse(extraAckAfterLead.get());
     }
 
     @Test
@@ -154,32 +157,26 @@ public void sendShouldHandleUncompressedMultiBlockResponse() throws Exception {
                 int clientPort = requestPacket.getPort();
                 byte[] lead = createLeadPacket(msgId, 2, false);
                 server.send(new DatagramPacket(lead, lead.length, clientAddress, clientPort));
-                // consume ACKs for lead
-                for (int i = 0; i < 2; i++) {
-                    server.setSoTimeout(2000);
-                    server.receive(new DatagramPacket(new byte[4096], 4096));
-                }
+                // consume ACK for lead
+                server.setSoTimeout(2000);
+                server.receive(new DatagramPacket(new byte[4096], 4096));
                 byte[] block1 = createBlockPacket(msgId, part1, false);
                 server.send(new DatagramPacket(block1, block1.length, clientAddress, clientPort));
-                for (int i = 0; i < 2; i++) {
-                    DatagramPacket ack = new DatagramPacket(new byte[4096], 4096);
-                    server.setSoTimeout(2000);
-                    server.receive(ack);
-                    if (ByteBuffer.wrap(ack.getData(), 0, ack.getLength()).getInt(16) ==
-                            HaywardMessageType.ACK.getMsgInt()) {
-                        block1Acks.incrementAndGet();
-                    }
+                DatagramPacket ack1 = new DatagramPacket(new byte[4096], 4096);
+                server.setSoTimeout(2000);
+                server.receive(ack1);
+                if (ByteBuffer.wrap(ack1.getData(), 0, ack1.getLength()).getInt(16) ==
+                        HaywardMessageType.ACK.getMsgInt()) {
+                    block1Acks.incrementAndGet();
                 }
                 byte[] block2 = createBlockPacket(msgId, part2, false);
                 server.send(new DatagramPacket(block2, block2.length, clientAddress, clientPort));
-                for (int i = 0; i < 2; i++) {
-                    DatagramPacket ack = new DatagramPacket(new byte[4096], 4096);
-                    server.setSoTimeout(2000);
-                    server.receive(ack);
-                    if (ByteBuffer.wrap(ack.getData(), 0, ack.getLength()).getInt(16) ==
-                            HaywardMessageType.ACK.getMsgInt()) {
-                        block2Acks.incrementAndGet();
-                    }
+                DatagramPacket ack2 = new DatagramPacket(new byte[4096], 4096);
+                server.setSoTimeout(2000);
+                server.receive(ack2);
+                if (ByteBuffer.wrap(ack2.getData(), 0, ack2.getLength()).getInt(16) ==
+                        HaywardMessageType.ACK.getMsgInt()) {
+                    block2Acks.incrementAndGet();
                 }
             } catch (Exception e) {
                 // ignore for test
@@ -194,8 +191,8 @@ public void sendShouldHandleUncompressedMultiBlockResponse() throws Exception {
         server.close();
 
         assertEquals(responseXml, response.getXml());
-        assertEquals(2, block1Acks.get());
-        assertEquals(2, block2Acks.get());
+        assertEquals(1, block1Acks.get());
+        assertEquals(1, block2Acks.get());
     }
 
     @Test