Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ipcamera] Fix connection checks with ONVIF cameras with no snapshots #15119

Merged
merged 5 commits into from
Sep 27, 2023
Merged

[ipcamera] Fix connection checks with ONVIF cameras with no snapshots #15119

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37dac82
Added connection check via IdleStateHandler events for sent onvif req…
tb4jc Jun 20, 2023
cf4fdde
Updates according to comments from Skinah.
tb4jc Jun 26, 2023
26c6386
Merge branch 'main' into 14910-fix-stop-poll-image-connection-handling
Skinah Sep 10, 2023
300ce48
Added missing braces.
tb4jc Sep 11, 2023
bdbbfc4
Fixed call of sendOnvifRequest (missed to remove one call of requestB…
tb4jc Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...ab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/CameraConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public class CameraConfig {
public boolean useToken = true;
private int onvifMediaProfile;
private int pollTime;
private boolean disableSnapshotAtStartup;
Skinah marked this conversation as resolved.
Show resolved Hide resolved
private String ffmpegInput = "";
private String snapshotUrl = "";
private String mjpegUrl = "";
Expand Down Expand Up @@ -169,4 +170,8 @@ public int getGifPreroll() {
public int getPort() {
return port;
}

public boolean getDisableSnapshotAtStartup() {
return disableSnapshotAtStartup;
}
}
51 changes: 42 additions & 9 deletions ...ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/handler/IpCameraHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,18 @@ private void checkCameraConnection() {
}
return;// ffmpeg snapshot stream is still alive
}

// if ONVIF cam also use connection state which is updated by regular messages to camera
if (thing.getThingTypeUID().getId().equals(ONVIF_THING) && snapshotUri.isEmpty()) {
logger.trace("is ONVIF with empty snapshot URI");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will create log entries way too often, if you want to log this is occuring, then consider placing a log comment on the first time the camera comes ONLINE and not every time the binding checks that the camera is still connected.

if (onvifCamera.isConnected()) {
logger.trace("cam is connected");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above, we do not need a log entry every time 'nothing is wrong'. There is a cost to performance if everything in your smart home started doing this, even if the logging level is not TRACE, there is a cost to check what the log level is.

return;
} else {
logger.trace("cam not connected");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not like logging PLUS changing the THING STATUS as this is doubling up the info. Please remove as the cameraCommunicationError() line changes the thing status and this is the preferred way to let users know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, makes sense.

}

// Open a HTTP connection without sending any requests as we do not need a snapshot.
Bootstrap localBootstrap = mainBootstrap;
if (localBootstrap != null) {
Expand Down Expand Up @@ -663,7 +675,7 @@ public void operationComplete(@Nullable ChannelFuture future) {
break;
}
ch.writeAndFlush(request);
} else { // an error occured
} else { // an error occurred
cameraCommunicationError(
"Connection Timeout: Check your IP and PORT are correct and the camera can be reached.");
}
Expand Down Expand Up @@ -1397,10 +1409,16 @@ void snapshotIsFfmpeg() {
"Binding has no snapshot url. Will use your CPU and FFmpeg to create snapshots from the cameras RTSP.");
bringCameraOnline();
if (!rtspUri.isEmpty()) {
updateImageChannel = false;
ffmpegSnapshotGeneration = true;
setupFfmpegFormat(FFmpegFormat.SNAPSHOT);
updateState(CHANNEL_POLL_IMAGE, OnOffType.ON);
if (!cameraConfig.getDisableSnapshotAtStartup()) {
updateImageChannel = false;
ffmpegSnapshotGeneration = true;
setupFfmpegFormat(FFmpegFormat.SNAPSHOT);
updateState(CHANNEL_POLL_IMAGE, OnOffType.ON);
} else {
logger.debug("snapshot polling with ffmpeg at startup is disabled");
ffmpegSnapshotGeneration = false;
updateState(CHANNEL_POLL_IMAGE, OnOffType.OFF);
}
} else {
cameraConfigError("Binding can not find a RTSP url for this camera, please provide a FFmpeg Input URL.");
}
Expand All @@ -1422,9 +1440,16 @@ void pollingCameraConnection() {
return;
}
if (cameraConfig.getOnvifPort() > 0 && !onvifCamera.isConnected()) {
if (onvifCamera.isConnectError()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, "Camera is not reachable");
} else if (onvifCamera.isRefusedError()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"Camera refused connection on ONVIF ports.");
}
logger.debug("About to connect to the IP Camera using the ONVIF PORT at IP:{}:{}", cameraConfig.getIp(),
cameraConfig.getOnvifPort());
onvifCamera.connect(thing.getThingTypeUID().getId().equals(ONVIF_THING));
return;
}
if ("ffmpeg".equals(snapshotUri)) {
snapshotIsFfmpeg();
Expand Down Expand Up @@ -1561,9 +1586,6 @@ void pollCameraRunnable() {
if (!snapshotPolling) {
checkCameraConnection();
}
if (!onvifCamera.isConnected()) {
onvifCamera.connect(true);
}
break;
case INSTAR_THING:
if (!snapshotPolling) {
Expand Down Expand Up @@ -1763,15 +1785,26 @@ public void initialize() {
}

private void tryConnecting() {
int firstDelay = 4;
int normalDelay = 12; // doesn't make sense to have faster retry than CONNECT_TIMEOUT, which is 10 seconds, if
// camera is off
if (!thing.getThingTypeUID().getId().equals(GENERIC_THING)
&& !thing.getThingTypeUID().getId().equals(DOORBIRD_THING) && cameraConfig.getOnvifPort() > 0) {
onvifCamera = new OnvifConnection(this, cameraConfig.getIp() + ":" + cameraConfig.getOnvifPort(),
cameraConfig.getUser(), cameraConfig.getPassword());
onvifCamera.setSelectedMediaProfile(cameraConfig.getOnvifMediaProfile());
// Only use ONVIF events if it is not an API camera.
onvifCamera.connect(supportsOnvifEvents());

if (supportsOnvifEvents()) {
// it takes some time to try to retrieve the ONVIF snapshot and stream URLs and update internal members
// on first connect; if connection lost, doesn't make sense to poll to often
firstDelay = 12;
normalDelay = 30;
}
}
cameraConnectionJob = threadPool.scheduleWithFixedDelay(this::pollingCameraConnection, 4, 8, TimeUnit.SECONDS);
cameraConnectionJob = threadPool.scheduleWithFixedDelay(this::pollingCameraConnection, firstDelay, normalDelay,
TimeUnit.SECONDS);
}

private boolean supportsOnvifEvents() {
Expand Down
17 changes: 17 additions & 0 deletions ...inding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/onvif/OnvifCodec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import io.netty.channel.ChannelHandlerContext;
import io.netty.handler.codec.http.HttpContent;
import io.netty.handler.codec.http.LastHttpContent;
import io.netty.handler.timeout.IdleStateEvent;
import io.netty.util.CharsetUtil;
import io.netty.util.ReferenceCountUtil;

Expand Down Expand Up @@ -59,6 +60,22 @@ public void channelRead(@Nullable ChannelHandlerContext ctx, @Nullable Object ms
}
}

@Override
public void userEventTriggered(@Nullable ChannelHandlerContext ctx, @Nullable Object evt) throws Exception {
if (ctx == null) {
return;
}
if (evt instanceof IdleStateEvent) {
IdleStateEvent e = (IdleStateEvent) evt;
logger.trace("IdleStateEvent received {}", e.state());
onvifConnection.setIsConnected(false);
// }
ctx.close();
} else {
logger.trace("other event received {}", evt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be a little more descriptive like "Other ONVIF netty channel event occured {}" as Netty is also used for the main camera comms and a separate set for ONVIF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea.

}
}

@Override
public void exceptionCaught(@Nullable ChannelHandlerContext ctx, @Nullable Throwable cause) {
if (ctx == null || cause == null) {
Expand Down
49 changes: 44 additions & 5 deletions ...g.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/onvif/OnvifConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

import static org.openhab.binding.ipcamera.internal.IpCameraBindingConstants.*;

import java.net.ConnectException;
import java.net.InetSocketAddress;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
Expand Down Expand Up @@ -49,6 +50,7 @@
import io.netty.channel.ChannelFutureListener;
import io.netty.channel.ChannelInitializer;
import io.netty.channel.ChannelOption;
import io.netty.channel.ConnectTimeoutException;
import io.netty.channel.EventLoopGroup;
import io.netty.channel.nio.NioEventLoopGroup;
import io.netty.channel.socket.SocketChannel;
Expand Down Expand Up @@ -127,6 +129,8 @@ public static enum RequestType {
private String imagingXAddr = "http://" + ipAddress + "/onvif/device_service";
private String ptzXAddr = "http://" + ipAddress + "/onvif/ptz_service";
private String subscriptionXAddr = "http://" + ipAddress + "/onvif/device_service";
private boolean connectError = false;
private boolean refusedError = false;
private boolean isConnected = false;
private int mediaProfileIndex = 0;
private String snapshotUri = "";
Expand Down Expand Up @@ -312,10 +316,13 @@ public void processReply(String message) {
} else if (message.contains("GetSystemDateAndTimeResponse")) {// 1st to be sent.
connecting.lock();
try {
connectError = false;
refusedError = false;
isConnected = true;
} finally {
connecting.unlock();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
connecting.lock();
try {
connectError = false;
refusedError = false;
isConnected = true;
} finally {
connecting.unlock();
}
setIsConnected(true);

Also need to add the following to the new setIsConnected(boolen) function:

connectError = false;
refusedError = false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I change both locations in the processReply() function to use the new function setIsConnected().
I reset connectError and refusedError also for case 'new connection state == false' as this leads to new reconnection attempts.

sendOnvifRequest(requestBuilder(RequestType.GetCapabilities, deviceXAddr));
parseDateAndTime(message);
logger.debug("Openhabs UTC dateTime is:{}", getUTCdateTime());
} else if (message.contains("GetCapabilitiesResponse")) {// 2nd to be sent.
Expand All @@ -324,6 +331,8 @@ public void processReply(String message) {
} else if (message.contains("GetProfilesResponse")) {// 3rd to be sent.
connecting.lock();
try {
connectError = false;
refusedError = false;
isConnected = true;
} finally {
connecting.unlock();
Expand Down Expand Up @@ -552,7 +561,7 @@ public void sendOnvifRequest(HttpRequest request) {

@Override
public void initChannel(SocketChannel socketChannel) throws Exception {
socketChannel.pipeline().addLast("idleStateHandler", new IdleStateHandler(0, 0, 70));
socketChannel.pipeline().addLast("idleStateHandler", new IdleStateHandler(20, 20, 20));
socketChannel.pipeline().addLast("HttpClientCodec", new HttpClientCodec());
socketChannel.pipeline().addLast("OnvifCodec", new OnvifCodec(getHandle()));
}
Expand All @@ -567,10 +576,22 @@ public void operationComplete(@Nullable ChannelFuture future) {
return;
}
if (future.isSuccess()) {
connectError = false;
Channel ch = future.channel();
ch.writeAndFlush(request);
} else { // an error occured
logger.debug("Camera is not reachable on ONVIF port:{} or the port may be wrong.", onvifPort);
if (future.isDone() && !future.isCancelled()) {
Throwable cause = future.cause();
logger.trace("connect failed - cause {}", cause.getMessage());
if (cause instanceof ConnectTimeoutException) {
logger.debug("Camera is not reachable on IP {}", ipAddress);
connectError = true;
} else if ((cause instanceof ConnectException)
&& cause.getMessage().contains("Connection refused")) {
logger.debug("Camera ONVIF port {} is refused.", onvifPort);
connectError = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be refusedError ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct

}
}
if (isConnected) {
disconnect();
}
Expand Down Expand Up @@ -913,13 +934,20 @@ public void connect(boolean useEvents) {
threadPool = Executors.newScheduledThreadPool(2);
sendOnvifRequest(requestBuilder(RequestType.GetSystemDateAndTime, deviceXAddr));
usingEvents = useEvents;
sendOnvifRequest(requestBuilder(RequestType.GetCapabilities, deviceXAddr));
}
} finally {
connecting.unlock();
}
}

public boolean isConnectError() {
return connectError;
}

public boolean isRefusedError() {
return refusedError;
}

public boolean isConnected() {
connecting.lock();
try {
Expand All @@ -929,6 +957,15 @@ public boolean isConnected() {
}
}

public void setIsConnected(boolean isConnected) {
connecting.lock();
try {
this.isConnected = isConnected;
} finally {
connecting.unlock();
}
}

private void cleanup() {
if (!isConnected && !mainEventLoopGroup.isShuttingDown()) {
try {
Expand All @@ -947,9 +984,9 @@ private void cleanup() {
public void disconnect() {
connecting.lock();// Lock out multiple disconnect()/connect() attempts as we try to send Unsubscribe.
try {
isConnected = false;// isConnected is not thread safe, connecting.lock() used as fix.
if (bootstrap != null) {
if (usingEvents && !mainEventLoopGroup.isShuttingDown()) {
if (isConnected && usingEvents && !mainEventLoopGroup.isShuttingDown()) {
// Only makes sense to send if connected
// Some cameras may continue to send events even when they can't reach a server.
sendOnvifRequest(requestBuilder(RequestType.Unsubscribe, subscriptionXAddr));
}
Expand All @@ -958,6 +995,8 @@ public void disconnect() {
} else {
cleanup();
}

isConnected = false;// isConnected is not thread safe, connecting.lock() used as fix.
} finally {
connecting.unlock();
}
Expand Down
2 changes: 2 additions & 0 deletions bundles/org.openhab.binding.ipcamera/src/main/resources/OH-INF/i18n/ipcamera.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,8 @@ thing-type.config.ipcamera.onvif.password.label = Password
thing-type.config.ipcamera.onvif.password.description = Enter the password for your camera. Leave blank if your camera does not use one.
thing-type.config.ipcamera.onvif.pollTime.label = Poll Time
thing-type.config.ipcamera.onvif.pollTime.description = Most features are made on demand and not polled, but some features require a regular snapshot to work. Default is "1000" which is 1 second.
thing-type.config.ipcamera.onvif.disableSnapshotAtStartup.lable = Disable Automatic Snapshot-Polling at Startup
thing-type.config.ipcamera.onvif.disableSnapshotAtStartup.description = For cameras without Snapshot URL, a ffmpeg process is automatically started for creating snapshots. Set this flag to true, if you don't want it.
thing-type.config.ipcamera.onvif.port.label = Port for HTTP
thing-type.config.ipcamera.onvif.port.description = This port will be used for HTTP calls for fetching the snapshot and alarm states.
thing-type.config.ipcamera.onvif.ptzContinuous.label = Use Continuous PTZ
Expand Down
9 changes: 9 additions & 0 deletions bundles/org.openhab.binding.ipcamera/src/main/resources/OH-INF/thing/thing-types.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,15 @@
<advanced>true</advanced>
</parameter>

<parameter name="disableSnapshotAtStartup" type="boolean" required="false" groupName="Settings">
<label>Disable Automatic Snapshot-Polling at Startup</label>
<description>For cameras without Snapshot URL, a ffmpeg process is automatically started for creating snapshots. Set
this flag to true, if you don't want it.
</description>
<default>false</default>
<advanced>true</advanced>
</parameter>

<parameter name="ptzContinuous" type="boolean" groupName="Settings">
<label>Use Continuous PTZ</label>
<description>Select if you want Relative (false) or Continuous (true) movements.
Expand Down