To implement notification control for SS messages

b-r-y commented 5 years ago

Hi @Pavion ,

This is to implement fine control over notifications the synology DS offers. The specific API is SYNO.SurveillanceStation.Notification.Filter. It allows to turn on/off notifications for specific events or means of notification. It is not a camera based API so the implementation should be on the bridge thing. The user used should be part of the administrator group on the Diskstation - it is NOT enough to be SS manager.

I started already on this and I would like to try on my own first but your review and help when i get stuck will be most welcome. I started from your mdparam branch. Let's see how far i can get... :)

Pavion commented 5 years ago

If you haven't started yet, I could merge the mdparam branch today evening so you'd get a clean source. From there you can fork the entire binding and make pull requests, whenever you've implemented something worth implementing. I'm not that good in Git and I don't really know what happens, if you'd make a pull request on an already merged branch. By the way, what is your native language? As much as I'm enjoying writing English, I'm somewhat slower with it :)

b-r-y commented 5 years ago

Hi @Pavion ,

Yes please - i would gladly wait for the merge. I also don't know what happens if i branch from a merged branch... I did do few things but only few and i can easily port those.

My native language is Bulgarian so i doubt we can switch to that and I only barely know some german ...

I will wait for the merge and continue. To that end let me ask something. I got to the point where i had described the new channels in the bridge.xml and added some constants to the java file. I somehow expected that this would at least show the channels in paperUI. It didn't. What is the minimum implementation required to start seeing those? Do i need to have them listed as options int eh handleCommand method?

Pavion commented 5 years ago

Hi there!

the branch is merged!
I can some Russian too but we'll better stick to English then 🙂
For a channel definition you just have to adjust bridge.xml, for example, to duplicate SID-channel:
```
        <channel id="sid2" typeId="sid-channel" />
```
But: You must recreate the corresponding Thing for channel changes to take effect or use text file configuration instead!

Pavion commented 5 years ago

By the way, as far I understand Git: you should fork this repository first (see Fork button at the top right side). Forked repo will then be shown in your profile as your own repo. From there you can make and commit even small changes and I can trace your progress too 👀. As soon as you've reached a final stand, you'd make a pull request (PR) and your fork could then be merged.

b-r-y commented 5 years ago

Hi @Pavion ,

slow start - please be patient :). I updated the XMLs and i finally managed to make it show up the changes... (i had missed the top of the file declaration). I re-orged the bridge channels to resemble the camera with groups. I could not figure out how to mix channels with channel groups...

Next step will be to figure out which thread to use or make a new one. Tips are welcome :).

Cheers! Boyan

Pavion commented 5 years ago

If you're going for the bridge, you have to take the bridge's only thread 'HomeMode' as example.

But frankly I'm unsure, why would someone want to tamper with these settings from within openHAB? Are you that sure, that disabling a notification is a best way to handle your problem? What is actually your problem you are trying to solve? Is there perhaps another way?

Note, to implement this API properly you'd actually need to create new Number channels for every eventType (0 to at least 83), but this would clutter the binding.

Afterthought: A cheap solution would be:

no getter (thus no threads or polling)
just one String channel, which could be set with e.g. 2=1 for please inform me if a camera was deleted (2) per E-Mail(1), returning OK if successful.
This String can then just be sent to the API as stated in the manual: GET /webapi/entry.cgi? 2=1&api="SYNO.SurveillanceStation.Notification.Filter"&version="1"&method="Set"
Its success response could then be passed back to the channel (TRUE || FALSE)

I look forward to your opinion (well, actually going to get some sleep first 🙂)

Pavion commented 5 years ago

Hi Boyan,

I had some time to review your work so far and would like to give some feedback. I admit your bridge channel definition is very nice and would be a good addition, but I also have to nitpick some 🙂 Please check my inline comments in your latest commit: https://github.com/b-r-y/synologysurveillancestation/commit/089bc89e211c6a75b2a7c9b40c109b8ee91a4d78

Greets Pav

b-r-y commented 5 years ago

Hi @Pavion ,

Let me address the motivation of this change. What i want to achieve is to have camera and motion detection on at all times but i want to avoid notifications when I am home.

A second big motivation for me is get started learning to do bindings because it is fun and the best way to learn is to do something marginally useful :).

Now how to implement that is a different story. I was going for full API implementation similar to the way you did the MD-param. Since i don't fully understand the implications of a thread/getter yet i don't know how big of a task this is or how cluttered the binding will get. I would like however to know my notification state and set it. To me this sounds like a thread...

Let me know what you think... may be there is a way to track the state in openhab itself based on the last sent command.

Pavion commented 5 years ago

Hi there!

I've now updated the headers so no errors are shown anymore

If you hadn't read that yet: by running mvn install -P check you can perform additional stylecheck, whose results you'll see in target/summary_report.html. There should be no errors except for AvoidScheduleAtFixedRateCheck. Note, /target folder must be removed prior to running this command, otherwise the report is buggy

Now to other things.. I can understand your motivation very well: I haven't studied coding and I doubt I've ever read a whole book on it, having learned mostly by trial and error and for fun 🙂

Your channel definition is fine, kept in a tidy group and is a nice addition, so I wrote a small guide for you 🎁

Implementing new API:

1. You just want to set your values and don't care which value they have now. After all, if I want notification X to become Y, why would I want to know which value it had before?

This is the shortest way and involves at least:

a) Adding XML description and constants (you've done it already) b) new SynoApi class for converting commands into API methods (setter) c) add this class to SynoWebApiHandler api and create an getApiXXX() d) handle your commands in SynoBridgeHandler (e.g. apiHandler.getApiXXX().setNotification(1);)

Note: values you've entered since binding start are kept by openHAB. If you'd never change those values in Surv., your latest value is a valid one [unless set failed!] Values you've not entered are undefined.

2. Second case: all above but you'd like know which values are valid at the binding start

You'll need additional:

a) new Response class for converting API response into an object with getter functions (see CameraEventResponse as newest tidy example) b) add a getter to your SynoApi class, returning new Response class c) call API and fill channels after Bridge Thing initalization and / or after reconnect (note: using REFRESH as in CameraHandler would be bad in this case)

As long as you don't make changes in Surv. your values are now always valid [unless set failed!]

3. Last case: you'd want all above but you also want to know, whether a value has been changed in Surv. or any of your set commands was rejected

You'll need additional:

a) new SynoApiThread (see SynoApiThreadCameraEvent as latest tidy reference and SynoApiThreadHomeMode for bridge related issues). This thread should call API and fill channels. b) handle it in BridgeHandler (put / change refresh rate) c) as in the 2c) but you just have to call runOnce() of this thread, simplifying call

As this is your preferred method, some additional notes on refresh

Using refresh rate of 0 for this thread makes 3) basically the same as 2), as done with MDParam. You can use existing refreshRateEvents to begin with but it's actually too fast for notifications. A new refreshRateNotifications (defaults to 0 or some larger number) should be created and used instead (incl. changes in XML / config files). Here I could help if needed.

wbr Pav

b-r-y commented 5 years ago

Thanks a lot as always @Pavion !!! I pushed a new commit with only partial implementation of the first step. I checked and it works! :) I will finish the rest over the weekend and hopefully next week i can do steps 2 and 3. I've been rather busy this week...

Please check if i am on the right track.

P.S. Will you be publishing this as an official OH binding? I mean... out of the box one...

Cheers, Boyan

Pavion commented 5 years ago

Hi Boyan. It looks really good so far, so keep going!

As for an official distribution, there are some issues to be considered. For this project to become official, it must be integrated into /openhab-addons. As I understand it, this would mean at least following:

integration itself would mean, that this repository have to be abandoned (?);
as this is not my own repository, I can't decide it alone. Although it's been months since Nibi79's last commit, it must be discussed with him first;
anyway, as openHAB guidelines state, all authoring must be done with real names (so Nibi's, mine and also yours) and I kinda like my nickname 🙂
integration would also mean, that the development cycles are longer: as of now, a user knowing this binding have to download and replace the binding's JAR in case of new features or fixes. If integrated, a user must wait for a new milestone or RC of openHAB (yes, it'd be possible to use JARs still, but then the integration was unneeded in the first place)
there is no auto update feature in openHAB (yet?), that means, each commit to master have to be stable and final: imagine me making a breaking change just before openHAB RC is compiled and distributed.
the last but not least: official distribution would mean a sudden increase of bug reporting and feature requests, taking time and nerve to handle.

See, being not official do have its benefits. After all, this is also a question of expectations:

If you have to search for this binding in internet, download and install a JAR from an unknown source, you have to have other technical understanding and expectations as if you just clicked on it in a PaperUI...

As always, i'd like to hear your thoughts on it 🙂

Pav

b-r-y commented 5 years ago

Hi @Pavion ,

I pushed one more commit to finish with the setter functionality related to the notification filter and camera event types. The binding does not support it seems to me other devices so no need for the rest of the notifications either... or do you think i should implement for completeness?

I also rolled back the camera.xml which for some reason i missed in my initial revert...

I don't know the details on why or how to make the binding official. I just prefer the ease of installation via paper UI and i thought it could be nice.

To the finer points: I think making it official gives you more access to testers (which i do see as positive). It is still open source and voluntary - no one can force you or me or anyone to work on something unless you want to so more tickets i good but no one says you have to address every ticket right away or even at all. I also don't quite see who can force you to close this repository - again it's all open source. If we want shorted dev cycles we can always do what we do now. For the names - i don't care - i am ok giving my full name. But ultimately i agree that since this is @nibi79 's project it is his/her call and yours since you are the most active developer.

Cheers, Boyan

Pavion commented 5 years ago

Hi Boyan,

I've checked your fork and it looks good.

note this quick link for comparing to master: https://github.com/nibi79/synologysurveillancestation/compare/master...b-r-y:notify_filter?expand=1

I assume, that you are going to rework or remove this function later:

public HomeModeResponse getHomeModeResponse()

Then I have but one issue: your .project file is still modified. Please revert your changes and avoid to commit this file if changed again.

The binding does not support it seems to me other devices so no need for the rest of the notifications either... or do you think i should implement for completeness?

What do you mean?

Now for being official. You're right with all you've said...

But for better understanding please check the official openhab2-addons repository: https://github.com/openhab/openhab2-addons/tree/master/addons/binding

Imagine now, that you have to work there, having no own issues, no own repository, no own releases but many people. I don't actually like it. If openHAB ever makes some kind of public plugin repository, where every developer could upload a new version as he/she prefers, which then would be distributed and updated automatically, I'll reconsider my position.

Please note, such repository already exists for the parent project Eclipse Smarthome and is found here: https://marketplace.eclipse.org/category/categories/eclipse-smarthome/popular

I'm keeping an eye on ESH and OH development, let's see, where it takes us 🙂

Pav

b-r-y commented 5 years ago

Hi @Pavion ,

Yes is will be doing the thread next and will fix that get part.

The notification filter API has hooks for digital IOs systems events and others. Since none of these are implemented (we cannot control digital IO devices with the binding at the moment) i figured controlling the notifications related to them is also not needed. There is the alternative approach that once we implement an API we complete it so we don't need to go back. I prefer to only implement the ones needed for the camera unless you suggest otherwise.

For the full OH integration i am fine with anything you decide. I have what i want anyway :)

Cheers, Boyan

Pavion commented 5 years ago

That's fine with me. You should only implement what you deem necessary and - should it be needed by others - let others participate 🙂

Pavion commented 2 years ago

purge old issues, please reopen if needed

nibi79 / synologysurveillancestation