[comfoair] improve data handling (#9401)

* improve data handling and suspend
* fix command position
* add ACK to subResponse
* fix data position
* fix reactivation, trim too long responses
* ensure correct setting of activation channel
* Remove @SuppressWarnings(null), which is not needed anymore
* Remove updateState on dispose

Signed-off-by: Hans Böhm <h.boehm@gmx.at>

bundles/org.openhab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/internal/ComfoAirCommandType.java

@@ -75,7 +75,7 @@ public enum ComfoAirCommandType {
      */
     ACTIVATE(ComfoAirBindingConstants.CG_CONTROL_PREFIX + ComfoAirBindingConstants.CHANNEL_ACTIVATE,
             DataTypeBoolean.getInstance(), new int[] { 0x03 }, Constants.REQUEST_SET_RS232, 1, 0,
-            Constants.EMPTY_TYPE_ARRAY, Constants.REPLY_SET_RS232, Constants.REPLY_SET_RS232, new int[] { 0 }, 0x03),
+            Constants.EMPTY_TYPE_ARRAY),
     MENU20_MODE(ComfoAirBindingConstants.CG_MENUP1_PREFIX + ComfoAirBindingConstants.CHANNEL_MENU20_MODE,
             DataTypeBoolean.getInstance(), Constants.REQUEST_GET_STATES, Constants.REPLY_GET_STATES, new int[] { 6 },
             0x01),
@@ -771,7 +771,7 @@ public enum ComfoAirCommandType {
     public static @Nullable ComfoAirCommand getReadCommand(String key) {
         ComfoAirCommandType commandType = getCommandTypeByKey(key);
 
-        if (commandType != null) {
+        if (commandType != null && commandType.readCommand > 0) {
             return new ComfoAirCommand(key);
         }
         return null;
@@ -891,7 +891,6 @@ public enum ComfoAirCommandType {
         return null;
     }
 
-    @SuppressWarnings("null")
     private static void uniteCommandsMap(Map<Integer, ComfoAirCommand> commands, ComfoAirCommandType commandType) {
         if (commandType.readReplyCommand != 0) {
             int replyCmd = commandType.readReplyCommand;

bundles/org.openhab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/internal/ComfoAirHandler.java

@@ -24,6 +24,7 @@ import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.comfoair.internal.datatypes.ComfoAirDataType;
 import org.openhab.core.io.transport.serial.SerialPortManager;
+import org.openhab.core.library.types.OnOffType;
 import org.openhab.core.thing.Channel;
 import org.openhab.core.thing.ChannelUID;
 import org.openhab.core.thing.Thing;
@@ -55,6 +56,8 @@ public class ComfoAirHandler extends BaseThingHandler {
     private @Nullable ComfoAirSerialConnector comfoAirConnector;
 
     public static final int BAUDRATE = 9600;
+    public static final String ACTIVATE_CHANNEL_ID = ComfoAirBindingConstants.CG_CONTROL_PREFIX
+            + ComfoAirBindingConstants.CHANNEL_ACTIVATE;
 
     public ComfoAirHandler(Thing thing, final SerialPortManager serialPortManager) {
         super(thing);
@@ -64,29 +67,36 @@ public class ComfoAirHandler extends BaseThingHandler {
     @Override
     public void handleCommand(ChannelUID channelUID, Command command) {
         String channelId = channelUID.getId();
+        if (comfoAirConnector != null) {
+            boolean isActive = !comfoAirConnector.getIsSuspended();
 
-        if (command instanceof RefreshType) {
+            if (isActive || channelId.equals(ACTIVATE_CHANNEL_ID)) {
-            Channel channel = this.thing.getChannel(channelUID);
+                if (command instanceof RefreshType) {
-            if (channel != null) {
+                    Channel channel = this.thing.getChannel(channelUID);
-                updateChannelState(channel);
+                    if (channel != null) {
-            }
+                        updateChannelState(channel);
-        } else {
+                    }
-            ComfoAirCommand changeCommand = ComfoAirCommandType.getChangeCommand(channelId, command);
+                } else {
+                    ComfoAirCommand changeCommand = ComfoAirCommandType.getChangeCommand(channelId, command);
 
-            if (changeCommand != null) {
+                    if (changeCommand != null) {
-                Set<String> keysToUpdate = getThing().getChannels().stream().map(Channel::getUID).filter(this::isLinked)
+                        Set<String> keysToUpdate = getThing().getChannels().stream().map(Channel::getUID)
-                        .map(ChannelUID::getId).collect(Collectors.toSet());
+                                .filter(this::isLinked).map(ChannelUID::getId).collect(Collectors.toSet());
-                sendCommand(changeCommand, channelId);
+                        sendCommand(changeCommand, channelId);
 
-                Collection<ComfoAirCommand> affectedReadCommands = ComfoAirCommandType
+                        Collection<ComfoAirCommand> affectedReadCommands = ComfoAirCommandType
-                        .getAffectedReadCommands(channelId, keysToUpdate);
+                                .getAffectedReadCommands(channelId, keysToUpdate);
 
-                if (affectedReadCommands.size() > 0) {
+                        if (affectedReadCommands.size() > 0) {
-                    Runnable updateThread = new AffectedItemsUpdateThread(affectedReadCommands);
+                            Runnable updateThread = new AffectedItemsUpdateThread(affectedReadCommands);
-                    affectedItemsPoller = scheduler.schedule(updateThread, 3, TimeUnit.SECONDS);
+                            affectedItemsPoller = scheduler.schedule(updateThread, 3, TimeUnit.SECONDS);
+                        }
+                    } else {
+                        logger.warn("Unhandled command type: {}, channelId: {}", command.toString(), channelId);
+                    }
                 }
             } else {
-                logger.warn("Unhandled command type: {}, channelId: {}", command.toString(), channelId);
+                logger.debug("Binding control is currently not active.");
             }
         }
     }
@@ -115,6 +125,8 @@ public class ComfoAirHandler extends BaseThingHandler {
                     updateStatus(ThingStatus.ONLINE);
                     pullDeviceProperties();
 
+                    updateState(ACTIVATE_CHANNEL_ID, OnOffType.ON);
+
                     List<Channel> channels = this.thing.getChannels();
 
                     poller = scheduler.scheduleWithFixedDelay(() -> {
@@ -159,14 +171,38 @@ public class ComfoAirHandler extends BaseThingHandler {
         if (!isLinked(channel.getUID())) {
             return;
         }
-        String commandKey = channel.getUID().getId();
 
-        ComfoAirCommand readCommand = ComfoAirCommandType.getReadCommand(commandKey);
+        if (comfoAirConnector != null) {
-        if (readCommand != null) {
+            boolean isActive = !comfoAirConnector.getIsSuspended();
-            scheduler.submit(() -> {
+
-                State state = sendCommand(readCommand, commandKey);
+            String commandKey = channel.getUID().getId();
+            if (commandKey.equals(ACTIVATE_CHANNEL_ID)) {
+                State state = OnOffType.from(isActive);
                 updateState(channel.getUID(), state);
-            });
+                return;
+            }
+
+            if (!isActive) {
+                logger.debug("Binding control is currently not active.");
+                return;
+            }
+
+            ComfoAirCommand readCommand = ComfoAirCommandType.getReadCommand(commandKey);
+            if (readCommand != null && readCommand.getRequestCmd() != null) {
+                scheduler.submit(() -> {
+                    State state = sendCommand(readCommand, commandKey);
+                    updateState(channel.getUID(), state);
+                });
+            }
+        }
+    }
+
+    @Override
+    public void channelLinked(ChannelUID channelUID) {
+        super.channelLinked(channelUID);
+        Channel channel = this.thing.getChannel(channelUID);
+        if (channel != null) {
+            updateChannelState(channel);
         }
     }
 

bundles/org.openhab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/internal/ComfoAirSerialConnector.java

@@ -47,8 +47,7 @@ public class ComfoAirSerialConnector {
     private static final byte[] END = { CTRL, (byte) 0x0f };
     private static final byte[] ACK = { CTRL, (byte) 0xf3 };
 
-    private static final int RS232_ENABLED_VALUE = 0x03;
+    private static final int MAX_RETRIES = 5;
-    private static final int RS232_DISABLED_VALUE = 0x00;
 
     private boolean isSuspended = true;
 
@@ -150,14 +149,17 @@ public class ComfoAirSerialConnector {
      */
     public synchronized int[] sendCommand(ComfoAirCommand command, int[] preRequestData) {
         Integer requestCmd = command.getRequestCmd();
+        Integer requestValue = command.getRequestValue();
         int retry = 0;
 
         if (requestCmd != null) {
             // Switch support for app or ccease control
-            if (requestCmd == ComfoAirCommandType.Constants.REQUEST_SET_RS232) {
+            if (requestCmd == ComfoAirCommandType.Constants.REQUEST_SET_RS232 && requestValue != null) {
-                isSuspended = !isSuspended;
+                if (requestValue == 1) {
-            } else if (requestCmd == ComfoAirCommandType.Constants.REPLY_SET_RS232) {
+                    isSuspended = false;
-                return new int[] { isSuspended ? RS232_DISABLED_VALUE : RS232_ENABLED_VALUE };
+                } else if (requestValue == 0) {
+                    isSuspended = true;
+                }
             } else if (isSuspended) {
                 logger.trace("Ignore cmd. Service is currently suspended");
                 return ComfoAirCommandType.Constants.EMPTY_INT_ARRAY;
@@ -226,13 +228,44 @@ public class ComfoAirSerialConnector {
                             return ComfoAirCommandType.Constants.EMPTY_INT_ARRAY;
                         }
 
+                        boolean isValidData = false;
+
                         // check for start and end sequence and if the response cmd
                         // matches
                         // 11 is the minimum response length with one data byte
                         if (responseBlock.length >= 11 && responseBlock[2] == START[0] && responseBlock[3] == START[1]
                                 && responseBlock[responseBlock.length - 2] == END[0]
-                                && responseBlock[responseBlock.length - 1] == END[1]
+                                && responseBlock[responseBlock.length - 1] == END[1]) {
-                                && (responseBlock[5] & 0xff) == command.getReplyCmd()) {
+                            if ((responseBlock[5] & 0xff) == command.getReplyCmd()) {
+                                isValidData = true;
+                            } else {
+                                int startIndex = -1;
+                                int endIndex = -1;
+
+                                for (int i = 4; i < (responseBlock.length - 11) && endIndex < 0; i++) {
+                                    if (responseBlock[i] == START[0] && responseBlock[i + 1] == START[1]
+                                            && ((responseBlock[i + 3] & 0xff) == command.getReplyCmd())) {
+                                        startIndex = i;
+                                        for (int j = startIndex; j < responseBlock.length; j++) {
+                                            if (responseBlock[j] == END[0] && responseBlock[j + 1] == END[1]) {
+                                                endIndex = j + 1;
+                                                break;
+                                            }
+                                        }
+                                    }
+                                }
+
+                                if (startIndex > -1 && endIndex > -1) {
+                                    byte[] subResponse = new byte[endIndex - startIndex + 3];
+                                    System.arraycopy(responseBlock, 0, subResponse, 0, 2);
+                                    System.arraycopy(responseBlock, startIndex, subResponse, 2, subResponse.length - 2);
+                                    responseBlock = subResponse;
+                                    isValidData = true;
+                                }
+                            }
+                        }
+
+                        if (isValidData) {
                             if (logger.isTraceEnabled()) {
                                 logger.trace("receive RAW DATA: {}", dumpData(responseBlock));
                             }
@@ -294,9 +327,9 @@ public class ComfoAirSerialConnector {
                     logger.warn("Transmission was interrupted: {}", e.getMessage());
                     throw new RuntimeException(e);
                 }
-            } while (retry++ < 5);
+            } while (retry++ < MAX_RETRIES);
 
-            if (retry == 5) {
+            if (retry >= MAX_RETRIES) {
                 logger.debug("Unable to send command. {} retries failed.", retry);
             }
         }
@@ -375,6 +408,10 @@ public class ComfoAirSerialConnector {
             }
             cleanedBuffer[pos] = processBuffer[i];
             pos++;
+            // Trim unrequested data in response
+            if (END[0] == processBuffer[i + 1] && END[1] == processBuffer[i + 2]) {
+                break;
+            }
         }
         return Arrays.copyOf(cleanedBuffer, pos);
     }
@@ -560,4 +597,8 @@ public class ComfoAirSerialConnector {
         }
         return 0;
     }
+
+    public boolean getIsSuspended() {
+        return isSuspended;
+    }
 }

bundles/org.openhab.binding.comfoair/src/main/java/org/openhab/binding/comfoair/internal/datatypes/DataTypeBoolean.java

@@ -49,7 +49,6 @@ public class DataTypeBoolean implements ComfoAirDataType {
         } else {
             int[] readReplyDataPos = commandType.getReadReplyDataPos();
             int readReplyDataBits = commandType.getReadReplyDataBits();
-            int readCommand = commandType.getReadCommand();
             boolean result;
 
             if (readReplyDataPos != null && readReplyDataPos[0] < data.length) {
@@ -59,8 +58,6 @@ public class DataTypeBoolean implements ComfoAirDataType {
                     result = (data[readReplyDataPos[0]] & readReplyDataBits) == readReplyDataBits;
                 }
                 return OnOffType.from(result);
-            } else if (readCommand == 0) {
-                return OnOffType.OFF; // handle write-only commands (resets)
             } else {
                 return UnDefType.NULL;
             }