[zoneminder] Added image channel, changed to jetty, Optimized performance #3583

Mr-Eskildsen · 2018-05-21T12:54:43Z

Signed-off-by: Martin Eskildsen martin@eskildsen.name (github:
Mr-Eskildsen)

ZoneMinder Binding
[zoneminder] Added Image channel to monitor
[zoneminder] Changed http client to jetty
[zoneminder] Optimized performance

JAR is available in the Eclipse IoT Marketplace, as well as here:
http://www.eskildsen.name/openHAB/zoneminder/org.openhab.binding.zoneminder-2.3.0-SNAPSHOT.jar

Signed-off-by: Martin Eskildsen <martin@eskildsen.name> (github: Mr-Eskildsen)

martinvw

Thanks for the big update.

I added some comments, note that quite a few of them should be applied in a general way. I did not yet review the complete code but I think that I have a good view of what changed and hope that you can improve the pull request given the pointers I gave you.

If you have any questions or if you finished please ping me.

martinvw · 2018-05-26T10:37:24Z

addons/binding/org.openhab.binding.zoneminder/.classpath

@@ -3,6 +3,6 @@
 	<classpathentry kind="con" path="org.eclipse.jdt.launching.JRE_CONTAINER/org.eclipse.jdt.internal.debug.ui.launcher.StandardVMType/JavaSE-1.8"/>
 	<classpathentry kind="con" path="org.eclipse.pde.core.requiredPlugins"/>
 	<classpathentry kind="src" path="src/main/java"/>
-	<classpathentry kind="lib" path="lib/zoneminder4j-0.9.7.jar"/>
+	<classpathentry kind="lib" path="lib/zoneminder4j-0.9.8.jar" sourcepath="D:/Development/Java/workspace/zoneminder4j"/>


The path looks very local to me?

martinvw · 2018-05-26T10:38:03Z

addons/binding/org.openhab.binding.zoneminder/ESH-INF/binding/binding.xml

-	<description>This binding interfaces a ZoneMinder Server</description>
-	<author>Martin S. Eskildsen</author>
-
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"


XML files should be formatted with tabs, like it was

martinvw · 2018-05-26T10:38:50Z

addons/binding/org.openhab.binding.zoneminder/ESH-INF/config/server-config.xml

@@ -0,0 +1,137 @@
+<?xml version="1.0" encoding="UTF-8"?>


This file should be formatted using tabs

martinvw · 2018-05-26T10:43:45Z

addons/binding/org.openhab.binding.zoneminder/ESH-INF/thing/bridge-server.xml

@@ -0,0 +1,39 @@
+<?xml version="1.0" encoding="UTF-8"?>


This file should formatted with tabs, note that the default openHAB setup using Eclipse should be able to do that for you , by just reformatting the file.

Please apply to all

martinvw · 2018-05-26T10:45:16Z

addons/binding/org.openhab.binding.zoneminder/OSGI-INF/ZoneMinderHandlerFactory.xml

@@ -1,20 +1,15 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <!--
+    Copyright (c) 2010-2018 by the respective copyright holders.


Or just remove this whole file and use DS annotations as I've done in https://github.com/openhab/openhab2-addons/pull/3382.

martinvw · 2018-05-28T18:34:14Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderServerBridgeHandler.java

+                updateStatus(newStatus, statusDetail, statusDescription);
+
+                logger.error("{}: context='{}' state='{}' check='FAILED' - {}", getLogIdentifier(), context,
+                        newStatus.toString(), statusDescription);


You don't have to call toString it will be called automatically, but only when the logging-level actually applies.

Please apply everywhere.

Note that this does not have to be error logging anymore because you already configured the thing status, it more likely that he will see that than that he will the error log.

martinvw · 2018-05-28T18:34:58Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderServerBridgeHandler.java

+                return status;
+
+            } else if (config.getHost() == null) {
+                newStatus = ThingStatus.OFFLINE;


This method looks to have a lot of repetition, can you implement this in smarter way?

The "problem" is that I need to check every parameter, to be able to log a precise error.
All the repetetive assignments removed.

martinvw · 2018-05-28T18:35:51Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderServerBridgeHandler.java


-                    return;
-                }
+            newStatus = ThingStatus.OFFLINE;


Why OFFLINE, you are even connecting, maybe UNKNOW would be more suitable?

Nope, at that specific place I have verified that config in openHAB looks good, Now I have to verify the configuration in the ZoneMidner Server, which could also be wrong (eg. impossible to establish connection). The only way to find out is trial and error .... :-(. I will add a few comments in the code :-)

Yes, but OFFLINE should not purely be about the connection but more about the state of the Thing to which you are connecting, so we don't know yet, but stating OFFLINE is premature.

martinvw · 2018-05-28T18:37:40Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderServerBridgeHandler.java

+                forcedPriority = RefreshPriority.UNKNOWN;
+                if (updateConnection) {
+                    try {
+                        setConnected(false);


If this method is not a setter, which is suggested by the number of exceptions it throws, it might be better to call it disconnect. Because now it confuse me as the reader

martinvw · 2018-05-28T18:39:01Z

...oneminder/src/main/java/org/openhab/binding/zoneminder/internal/state/MonitorThingState.java

+    private boolean isDirty;
+    private String logIdentifier = "";
+
+    private IZoneMinderEventData activeEvent = null;


These values would also be assigned here, if you left out the null

wborn · 2018-07-08T12:18:39Z

addons/binding/org.openhab.binding.zoneminder/out.txt

@@ -0,0 +1,300 @@
+[INFO] Scanning for projects...


Remove this file.

wborn · 2018-07-08T12:19:08Z

addons/binding/org.openhab.binding.zoneminder/README.md

+| streamingUser            |    X     | If 'useSpecificUserStreaming' is true, username must be specified here                     |
+| streamingPassword        |    X     | If 'useSpecificUserStreaming' is true, password must be specified here                     |
+
+#### Monitor Configuration


Add new line after each header.

wborn · 2018-07-08T12:19:24Z

addons/binding/org.openhab.binding.zoneminder/README.md


 ### Things configuration

+#### Bridge Configuration


Add new line after each header.

wborn · 2018-07-08T12:20:02Z

addons/binding/org.openhab.binding.zoneminder/README.md

+| imageScale               |    X     | Size (scale) of image. Default: 100                                                           |
+
+
+### Things file


Add new line after each header.

wborn · 2018-07-08T12:20:22Z

addons/binding/org.openhab.binding.zoneminder/build.properties

@@ -7,3 +7,4 @@ bin.includes = META-INF/,\
               lib/,\
               about.html

+


Please remove this unnecessary change.

wborn · 2018-07-08T14:17:35Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderThingMonitorHandler.java

+                    logger.error(
+                            "{}: context='fetchMonitorDaemonStatus' error in call to 'getAnalysisDaemonStatus' - Exception='{}' ",
+                            getLogIdentifier(), zme.getClass().getCanonicalName(), zme.getCause());
+                } catch (Exception ex) {


Please catch specific exceptions instead of catching all exceptions. That way it is more clear what exceptions need to be handled and how this is done.

wborn · 2018-07-08T14:17:42Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderThingMonitorHandler.java

+                    logger.error(
+                            "{}: context='fetchMonitorDaemonStatus' error in call to 'getFrameDaemonStatus' - Exception='{}'",
+                            getLogIdentifier(), zme.getClass().getCanonicalName(), zme.getCause());
+                } catch (Exception ex) {


Please catch specific exceptions instead of catching all exceptions. That way it is more clear what exceptions need to be handled and how this is done.

wborn · 2018-07-08T14:17:51Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderThingMonitorHandler.java

@@ -893,14 +1194,28 @@ public String getLogIdentifier() {

        try {
            if (config != null) {
-
                result = String.format("[MONITOR-%s]", config.getZoneMinderId().toString());
            }

        } catch (Exception ex) {


Please catch specific exceptions instead of catching all exceptions. That way it is more clear what exceptions need to be handled and how this is done.

wborn · 2018-07-08T14:18:01Z

...oneminder/src/main/java/org/openhab/binding/zoneminder/internal/state/GenericThingState.java

+            if (getChannelStateHandler(channelUID) != null) {
+                getChannelStateHandler(channelUID).subscribe();
+            }
+        } catch (Exception ex) {


Please catch specific exceptions instead of catching all exceptions. That way it is more clear what exceptions need to be handled and how this is done.

wborn · 2018-07-08T14:19:04Z

...nder/src/main/java/org/openhab/binding/zoneminder/handler/ZoneMinderThingMonitorHandler.java

-            logger.debug("{}:    Monitor Id:         {}", getLogIdentifier(), config.getZoneMinderId());
+
+            super.initialize();
+            logger.info("{}: context='initialize' Monitor Handler Initialized", getLogIdentifier());


Reduce to debug.

wborn · 2018-07-08T14:37:49Z

...rc/main/java/org/openhab/binding/zoneminder/internal/state/ChannelStateChangeSubscriber.java

+import org.eclipse.smarthome.core.types.State;
+
+/**
+ * The {@link GenericThingState} is responsible for handling commands, which are


GenericThingState? Please add an appropriate JavaDoc.

wborn · 2018-07-08T14:37:57Z

...oneminder/src/main/java/org/openhab/binding/zoneminder/internal/state/ChannelStringType.java

+import org.eclipse.smarthome.core.types.UnDefType;
+
+/**
+ * The {@link GenericThingState} is responsible for handling commands, which are


GenericThingState? Please add an appropriate JavaDoc.

wborn · 2018-07-08T14:38:06Z

...eminder/src/main/java/org/openhab/binding/zoneminder/internal/state/GenericChannelState.java

+import org.eclipse.smarthome.core.types.UnDefType;
+
+/**
+ * The {@link GenericThingState} is responsible for handling commands, which are


GenericThingState? Please add an appropriate JavaDoc.

openhab-bot · 2018-08-17T21:08:30Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/zoneminder-binding-in-marketplace-beta/39697/31

martinvw · 2018-11-11T13:35:39Z

@Mr-Eskildsen thanks for your contribution, do you expect to be able to wrap up this PR? Thanks for keeping us updated!

openhab-bot · 2018-12-13T21:26:54Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/zoneminder-binding-in-marketplace-beta/39697/48

wborn · 2019-01-25T20:55:08Z

Any plans for addressing the review comments @Mr-Eskildsen ? We've got several PRs waiting on this PR. Or is it OK with you that we merge #3382 and #4037?

Mr-Eskildsen · 2019-01-25T20:56:54Z

Sorry, I am little bit stucked with this. I think we should just reject the PR. /Martin Den fre. 25. jan. 2019 kl. 21.55 skrev Wouter Born <notifications@github.com

…

: Any plans for addressing the review comments @Mr-Eskildsen <https://github.com/Mr-Eskildsen> ? We've got several PRs waiting on this PR. Or is it OK with you that we merge #3382 <https://github.com/openhab/openhab2-addons/pull/3382> and #4037 <https://github.com/openhab/openhab2-addons/pull/4037>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/openhab/openhab2-addons/pull/3583#issuecomment-457724707>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFTMexVn9jJ8iQ2oE6ViNufRCaiDWUBNks5vG28zgaJpZM4UG8b2> .

wborn · 2019-01-25T21:07:45Z

Thanks for the update! 👍 We'll then merge the other two PRs. It would be awesome if you can still find some time at a later moment to continue your great work on this PR!

J-N-K · 2019-04-12T15:55:26Z

Any progress here? We would like to move the binding to the bnd build system.

davidgraeff · 2019-04-13T12:16:33Z

I'd say we close this for now. @Mr-Eskildsen maybe you can copy paste part of the this PR back to master later on and provide smaller pull requests that result in faster merges :)

Added image channel, changed to jetty, Optimized performance

c76d218

Signed-off-by: Martin Eskildsen <martin@eskildsen.name> (github: Mr-Eskildsen)

Mr-Eskildsen changed the title ~~Added image channel, changed to jetty, Optimized performance~~ [zoneminder] Added image channel, changed to jetty, Optimized performance May 21, 2018

martinvw added this to the After 2.3 milestone May 25, 2018

martinvw reviewed May 28, 2018

View reviewed changes

wborn requested changes Jul 8, 2018

View reviewed changes

wborn added the awaiting feedback label Jul 8, 2018

wborn reviewed Jul 8, 2018

View reviewed changes

wborn mentioned this pull request Jul 13, 2018

[zoneminder] Motion detection is slow #3730

Closed

This was referenced Sep 22, 2018

[zoneminder] Use DS annotations, fix static code analysis findings #3382

Merged

[zoneminder] Made constants and handler packages ineternal #4037

Merged

wborn removed this from the 2.4 milestone Dec 19, 2018

maggu2810 mentioned this pull request Mar 29, 2019

Migration to a Maven + Bnd based build system #5005

Closed

davidgraeff closed this Apr 14, 2019

mhilbush mentioned this pull request Apr 18, 2020

[zm] New Zoneminder binding initial contribution #7412

Closed

mhilbush mentioned this pull request Sep 21, 2020

[zoneminder] Replacement for ZoneMinder binding #8530

Merged

		\| imageScale \| X \| Size (scale) of image. Default: 100 \|


		### Things file

[zoneminder] Added image channel, changed to jetty, Optimized performance #3583

[zoneminder] Added image channel, changed to jetty, Optimized performance #3583

Conversation

Mr-Eskildsen commented May 21, 2018 • edited

martinvw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mr-Eskildsen May 29, 2018 • edited

Choose a reason for hiding this comment

martinvw May 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openhab-bot commented Aug 17, 2018

martinvw commented Nov 11, 2018

openhab-bot commented Dec 13, 2018

wborn commented Jan 25, 2019

Mr-Eskildsen commented Jan 25, 2019 via email

wborn commented Jan 25, 2019

J-N-K commented Apr 12, 2019

davidgraeff commented Apr 13, 2019

Mr-Eskildsen commented May 21, 2018 •

edited

Mr-Eskildsen May 29, 2018 •

edited

martinvw May 30, 2018 •

edited