[netatmo] Support for switching video surveillance on/off (#7938) #7968

Novanic · 2020-06-21T09:40:54Z

With this change it is possible to switch the video surveillance on and off (for all cameras / Welcome and Presence). See issue #7938 for more information.

When a camera related command is executed a refresh is executed after 2 seconds to update all channels. This has for example following effects:

When the floodlight is switched on/off a new live picture gets received with the new floodlight state.
When the video surveillance is switched on/off an event is received to confirm the new state.

Update

- The video surveillance can now get switched on/off - When the command is executed (floodlight or video surveillance switched on/off) a refresh is executed after 2 seconds to update all channels Signed-off-by: Sven Strohschein <[email protected]>

Signed-off-by: Sven Strohschein <[email protected]>

TravisBuddy · 2020-06-21T09:44:15Z

Travis tests were successful

Hey @Novanic,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-06-21T10:19:22Z

Version deployed and I can confirm that last event and live picture are now correctly refreshed quickly just after switching on or off the surveillance.

This is probably the enhancement that will add the best value to my home automation system since months. Thanks for finding that.

Remains to review the code.

lolodomo

Just a comment about the log level for me.

Your new method getModule() could be used in the same way in other handlers (to be done in separate PRs).

lolodomo · 2020-06-21T10:31:11Z

...binding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/camera/CameraHandler.java

+        try {
+            return executeGETRequest(url).map(JSONObject::new);
+        } catch (JSONException e) {
+            logger.warn("Error on parsing the content as JSON!", e);


A DEBUG log will be sufficient I think.

Debug? When this error occurs, it is an error, the functionality is broken when it occurs. That shouldn't happen. Actually warning isn't enough.

My general rule for logging levels is:

trace - used for normal expected execution paths

debug - used for unexepected execution paths, but well within normal operation

info - used for notable points in an execution path, like a milestone. (in openhab we try reserve this logging level for the core, so bindings should rarely ever call this.)

warn - used for notable unexpected execution paths that a regular user (not just a developer) should be notified of. Warnings should be used to indicate that something not-normal occurred and user intervention is required to resolve. Warnings do not indicate a failure to operate merely an abnormal condition of operation that can still be handled by the binding. Failures in binding operation should be indicated by changing the thing status to offline.

error - used to indicate catastrophic program failure. This should be used to indicate a catastrophic failure in openhab's ability to operate. A failure in a binding would never cause openhab as a whole to fail so a failure in a bindings should never log an error. Instead that failure should be indicated by changing the thing status.

Now if a JSONException does occur here it doesn't really reflect a failure in the binding so much a failure in an external system and can be thought of more or less akin to connection loss. If you think that such a json parsing failure should be elevated above "warn" then I suggest changing the thing status to OFFLINE to reflect that.

lolodomo · 2020-06-21T10:31:51Z

...binding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/camera/CameraHandler.java

+                return Optional.of(content);
+            }
+        } catch (IOException e) {
+            logger.warn("Error on accessing local camera url!", e);


A DEBUG log will be sufficient I think.

I think that such an exception should probably be elevated to the caller since I don't think that simple logging would be how all callers would like this particular issue to be handled.

Novanic · 2020-06-21T15:20:39Z

Debug? When this error occurs, it is an error, the functionality is broken when it occurs. That shouldn't happen. Actually warning isn't enough.

lolodomo · 2020-06-21T15:35:23Z

Please read the coding guidelines for binding developers, there is an explanation about log levels.

lolodomo · 2020-06-21T15:37:42Z

https://www.openhab.org/docs/developer/guidelines.html#f-logging

lolodomo · 2020-06-21T15:40:41Z

@cpmeister will correct me if I am wrong in my comments.

Signed-off-by: Sven Strohschein <[email protected]>

Novanic · 2020-06-21T18:15:31Z

I have found a performance / network resource optimization. At the moment the home data (which is requested at least every 5 minutes and after every executed command) is requested with 30 events by default (+ sub-events), but the binding processes only the latest event. So the request within NetatmoBridgeHandler#getWelcomeDataBody(...) could get changed to request just 1 event (via a query-parameter). Should I change the line with this pull-request or should I open a new issue and pull-request?

lolodomo · 2020-06-21T18:24:56Z

I have found a performance / network resource optimization. At the moment the home data (which is requested at least every 5 minutes and after every executed command) is requested with 30 events by default (+ sub-events), but the binding processes only the latest event. So the request within NetatmoBridgeHandler#getWelcomeDataBody(...) could get changed to request just 1 event (via a query-parameter). Should I change the line with this pull-request or should I open a new issue and pull-request?

I would suggest a separate PR.
Maybe wait for @clinique comment, I have in mind this point was discussed a long time ago. We have to be sure that only the last event is processed, and not all new events since the last retrieval. For the persons detection, are you really sure that some of the events are ignored ?

Novanic · 2020-06-21T18:31:10Z

I have found a performance / network resource optimization. At the moment the home data (which is requested at least every 5 minutes and after every executed command) is requested with 30 events by default (+ sub-events), but the binding processes only the latest event. So the request within NetatmoBridgeHandler#getWelcomeDataBody(...) could get changed to request just 1 event (via a query-parameter). Should I change the line with this pull-request or should I open a new issue and pull-request?

I would suggest a separate PR.
Maybe wait for @clinique comment, I have in mind this point was discussed a long time ago. We have to be sure that only the last event is processed, and not all new events since the last retrieval. For the persons detection, are you really sure that some of the events are ignored ?

Yes, I'm sure. Only the event with the newest time is processed, all other events are ignored/filtered. You can search for usages of NAWelcomeHome#getEvents(). There is only this one usage.

TravisBuddy · 2020-06-21T18:37:03Z

Travis tests were successful

Hey @Novanic,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

lolodomo · 2020-06-21T20:15:45Z

At this place, I see we are going through the list of events to filter on events relative to a person, that means it is not necessarily the most recent event. This must not be broken.
https://github.com/openhab/openhab-addons/blob/2.5.x/bundles/org.openhab.binding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/welcome/NAWelcomePersonHandler.java#L59

lolodomo · 2020-06-24T10:47:38Z

@cpmeister : could you please review and tell us what should be done regarding log levels (my only comments about this PR).

PS: I am waiting for the merge of this PR to restart adding null annotations.

cpmeister · 2020-06-24T22:23:44Z

...binding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/camera/CameraHandler.java

+        try {
+            return executeGETRequest(url).map(JSONObject::new);
+        } catch (JSONException e) {
+            logger.warn("Error on parsing the content as JSON!", e);


My general rule for logging levels is:

trace - used for normal expected execution paths

debug - used for unexepected execution paths, but well within normal operation

info - used for notable points in an execution path, like a milestone. (in openhab we try reserve this logging level for the core, so bindings should rarely ever call this.)

warn - used for notable unexpected execution paths that a regular user (not just a developer) should be notified of. Warnings should be used to indicate that something not-normal occurred and user intervention is required to resolve. Warnings do not indicate a failure to operate merely an abnormal condition of operation that can still be handled by the binding. Failures in binding operation should be indicated by changing the thing status to offline.

error - used to indicate catastrophic program failure. This should be used to indicate a catastrophic failure in openhab's ability to operate. A failure in a binding would never cause openhab as a whole to fail so a failure in a bindings should never log an error. Instead that failure should be indicated by changing the thing status.

Now if a JSONException does occur here it doesn't really reflect a failure in the binding so much a failure in an external system and can be thought of more or less akin to connection loss. If you think that such a json parsing failure should be elevated above "warn" then I suggest changing the thing status to OFFLINE to reflect that.

cpmeister · 2020-06-24T22:24:56Z

...binding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/camera/CameraHandler.java

+                return Optional.of(content);
+            }
+        } catch (IOException e) {
+            logger.warn("Error on accessing local camera url!", e);


I think that such an exception should probably be elevated to the caller since I don't think that simple logging would be how all callers would like this particular issue to be handled.

Novanic · 2020-06-25T19:45:34Z

@cpmeister : could you please review and tell us what should be done regarding log levels (my only comments about this PR).

PS: I am waiting for the merge of this PR to restart adding null annotations.

Hm, that you for the explanation, but I don't know what I should do.
1.) Debug?
2.) Debug + Thing to offline?
3.) Warn?
4.) Warn + Thing to offline?

cpmeister · 2020-06-25T20:51:13Z

Hm, that you for the explanation, but I don't know what I should do.
1.) Debug?
2.) Debug + Thing to offline?
3.) Warn?
4.) Warn + Thing to offline?

If you think that an IOException can be considered making the binding inoperable, then just set the thing to offline along with a message. Otherwise log it as a warn in order for the user to be notified of the issue. Debug should be used if those errors would be a regular occurrence during binding operation.

lolodomo · 2020-06-26T04:24:11Z

So finally the WARN level was no so bad ;)
We could keep your code as it is.

lolodomo

LGTM

lolodomo · 2020-06-28T07:08:08Z

I need the merge of this PR to progress.
@clinique can you review it?
@cpmeister : not available?

clinique

Good job. LGTM

cpmeister

LGTM

* [7938] Support for switching video surveillance on/off - The video surveillance can now get switched on/off - When the command is executed (floodlight or video surveillance switched on/off) a refresh is executed after 2 seconds to update all channels * Warnings fixed * New tests for isNewLastEvent added Signed-off-by: Sven Strohschein <[email protected]>

* [7938] Support for switching video surveillance on/off - The video surveillance can now get switched on/off - When the command is executed (floodlight or video surveillance switched on/off) a refresh is executed after 2 seconds to update all channels * Warnings fixed * New tests for isNewLastEvent added Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: CSchlipp <[email protected]>

* [7938] Support for switching video surveillance on/off - The video surveillance can now get switched on/off - When the command is executed (floodlight or video surveillance switched on/off) a refresh is executed after 2 seconds to update all channels * Warnings fixed * New tests for isNewLastEvent added Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: MPH80 <[email protected]>

* [7938] Support for switching video surveillance on/off - The video surveillance can now get switched on/off - When the command is executed (floodlight or video surveillance switched on/off) a refresh is executed after 2 seconds to update all channels * Warnings fixed * New tests for isNewLastEvent added Signed-off-by: Sven Strohschein <[email protected]>

* [7938] Support for switching video surveillance on/off - The video surveillance can now get switched on/off - When the command is executed (floodlight or video surveillance switched on/off) a refresh is executed after 2 seconds to update all channels * Warnings fixed * New tests for isNewLastEvent added Signed-off-by: Sven Strohschein <[email protected]> Signed-off-by: Daan Meijer <[email protected]>

* [7938] Support for switching video surveillance on/off - The video surveillance can now get switched on/off - When the command is executed (floodlight or video surveillance switched on/off) a refresh is executed after 2 seconds to update all channels * Warnings fixed * New tests for isNewLastEvent added Signed-off-by: Sven Strohschein <[email protected]>

Novanic and others added 10 commits June 10, 2020 19:50

Merge pull request #1 from openhab/2.5.x

78699f9

Update

Merge pull request #2 from openhab/2.5.x

ae6b891

Update

Merge pull request #3 from openhab/2.5.x

66abc11

Update

Merge pull request #4 from openhab/2.5.x

4f2cd51

Update

Merge pull request #5 from openhab/2.5.x

bd9cfc1

Update

Merge pull request #6 from openhab/2.5.x

de62002

Update

Merge pull request #7 from openhab/2.5.x

ad272b7

Update

Merge pull request #8 from openhab/2.5.x

76c47b8

Update

Warnings fixed

f875381

Signed-off-by: Sven Strohschein <[email protected]>

Novanic requested review from clinique, cweitkamp and lolodomo as code owners June 21, 2020 09:40

Novanic mentioned this pull request Jun 21, 2020

[netatmo] Support for switching video surveillance on/off #7938

Closed

lolodomo reviewed Jun 21, 2020

View reviewed changes

kaikreuzer force-pushed the 2.5.x branch from 536fc42 to 3ab1af1 Compare June 21, 2020 16:11

New tests for isNewLastEvent added

8d35063

Signed-off-by: Sven Strohschein <[email protected]>

cpmeister requested changes Jun 24, 2020

View reviewed changes

cpmeister added the enhancement An enhancement or new feature for an existing add-on label Jun 25, 2020

lolodomo approved these changes Jun 26, 2020

View reviewed changes

lolodomo mentioned this pull request Jun 27, 2020

[netatmo] Null annotations #7913

Closed

clinique approved these changes Jun 28, 2020

View reviewed changes

cpmeister approved these changes Jun 28, 2020

View reviewed changes

cpmeister linked an issue Jun 28, 2020 that may be closed by this pull request

[netatmo] Support for switching video surveillance on/off #7938

Closed

cpmeister merged commit 05eb61d into openhab:2.5.x Jun 28, 2020

cpmeister added this to the 2.5.7 milestone Jun 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[netatmo] Support for switching video surveillance on/off (#7938) #7968

[netatmo] Support for switching video surveillance on/off (#7938) #7968

Novanic commented Jun 21, 2020 •

edited

Loading

TravisBuddy commented Jun 21, 2020

lolodomo commented Jun 21, 2020 •

edited

Loading

lolodomo left a comment

lolodomo Jun 21, 2020

Novanic Jun 21, 2020 •

edited

Loading

cpmeister Jun 24, 2020

lolodomo Jun 21, 2020

cpmeister Jun 24, 2020

Novanic commented Jun 21, 2020

lolodomo commented Jun 21, 2020 •

edited

Loading

lolodomo commented Jun 21, 2020

lolodomo commented Jun 21, 2020

Novanic commented Jun 21, 2020 •

edited

Loading

lolodomo commented Jun 21, 2020

Novanic commented Jun 21, 2020

TravisBuddy commented Jun 21, 2020

lolodomo commented Jun 21, 2020 •

edited

Loading

lolodomo commented Jun 24, 2020

cpmeister Jun 24, 2020

cpmeister Jun 24, 2020

Novanic commented Jun 25, 2020

cpmeister commented Jun 25, 2020

lolodomo commented Jun 26, 2020 •

edited

Loading

lolodomo left a comment

lolodomo commented Jun 28, 2020

clinique left a comment

cpmeister left a comment

[netatmo] Support for switching video surveillance on/off (#7938) #7968

[netatmo] Support for switching video surveillance on/off (#7938) #7968

Conversation

Novanic commented Jun 21, 2020 • edited Loading

TravisBuddy commented Jun 21, 2020

Travis tests were successful

lolodomo commented Jun 21, 2020 • edited Loading

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo Jun 21, 2020

Choose a reason for hiding this comment

Novanic Jun 21, 2020 • edited Loading

Choose a reason for hiding this comment

cpmeister Jun 24, 2020

Choose a reason for hiding this comment

lolodomo Jun 21, 2020

Choose a reason for hiding this comment

cpmeister Jun 24, 2020

Choose a reason for hiding this comment

Novanic commented Jun 21, 2020

lolodomo commented Jun 21, 2020 • edited Loading

lolodomo commented Jun 21, 2020

lolodomo commented Jun 21, 2020

Novanic commented Jun 21, 2020 • edited Loading

lolodomo commented Jun 21, 2020

Novanic commented Jun 21, 2020

TravisBuddy commented Jun 21, 2020

Travis tests were successful

lolodomo commented Jun 21, 2020 • edited Loading

lolodomo commented Jun 24, 2020

cpmeister Jun 24, 2020

Choose a reason for hiding this comment

cpmeister Jun 24, 2020

Choose a reason for hiding this comment

Novanic commented Jun 25, 2020

cpmeister commented Jun 25, 2020

lolodomo commented Jun 26, 2020 • edited Loading

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo commented Jun 28, 2020

clinique left a comment

Choose a reason for hiding this comment

cpmeister left a comment

Choose a reason for hiding this comment

Novanic commented Jun 21, 2020 •

edited

Loading

lolodomo commented Jun 21, 2020 •

edited

Loading

Novanic Jun 21, 2020 •

edited

Loading

lolodomo commented Jun 21, 2020 •

edited

Loading

Novanic commented Jun 21, 2020 •

edited

Loading

lolodomo commented Jun 21, 2020 •

edited

Loading

lolodomo commented Jun 26, 2020 •

edited

Loading