Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[resol][WIP] Add Resol Controller Binding - Initial Contribution #8302

Closed
wants to merge 15 commits into from

Conversation

ramack
Copy link
Contributor

@ramack ramack commented Aug 16, 2020

Target of this PR is to contribute the Resol binding to openhab and close #7921 to support connection of devices from RESOL - Elektronische Regelungen GmbH, which are solar and heating controllers and heat count units. This includes support of many branded versions from Viessmann, SOLEX, COSMO, SOLTEX, DeDietrich and others.

The binding is already around and in production use since some years, discussed in the community forum and available in Eclipse IoT Marketplace. Now it is the time to clean it up and submit a PR into openhab.

@ramack ramack requested a review from a team as a code owner August 16, 2020 15:37
@TravisBuddy
Copy link

TravisBuddy commented Aug 16, 2020

Travis tests were successful

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

@fwolter fwolter added new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged labels Aug 16, 2020
@fwolter fwolter changed the title [WIP] [resol] Add Resol Controller Binding - Initial Contribution [resol][WIP] Add Resol Controller Binding - Initial Contribution Aug 16, 2020
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I peeked into your binding. Here is my feedback.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

There are some formatting issues. You can view them with mvn spotless:check -Dspotless.check.skip=false and fix them with mvn spotless:apply.

The JAR of the external library should not be part of this PR. It should be referenced in the pom.xml.

You need to sign your commits. See https://www.openhab.org/docs/developer/contributing.html#sign-your-work and https://community.openhab.org/t/dco-check-signing-off-with-github-web-editor-explanation/83330

@ramack
Copy link
Contributor Author

ramack commented Aug 16, 2020

I peeked into your binding. Here is my feedback.

Thanks a lot for the very fast reaction. I did not expect that, especially as I tagged it WIP, and was mainly hoping for the automatic feedback from travis&Jenkins. But for sure I appreciate every human comment!

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

This might also be due to working with an old upstream base. Is 2.5.x the correct branch to work with? - Is there already an ETA for the next release? And is the 2.5.x branch currently sufficiently stable? - So far I had always heavy issues after rebasing.

The JAR of the external library should not be part of this PR. It should be referenced in the pom.xml.
Can you point me to an example or tutorial on how to work with the external jar? - I was already happy to make it work like this since I Initially even had the Java code copied into the repo - which is probably neither want we want...

You need to sign your commits. See https://www.openhab.org/docs/developer/contributing.html#sign-your-work and https://community.openhab.org/t/dco-check-signing-off-with-github-web-editor-explanation/83330

Did this most of the time. - The only one where I forgot is the one adding the jar - where it is even the question whether it should be there :-)

Beside that there are also a few TODO comments and other things left open so you don't need to go into all details yet, but if you see further general things as above I'd be happy.

@fwolter
Copy link
Member

fwolter commented Aug 16, 2020

This might also be due to working with an old upstream base. Is 2.5.x the correct branch to work with? - Is there already an ETA for the next release? And is the 2.5.x branch currently sufficiently stable? - So far I had always heavy issues after rebasing.

The 2.5.x branch is the current stable branch. Committing to it is correct. The releases are mostly around the 20th day of a month.

The only one where I forgot is the one adding the jar - where it is even the question whether it should be there :-)

It should be signed-off, too, because you take responsibility, that the library may be used within this binding.

@ramack
Copy link
Contributor Author

ramack commented Aug 16, 2020

The only one where I forgot is the one adding the jar - where it is even the question whether it should be there :-)

It should be signed-off, too, because you take responsibility, that the library may be used within this binding.

Ah, my main question about the jar got lost in formatting above, so let me put it again:
Can you point me to an example or tutorial on how to work with the external jar? - I was already happy to make it work like this since I Initially even had the Java code copied into the repo - which is probably neither want we want...

@fwolter
Copy link
Member

fwolter commented Aug 16, 2020

Can you point me to an example or tutorial on how to work with the external jar? - I was already happy to make it work like this since I Initially even had the Java code copied into the repo - which is probably neither want we want...

You could take a look at this PR, which uses an external library. The library must be in a public maven repository.
https://github.com/openhab/openhab-addons/pull/6213/files#diff-17e0030112737d2670ec2efafde171dc

@TravisBuddy
Copy link

Travis tests have failed

Hey @ramack,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests were successful

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

@TravisBuddy
Copy link

Travis tests have failed

Hey @ramack,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@Hilbrand
Copy link
Member

Hilbrand commented Sep 7, 2020

When updating the version number you need to rebase (git pull --rebase) so the parent pom is also brought in sync. and then push force (git push --force-with-lease) back to GitHub.
When the version is updated you also need to update the openhab-distro project, which contains the demo project.

@ramack
Copy link
Contributor Author

ramack commented Sep 7, 2020

When updating the version number you need to rebase (git pull --rebase) so the parent pom is also brought in sync. and then push force (git push --force-with-lease) back to GitHub.
When the version is updated you also need to update the openhab-distro project, which contains the demo project.

This is more or less what I tried. I did git rebase upstream/2.5.x instead, but I would expect that this is the same. I did also a git pull in the openhab-core, which caused some 3.x errors (there is no 2.5.x branch in core) and I wanted to go back to some older status in core but all the things with mvn, bndtools and how it is integrated in eclipse is for a big mystery and the errors messages I get do not tell me anything. So always when I try something here I get in the mess of trying things where I cannot judge the consequences, such that I invest more time in the infrastructure than in working on the binding itself. This is really unsatisfactory. If you can point me to a clear doc which explains what I need to know about the toolchain to work with it in developing a binding it would help me a lot. I would like to contribute to binding development, but I don't have the time to understand all those details about the build system :-(

@ramack
Copy link
Contributor Author

ramack commented Sep 7, 2020

It es especially crazy, if a magic mvn command compiles the binding successfully but in eclipse I only get weird errors. So if you can recommend another way than using eclipse to debug the binding I'd potentially always become more efficient.

@Hilbrand
Copy link
Member

Hilbrand commented Sep 7, 2020

I did also a git pull in the openhab-core

Get rid of all the openhab-core projects in eclipse you don't need it when developing a binding. You only need to import the demo project (which is done when selecting openhab-development during eclipse setup) and your own binding.

@Hilbrand
Copy link
Member

Hilbrand commented Sep 7, 2020

Did you succeed in getting eclipse running? It should work out of the box with only the demo project and your binding and the instructions on the openHAB eclipse page.

@ramack
Copy link
Contributor Author

ramack commented Sep 7, 2020

Did you succeed in getting eclipse running? It should work out of the box with only the demo project and your binding and the instructions on the openHAB eclipse page.

Not yet. I removed the openhab-core projects and at least some of the errors are gone. But the pom.xml of my binding is still showing

Non-resolvable parent POM org.openhab:openhab-super-pom:[1.0, 2.0) for 
 org.openhab.addons:org.openhab.addons.reactor:2.5.9-SNAPSHOT: No versions matched the requested parent 
 version range '[1.0, 2.0)' and 'parent.relativePath' points at wrong local POM

And I am trying now to build the demo app on the command line.

@Hilbrand
Copy link
Member

Hilbrand commented Sep 7, 2020

What version of eclipse are you running. I had some issues with version 2020--06 that gaven an error on the parent element in the pom. I don't know why it does so, but I switched back to 2020-03.

@ramack
Copy link
Contributor Author

ramack commented Sep 7, 2020

What version of eclipse are you running. I had some issues with version 2020--06 that gaven an error on the parent element in the pom. I don't know why it does so, but I switched back to 2020-03.

Yes, I am on 2020-06, I'll try with 2020-03.

Also it seems that another show stopper is that eclipse doesn't find the external maven repository which I use for the external library vbus-java.

@Hilbrand
Copy link
Member

Hilbrand commented Sep 7, 2020

Ah ok, the eclipse verion might be a problem to. I haven't had time to look into it in detail and just hoped it's fixed when 202-09 is released.

About the external repository. I was reviewing your code, (but haven't yet finished, so you can't see it yet). But the repository must be one of the known repostories, like maven central. The problem is, we can't just add any random repository. Because if that repository is not available it will break the build and that is not something that should happen.

@ramack
Copy link
Contributor Author

ramack commented Sep 7, 2020

About the external repository. I was reviewing your code, (but haven't yet finished, so you can't see it yet). But the repository must be one of the known repostories, like maven central. The problem is, we can't just add any random repository. Because if that repository is not available it will break the build and that is not something that should happen.

ok. Can you help @danielwippermann and explain in the ticket danielwippermann/resol-vbus-java#17 how to do that?

@ramack
Copy link
Contributor Author

ramack commented Sep 7, 2020

Ah ok, the eclipse verion might be a problem to. I haven't had time to look into it in detail and just hoped it's fixed when 202-09 is released.

With 2020-03 I get:
Plugin execution not covered by lifecycle configuration: com.diffplug.spotless:spotless-maven-plugin:2.0.3:check (execution: codestyle_check, phase: initialize)

but this seems to be "ignorable" with the proposed quickfix. Now eclipse (2020-03) does not show any error anymore, but as soon as I add the binding in the pom of the demo app, the app.bndrun dialog does not show any repository anymore, so I cannot add the binding as run requirement.

I leave it for today, after hours of reinstalling eclipse and trying... - while command line compilation still works. Thanks for your help, but I am still lost :-(

@Hilbrand
Copy link
Member

Hilbrand commented Sep 7, 2020

I'll post information on what the easiest way to publish a jar is in relation to usage with openHAB.

The error about lifecyle can indeed be suppressed, it's a maven/eclipse thing that has no effect on working with eclipse.

If I have some time tommorrow I can check if i can import the binding in my eclipse environment.

@ramack
Copy link
Contributor Author

ramack commented Sep 8, 2020

I'll post information on what the easiest way to publish a jar is in relation to usage with openHAB.

Perfect, this would help!

I reached one further conclusion: without the external dependency to vbus-java lib in my binding the bndrun-dialog seems happy (but for sure some compile issues occur.) If I readd the dependency, the compile errors vanish but bndrun dialog shows a lot of issues, especially with the OSGI framework:

Bildschirmfoto von 2020-09-08 16-20-04

So the last issue I have after downgrading to eclipse 2020-03 might be the mvn dependency to the external library.

@Hilbrand
Copy link
Member

Hilbrand commented Sep 8, 2020

If you add the repository to the repositories in the demo app pom.xml it should resolve.

@ramack
Copy link
Contributor Author

ramack commented Sep 8, 2020

If you add the repository to the repositories in the demo app pom.xml it should resolve.

Yes, this did the trick. Thanks a lot! Will hopefully work on it again tomorrow!

@danielwippermann
Copy link

I've started the application process for including the "de.resol" group ID into the central repo. I'll keep you posted :)

https://issues.sonatype.org/browse/OSSRH-60500

@danielwippermann
Copy link

The library has been released to the central Maven repo.

https://repo1.maven.org/maven2/de/resol/vbus/

Please update your dependencies to use version "0.4.0".

ramack and others added 10 commits September 13, 2020 16:05
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
…n number

Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
@ramack
Copy link
Contributor Author

ramack commented Sep 13, 2020

I had the vbus_specification.vsf file in src/main/resources/de/resol/vbus/ since I used the library in src code, as I now switched to the usage of the maven dependency I expected to be able to remove the file (as it is included in the lib jar), but then loading the resource in the vbus-java library fails. @Hilbrand or @danielwippermann do you have an idea why and what I can do to remove the explicit vsf file instance from the binding?

Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
Signed-off-by: Raphael Mack <ramack@raphael-mack.de>
@openhab-bot
Copy link
Collaborator

Automatic code migration to openHAB 3 succeeded.

The resulting code can be found at https://ci.openhab.org/job/openHAB-Addons-Migration/97/artifact/bundles/.

You can download the migrated code from there and create a new PR against the master branch of the openhab-addons repo to contribute it for being included in openHAB 3.x.

Please see this issue about the details on how to proceed with your existing PR.

@wborn
Copy link
Member

wborn commented Dec 15, 2020

Hi @ramack! I ported your code to OH3 as some users would like to continue using the Resol Binding after their upgrade.
So when you continue your development for OH3, you can also have a look at or fork this resol branch.
I also made a org.openhab.binding.resol-3.0.0-SNAPSHOT.jar for those who want to use it with OH3.

@wborn wborn added the bounty Issues with a Bountysource reward and the PRs that solve these label Dec 15, 2020
@ramack
Copy link
Contributor Author

ramack commented Dec 15, 2020

@wborn thanks for your efforts. I have already started to work on that also here: https://github.com/ramack/openhab-addons/tree/Resol3 but your solutions look more clean. In both versions though I have trouble with some data points and for example the temperatures are not getting updated. Also I only see the String and DateTime channels in the Thing (in the new OH3 UI). So it seems that the channel update (or the channel creation) in the BridgeHandle doesn't properly work.

@wborn
Copy link
Member

wborn commented Dec 15, 2020

That's great that you've started working on an OH3 version! 👍 I only ran the script we use for porting add-ons on the branch of this PR and fixed a few compilation errors and warnings just so I was able to build a JAR. 🙂

@ramack
Copy link
Contributor Author

ramack commented Dec 15, 2020

That's great that you've started working on an OH3 version! +1 I only ran the script we use for porting add-ons on the branch of this PR and fixed a few compilation errors and warnings just so I was able to build a JAR. slightly_smiling_face

Ok, so we have to admit that it doesn't work to read the most interesting dataponts right now, but I am on track of it, looks like the unit handling the the used resol-vbus library improved and we get a real unit now for many of the data points, and in the thing-types.xml I mapped everything to the generic, magic type "None", as soon as I add there a type

    <channel-type id="DegreesCelsius">
        <item-type>Number:Temperature</item-type>
        <label>Temperature</label>
        <state readOnly="true"/>
    </channel-type>

I start to get interesting values.

But to be honest, I am not sure to do the right thing here in the thing-types, can you give me some advice here? - The main challenge is, that I create the channels dynamically, based on the datapoints returned by the library. So I tried to put there a "generic" type into the thing-types and adjust the label and description in the channel, but it seems somehow wrong...

@wborn
Copy link
Member

wborn commented Dec 15, 2020

Did you also try adding the dimension in the ResolChannelTypeProvider?

They are all number now:

ChannelTypeBuilder.state(channelTypeUID, u.getUnitFamily().toString(), "Number")

Looks like you can map the result of u.getUnitFamily() for that. So if it returns UnitFamily.Temperature you can use Number:Temperature instead of Number etc.

@ramack
Copy link
Contributor Author

ramack commented Dec 21, 2020

Closing this in favor of the OH3 rework in #9449

@ramack ramack closed this Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty Issues with a Bountysource reward and the PRs that solve these new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[resol] Add Resol Controller Binding
8 participants