[OmniLink] Binding Initial Contribution

[ecdye]

Initial contribution / migration from OH1 of the OmniLink Binding. Ported with permission from @craigham and @boc-tothefuture who were the primary developers of this code.

This was created to add support to the main OH code-base for the OmniLink protocol to allow for more usage of it's abilities.

Signed-off-by: Ethan Dye [email protected]

[digitaldan]

Hi @ecdye to be clear, you are submitting the work that @boc-tothefuture, @craigham, and myself had worked on as part of the OH2/3 port that we never got across the line for review (my guess is there will be a lot of review issues). Are you volunteering to maintain this code going forward and update it as necessary, both when this gets reviewed and after it gets published? Have you contributed any code to this binding yet other than some cosmetic logging changes? If you are planning on maintaining and improving it that would be awesome, but if not i would want our reviewers to know as there is a lot of code to review.

[digitaldan]

I also noticed you forked my jomnilink project , made a small change to use a newer java version, then repointed this binding to your own version instead of submitting a PR on my project? any reason why? Would be nice to update the main one which gets pushed to maven central and used by other people.

[ecdye]

Hi @ecdye to be clear, you are submitting the work that @boc-tothefuture, @craigham, and myself had worked on as part of the OH2/3 port that we never got across the line for review (my guess is there will be a lot of review issues).

Correct.

Are you volunteering to maintain this code going forward and update it as necessary, both when this gets reviewed and after it gets published?

Yes, for the foreseeable future.

Have you contributed any code to this binding yet other than some cosmetic logging changes? If you are planning on maintaining and improving it that would be awesome, but if not i would want our reviewers to know as there is a lot of code to review.

Nothing major, I have rewritten small parts for OH3 and fixed broken support for certain 'Things' like the Console Thing. Additionally, I have rewritten parts of the code as necessary in order to incorporate null annotations. I do not know how many new features I would add but I would certainly like to improve the current ones and make the binding more user friendly overall.

[ecdye]

I also noticed you forked my jomnilink project , made a small change to use a newer java version, then repointed this binding to your own version instead of submitting a PR on my project? any reason why? Would be nice to update the main one which gets pushed to maven central and used by other people.

I was simply testing a change that I intend to submit to your repo but haven't yet. The change was relating to the only outstanding issue in your jomnilink repo. I will make a PR for you to review and incorporate in the next few days.

[boc-tothefuture]

@ecdye I think it is important to note @digitaldan contributions to this binding and the openhab library. It wasn't just @craigham and myself.. I am also willing to support this binding going forward along with you. I just didn't have the time to put into the documentation to get it ready.

[digitaldan]

Yes, for the foreseeable future.

Sounds great, happy to see someone pushing this across the line, although its a pity Leviton discontinued the Omni line of panels. Awesome that @boc-tothefuture volunteered to help, I'm available if you get stuck as well, although I just ordered a konnected.io system and plan to play around with that + noonlight for monitoring in leu of my omni, but thats a little ways off from being proved out, so my omni is still in use for the foreseeable future.

[boc-tothefuture]

I would like to see @craigham sign off on this as well.. As it is his original work we all iterated off of. I would also think that the three of us (@digitaldan, @craigham and myself) would need to officially sign this work since we wrote the majority of it?

[craigham]

I have no problem handing this off to someone that can try and get this accepted. Lately I have become more of a user of openhab rather than helping develop bindings.

I do have 1 question. I have worked on several bindings in the past and it was nice to have each in a different branch within my addons fork. This PR looks to be merging to main. Is this necessary, or could it be done on a more specifically named branch?

What do you think @boc-tothefuture, @digitaldan ?

[boc-tothefuture]

I have no problem handing this off to someone that can try and get this accepted. Lately I have become more of a user of openhab rather than helping develop bindings.

I do have 1 question. I have worked on several bindings in the past and it was nice to have each in a different branch within my addons fork. This PR looks to be merging to main. Is this necessary, or could it be done on a more specifically named branch?

What do you think @boc-tothefuture, @digitaldan ?

I think it goes to main when we PR it for official review.

[ecdye]

@ecdye I think it is important to note @digitaldan contributions to this binding and the openhab library. It wasn't just @craigham and myself.. I am also willing to support this binding going forward along with you. I just didn't have the time to put into the documentation to get it ready.

I agree, it would be important for @digitaldan to sign it off, I am merely the one who is facilitating the review and submission of this.

I do have 1 question. I have worked on several bindings in the past and it was nice to have each in a different branch within my addons fork. This PR looks to be merging to main. Is this necessary, or could it be done on a more specifically named branch?

The main branch is used to facilitate the central deployment and development of bindings, in my branch of the openhab-addons repo, it is being developed on a separate branch.

[craigham]

ah, i see. it's going getting submitted now to openhab for review. great, that would be nice. If there is something I can help with, I will. I've given up keeping an up to date dev environment for openhab, but I could get it going again if i can help. Is there a formal way I need to sign off on any of the code? Would be really nice to see the binding approved...

…

On Mon, 2 Nov 2020 at 09:10, boc-tothefuture ***@***.***> wrote: I have no problem handing this off to someone that can try and get this accepted. Lately I have become more of a user of openhab rather than helping develop bindings. I do have 1 question. I have worked on several bindings in the past and it was nice to have each in a different branch within my addons fork. This PR looks to be merging to main. Is this necessary, or could it be done on a more specifically named branch? What do you think @boc-tothefuture <https://github.com/boc-tothefuture>, @digitaldan <https://github.com/digitaldan> ? I think it goes to main when we PR it for official review. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8922 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACXGUXQCRFCRCHHZY7L5CHDSN3RZ5ANCNFSM4TGIOUMQ> .

[ecdye]

I have now linked everyone who helped in the creation of this binding as co-authors in the commit.

[fwolter]

I reviewed 17 / 55 files that you get an impression what needs to be done.

Is the library jomnilink used by other projects than OH? How many active maintainers does it have?

[ecdye]

@fwolter Thanks for the suggestions, I have made many uncommitted improvements as well and will add these concerns into my changes. jomnilink is not used by any other OH addons however it is essential to communicating with the OmniPro or Lumina controller, it is maintained by primarily @digitaldan with minor contributions from myself and @boc-tothefuture. I currently have an outstanding PR on the jomnilink repo that will provide the solution for many of the uncomitted changes that I mentioned.

Overall, once completed, with your suggestions, I feel that this PR will be much closer to a finished product.

[swamiller]

@ecdye I’m starting to dabble with OH3. If you need a tester, I’d be happy to help.

[ecdye]

That would be great, mostly I going by what I know works because I use it. Right now I'm just waiting for @digitaldan to finish getting a PR for jomnilink so I can push a bunch of new changes.

[randye007]

Thanks so much @ecdye and @digitaldan for reviving the jomnilink binding for OH3. Is there a date you are working towards to getting this released?

Cheers,
Randy

[ecdye]

Not right now, still waiting on @digitaldan but he is busy, I don't see any need to rush.

[fwolter]

Your code doesn't compile.
OmnilinkHandlerFactory.java:[37,8] The import org.openhab.binding.omnilink.internal.handler.units cannot be resolved among others

[fwolter]

LGTM

[ecdye]

@fwolter I'm not sure how to fix it, but since I changed the Discovery Service implementation I can no longer scan to discover new items in the openHAB UI any ideas on how to fix this?

[digitaldan]

@ecdye its hard for others to help if you force push every change as we can not see the change history, so it's not clear what the discovery looked like before and after. I'm guessing you are relying on the bridge handler now being passed in by the framework, have you added logging to make sure this is happening, can you narrow down if anything is being called in that class?

[digitaldan]

Also i noticed this is in the logs when i start the binding:

- Could not register service for class=org.openhab.binding.omnilink.internal.discovery.OmnilinkDiscoveryService
java.lang.NoSuchMethodException: org.openhab.binding.omnilink.internal.discovery.OmnilinkDiscoveryService.<init>()

[ecdye]

I understand this, but what I am referring to is @fwolter directed me to change the Discovery Service implementation to use ThingHandlerService but since I implemented that I can no longer find the automatic discovery option in the openHAB UI.

Before the service was using a componenet annotation and was working properly. I simply can't find any documentation about this issue as the documentation provided by @fwolter does not indicate that anything else needs to be done for this to work.

[digitaldan]

but since I implemented that I can no longer find the automatic discovery option in the openHAB UI.

Did you see the error message i posted above ^^, there is an exception being thrown when it tries to register that class, its telling you
java.lang.NoSuchMethodException: org.openhab.binding.omnilink.internal.discovery.OmnilinkDiscoveryService.<init>() ,
this means the class does not have an empty (no params) constructor, it needs a public OmnilinkDiscoveryService(){...} constructor.

[ecdye]

but since I implemented that I can no longer find the automatic discovery option in the openHAB UI.

Did you see the error message i posted above ^^, there is an exception being thrown when it tries to register that class, its telling you
java.lang.NoSuchMethodException: org.openhab.binding.omnilink.internal.discovery.OmnilinkDiscoveryService.<init>() ,
this means the class does not have an empty (no params) constructor, it needs a public OmnilinkDiscoveryService(){...} constructor.

Apparently I cannot read, testing now.

[randye007]

@ecdye - Thanks so much for your work on this binding port! I can confirm that I can successfully discover my OMNI items and that so far all functionality is working well. Will let you know if I run into any issues.

[ecdye]

@J-N-K Any chance you could help get this in before the next release?

[J-N-K]

Thanks. I have left some comments inside. Please make sure to go through all occurences of the points I mentioned (regarding REFRESH handlig, channelLinked handling, logging, datatypes - DecimalType vs. QuantityType).

[ecdye]

@J-N-K I have updated the files to all use REFRESH would you take a look and see what you think?

[ecdye]

@J-N-K Any chance for a quick finish up of this PR?

[J-N-K]

Last comment from my side.

[ecdye]

@J-N-K Done. Can you approve and merge?

[J-N-K]

Just waiting for Jenkins.

[J-N-K]

What changed with your last push? If you force push the changes can‘t be shown.

PR changed after approval.

[ecdye]

Hmm, nothing changed... I updated for upstream change to allow PR to build correctly as @fwolter described.

[J-N-K]

Ok

[boc-tothefuture]

As it appears we are close to merging this I want to thank @ecdye for pushing this forward as a PR and @J-N-K for his devotion to ensuring there is a maintainable code base that contains best practices.

👏

[ecdye]

All tests are good. I think we are ready.