[ipcamera] Fix ONVIF fails to reconnect (#13396)

* Fix reconnecting issues
* Cleanup logging.

Signed-off-by: Matthew Skinner <matt@pcmus.com>

bundles/org.openhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/handler/IpCameraHandler.java

@@ -492,10 +492,17 @@ public class IpCameraHandler extends BaseThingHandler {
     }
 
     private void checkCameraConnection() {
-        if (snapshotUri.isEmpty() || snapshotPolling) {
+        if (snapshotPolling) {// Currently polling a real URL for snapshots, so camera must be online.
-            // Already polling or camera has RTSP only and no HTTP server
             return;
+        } else if (ffmpegSnapshotGeneration) {// Use RTSP stream creating snapshots to know camera is online.
+            Ffmpeg localSnapshot = ffmpegSnapshot;
+            if (localSnapshot != null && !localSnapshot.getIsAlive()) {
+                cameraCommunicationError("FFmpeg Snapshots Stopped: Check your camera can be reached.");
+                return;
+            }
+            return;// ffmpeg snapshot stream is still alive
         }
+        // Open a HTTP connection without sending any requests as we do not need a snapshot.
         Bootstrap localBootstrap = mainBootstrap;
         if (localBootstrap != null) {
             ChannelFuture chFuture = localBootstrap
@@ -1362,6 +1369,7 @@ public class IpCameraHandler extends BaseThingHandler {
             if (snapshotUri.isEmpty() || "ffmpeg".equals(snapshotUri)) {
                 snapshotIsFfmpeg();
             } else {
+                ffmpegSnapshotGeneration = false;
                 updateSnapshot();
             }
             return;
@@ -1374,6 +1382,7 @@ public class IpCameraHandler extends BaseThingHandler {
         if ("ffmpeg".equals(snapshotUri)) {
             snapshotIsFfmpeg();
         } else if (!snapshotUri.isEmpty()) {
+            ffmpegSnapshotGeneration = false;
             updateSnapshot();
         } else if (!rtspUri.isEmpty()) {
             snapshotIsFfmpeg();
@@ -1497,16 +1506,9 @@ public class IpCameraHandler extends BaseThingHandler {
         // what needs to be done every poll//
         switch (thing.getThingTypeUID().getId()) {
             case GENERIC_THING:
-                if (!snapshotUri.isEmpty() && !snapshotPolling) {
+                if (!snapshotPolling) {
                     checkCameraConnection();
                 }
-                // RTSP stream has stopped and we need it for snapshots
-                if (ffmpegSnapshotGeneration) {
-                    Ffmpeg localSnapshot = ffmpegSnapshot;
-                    if (localSnapshot != null && !localSnapshot.getIsAlive()) {
-                        localSnapshot.startConverting();
-                    }
-                }
                 break;
             case ONVIF_THING:
                 if (!snapshotPolling) {

bundles/org.openhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/onvif/OnvifConnection.java

@@ -29,6 +29,7 @@ import java.util.TimeZone;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -114,6 +115,7 @@ public class OnvifConnection {
     private ScheduledExecutorService threadPool = Executors.newScheduledThreadPool(2);
     private @Nullable Bootstrap bootstrap;
     private EventLoopGroup mainEventLoopGroup = new NioEventLoopGroup(2);
+    private ReentrantLock connecting = new ReentrantLock();
     private String ipAddress = "";
     private String user = "";
     private String password = "";
@@ -308,7 +310,12 @@ public class OnvifConnection {
         } else if (message.contains("RenewResponse")) {
             sendOnvifRequest(requestBuilder(RequestType.PullMessages, subscriptionXAddr));
         } else if (message.contains("GetSystemDateAndTimeResponse")) {// 1st to be sent.
-            isConnected = true;
+            connecting.lock();
+            try {
+                isConnected = true;
+            } finally {
+                connecting.unlock();
+            }
             sendOnvifRequest(requestBuilder(RequestType.GetCapabilities, deviceXAddr));
             parseDateAndTime(message);
             logger.debug("Openhabs UTC dateTime is:{}", getUTCdateTime());
@@ -355,7 +362,8 @@ public class OnvifConnection {
         } else if (message.contains("GetSnapshotUriResponse")) {
             snapshotUri = removeIPfromUrl(Helper.fetchXML(message, ":MediaUri", ":Uri"));
             logger.debug("GetSnapshotUri:{}", snapshotUri);
-            if (ipCameraHandler.snapshotUri.isEmpty()) {
+            if (ipCameraHandler.snapshotUri.isEmpty()
+                    && !"ffmpeg".equals(ipCameraHandler.cameraConfig.getSnapshotUrl())) {
                 ipCameraHandler.snapshotUri = snapshotUri;
             }
         } else if (message.contains("GetStreamUriResponse")) {
@@ -512,6 +520,7 @@ public class OnvifConnection {
     @SuppressWarnings("null")
     public void sendOnvifRequest(HttpRequest request) {
         if (bootstrap == null) {
+            mainEventLoopGroup = new NioEventLoopGroup(2);
             bootstrap = new Bootstrap();
             bootstrap.group(mainEventLoopGroup);
             bootstrap.channel(NioSocketChannel.class);
@@ -530,24 +539,28 @@ public class OnvifConnection {
                 }
             });
         }
-        bootstrap.connect(new InetSocketAddress(ipAddress, onvifPort)).addListener(new ChannelFutureListener() {
+        if (!mainEventLoopGroup.isShuttingDown()) {
+            bootstrap.connect(new InetSocketAddress(ipAddress, onvifPort)).addListener(new ChannelFutureListener() {
 
-            @Override
+                @Override
-            public void operationComplete(@Nullable ChannelFuture future) {
+                public void operationComplete(@Nullable ChannelFuture future) {
-                if (future == null) {
+                    if (future == null) {
-                    return;
+                        return;
-                }
+                    }
-                if (future.isDone() && future.isSuccess()) {
+                    if (future.isSuccess()) {
-                    Channel ch = future.channel();
+                        Channel ch = future.channel();
-                    ch.writeAndFlush(request);
+                        ch.writeAndFlush(request);
-                } else { // an error occured
+                    } else { // an error occured
-                    logger.debug("Camera is not reachable on ONVIF port:{} or the port may be wrong.", onvifPort);
+                        logger.debug("Camera is not reachable on ONVIF port:{} or the port may be wrong.", onvifPort);
-                    if (isConnected) {
+                        if (isConnected) {
-                        disconnect();
+                            disconnect();
+                        }
                     }
                 }
-            }
+            });
-        });
+        } else {
+            logger.debug("ONVIF message not sent as connection is shutting down");
+        }
     }
 
     OnvifConnection getHandle() {
@@ -833,42 +846,59 @@ public class OnvifConnection {
     }
 
     public void connect(boolean useEvents) {
-        if (!isConnected) {
+        connecting.lock();
-            sendOnvifRequest(requestBuilder(RequestType.GetSystemDateAndTime, deviceXAddr));
+        try {
-            usingEvents = useEvents;
+            if (!isConnected) {
+                logger.debug("Connecting {} to ONVIF", ipAddress);
+                threadPool = Executors.newScheduledThreadPool(2);
+                sendOnvifRequest(requestBuilder(RequestType.GetSystemDateAndTime, deviceXAddr));
+                usingEvents = useEvents;
+            }
+        } finally {
+            connecting.unlock();
         }
     }
 
     public boolean isConnected() {
-        return isConnected;
+        connecting.lock();
+        try {
+            return isConnected;
+        } finally {
+            connecting.unlock();
+        }
     }
 
     private void cleanup() {
-        mainEventLoopGroup.shutdownGracefully();
+        if (!isConnected && !mainEventLoopGroup.isShuttingDown()) {
-        isConnected = false;
-        if (!mainEventLoopGroup.isShutdown()) {
             try {
+                mainEventLoopGroup.shutdownGracefully();
                 mainEventLoopGroup.awaitTermination(3, TimeUnit.SECONDS);
             } catch (InterruptedException e) {
                 logger.warn("ONVIF was not cleanly shutdown, due to being interrupted");
             } finally {
                 logger.debug("Eventloop is shutdown:{}", mainEventLoopGroup.isShutdown());
                 bootstrap = null;
+                threadPool.shutdown();
             }
         }
-        threadPool.shutdown();
     }
 
     public void disconnect() {
-        if (bootstrap != null) {
+        connecting.lock();// Lock out multiple disconnect()/connect() attempts as we try to send Unsubscribe.
-            if (usingEvents && isConnected && !mainEventLoopGroup.isShuttingDown()) {
+        try {
-                // Some cameras may continue to send events even when they can't reach a server.
+            isConnected = false;// isConnected is not thread safe, connecting.lock() used as fix.
-                sendOnvifRequest(requestBuilder(RequestType.Unsubscribe, subscriptionXAddr));
+            if (bootstrap != null) {
+                if (usingEvents && !mainEventLoopGroup.isShuttingDown()) {
+                    // Some cameras may continue to send events even when they can't reach a server.
+                    sendOnvifRequest(requestBuilder(RequestType.Unsubscribe, subscriptionXAddr));
+                }
+                // give time for the Unsubscribe request to be sent, shutdownGracefully will try to send it first.
+                threadPool.schedule(this::cleanup, 50, TimeUnit.MILLISECONDS);
+            } else {
+                cleanup();
             }
-            // give time for the Unsubscribe request to be sent to the camera.
+        } finally {
-            threadPool.schedule(this::cleanup, 50, TimeUnit.MILLISECONDS);
+            connecting.unlock();
-        } else {
-            cleanup();
         }
     }
 }

bundles/org.openhab.binding.ipcamera/src/main/resources/OH-INF/thing/thing-types.xml

@@ -1913,7 +1913,7 @@
 				<advanced>true</advanced>
 			</parameter>
 
-			<parameter name="nvrChannel" type="integer" required="true" min="1" max="9" groupName="Settings">
+			<parameter name="nvrChannel" type="integer" required="true" min="1" max="99" groupName="Settings">
 				<label>NVR Input Channel</label>
 				<description>Set this to 1 if it is a stand alone camera, or to the input channel number if you use a compatible
 					NVR.