[alarmdecoder] Initial contribution

[bobadair]

This PR is for a new OH2 version of the OH1 alarmdecoder binding. It has been developed jointly with @billfor.

The original OH1 version of this binding was contributed by @berndpfrommer. A significant amount of code has been retained from the original binding, although it may not be immediately obvious due to the extensive changes and restructuring required to fit the OH2 binding model and to meet current code requirements.

The binding is fully functional, but has been through only limited testing.

Closes #7081

[cpmeister]

Make sure to remove '[WIP]' from the title if you want reviewers to review.

[bobadair]

Removed WIP from the title. We're still looking for more testers, but I think this is far enough along to get the review process started.

[bobadair]

Ok. I think that completes the list of high-priority changes, and eliminates all of the "TODO" comments in the code. Unless the testers find bugs that need to be fixed, I will now stop pushing commits until the initial review has been done.

One dumb question: Since I've now re-written the serial bridge handler to use the OHC serial transport rather than using gnu.io directly, can I remove the <feature>openhab-transport-serial</feature> line from the feature.xml file?

[bobadair]

Thanks for your comments @cpmeister, and for your very speedy initial review!
I just pushed a commit that I think addresses most of your comments. A few that I had questions about I commented on above.

[cpmeister]

can I remove the openhab-transport-serial line from the feature.xml file?

Nope, you need to keep that

[J-N-K]

I have a general question regarding the discovery. If I understand it right there is some sort of message (RFX) received by the bridge (Where does this message come from? Is it triggered by something or sent by the device at some interval?) Which is then passed to the discovery service. It is not possible to trigger a scan for devices and the discovery service uses only the content of the message and the brigde-UID to create the discovered things, right?

Then why do you need a seprerate service for each bridge? You could create one service, inject that into the handler factory and pass it to the bridge handlers on creation. Then - when the proper message is received you pass that message and the bridge-uid to the service. You could then remove the whole register/unregister logic from the handler factory and simplify the discovery service.

[J-N-K]

But why is this needed at all?

[bobadair]

I have a general question regarding the discovery. If I understand it right there is some sort of message (RFX) received by the bridge (Where does this message come from? Is it triggered by something or sent by the device at some interval?) Which is then passed to the discovery service. It is not possible to trigger a scan for devices and the discovery service uses only the content of the message and the brigde-UID to create the discovered things, right?

Yes, exactly. There is, unfortunately, no way to query the AD device for zone information, so when REL or EXP messages (for zone things) or RFX messages (for rfzone things) are received by the bridge handler, it passes them along to the discovery code. These all indicate state transitions for different types of zones. So, for example, when you first open a door or trigger a motion detector it would be discovered. It keeps hash sets of zones it has seen so it only "discovers" them once (per run, anyway). It is all just passive listening. There also does not appear to be any good way to discover the AD device itself.

Then why do you need a seprerate service for each bridge? You could create one service, inject that into the handler factory and pass it to the bridge handlers on creation. Then - when the proper message is received you pass that message and the bridge-uid to the service. You could then remove the whole register/unregister logic from the handler factory and simplify the discovery service.

You're right. It could be done using a single discovery service, and that would simplify the handler factory. The downside would be that it would complicate the discovery code - not so much because it would have to deal with zone sets for multiple bridge UIDs as because it would have to deal with concurrency. Right now it doesn't, as it can only be called by the bridge's single reader thread. Since I think it will be rare for users to run more than one bridge at a time, it may be best to let sleeping dogs lie for now. WDYT?

[TravisBuddy]

Travis tests were successful

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

[bobadair]

Sorry, I've been doing some work on the Lutron binding, and switching my dev environment back and forth to modify and test this one has been slowing me down. Not to mention driving me nuts. Or more nuts than I already was? Anyway, I think the commit I just pushed should address a lot of the remaining comments.

[TravisBuddy]

Travis tests were successful

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

[bobadair]

Ok @J-N-K. I think I've finally addressed all of the comments that I didn't have questions about. Can you take a look? Thanks.

[J-N-K]

I left one last comment.

Please rebase on 2.5.x and run mvn spotless:apply. We don't want to introduce new codestyle errors.

[J-N-K]

Did @berndpfrommer recently work on this binding? Since the CODEOWNERS is used to request reviewers, we should stick to those people that recenetly did major work on that code.

[bobadair]

He is the author of the V1 binding, which this is derived from. I know he said he was short of time to work on an OH2 version, so I'm not sure if he even wants to see review notifications regarding it. @berndpfrommer, do you want to be on this list?

[bobadair]

Please rebase on 2.5.x and run mvn spotless:apply. We don't want to introduce new codestyle errors.

I did a spotless:apply and committed the changes on April 22. Does it need to be run again?

[TravisBuddy]

Travis tests were successful

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

[J-N-K]

We did only change some XML formatters for comments. I guess that is not a problem in this PR. I wanted to approve and merge and therefore looked at the report from Jenkins:

05:37:54 [INFO] --- sat-plugin:0.9.0:report (sat-all) @ org.openhab.binding.alarmdecoder ---
05:37:54 [INFO] Individual report appended to summary report.
05:37:55 [ERROR] Code Analysis Tool has found: 
05:37:55  1 error(s)! 
05:37:55  1 warning(s) 
05:37:55  2 info(s)
05:37:55 [WARNING] org.openhab.binding.alarmdecoder.internal.handler.ADBridgeHandler.java:[367]
05:37:55 Classes/Interfaces should be annotated with @NonNullByDefault
05:37:55 [ERROR] .binding.alarmdecoder/pom.xml:[0]
05:37:55 The pom version is different from the parent pom version

You can ignore the pom-error, 2.5.5-SNAPSHOT is the correct version. But can you fix the other warning (@NonNullByDefault for the bridge handler? Thanks.

[bobadair]

But can you fix the other warning (@NonNullByDefault for the bridge handler?

Thanks @J-N-K. I thought that code analyzer warning was invalid. It is on an exception class defined within ADBridgeHandler. When I annotate it with @NonNullByDefault I get the compiler warning "Nullness default is redundant with a default specified for the enclosing type ADBridgeHandler". What is the right way to get rid it? I thought I had read that SCA would no longer generate these warnings for inner classes.

[J-N-K]

Mhm. Probably that is fixed in SAT 0.10.0 which is only available for the OH3 branch.

[bobadair]

Thanks to everyone who tested this, commented on it, and reviewed the code!