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

[boschshc] Initial contribution - Bindings for Bosch Smart Home devices #8629

Merged
merged 203 commits into from
Jan 17, 2021

Conversation

coeing
Copy link
Contributor

@coeing coeing commented Oct 1, 2020

Hi there,

@stefan-kaestle started a fork of your repository in November 2019 to implement the bindings for Bosch Smart Home devices which are pretty popular in Germany. @GerdZanker and I joined earlier this year.

Together we managed to built a good base for a first release, which already has the base structure to setup and communicate with the Bosch Smart Home controller and supports a lot of devices:

  • Bosch In-Wall switches
  • Bosch Smart Plugs (use "in-wall-switch" thing too).
  • Bosch TwinGuard smoke detector
  • Bosch Window/Door contacts
  • Bosch Motion Detector
  • Bosch Shutter Control in-wall
  • Bosch Thermostat
  • Bosch Climate Control

Before we continue our development we'd like to do a first public release to gather user feedback and your feedback as openHAB maintainers :)

We already found some fellow openHAB users who tested pre-release versions and apart from some minor issues which we fixed, they were already quite content with the result (and had some ideas for enhancements for the next version of course :) ).

We tried to get some upfront feedback about our code base from one of your maintainers, but got no answer, so we thought we just create a pull request to discuss any further issues here: https://community.openhab.org/t/towards-merging-the-new-bosch-shc-binding/101952

You can find the fork here: https://github.com/stefan-kaestle/openhab2-addons We already tried to sign-off each commit and follow your coding guidelines (https://www.openhab.org/docs/developer/development/guidelines.html), but might have missed some points as it is our first contribution to your project.

We have a thread in the community forum: https://community.openhab.org/t/will-there-be-a-bosch-smart-home-binding

The last pre-release is up-to-date apart from some file moving: https://github.com/stefan-kaestle/openhab2-addons/releases/tag/v1.0-beta.1

Feel free to ask any questions and let us know what work needs to be done before our work can be merged. Thanks for your help and for your great open-source Smart Home platform!

This pull request continues the first merge of our binding from the previous pull request #8371 Due to openHAB 3 we rebased our binding on the new code base.

@TravisBuddy
Copy link

TravisBuddy commented Oct 1, 2020

Travis tests were successful

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

@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label Oct 1, 2020
@coeing coeing marked this pull request as ready for review October 18, 2020 19:27
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

I wasn't able to review the entire thing, but I found a few issues to get you started.

@coeing
Copy link
Contributor Author

coeing commented Oct 26, 2020

I wasn't able to review the entire thing, but I found a few issues to get you started.

Thanks @cpmeister for your review! I will try to fix the issues you found this week :)

@coeing coeing force-pushed the bosch-shc-3.0 branch 2 times, most recently from a251db1 to 83a86f5 Compare October 26, 2020 19:54
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

Second review pass

@coeing coeing requested a review from cpmeister November 2, 2020 20:13
@kaikreuzer kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Nov 17, 2020
Comment on lines 64 to 68
updateState(CHANNEL_TEMPERATURE, new DecimalType(state.temperature));
updateState(CHANNEL_TEMPERATURE_RATING, new StringType(state.temperatureRating));
updateState(CHANNEL_HUMIDITY, new DecimalType(state.humidity));
updateState(CHANNEL_HUMIDITY_RATING, new StringType(state.humidityRating));
updateState(CHANNEL_PURITY, new DecimalType(state.purity));
Copy link
Contributor

Choose a reason for hiding this comment

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

Temperature, humidity, and purity should be QuantityType instead of DecimalType.

Copy link
Contributor Author

@coeing coeing Nov 18, 2020

Choose a reason for hiding this comment

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

Changed it :) Hope the QuantityType<Dimensionless> is okay for humidity and purity as the channels are of type Number:Dimensionless?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpmeister Are the current types okay?

@cpmeister
Copy link
Contributor

Oh, also your build is failing.

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.

This is pretty good code, you made a really good job!

I receive compiler errors. E.g.

BoschSHCBridgeHandler.java:[355,19] Null type mismatch (type annotations): required 'T extends org.openhab.binding.boschshc.internal.services.dto.@NonNull BoschSHCServiceState' but this expression has type '@Nullable T extends org.openhab.binding.boschshc.internal.services.dto.@NonNull BoschSHCServiceState'

@coeing coeing requested a review from a team as a code owner December 3, 2020 11:15
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

I think these are the last changes I'll request, other than these it looks good to me.

@fwolter
Copy link
Member

fwolter commented Dec 10, 2020

SmartHomeUnits has been renamed to Units. That needs to be adapted in your code.

@coeing
Copy link
Contributor Author

coeing commented Dec 10, 2020

SmartHomeUnits has been renamed to Units. That needs to be adapted in your code.

Found this issue while you wrote your comment :) Changed it in my last commit.

Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

Almost done from me I think.

@coeing coeing requested a review from fwolter December 15, 2020 14:38
added back the removed @nullable annotation in sendRequest()
replaced deprecated SslContextFactory constructor call

Signed-off-by: Gerd Zanker <gerd.zanker@web.de>
@cpmeister cpmeister added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 7, 2021
Comment on lines 17 to 30
<dependencies>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcpkix-jdk15on</artifactId>
<version>1.54</version>
<scope>compile</scope>
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk15on</artifactId>
<version>1.54</version>
<scope>compile</scope>
</dependency>
</dependencies>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a documentation which information of the dependencies should go into the NOTICE file?

Not that I'm aware of, but you can look at other bindings as an example.

@Hilbrand
Copy link
Member

Hilbrand commented Jan 8, 2021

There is no documentation on format of additional licenses in NOTICE. It's something like this:

## Third-party Content

bcpkix-jdk15on
bcprov-jdk15on
* License: Bouncy Castle License
* Project https://www.bouncycastle.org
* Source: https://github.com/bcgit/bc-java

One other thing. It looks like the library bcpkix-jdk15on is also available repackaged in core, but only at runtime. This is version 1.52. If that is indeed the case than this library could be added in this binding with scope provided. It may be worth testing this to reduce the download size of the jar (this would mean the version number here needs to be changed match the version in openhab-core).

@coeing
Copy link
Contributor Author

coeing commented Jan 8, 2021

There is no documentation on format of additional licenses in NOTICE. It's something like this:

## Third-party Content

bcpkix-jdk15on
bcprov-jdk15on
* License: Bouncy Castle License
* Project https://www.bouncycastle.org
* Source: https://github.com/bcgit/bc-java

One other thing. It looks like the library bcpkix-jdk15on is also available repackaged in core, but only at runtime. This is version 1.52. If that is indeed the case than this library could be added in this binding with scope provided. It may be worth testing this to reduce the download size of the jar (this would mean the version number here needs to be changed match the version in openhab-core).

@GerdZanker Could you try this out? Would be nice to have no additional dependency for our binding :) I am currently working on the Long Polling cancelation after 24 hours (stefan-kaestle#62)

Signed-off-by: Gerd Zanker <gerd.zanker@web.de>
…g.openhab.core.io.jetty.certificate to later reduce download size

Signed-off-by: Gerd Zanker <gerd.zanker@web.de>
@GerdZanker
Copy link
Contributor

GerdZanker commented Jan 9, 2021

@GerdZanker Could you try this out?

I'm on the way. The NOTICE is updated and I downgraded the Bouncy Castle version to 1.52 and certificate can still be created.

@GerdZanker
Copy link
Contributor

this library could be added in this binding with scope provided. It may be worth testing this to reduce the download size of the jar (this would mean the version number here needs to be changed match the version in openhab-core)

The version number 1.52 now matches to the version used in org.openhab.io.jetty.certificate, see dc03646.
I changed locally the scope of the <artifactId>bcpkix-jdk15on</artifactId> dependency to <scope>provided</scope>, but I get errors during mvn install:

[ERROR] Failed to execute goal org.apache.karaf.tooling:karaf-maven-plugin:4.2.7:verify (karaf-feature-verification) on project org.openhab.binding.boschshc: Feature resolution failed for [openhab-binding-boschshc/3.1.0.SNAPSHOT]
[ERROR] Message: Unable to resolve root: missing requirement [root] osgi.identity; osgi.identity=openhab-binding-boschshc; type=karaf.feature; version=3.1.0.SNAPSHOT; filter:="(&(osgi.identity=openhab-binding-boschshc)(type=karaf.feature)(version>=3.1.0.SNAPSHOT))" [caused by: Unable to resolve openhab-binding-boschshc/3.1.0.SNAPSHOT: missing requirement [openhab-binding-boschshc/3.1.0.SNAPSHOT] osgi.identity; osgi.identity=org.openhab.binding.boschshc; type=osgi.bundle; version="[3.1.0.202101091740,3.1.0.202101091740]"; resolution:=mandatory [caused by: Unable to resolve org.openhab.binding.boschshc/3.1.0.202101091740: missing requirement [org.openhab.binding.boschshc/3.1.0.202101091740] osgi.wiring.package; filter:="(&(osgi.wiring.package=org.bouncycastle.cert)(version>=1.52.0)(!(version>=2.0.0)))"]]
[ERROR] Repositories: {
[ERROR]         file:...\openhab-addons\bundles\org.openhab.binding.boschshc\target/feature/feature.xml
[ERROR]         mvn:org.apache.karaf.features/framework/4.2.7/xml/features
[ERROR]         mvn:org.apache.karaf.features/standard/4.2.7/xml/features
[ERROR]         mvn:org.openhab.core.features.karaf/org.openhab.core.features.karaf.openhab-core/3.1.0-SNAPSHOT/xml/features
[ERROR]         mvn:org.openhab.core.features.karaf/org.openhab.core.features.karaf.openhab-tp/3.1.0-SNAPSHOT/xml/features
[ERROR]         mvn:org.ops4j.pax.web/pax-web-features/7.2.11/xml/features
[ERROR] }
[ERROR] Resources: {
...
[ERROR]         mvn:org.apache.xbean/xbean-reflect/4.14
[ERROR]         mvn:org.bitbucket.b_c/jose4j/0.7.0
[ERROR]         mvn:org.eclipse.emf/org.eclipse.emf.common/2.17.0
...
[ERROR]         mvn:org.openhab.addons.bundles/org.openhab.binding.boschshc/3.1.0-SNAPSHOT
...
[ERROR]         mvn:org.openhab.core.bundles/org.openhab.core.io.jetty.certificate/3.1.0-SNAPSHOT
...
[ERROR] }

Can you @Hilbrand support us, because I'm not a maven expert and know how to solve this? It would be great if you can suggest necessary pom.xml changes to fix this, because I have now idea what the problem is.

GSON will not return null if there is no "result" field, but will just set the "result" member to null.

Signed-off-by: Christian Oeing <christian.oeing@slashgames.org>
…method name

Signed-off-by: Christian Oeing <christian.oeing@slashgames.org>
Comment on lines +6 to +7
<feature>openhab-runtime-base</feature>
<bundle start-level="80">mvn:org.openhab.addons.bundles/org.openhab.binding.boschshc/${project.version}</bundle>
Copy link
Contributor

Choose a reason for hiding this comment

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

After switch the dependencies to provided you would then need to update your feature.xml to let the osgi system know where it can find those dependencies. This should solve the error your are seeing in the build.

Suggested change
<feature>openhab-runtime-base</feature>
<bundle start-level="80">mvn:org.openhab.addons.bundles/org.openhab.binding.boschshc/${project.version}</bundle>
<feature>openhab-runtime-base</feature>
<bundle dependency="true">mvn:org.bouncycastle/bcpkix-jdk15on/1.52</bundle>
<bundle dependency="true">mvn:org.bouncycastle/bcprov-jdk15on/1.52</bundle>
<bundle start-level="80">mvn:org.openhab.addons.bundles/org.openhab.binding.boschshc/${project.version}</bundle>

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot @cpmeister! Your suggestion solves the error on my machine.

This means that the maven pom.xml contains the "compile time" dependencies and the feature.xml defines the runtime OSGi environment.

I will tests the new binding jar - now only 88Kb big - and commit the changes afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

88kb?! 🎆

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh no 😧. During runtime I get now problems: Not all used class from org.bouncycastle.* are contained in org.openhab.io.jetty.certificate and this binding code uses of cause missing classes.

I will evaluate the following option:
a) rewrite the binding certificate code to only use available classes
b) use the maven-shade-plugin as in the org.openhab.io.jetty.certificate bundle pom.xml to reduce the jar size

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can tell, org.openhab.io.jetty.certificate doesn't export any of the org.bouncycastle.* classes that it has shaded internally. This means that you can't rely on org.openhab.io.jetty.certificate as a dependency to get those classes from.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you having any trouble getting the binding to install? Or does it install and just throw errors?

The trouble started after a successful compilation and bundle installation. It seems to me that all dependencies inside the OSGi container were resolved, because I was able to start the Bosch SHC Bridge Thing. But during the very first initialization I got exceptions during the keystore and the self signed certificate creation.

I will write unit tests to have a defined environment and reproduce steps for keystore and self signed certificate creation. Then I can evaluate different dependencies and code combinations.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: A first "copy & paste" of the maven-shade-plugin settings from the org.openhab.io.jetty.certificate pom.xml to this binding pom resulted in the error

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-shade-plugin:3.2.1:shade (default) on project org.openhab.binding.boschshc: Error creating shaded jar: Invalid signature file digest for Manifest main attributes -> [Help 1]

Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are going to shade the bouncycastle libraries into you binding anyway, the easiest way to do that would be to give those dependencies a compile scope in your pom file. Doing so would automatically get those dependencies shaded into your final binding jar. You also wouldn't need to list the dependencies in your feature.xml file either in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

The current PR content has the compile scope for the bouncycastle dependencies and the shading results in a 3.4MB big jar which is working.
It was requested to reduce the jar size and up to now I found no feasible way, because
a) the org.openhab.io.jetty.certificate jar doesn't provide necessary classes currently used in the BoschSslUtil class during runtime
b) I have no working maven pom to shade and reduce the jar size during compile time

Copy link
Contributor

Choose a reason for hiding this comment

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

When @Hilbrand made his suggestion I don't think he was aware that the org.openhab.io.jetty.certificate doesn't export the bouncycastle packages. So depending on the core to provide those dependencies isn't an option at the moment. For now I suggest leaving the dependencies embedded for now and once the core exposes the bouncycastle packages we can try to readdress the sizing issue.

Signed-off-by: Gerd Zanker <gerd.zanker@web.de>
Signed-off-by: Gerd Zanker <gerd.zanker@web.de>
The changeable SHC system password for the keystore is replaced by a static string in the code.
The keyStore name is now based on SHC ipAddress to support multiple SmartHomeControllers.

Signed-off-by: Gerd Zanker <gerd.zanker@web.de>
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

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

The code looks good to me right now. Is there anything else you want to add before this gets merged? I'd suggest leaving new features for later PRs.

@cpmeister cpmeister added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 17, 2021
@GerdZanker
Copy link
Contributor

Is there anything else you want to add before this gets merged?

Hello @coeing, I know you currently in progress with this issue.
Do you want to add modifications before a merge?

I'm done: The JAR size is OK and the initial setup is improved, because an internal password is used for the keystore.

@coeing
Copy link
Contributor Author

coeing commented Jan 17, 2021

@cpmeister @GerdZanker I am fine with the current state of the binding. As Gerd mentioned, our binding disconnects after 24 hours (stefan-kaestle#62), but I need some time to investigate the issue as it is not easy to reproduce (having to wait 24 hours each time...). Having some more people test the binding will help with finding the issue.

So from my side: Ready to merge! 👍

@cpmeister cpmeister merged commit 2a5bdf3 into openhab:main Jan 17, 2021
@cpmeister cpmeister added this to the 3.1 milestone Jan 17, 2021
nowaterman pushed a commit to nowaterman/openhab-addons that referenced this pull request Jan 19, 2021
…es (openhab#8629)

* Initial code from create_openhab_binding_skeleton.sh


Co-authored-by: Stefan Kaestle <stefan@mad-kow.de>
Co-authored-by: Gerd Zanker <gerd.zanker@web.de>
Co-authored-by: Christian Oeing <christian.oeing@scalamat.de>
Co-authored-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
@coeing coeing deleted the bosch-shc-3.0 branch February 7, 2021 20:07
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
…es (openhab#8629)

* Initial code from create_openhab_binding_skeleton.sh

Signed-off-by: Stefan Kaestle <stefan@mad-kow.de>
Signed-off-by: Christian Oeing <christian.oeing@slashgames.org>
Signed-off-by: Gerd Zanker <gerd.zanker@web.de>

Co-authored-by: Stefan Kaestle <stefan@mad-kow.de>
Co-authored-by: Gerd Zanker <gerd.zanker@web.de>
Co-authored-by: Christian Oeing <christian.oeing@scalamat.de>
Co-authored-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
…es (openhab#8629)

* Initial code from create_openhab_binding_skeleton.sh

Signed-off-by: Stefan Kaestle <stefan@mad-kow.de>
Signed-off-by: Christian Oeing <christian.oeing@slashgames.org>
Signed-off-by: Gerd Zanker <gerd.zanker@web.de>

Co-authored-by: Stefan Kaestle <stefan@mad-kow.de>
Co-authored-by: Gerd Zanker <gerd.zanker@web.de>
Co-authored-by: Christian Oeing <christian.oeing@scalamat.de>
Co-authored-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
…es (openhab#8629)

* Initial code from create_openhab_binding_skeleton.sh

Signed-off-by: Stefan Kaestle <stefan@mad-kow.de>
Signed-off-by: Christian Oeing <christian.oeing@slashgames.org>
Signed-off-by: Gerd Zanker <gerd.zanker@web.de>

Co-authored-by: Stefan Kaestle <stefan@mad-kow.de>
Co-authored-by: Gerd Zanker <gerd.zanker@web.de>
Co-authored-by: Christian Oeing <christian.oeing@scalamat.de>
Co-authored-by: Hilbrand Bouwkamp <hilbrand@h72.nl>
Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Co-authored-by: Connor Petty <mistercpp2000+gitsignoff@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants