[netatmo] Add support for Presence camera events (#3059)

[Novanic]

This pull-request adds support for the events of the Netatmo Presence camera.

It ...

• uses the updated Netatmo API (to support the sub-event structure which is used by the Presence events)
• delivers all information of the Presence events to the home thing (the message and snapshot is now available/set)
• adds new channels to the home thing to report if a human, animal or vehicle was detected
• simplifies the process of updating the Netatmo API (the retrofit code generation is now embedded within the Maven build)

Please see issue 3059 for more information: #3059

[clinique]

@Novanic : I really like the update of the pom regarding swagger generation. Nice shot.
Would you mind by the way starting the refactoring of the NAWelcomeHandler with @NonNullByDefault ?

[Novanic]

Would you mind by the way starting the refactoring of the NAWelcomeHandler with @NonNullByDefault ?
I don't know what you mean? Did we talk about it before? ;-) And what is the meaning of NonNullByDefault?

[Owlbertz]

Hi @Novanic,
I am currently working on making use of your last PR regarding this topic. However, I am not able to get my Presence camera working with openHAB, as I am not sure about the correct Things configuration.
Would you mind enhancing the existing documentation, especially the Things configuration example, to include more details reagarding the Presence camera?

[Novanic]

Hi @Novanic,
I am currently working on making use of your last PR regarding this topic. However, I am not able to get my Presence camera working with openHAB, as I am not sure about the correct Things configuration.
Would you mind enhancing the existing documentation, especially the Things configuration example, to include more details reagarding the Presence camera?

Hm, interesting, this is just missing in the README, for the welcome camera too. Just use PaperUI, there the things are discovered automatically after the Netatmo API thing is created. ;-) Maybe we can extract it from there (to document it)?

[Novanic]

Hi @Novanic,
I am currently working on making use of your last PR regarding this topic. However, I am not able to get my Presence camera working with openHAB, as I am not sure about the correct Things configuration.
Would you mind enhancing the existing documentation, especially the Things configuration example, to include more details reagarding the Presence camera?

Ok, I got it working now and have now updated the readme with this pull-request. Here is my example. The home thing is also required and the IDs from Netatmo are important, maybe that was missing/broken in your configuration. To use PaperUI instead is really simple, just enter the credentials for the Netatmo API and everything is found and configured. ;-)

Bridge netatmo:netatmoapi:home [ clientId="<CLIENTID>", clientSecret="<CLIENTSECRET>", username = "<USERNAME>", password = "<PASSWORD>", readStation=false, readHealthyHomeCoach=false, readThermostat=false, readWelcome=false, readPresence=true] { Thing NAWelcomeHome home [ id="<HOME-ID-FROM-NETATMO>", refreshInterval=600000 ] Thing NOC presenceOutdoorCamera [ id="CAMERA-ID-FROM-NETATMO", parentId="<HOME-ID-FROM-NETATMO>" ] }

[clinique]

Would you mind by the way starting the refactoring of the NAWelcomeHandler with @NonNullByDefault ?
I don't know what you mean? Did we talk about it before? ;-) And what is the meaning of NonNullByDefault?

Put @NonNullByDefaultjust before the class declaration, and discover a whole new world :-)

Example here

[Novanic]

Would you mind by the way starting the refactoring of the NAWelcomeHandler with @NonNullByDefault ?
I don't know what you mean? Did we talk about it before? ;-) And what is the meaning of NonNullByDefault?

Put @NonNullByDefaultjust before the class declaration, and discover a whole new world :-)

Example here

Wow, are you sure that it is a good approach? ;-) I think it makes it more unreadable and it's not necessary at some places (but the IDE gives a warning when the annotation isn't present). Example:
@Nullable String url = findSnapshotURL(); if(url != null) { return HttpUtil.downloadImage(url); }
It was/is already clarified that "url" can be NULL, NULL is checked directly after the Nullable declaration... ;-)

[clinique]

Wow, are you sure that it is a good approach? ;-) I think it makes it more unreadable and it's not necessary at some places (but the IDE gives a warning when the annotation isn't present).

All the currently PR merge takes this approach, the whole OH core is underconvertion on this.

[lolodomo]

Normally no new PR is accepted without null annotations.
There is of course a tolerance when updating a class created before null annotations were a requirement.

[cweitkamp]

@Novanic Thanks for your contribution. This is a really nice example for using a Trigger Channel instead of three separate State Channels. You could emit a special event for each entity detected by the Netatmo Camera. Trigger Channels can be nicely combined with a specific binding Profile or used directly as Channel-Based Trigger in a Rule. Wdyt?

<channel-type id="camara-event">
    <kind>trigger</kind>
    <label>Camara Event</label>
    <event>
        <options>
            <option value="ANIMAL_DETECTED">Animal Detected</option>
            <option value="HUMAN_DETECTED">Human Detected</option>
            <option value="VEHICLE_DETECTED">Vehicle Detected</option>
        </options>
    </event>
</channel-type>

[Novanic]

NonNullByDefault is now used

Update: OMG, it breaks the build without showing issues within the IDE. How terrible is that...?! I have now solved it, but I think the "feature" is also buggy. See method NAWelcomeHomeHandler#findFirstSubEvent(...). I had to make this method to be NULL-safe, but it is never called with NULL...

[clinique]

@Novanic : I think @cweitkamp remark regarding event channels is very valid and make a cleaner interface.

[Novanic]

Ok, I will tryout the trigger channel. Is there anything more which I should change before the pull-request can get accepted and merged?

[Novanic]

Ok, I got the trigger channel now running. But it is not yet within the pull-request, because I don't know if it's not too confusing for the user. What is your opinion?

There are currently 2 solutions to provide event information:
1.) There are channels at the home thing level (one for each information of an event) and these are updated with a delay up to a few minutes but without the usage of a complicated and risky public webhook. Therefore I added the 3 channels to react on human, animal and vehicle and to match that concept to provide this missing data to make this concept usable for the Presence camera.
2.) There is already a "homeEvent" trigger channel to provide events via the public webhook. I will try to extend that for the Presence later (with another pull-request).

Now the new "cameraEvent" trigger channel will be introduced for concept 1 also at the home thing level. The user has to know that the homeEvent trigger channel has another concept of usage than the new cameraEvent trigger channel. The cameraEvent channel has to get used in combination with the other channels and the homeEvent channel has another trigger (webhook) and has nothing to do with the other channels of the home thing.

To add the cameraEvent channel only to the Presence thing wouldn't work. That wouldn't fit to concept 1 and all the other event information would be missing because the channels are on the home thing level.

I see 2 options:
1.) We use the 3 channels with concept 1, which is already within this pull-request. Then it is clear how to use these channels.
or 2.) I introduce the cameraEvent trigger channel and I document within the README that the cameraEvent channel is only useful in combination with the other home thing channels excepting the homeEvent channel. And I would realize that detected persons of the Welcome camera are also reported as a human to this cameraEvent channel so that this channel is usable for all camera models. One problem with this solution is that we can not simply process "lastEvent" anymore which is done with updating the channels, it is now required to check if the lastEvent is new, because we don't want to trigger the channel multiple times... To update channels with the same value is not a problem, but triggering channels multiple times is a problem / isn't expected...

Which solution are you prefering? Or do you have other ideas? (but I would not make it too compilicated, I hope that the pull-request can get applied for OpenHAB 2.5.6 to complete the support of the Presence camera which has begun with 2.5.6) ;-) Thank you in advance.

[Novanic]

I have now updated the pull-request to use the suggested cameraEvent trigger channel instead of 3 separate channels for human, animal and vehicle detected. New is also:

• Detected persons of a Welcome / indoor camera are also reported as a human to this new channel. That makes this cameraEvent trigger channel solution compatible with all cameras.
• Unspecified movements which are also reported by the Presence camera can also get handled via this trigger channel (there is a now a fourth event option named "MOVEMENT_DETECTED").

Please check if you like this solution more than the 3 channels and if the pull-request can get successfully reviewed and merged soon.

[TravisBuddy]

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.

[TravisBuddy]

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.

[cpmeister]

It was not my intention to set you down a painful road of refactoring, I only mentioned dealing with the warnings because you decided to add @NonNullByDefault to the top of the handler class.
I guess I misunderstood your intent when you added that annotation as an indication that you wanted to update the code to modern binding coding guidelines. I would have been perfectly ok with you not adding that annotation since you were only modifying the code of an existing binding. But I guess we are past the point of going back so lets progress forward.

I really appreciate you taking the time to deal with the warnings even if it wasn't what you intended to do.

Also wrt addressing most of the "false positive" warnings the easiest solution is often to cache fields to local variables and perform all the logic on the local variables instead. The reason that you see these "false positives" is that the null checker is trying to prevent NPE from occurring in a concurrent environments where the field values might be changed by another thread during execution.

Anyway, other than these last few changes I think everything looks good.

[Novanic]

@cpmeister It was requested as a review result within this pull-request "Would you mind by the way starting the refactoring of the NAWelcomeHandler with @NonNullByDefault ?" I think I should have answered with just "No" / "No, I don't start it"... ;-)

[TravisBuddy]

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.

[cpmeister]

@clinique Is there anything else you want changed on this PR?

[clinique]

LGTM, thanks @Novanic

[Novanic]

Yeah, thank you for your review and merge. :-) Now it's done for 2.5.6.

@clinique @cpmeister @lolodomo @cpmeister Is someone of you an employee of Netatmo or has a good connection to Netatmo? I tried to contact Netatmo several times but got no answer... For example it would be interesting if it is allowed (for this project) to use inofficial API endpoints. That would allow to switch the floodlight of the Presence camera or start and stop the video recording.

[clinique]

@Novanic : unfortunately no, I have no connection with Netatmo - but if you've identified unofficial API endpoints, nothing prevents you to use them. The only issue may be that unofficial endpoints may change without notification.