[gce] File based items are not updated (#13545)

* [gce] Correcting non updated input channels

Signed-off-by: clinique <gael@lhopital.org>

bundles/org.openhab.binding.gce/src/main/java/org/openhab/binding/gce/internal/GCEBindingConstants.java

@@ -38,6 +38,4 @@ public class GCEBindingConstants {
     public static final String EVENT_SHORT_PRESS = "SHORT_PRESS";
     public static final String EVENT_LONG_PRESS = "LONG_PRESS";
     public static final String EVENT_PULSE = "PULSE";
-
-    // Adressable thing
 }

bundles/org.openhab.binding.gce/src/main/java/org/openhab/binding/gce/internal/handler/Ipx800DeviceConnector.java

@@ -110,7 +110,7 @@ public class Ipx800DeviceConnector extends Thread {
     /**
      * Stop the device thread
      */
-    public void destroyAndExit() {
+    public void dispose() {
         interrupt();
         disconnect();
     }
@@ -161,7 +161,7 @@ public class Ipx800DeviceConnector extends Thread {
         try {
             Thread.sleep(DEFAULT_RECONNECT_TIMEOUT_MS);
         } catch (InterruptedException e) {
-            destroyAndExit();
+            dispose();
         }
     }
 

bundles/org.openhab.binding.gce/src/main/java/org/openhab/binding/gce/internal/handler/Ipx800v3Handler.java

@@ -29,7 +29,6 @@ import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
-import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.gce.internal.action.Ipx800Actions;
 import org.openhab.binding.gce.internal.config.AnalogInputConfiguration;
 import org.openhab.binding.gce.internal.config.DigitalInputConfiguration;
@@ -56,7 +55,6 @@ import org.openhab.core.thing.ThingStatusDetail;
 import org.openhab.core.thing.binding.BaseThingHandler;
 import org.openhab.core.thing.binding.ThingHandlerService;
 import org.openhab.core.thing.binding.builder.ChannelBuilder;
-import org.openhab.core.thing.binding.builder.ThingBuilder;
 import org.openhab.core.thing.type.ChannelKind;
 import org.openhab.core.thing.type.ChannelTypeUID;
 import org.openhab.core.types.Command;
@@ -78,14 +76,11 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
 
     private final Logger logger = LoggerFactory.getLogger(Ipx800v3Handler.class);
 
-    private @NonNullByDefault({}) Ipx800Configuration configuration;
+    private Optional<Ipx800DeviceConnector> connector = Optional.empty();
-    private @NonNullByDefault({}) Ipx800DeviceConnector connector;
-    private @NonNullByDefault({}) StatusFileInterpreter statusFile;
-
     private Optional<M2MMessageParser> parser = Optional.empty();
     private Optional<ScheduledFuture<?>> refreshJob = Optional.empty();
 
-    private final Map<String, @Nullable PortData> portDatas = new HashMap<>();
+    private final Map<String, PortData> portDatas = new HashMap<>();
 
     private class LongPressEvaluator implements Runnable {
         private final ZonedDateTime referenceTime;
@@ -115,36 +110,37 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
 
     @Override
     public void initialize() {
-        configuration = getConfigAs(Ipx800Configuration.class);
-
         logger.debug("Initializing IPX800 handler for uid '{}'", getThing().getUID());
 
-        statusFile = new StatusFileInterpreter(configuration.hostname, this);
+        Ipx800Configuration config = getConfigAs(Ipx800Configuration.class);
+        StatusFileInterpreter statusFile = new StatusFileInterpreter(config.hostname, this);
 
         if (thing.getProperties().isEmpty()) {
-            discoverAttributes();
+            updateProperties(Map.of(Thing.PROPERTY_VENDOR, "GCE Electronics", Thing.PROPERTY_FIRMWARE_VERSION,
+                    statusFile.getElement(StatusEntry.VERSION), Thing.PROPERTY_MAC_ADDRESS,
+                    statusFile.getElement(StatusEntry.CONFIG_MAC)));
         }
 
         List<Channel> channels = new ArrayList<>(getThing().getChannels());
-        ThingBuilder thingBuilder = editThing();
         PortDefinition.asStream().forEach(portDefinition -> {
             int nbElements = statusFile.getMaxNumberofNodeType(portDefinition);
             for (int i = 0; i < nbElements; i++) {
-                createChannels(portDefinition, i, channels);
+                ChannelUID portChannelUID = createChannels(portDefinition, i, channels);
+                portDatas.put(portChannelUID.getId(), new PortData());
             }
         });
-        thingBuilder.withChannels(channels);
-        updateThing(thingBuilder.build());
 
-        connector = new Ipx800DeviceConnector(configuration.hostname, configuration.portNumber, getThing().getUID());
+        updateThing(editThing().withChannels(channels).build());
-        parser = Optional.of(new M2MMessageParser(connector, this));
+
+        connector = Optional.of(new Ipx800DeviceConnector(config.hostname, config.portNumber, getThing().getUID()));
+        parser = Optional.of(new M2MMessageParser(connector.get(), this));
 
         updateStatus(ThingStatus.UNKNOWN);
 
-        refreshJob = Optional.of(scheduler.scheduleWithFixedDelay(statusFile::read, 3000, configuration.pullInterval,
+        refreshJob = Optional.of(
-                TimeUnit.MILLISECONDS));
+                scheduler.scheduleWithFixedDelay(statusFile::read, 3000, config.pullInterval, TimeUnit.MILLISECONDS));
 
-        connector.start();
+        connector.get().start();
     }
 
     @Override
@@ -152,25 +148,15 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
         refreshJob.ifPresent(job -> job.cancel(true));
         refreshJob = Optional.empty();
 
-        if (connector != null) {
+        connector.ifPresent(Ipx800DeviceConnector::dispose);
-            connector.destroyAndExit();
+        connector = Optional.empty();
-        }
+
         parser = Optional.empty();
 
-        portDatas.values().stream().forEach(portData -> {
+        portDatas.values().stream().forEach(PortData::dispose);
-            if (portData != null) {
-                portData.dispose();
-            }
-        });
         super.dispose();
     }
 
-    protected void discoverAttributes() {
-        updateProperties(Map.of(Thing.PROPERTY_VENDOR, "GCE Electronics", Thing.PROPERTY_FIRMWARE_VERSION,
-                statusFile.getElement(StatusEntry.VERSION), Thing.PROPERTY_MAC_ADDRESS,
-                statusFile.getElement(StatusEntry.CONFIG_MAC)));
-    }
-
     private void addIfChannelAbsent(ChannelBuilder channelBuilder, List<Channel> channels) {
         Channel newChannel = channelBuilder.build();
         if (channels.stream().noneMatch(c -> c.getUID().equals(newChannel.getUID()))) {
@@ -178,7 +164,7 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
         }
     }
 
-    private void createChannels(PortDefinition portDefinition, int portIndex, List<Channel> channels) {
+    private ChannelUID createChannels(PortDefinition portDefinition, int portIndex, List<Channel> channels) {
         String ndx = Integer.toString(portIndex + 1);
         String advancedChannelTypeName = portDefinition.toString()
                 + (portDefinition.isAdvanced(portIndex) ? "Advanced" : "");
@@ -210,9 +196,12 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
                         .withLabel("Relay " + ndx).withType(channelType), channels);
                 break;
         }
+
         addIfChannelAbsent(ChannelBuilder.create(new ChannelUID(groupUID, ndx + "-duration"), "Number:Time")
                 .withType(new ChannelTypeUID(BINDING_ID, CHANNEL_LAST_STATE_DURATION))
                 .withLabel("Previous state duration " + ndx), channels);
+
+        return mainChannelUID;
     }
 
     @Override
@@ -258,7 +247,7 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
                     return;
                 }
                 logger.debug("About to update port '{}' with data '{}'", port, value);
-                State state = UnDefType.UNDEF;
+                State state = UnDefType.NULL;
                 switch (portDefinition) {
                     case COUNTER:
                         state = new DecimalType(value);
@@ -268,7 +257,7 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
                         break;
                     case ANALOG:
                         state = new DecimalType(value);
-                        updateState(channelId + PROPERTY_SEPARATOR + CHANNEL_VOLTAGE,
+                        updateIfLinked(channelId + PROPERTY_SEPARATOR + CHANNEL_VOLTAGE,
                                 new QuantityType<>(value * ANALOG_SAMPLING, Units.VOLT));
                         break;
                     case CONTACT:
@@ -303,9 +292,9 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
                         break;
                 }
 
-                updateState(channelId, state);
+                updateIfLinked(channelId, state);
                 if (!portData.isInitializing()) {
-                    updateState(channelId + PROPERTY_SEPARATOR + CHANNEL_LAST_STATE_DURATION,
+                    updateIfLinked(channelId + PROPERTY_SEPARATOR + CHANNEL_LAST_STATE_DURATION,
                             new QuantityType<>(sinceLastChange / 1000, Units.SECOND));
                 }
                 portData.setData(value, now);
@@ -317,6 +306,12 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
         }
     }
 
+    private void updateIfLinked(String channelId, State state) {
+        if (isLinked(channelId)) {
+            updateState(channelId, state);
+        }
+    }
+
     protected void triggerPushButtonChannel(Channel channel, String event) {
         logger.debug("Triggering event '{}' on channel '{}'", event, channel.getUID());
         triggerChannel(channel.getUID().getId() + PROPERTY_SEPARATOR + TRIGGER_CONTACT, event);
@@ -342,32 +337,10 @@ public class Ipx800v3Handler extends BaseThingHandler implements Ipx800EventList
         logger.debug("Can not handle command '{}' on channel '{}'", command, channelUID);
     }
 
-    @Override
-    public void channelLinked(ChannelUID channelUID) {
-        logger.debug("channelLinked: {}", channelUID);
-        final String channelId = channelUID.getId();
-        if (isValidPortId(channelUID)) {
-            Channel channel = thing.getChannel(channelUID);
-            if (channel != null) {
-                PortData data = new PortData();
-                portDatas.put(channelId, data);
-            }
-        }
-    }
-
     private boolean isValidPortId(ChannelUID channelUID) {
         return channelUID.getIdWithoutGroup().chars().allMatch(Character::isDigit);
     }
 
-    @Override
-    public void channelUnlinked(ChannelUID channelUID) {
-        super.channelUnlinked(channelUID);
-        PortData portData = portDatas.remove(channelUID.getId());
-        if (portData != null) {
-            portData.dispose();
-        }
-    }
-
     public void resetCounter(int counter) {
         parser.ifPresent(p -> p.resetCounter(counter));
     }

bundles/org.openhab.binding.gce/src/main/java/org/openhab/binding/gce/internal/model/M2MMessageParser.java

@@ -15,7 +15,6 @@ package org.openhab.binding.gce.internal.model;
 import java.util.regex.Pattern;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
-import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.gce.internal.handler.Ipx800DeviceConnector;
 import org.openhab.binding.gce.internal.handler.Ipx800EventListener;
 import org.slf4j.Logger;
@@ -35,11 +34,11 @@ public class M2MMessageParser {
 
     private final Logger logger = LoggerFactory.getLogger(M2MMessageParser.class);
     private final Ipx800DeviceConnector connector;
-    private final @Nullable Ipx800EventListener listener;
+    private final Ipx800EventListener listener;
 
     private String expectedResponse = "";
 
-    public M2MMessageParser(Ipx800DeviceConnector connector, @Nullable Ipx800EventListener listener) {
+    public M2MMessageParser(Ipx800DeviceConnector connector, Ipx800EventListener listener) {
         this.connector = connector;
         this.listener = listener;
         connector.setParser(this);
@@ -87,10 +86,8 @@ public class M2MMessageParser {
 
     private void setStatus(String port, double value) {
         logger.debug("Received {} : {}", port, value);
-        if (listener != null) {
         listener.dataReceived(port, value);
     }
-    }
 
     public void setExpectedResponse(String expectedResponse) {
         if (expectedResponse.endsWith("s")) { // GetInputs or GetOutputs
@@ -125,10 +122,8 @@ public class M2MMessageParser {
 
     public void errorOccurred(Exception e) {
         logger.warn("Error received from connector : {}", e.getMessage());
-        if (listener != null) {
         listener.errorOccurred(e);
     }
-    }
 
     public void resetPLC() {
         connector.send("Reset");

bundles/org.openhab.binding.gce/src/main/java/org/openhab/binding/gce/internal/model/StatusFileInterpreter.java

@@ -17,6 +17,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 
@@ -25,7 +26,6 @@ import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
-import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.binding.gce.internal.handler.Ipx800EventListener;
 import org.openhab.core.io.net.http.HttpUtil;
 import org.slf4j.Logger;
@@ -44,47 +44,53 @@ import org.xml.sax.SAXException;
 @NonNullByDefault
 public class StatusFileInterpreter {
     private static final String URL_TEMPLATE = "http://%s/globalstatus.xml";
+
     private final Logger logger = LoggerFactory.getLogger(StatusFileInterpreter.class);
-    private final String hostname;
+    private final DocumentBuilder builder;
-    private @Nullable Document doc;
+    private final String url;
     private final Ipx800EventListener listener;
 
+    private Optional<Document> doc = Optional.empty();
+
     public static enum StatusEntry {
         VERSION,
         CONFIG_MAC;
     }
 
     public StatusFileInterpreter(String hostname, Ipx800EventListener listener) {
-        this.hostname = hostname;
+        this.url = String.format(URL_TEMPLATE, hostname);
         this.listener = listener;
-    }
-
-    public void read() {
-        try {
         DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
         // see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+        try {
             factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
             factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
             factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
             factory.setXIncludeAware(false);
             factory.setExpandEntityReferences(false);
-            DocumentBuilder builder = factory.newDocumentBuilder();
+            builder = factory.newDocumentBuilder();
-            String statusPage = HttpUtil.executeUrl("GET", String.format(URL_TEMPLATE, hostname), 5000);
+        } catch (ParserConfigurationException e) {
+            logger.warn("Error initializing StatusFileInterpreter :{}", e.getMessage());
+            throw new IllegalArgumentException(e);
+        }
+    }
+
+    public void read() {
+        try {
+            String statusPage = HttpUtil.executeUrl("GET", url, 5000);
             InputStream inputStream = new ByteArrayInputStream(statusPage.getBytes());
             Document document = builder.parse(inputStream);
             document.getDocumentElement().normalize();
-            doc = document;
-            pushDatas();
             inputStream.close();
-        } catch (IOException | SAXException | ParserConfigurationException e) {
+            this.doc = Optional.of(document);
+            pushDatas();
+        } catch (IOException | SAXException e) {
             logger.warn("Unable to read IPX800 status page : {}", e.getMessage());
-            doc = null;
         }
     }
 
     private void pushDatas() {
-        Element root = getRoot();
+        getRoot().ifPresent(root -> {
-        if (root != null) {
             PortDefinition.asStream().forEach(portDefinition -> {
                 List<Node> xmlNodes = getMatchingNodes(root.getChildNodes(), portDefinition.getNodeName());
                 xmlNodes.forEach(xmlNode -> {
@@ -94,40 +100,29 @@ public class StatusFileInterpreter {
                     listener.dataReceived(String.format("%s%d", portDefinition.getPortName(), portNum), value);
                 });
             });
-        }
+        });
     }
 
     public String getElement(StatusEntry entry) {
-        Element root = getRoot();
+        return getRoot().map(root -> root.getElementsByTagName(entry.name().toLowerCase()).item(0).getTextContent())
-        if (root != null) {
+                .orElse("");
-            return root.getElementsByTagName(entry.name().toLowerCase()).item(0).getTextContent();
-        } else {
-            return "";
-        }
     }
 
     private List<Node> getMatchingNodes(NodeList nodeList, String criteria) {
         return IntStream.range(0, nodeList.getLength()).boxed().map(nodeList::item)
-                .filter(node -> node.getNodeName().startsWith(criteria))
+                .filter(node -> node.getNodeName().startsWith(criteria)).sorted(Comparator.comparing(Node::getNodeName))
-                .sorted(Comparator.comparing(o -> o.getNodeName())).collect(Collectors.toList());
+                .collect(Collectors.toList());
     }
 
     public int getMaxNumberofNodeType(PortDefinition portDefinition) {
-        Element root = getRoot();
+        return getRoot().map(root -> getMatchingNodes(root.getChildNodes(), portDefinition.getNodeName()).size())
-        if (root != null) {
+                .orElse(0);
-            List<Node> filteredNodes = getMatchingNodes(root.getChildNodes(), portDefinition.getNodeName());
-            return filteredNodes.size();
-        }
-        return 0;
     }
 
-    private @Nullable Element getRoot() {
+    private Optional<Element> getRoot() {
-        if (doc == null) {
+        if (doc.isEmpty()) {
             read();
         }
-        if (doc != null) {
+        return Optional.ofNullable(doc.map(Document::getDocumentElement).orElse(null));
-            return doc.getDocumentElement();
-        }
-        return null;
     }
 }