-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
[ipcamera] Fix connection checks with ONVIF cameras with no snapshots #15119
Conversation
64c0a7a
to
91a01bc
Compare
…uests. Also checking connect errors and setting new states connectError or refusedError accordingly. On connect, only one request is sent to have less parallel actions in case of reconnect, timeout. Moved GetCapabilities call to GetSystemDateAndTime response handler part. Added support to disable automatic polling at startup. Signed-off-by: Thomas Burri <th@thonojato.ch>
fba4072
to
37dac82
Compare
} else { | ||
logger.trace("cam not connected"); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, makes sense.
|
||
// 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"); |
There was a problem hiding this comment.
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 (thing.getThingTypeUID().getId().equals(ONVIF_THING) && snapshotUri.isEmpty()) { | ||
logger.trace("is ONVIF with empty snapshot URI"); | ||
if (onvifCamera.isConnected()) { | ||
logger.trace("cam is connected"); |
There was a problem hiding this comment.
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.
...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/CameraConfig.java
Outdated
Show resolved
Hide resolved
// } | ||
ctx.close(); | ||
} else { | ||
logger.trace("other event received {}", evt); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea.
} else if ((cause instanceof ConnectException) | ||
&& cause.getMessage().contains("Connection refused")) { | ||
logger.debug("Camera ONVIF port {} is refused.", onvifPort); | ||
connectError = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be refusedError ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct
connecting.lock(); | ||
try { | ||
connectError = false; | ||
refusedError = false; | ||
isConnected = true; | ||
} finally { | ||
connecting.unlock(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
There was a problem hiding this comment.
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.
Thanks for the changes, have made all comments and the rest looks good. Just need to test these on my cameras to ensure its all good if the next build will be a STABLE build. Best this gets merged with some time to trial as a milestone as we must be getting close to the first 4.0 STABLE |
Removed thing configuration 'disableSnapshotAtStartup' again. Signed-off-by: Thomas Burri <th@thonojato.ch>
@Skinah : did you approve this PR finally ? |
@lolodomo LGTM |
There is now a conflict to resolve. Probably after I merged another PR. |
Signed-off-by: Matthew Skinner <matt@pcmus.com>
@tb4jc I tried to fix the merge conflicts but it is now failing to build, can you look at it please so this can be merged? thanks |
Signed-off-by: Thomas Burri <th@thonojato.ch>
…uilder). Signed-off-by: Thomas Burri <th@thonojato.ch>
@Skinah: I fixed the conflicts |
@Skinah Any update? Possible to merge now? |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you
…openhab#15119) * Added connection check via IdleStateHandler events for sent onvif requests. Also checking connect errors and setting new states connectError or refusedError accordingly. On connect, only one request is sent to have less parallel actions in case of reconnect, timeout. Moved GetCapabilities call to GetSystemDateAndTime response handler part. Added support to disable automatic polling at startup. * Fixed call of sendOnvifRequest (missed to remove one call of requestBuilder). --------- Signed-off-by: Thomas Burri <th@thonojato.ch> Signed-off-by: Matthew Skinner <matt@pcmus.com> Co-authored-by: Matthew Skinner <matt@pcmus.com>
…openhab#15119) * Added connection check via IdleStateHandler events for sent onvif requests. Also checking connect errors and setting new states connectError or refusedError accordingly. On connect, only one request is sent to have less parallel actions in case of reconnect, timeout. Moved GetCapabilities call to GetSystemDateAndTime response handler part. Added support to disable automatic polling at startup. * Fixed call of sendOnvifRequest (missed to remove one call of requestBuilder). --------- Signed-off-by: Thomas Burri <th@thonojato.ch> Signed-off-by: Matthew Skinner <matt@pcmus.com> Co-authored-by: Matthew Skinner <matt@pcmus.com>
…openhab#15119) * Added connection check via IdleStateHandler events for sent onvif requests. Also checking connect errors and setting new states connectError or refusedError accordingly. On connect, only one request is sent to have less parallel actions in case of reconnect, timeout. Moved GetCapabilities call to GetSystemDateAndTime response handler part. Added support to disable automatic polling at startup. * Fixed call of sendOnvifRequest (missed to remove one call of requestBuilder). --------- Signed-off-by: Thomas Burri <th@thonojato.ch> Signed-off-by: Matthew Skinner <matt@pcmus.com> Co-authored-by: Matthew Skinner <matt@pcmus.com>
…openhab#15119) * Added connection check via IdleStateHandler events for sent onvif requests. Also checking connect errors and setting new states connectError or refusedError accordingly. On connect, only one request is sent to have less parallel actions in case of reconnect, timeout. Moved GetCapabilities call to GetSystemDateAndTime response handler part. Added support to disable automatic polling at startup. * Fixed call of sendOnvifRequest (missed to remove one call of requestBuilder). --------- Signed-off-by: Thomas Burri <th@thonojato.ch> Signed-off-by: Matthew Skinner <matt@pcmus.com> Co-authored-by: Matthew Skinner <matt@pcmus.com> Signed-off-by: querdenker2k <querdenker2k@gmx.de>
…openhab#15119) * Added connection check via IdleStateHandler events for sent onvif requests. Also checking connect errors and setting new states connectError or refusedError accordingly. On connect, only one request is sent to have less parallel actions in case of reconnect, timeout. Moved GetCapabilities call to GetSystemDateAndTime response handler part. Added support to disable automatic polling at startup. * Fixed call of sendOnvifRequest (missed to remove one call of requestBuilder). --------- Signed-off-by: Thomas Burri <th@thonojato.ch> Signed-off-by: Matthew Skinner <matt@pcmus.com> Co-authored-by: Matthew Skinner <matt@pcmus.com> Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
This pull request is another try to fix the issue that connection check on ONVIF IP cams right now only work if snapshotPollling is active. Reported in issue 14910
Additionally, it gives the possibility to disable polling at startup.