[ipcamera] Add new channels for Dahua API-based doorphones #13313

mesetka · 2022-08-24T13:16:08Z

This commit is adding some new possibilities for Dahua API-based doorphones
exit button state
door contact state
accepted access card channel
door unlock channel
motion detection sensitivity level

And a few spaces cleaning for README.md.

Signed-off-by: mesetka yarkiyluch@gmail.com

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

Skinah · 2022-09-16T08:52:05Z

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

                        ipCameraHandler.setChannelState(CHANNEL_EXTERNAL_ALARM_INPUT2, OnOffType.OFF);
+                        ipCameraHandler.logger.debug("External alarm Dahua event, content={}", content);


Same here, IMHO remove the logging as the alarm is going OFF I really doubt the content is useful.

lolodomo · 2022-09-25T11:04:24Z

The PR is badly signed.

Maybe you can check @Skinah if your review comments were all considered.

Skinah · 2022-09-26T05:24:07Z

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

@@ -352,6 +437,16 @@ public void handleCommand(ChannelUID channelUID, Command command) {
                            .sendHttpGET("/cgi-bin/configManager.cgi?action=setConfig&MotionDetect[0].Enable=false");
                }
                return;
+            case CHANNEL_MOTION_DETECTION_LEVEL:
+                int motionlevel = Math.round(Float.valueOf(command.toString()));


This channel is a DIMMER so this line of code will fail if ON or OFF commands are sent which are valid for this channel type. You could use something like the following if (command instanceof DecimalType), but a better way would be not to turn into a String and then parse and round it if a less costly approach can be used.

Tests required to set actually usable Motion Detection Level limits. I've found out for my VTO that I do need levels from 1 to 4, and level 4 is extremely sensitive in my case. But my VTO is placed outdoors, and objects max distance from camera is something like 10 meters. Could you test those settings with your camera? If you'll find out the same results, item's type could be changed to type Number, limited from 1 to 4.

Sorry I do not own a VTO camera, is that a requirement to test?

Ofc no. Should work with any Dahua-like camera with motion detection.

Skinah · 2022-09-26T05:31:29Z

Thanks for the contribution. Before being happy with this I would like to discuss the extra channels and if they are needed, or if the lastEventData channel can be used? Please read this for the background... #11391

The change to using it is pretty easy..

ipCameraHandler.motionDetected(CHANNEL_DOOR_UNLOCK);
ipCameraHandler.noMotionDetected(CHANNEL_DOOR_UNLOCK);

The decision was made not to create hundreds of channels to expose the extra event data, but to offer the data in a single channel. The number of the card in my opinion falls into this category. If every piece of advanced data from an event was filtered into separate channels, this binding would have 200+ channels and be a pain to navigate and find the right channel.

We should discuss the above point FIRST and then I would like to go over the code and the readme a little more carefully, but it looks pretty good already.

mesetka · 2022-09-27T06:49:29Z

Thanks for the contribution. Before being happy with this I would like to discuss the extra channels and if they are needed, or if the lastEventData channel can be used? Please read this for the background... #11391
The decision was made not to create hundreds of channels to expose the extra event data, but to offer the data in a single channel. The number of the card in my opinion falls into this category. If every piece of advanced data from an event was filtered into separate channels, this binding would have 200+ channels and be a pain to navigate and find the right channel.

We should discuss the above point FIRST and then I would like to go over the code and the readme a little more carefully, but it looks pretty good already.

As a user - I always will prefer just to choose a right channel, than to look at lastEventData string trying to figure out, where are all my camera's advanced possibilities. Average ipcamera user never ever heard about json, has no way to get camera's API description, fail even at simple rules, not saying about those which will require parsing. lastEventData usage will need a creation of additional rule, a dummy item, resolve possible persistence problems - user will have to do all those things binding is intended to do manually. Not mentioning Openhab's rules are often rather limited and not allowing to do all those things, which are available inside bindings code.
In case of 200+ channels user will just have to find a channel once at camera's setup, read it's description, easily create a ready to use item, easily integrate that item into widget from community, or a simple UI created rule without writing even a line of code and live happily forever, never ever seeing those 200 channels, leaving JSON parsing to @Skinah =)
lastEventData is a VERY useful channel, but only for advanced user, who wants to use camera's advanced possibilities which aren't yet implemented into binding without waiting for next binding/Openhab release.

Skinah · 2022-09-28T00:15:34Z

I do agree with your comments, and I am looking for your feedback to discuss this with an open mind. Separate channels are what I prefer for the reasons you have listed, I also do not like a user scrolling through 200 channels that their model of camera does not support just to find the one which does do something... The other option would be to REMOVE the channels if the binding can ask the camera if they are supported or not with an API call? Is it possible to make an API call to see if the camera supports each channel, or is it possible to ask if the camera is a SIP capable model and then bulk remove them if the camera does not? If the approach to remove the channels were made, then you leave all the coding the way it is and only add a small amount, which could be done in this PR or a future one.

Once this is decided then I can review the readme and go over the code 1 more time.

mesetka · 2022-10-04T15:39:55Z

I do agree with your comments, and I am looking for your feedback to discuss this with an open mind. Separate channels are what I prefer for the reasons you have listed, I also do not like a user scrolling through 200 channels that their model of camera does not support just to find the one which does do something... The other option would be to REMOVE the channels if the binding can ask the camera if they are supported or not with an API call? Is it possible to make an API call to see if the camera supports each channel, or is it possible to ask if the camera is a SIP capable model and then bulk remove them if the camera does not? If the approach to remove the channels were made, then you leave all the coding the way it is and only add a small amount, which could be done in this PR or a future one.

Once this is decided then I can review the readme and go over the code 1 more time.

Check could be done by API call like http://%3Cserver%3E/cgi-bin/magicBox.cgi?action=getDeviceClass, for VTO the answer is class=VTO.
But I couldn't find docs about adding/deleting channels, just a few posts on community forum, including your own.

bundles/org.openhab.binding.ipcamera/README.md

bundles/org.openhab.binding.ipcamera/src/main/resources/OH-INF/thing/thing-types.xml

Skinah · 2022-10-10T04:23:56Z

Ok that is everything I can contribute to the review. As a summary:

I will look at adding the deleting of channels in the future and use your suggested url and method to determine if the camera is VTO capable or not.
I may need to do a quick look at other brands API documentation and see if they have ID cards and if they return a number or user name if you wish to keep as is, I think it is worth changing it... This is in regards to making the acceptedCardNumber channel more generic and re-usable by removing the NUMBER from the channel name.
Address the changes suggested in the review comments.

Thanks for the PR.

mesetka · 2022-10-10T13:25:44Z

I will look at adding the deleting of channels in the future and use your suggested url and method to determine if the camera is VTO capable or not.

I may need to do a quick look at other brands API documentation and see if they have ID cards and if they return a number or user name if you wish to keep as is, I think it is worth changing it... This is in regards to making the acceptedCardNumber channel more generic and re-usable by removing the NUMBER from the channel name.

Address the changes suggested in the review comments.

Maybe this should go farther than that - if you'll check miio binding, you can see, that Mr. Marcel is creating all channels dynamically. That's the only possible way for him due to all the variety of devices his binding should work with, maybe his approach is suitable for this binding's advanced channels.
There is a nuance in this channel, I use it to process card number by VTO itself instead of getting openHAB server into this, things do work a bit faster this way, however there is another message about mifare cards:
Code=DoorCard;action=Pulse;index=0;data={ "Check" : [ 0, 0 ], "Number" : "MIFARE CARD NUMBER", "Type" : 0 }
This one requires card number processing on openHAB itself, it's a bit slower to react on cards, but it's more universal - no need to program cards into VTO, some cards could open door only at defined time. It's for you to choose, which one must be implemented, or maybe both of them.

Skinah · 2022-10-24T09:25:26Z

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

+                int motionlevel = Math.round(Float.valueOf(command.toString()));
+
+                if (motionlevel == 0) {
+                    ipCameraHandler.sendHttpGET("/cgi-bin/configManager.cgi?action=setConfig&MotionDetect[0].Level=1");
+                } else {
+                    ipCameraHandler.sendHttpGET(
+                            "/cgi-bin/configManager.cgi?action=setConfig&MotionDetect[0].Level=" + motionlevel);
+                }


Suggested change

int motionlevel = Math.round(Float.valueOf(command.toString()));

if (motionlevel == 0) {

ipCameraHandler.sendHttpGET("/cgi-bin/configManager.cgi?action=setConfig&MotionDetect[0].Level=1");

} else {

ipCameraHandler.sendHttpGET(

"/cgi-bin/configManager.cgi?action=setConfig&MotionDetect[0].Level=" + motionlevel);

}

if (command instanceof PercentType) {

if (PercentType.ZERO.equals(command)) {

ipCameraHandler.sendHttpGET("/cgi-bin/configManager.cgi?action=setConfig&MotionDetect[0].Level=1");

}else{

ipCameraHandler.sendHttpGET(

"/cgi-bin/configManager.cgi?action=setConfig&MotionDetect[0].Level=" + ((PercentType) command).intValue());

}

}

Please test and use this code instead of yours, it should do the exact same result. This is much more CPU efficient and also will not create issues if someone does not send a slider value to the channel. PercentType is a BigDecimal internally, so it is very simple to get it as an Integer. If someone was to send a String command, my guess is your code would fail with an error of some kind.

Skinah · 2022-10-24T09:34:27Z

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

+                            ipCameraHandler.setChannelState(CHANNEL_DOOR_UNLOCK, OnOffType.ON);
+                        }
+                    }
+                    if (content.contains("\"Method\" : 5")) {


Should this be ELSE IF instead of IF? If it matches method1 do we need to keep checking?

Oh yeah, fixed.

It is not showing as changed in this PR, can you push the updated code or tell me if I am wrong, thanks.

Not pushed, as I'm a bit stuck with other issue - what if we'll just organize additional channels to make channels easier to use for binding's user?
<channel-group-type id="doorphoneProperties"> <label>Doorphone Properties</label> <description>This is a group of channels relating to doorphone properties</description> <category>Door</category> <channels> <channel id="acceptedCardNumber" typeId="acceptedCardNumber"/> <channel id="doorUnlock" typeId="doorUnlock"/> <channel id="doorContact" typeId="doorContact"/> <channel id="exitButton" typeId="exitButton"/> <channel id="motionDetectionLevel" typeId="motionDetectionLevel"/> </channels> </channel-group-type>
This way I will just implement all VTO's additional channels. There will be a plenty of them, but they will be at the bottom and categorized.

Skinah · 2022-10-24T09:35:12Z

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

+                        ipCameraHandler.setChannelState(CHANNEL_DOOR_UNLOCK, OnOffType.ON);
+                        ipCameraHandler.logger.debug("Door opened from button");
+                    }
+                    if (content.contains("\"Method\" : 4")) {


Should this be ELSE IF instead of IF? If it matches method1 or Method 5, do we need to keep checking?

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

lolodomo · 2022-11-05T12:16:33Z

@mesetka : any feedback?

Please also note that something is not well signed off.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lsiepel · 2024-08-23T19:55:20Z

As this PR was stale, i tried to revive it. @mesetka hope you are fine with it.

Solved the conflicts
Fixed (two) very minor readme table markup issues
Added thing type instructions

I think this is in a mergable state. Would be nice of @Skinah kan review.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lsiepel · 2024-10-06T20:30:16Z

Fixed another merge conflict.

Skinah · 2024-10-11T22:57:44Z

LGTM

lolodomo · 2024-10-15T19:29:31Z

I am going to review this PR but please fix the conflicting file.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

bundles/org.openhab.binding.ipcamera/README.md

bundles/org.openhab.binding.ipcamera/src/main/resources/OH-INF/update/instructions.xml

bundles/org.openhab.binding.ipcamera/src/main/resources/OH-INF/thing/thing-types.xml

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

wborn

The default translations are also missing.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lolodomo

LGTM, thank you

…3313) * Made all changes requested by codeowner, added additional useful channels, fixing changes before going to channels organizing into groups and dynamic channels deletion. Signed-off-by: mesetka <yarkiyluch@gmail.com> Co-authored-by: Matthew Skinner <matt@pcmus.com> Co-authored-by: Leo Siepel <leosiepel@gmail.com>

mesetka requested a review from Skinah as a code owner August 24, 2022 13:16

wborn added the enhancement An enhancement or new feature for an existing add-on label Aug 26, 2022

Skinah reviewed Sep 16, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Sep 16, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Sep 16, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Sep 16, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Sep 16, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Sep 16, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Sep 16, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Sep 16, 2022

View reviewed changes

Skinah reviewed Sep 26, 2022

View reviewed changes

mesetka closed this Sep 27, 2022

mesetka reopened this Sep 27, 2022

Skinah reviewed Oct 7, 2022

View reviewed changes

bundles/org.openhab.binding.ipcamera/README.md Outdated Show resolved Hide resolved

Skinah reviewed Oct 7, 2022

View reviewed changes

bundles/org.openhab.binding.ipcamera/README.md Outdated Show resolved Hide resolved

Skinah reviewed Oct 10, 2022

View reviewed changes

bundles/org.openhab.binding.ipcamera/src/main/resources/OH-INF/thing/thing-types.xml Outdated Show resolved Hide resolved

Skinah reviewed Oct 24, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Skinah reviewed Oct 24, 2022

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

lolodomo added the awaiting feedback Awaiting feedback from the pull request author label Nov 14, 2022

lsiepel added 2 commits August 23, 2024 21:39

Spotless after merge

4beabc5

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

Add thing upgrade instructions

57cb162

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lsiepel added rebuild Triggers Jenkins PR build and removed awaiting feedback Awaiting feedback from the pull request author rebuild Triggers Jenkins PR build labels Aug 23, 2024

lsiepel changed the title ~~[ipcamera] Add some new possibilities for Dahua API-based doorphones~~ [ipcamera] Add new channels for Dahua API-based doorphones Aug 23, 2024

lsiepel requested a review from jlaur September 8, 2024 20:54

Merge branch 'main' into pr/mesetka/13313

bb1b008

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lsiepel added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 29, 2024

Merge branch 'main' into pr/mesetka/13313

8448437

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lsiepel requested review from a team and removed request for jlaur October 11, 2024 12:57

Merge branch 'main' into pr/mesetka/13313

66022db

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lolodomo requested changes Oct 19, 2024

View reviewed changes

wborn reviewed Oct 19, 2024

View reviewed changes

Review comments

0ad36e8

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lolodomo reviewed Oct 22, 2024

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Adapt to OpenClosedType

9a84c9f

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lolodomo reviewed Oct 26, 2024

View reviewed changes

...enhab.binding.ipcamera/src/main/java/org/openhab/binding/ipcamera/internal/DahuaHandler.java Outdated Show resolved Hide resolved

Review comment.

d637f7c

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lolodomo approved these changes Oct 26, 2024

View reviewed changes

lolodomo merged commit fd4284a into openhab:main Oct 26, 2024
5 checks passed

lolodomo added this to the 4.3 milestone Oct 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ipcamera] Add new channels for Dahua API-based doorphones #13313

[ipcamera] Add new channels for Dahua API-based doorphones #13313

mesetka commented Aug 24, 2022

Skinah Sep 16, 2022 •

edited

Loading

lolodomo commented Sep 25, 2022

Skinah Sep 26, 2022

mesetka Oct 10, 2022

Skinah Oct 14, 2022

mesetka Oct 16, 2022

Skinah commented Sep 26, 2022

mesetka commented Sep 27, 2022 •

edited

Loading

Skinah commented Sep 28, 2022

mesetka commented Oct 4, 2022 •

edited

Loading

Skinah commented Oct 10, 2022

mesetka commented Oct 10, 2022

Skinah Oct 24, 2022 •

edited

Loading

Skinah Oct 24, 2022

mesetka Nov 19, 2022

Skinah Nov 26, 2022

mesetka Nov 26, 2022

Skinah Oct 24, 2022

lolodomo commented Nov 5, 2022 •

edited

Loading

lsiepel commented Aug 23, 2024 •

edited

Loading

lsiepel commented Oct 6, 2024

Skinah commented Oct 11, 2024

lolodomo commented Oct 15, 2024

wborn left a comment

lolodomo left a comment

		ipCameraHandler.setChannelState(CHANNEL_EXTERNAL_ALARM_INPUT2, OnOffType.OFF);
		ipCameraHandler.logger.debug("External alarm Dahua event, content={}", content);

[ipcamera] Add new channels for Dahua API-based doorphones #13313

[ipcamera] Add new channels for Dahua API-based doorphones #13313

Conversation

mesetka commented Aug 24, 2022

Skinah Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

lolodomo commented Sep 25, 2022

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

Skinah commented Sep 26, 2022

mesetka commented Sep 27, 2022 • edited Loading

Skinah commented Sep 28, 2022

mesetka commented Oct 4, 2022 • edited Loading

Skinah commented Oct 10, 2022

mesetka commented Oct 10, 2022

Skinah Oct 24, 2022 • edited Loading

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

lolodomo commented Nov 5, 2022 • edited Loading

lsiepel commented Aug 23, 2024 • edited Loading

lsiepel commented Oct 6, 2024

Skinah commented Oct 11, 2024

lolodomo commented Oct 15, 2024

wborn left a comment

Choose a reason for hiding this comment

lolodomo left a comment

Choose a reason for hiding this comment

Skinah Sep 16, 2022 •

edited

Loading

mesetka commented Sep 27, 2022 •

edited

Loading

mesetka commented Oct 4, 2022 •

edited

Loading

Skinah Oct 24, 2022 •

edited

Loading

lolodomo commented Nov 5, 2022 •

edited

Loading

lsiepel commented Aug 23, 2024 •

edited

Loading