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

[mesetka]

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 [email protected]

[lolodomo]

The PR is badly signed.

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

[Skinah]

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... https://github.com/openhab/openhab-addons/issues/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]

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]

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]

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.

[Skinah]

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

1. 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.
2. 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.
3. Address the changes suggested in the review comments.

Thanks for the PR.

[mesetka]

1. 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.
2. 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.
3. Address the changes suggested in the review comments.

1. 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.
2. 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.

[lolodomo]

@mesetka : any feedback?

Please also note that something is not well signed off.

[lolodomo]

A conflict with README has to be resolved.

@Skinah : any chance you have time to review the last changes quickly ?

[lsiepel]

Please also run mvn i18n:generate-default-translations

[jlaur]

Please also run mvn i18n:generate-default-translations

And please also provide update instructions for the added channels - see https://github.com/openhab/openhab-docs/blob/088e0929d467d23014af613c1b3ced389c8ab0b9/developers/bindings/thing-xml.md#updating-thing-types

[Skinah]

@lolodomo All my comments seem to be addressed. Have been away a lot recently.

[lsiepel]

@mesetka are you able to continue on this PR and fix the i18n file and upgrade instructions?

[Skinah]

It is now more then a year ago since the last comment or changes made to the PR. Please feel free to remove the tags when you do so someone starts looking at this again. Would be great to have this completed when you have the time.