[ecovacs] Address more review comments

- Explicitly roster reloading on connection (before, roster loading was pulled in implicitly via XMPP client binding) - Use ping code from smack-extensions
openhab · Feb 8, 2022 · a53c34d · a53c34d
1 parent 8a0afd2
commit a53c34d
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 21 deletions.
diff --git a/bundles/org.openhab.binding.ecovacs/pom.xml b/bundles/org.openhab.binding.ecovacs/pom.xml
@@ -24,6 +24,18 @@
       <version>${smack.version}</version>
       <scope>provided</scope>
     </dependency>
+    <dependency>
+      <groupId>org.igniterealtime.smack</groupId>
+      <artifactId>smack-im</artifactId>
+      <version>${smack.version}</version>
+      <scope>provided</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.igniterealtime.smack</groupId>
+      <artifactId>smack-extensions</artifactId>
+      <version>${smack.version}</version>
+      <scope>provided</scope>
+    </dependency>
     <dependency>
       <groupId>org.igniterealtime.smack</groupId>
       <artifactId>smack-java7</artifactId>

diff --git a/bundles/org.openhab.binding.ecovacs/src/main/feature/feature.xml b/bundles/org.openhab.binding.ecovacs/src/main/feature/feature.xml
@@ -11,6 +11,8 @@
 		<bundle dependency="true">mvn:org.jxmpp/jxmpp-util-cache/0.6.3</bundle>
 		<bundle dependency="true">mvn:org.minidns/minidns-core/0.3.3</bundle>
 		<bundle dependency="true">mvn:org.igniterealtime.smack/smack-core/4.3.3</bundle>
+		<bundle dependency="true">mvn:org.igniterealtime.smack/smack-im/4.3.3</bundle>
+		<bundle dependency="true">mvn:org.igniterealtime.smack/smack-extensions/4.3.3</bundle>
 		<bundle dependency="true">mvn:org.igniterealtime.smack/smack-sasl-javax/4.3.3</bundle>
 		<bundle dependency="true">mvn:org.apache.servicemix.bundles/org.apache.servicemix.bundles.xpp3/1.1.4c_7</bundle>
 		<bundle start-level="80">mvn:org.igniterealtime.smack/smack-resolver-javax/4.3.3</bundle>

diff --git a/...covacs/src/main/java/org/openhab/binding/ecovacs/internal/api/impl/EcovacsXmppDevice.java b/...covacs/src/main/java/org/openhab/binding/ecovacs/internal/api/impl/EcovacsXmppDevice.java
@@ -31,13 +31,14 @@
 import org.jivesoftware.smack.packet.ErrorIQ;
 import org.jivesoftware.smack.packet.IQ;
 import org.jivesoftware.smack.packet.IQ.Type;
-import org.jivesoftware.smack.packet.SimpleIQ;
 import org.jivesoftware.smack.packet.StanzaError;
 import org.jivesoftware.smack.provider.IQProvider;
 import org.jivesoftware.smack.provider.ProviderManager;
+import org.jivesoftware.smack.roster.Roster;
 import org.jivesoftware.smack.tcp.XMPPTCPConnection;
 import org.jivesoftware.smack.tcp.XMPPTCPConnectionConfiguration;
 import org.jivesoftware.smack.util.PacketParserUtils;
+import org.jivesoftware.smackx.ping.PingManager;
 import org.jxmpp.jid.Jid;
 import org.jxmpp.jid.impl.JidCreate;
 import org.openhab.binding.ecovacs.internal.api.EcovacsApiConfiguration;
@@ -200,7 +201,10 @@ public void connectionClosedOnError(@Nullable Exception e) {
             });
 
             messageHandler = new IncomingMessageHandler(listener);
-            pingHandler = new PingHandler(conn, scheduler, listener, ownAddress, targetAddress);
+            pingHandler = new PingHandler(conn, scheduler, listener, targetAddress);
+
+            Roster roster = Roster.getInstanceFor(conn);
+            roster.setRosterLoadedAtLogin(false);
 
             conn.registerIQRequestHandler(messageHandler);
             conn.connect();
@@ -244,21 +248,18 @@ private class PingHandler {
         private static final long INTERVAL = 30; // seconds
         private static final int MAX_FAILURES = 4;
 
-        private final XMPPTCPConnection connection;
+        private final PingManager pingManager;
         private final ScheduledExecutorService scheduler;
         private final EventListener listener;
-        private final Jid fromAddress;
         private final Jid toAddress;
         private @Nullable Future<?> nextPing;
         private boolean started = false;
         private int failedPings = 0;
 
-        PingHandler(XMPPTCPConnection connection, ScheduledExecutorService scheduler, EventListener listener, Jid from,
-                Jid to) {
-            this.connection = connection;
+        PingHandler(XMPPTCPConnection connection, ScheduledExecutorService scheduler, EventListener listener, Jid to) {
+            this.pingManager = PingManager.getInstanceFor(connection);
             this.scheduler = scheduler;
             this.listener = listener;
-            this.fromAddress = from;
             this.toAddress = to;
         }
 
@@ -283,12 +284,13 @@ private void sendPing() {
             }
 
             try {
-                connection.sendIqRequestAndWaitForResponse(new PingIQ(fromAddress, toAddress));
-                logger.trace("{}: Pinged device", getSerialNumber());
-                failedPings = 0;
+                if (pingManager.ping(this.toAddress)) {
+                    logger.trace("{}: Pinged device", getSerialNumber());
+                    failedPings = 0;
+                }
             } catch (InterruptedException e) {
                 // only happens when we're stopped
-            } catch (XMPPException | SmackException e) {
+            } catch (SmackException e) {
                 ++failedPings;
                 logger.debug("{}: Ping failed (#{}): {})", getSerialNumber(), failedPings, e.getMessage());
                 if (failedPings >= MAX_FAILURES) {
@@ -421,15 +423,6 @@ private String createRequestId() {
         }
     }
 
-    private static class PingIQ extends SimpleIQ {
-        public PingIQ(Jid from, Jid to) {
-            super("ping", "urn:xmpp:ping");
-            setType(Type.get);
-            setFrom(from);
-            setTo(to);
-        }
-    }
-
     private static class CommandIQProvider extends IQProvider<@Nullable DeviceCommandIQ> {
         @Override
         public @Nullable DeviceCommandIQ parse(@Nullable XmlPullParser parser, int initialDepth) throws Exception {