[stiebeleltron] Initial commit #7718

pail23 · 2020-05-21T18:43:41Z

[stiebeleltron] Initial contribution of a ISG Binding (Modbus)

This is a initial contribution for a binding to connect to the Internet Service Gateway of Stiebel Eltron heat pumps through modbus. The binding is heavily inspired by and based on the modbus spunspec binding.
A test version of the binding can be downloaded here.

TravisBuddy · 2020-05-21T19:00:20Z

Travis tests were successful

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

bundles/org.openhab.binding.modbus.stiebeleltron/README.md

fwolter · 2020-06-15T19:53:56Z

bundles/org.openhab.binding.modbus.stiebeleltron/README.md

+
+| Channel ID                  | Item Type          | Read only | Description                                 |
+| --------------------------- | ------------------ | --------- | ------------------------------------------- |
+| operation-mode              | Number             | false     | The current operation mode of the heat pump |


Can you specify what the numbers refer to?

I have added the possible values for the operation mode

fwolter · 2020-06-15T19:56:31Z

bundles/org.openhab.binding.modbus.stiebeleltron/README.md

+
+| Channel ID              | Item Type     | Read only | Description                                      |
+| ----------------------- | ------------- | --------- | ------------------------------------------------ |
+| production_heat_today   | Number:Energy | true      | The heat quantity delivered today                |


The above channel IDs use dashes for sepration. Is there a reason why you use underscores, here?

Good catch. Fixed. Thanks

fwolter · 2020-06-15T19:57:33Z

bundles/org.openhab.binding.modbus.stiebeleltron/README.md

+
+| Thing                    | Description                                        |
+| ------------------------ | -------------------------------------------------- |
+| Stiebel Eltron ISG | A stiebel eltron heat pump connected through a ISG |


Can you mention the ThingID here?

fwolter · 2020-06-15T20:00:07Z

...main/java/org/openhab/binding/modbus/stiebeleltron/internal/StiebelEltronHandlerFactory.java

+    /**
+     * Reference to the modbus manager
+     */


This comment has no benefit. You could remove it. Same for similar. Private fields don't need to be commented.

Comment removed

...enhab.binding.modbus.stiebeleltron/src/main/resources/ESH-INF/config/config-descriptions.xml

fwolter · 2020-06-15T21:29:52Z

...g.modbus.stiebeleltron/src/main/resources/ESH-INF/i18n/modbus.stiebeleltron_xx_XX.properties

@@ -0,0 +1,17 @@
+# FIXME: please substitute the xx_XX with a proper locale, ie. de_DE


Did this file find its way into your binding by intention?

File removed

Again with the unchecked exception. The user could configure an unsupported unit.

I guess this comment goes to StiebelEltronHandler (and not to the .properties file)

fwolter · 2020-06-15T21:31:49Z

...hab.binding.modbus.stiebeleltron/src/main/resources/ESH-INF/thing/heatpump-channel-types.xml

+
+	<channel-type id="fek-temperature-type">
+		<item-type>Number:Temperature</item-type>
+		<label>FFK temperature</label>


Words in labels should be capitalized. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions

Fixed on all occasions

fwolter · 2020-06-15T21:36:27Z

bundles/pom.xml

@@ -1,501 +1,982 @@
-<?xml version="1.0" encoding="UTF-8" standalone="no"?><project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
-
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>


Can you remove the formatting from this file?

pail23 · 2020-06-17T19:17:27Z

@fwolter many thanks for your review. I tried to address all your comments.

fwolter

Did you notice my comments in the handler file?

fwolter · 2020-06-17T20:34:41Z

...in/java/org/openhab/binding/modbus/stiebeleltron/internal/StiebelEltronBindingConstants.java

+    private static final String BINDING_ID = ModbusBindingConstants.BINDING_ID;
+
+    // List of all Thing Type UIDs
+    public static final ThingTypeUID THING_TYPE_SAMPLE = new ThingTypeUID(BINDING_ID, "heatpump");


The field is named THING_TYPE_SAMPLE.

pail23 · 2020-06-18T05:27:57Z

@fwolter Thanks for the hint. It should be fixed now

fwolter · 2020-06-18T20:42:26Z

Did you click on "Load more ..."? There are 19 comments, you didn't react to.

pail23 · 2020-06-19T19:47:05Z

@fwolter Indeed I did not see the 19 other review comments before. Thanks!

pail23 · 2020-06-22T18:55:45Z

@fwolter All your comments should be fixed now. Many thanks for your review. Please let me know if there are other things I should change.

fwolter

Your changes look good. I found another few minor things, I overlooked at the first review.

openHAB 2.5.6 has been released. Can you rebase your code and change the version number to 2.5.7-SNAPSHOT?

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

bundles/org.openhab.binding.modbus.stiebeleltron/README.md

.../main/java/org/openhab/binding/modbus/stiebeleltron/internal/dto/SystemInformationBlock.java

fwolter · 2020-06-23T19:05:54Z

...rc/main/java/org/openhab/binding/modbus/stiebeleltron/internal/dto/SystemParameterBlock.java

+ */
+public class SystemParameterBlock {
+
+    public Integer operation_mode;


See above (camel case)

changed to camel case

fwolter · 2020-06-23T19:07:47Z

...rc/main/java/org/openhab/binding/modbus/stiebeleltron/internal/dto/SystemParameterBlock.java

+ */
+public class SystemParameterBlock {
+
+    public Integer operation_mode;


Is there a reason why you use complex type? Can this be int? Same for below.

changed to int

fwolter · 2020-06-23T19:08:25Z

...on/src/main/java/org/openhab/binding/modbus/stiebeleltron/internal/dto/SystemStateBlock.java

+ */
+public class SystemStateBlock {
+
+    public Integer state;


changed to int

...ain/java/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java

fwolter · 2020-06-23T19:43:22Z

...ain/java/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java

+    public void bridgeStatusChanged(ThingStatusInfo bridgeStatusInfo) {
+        super.bridgeStatusChanged(bridgeStatusInfo);
+
+        logger.debug("Thing status changed to {}", this.getThing().getStatus().name());


fwolter · 2020-06-23T19:47:19Z

...ain/java/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java

+        if (getThing().getStatus() == ThingStatus.ONLINE) {
+            startUp();
+        } else if (getThing().getStatus() == ThingStatus.OFFLINE) {
+            tearDown();
+        }


You are checking for the Thing status here. Do you want to check the bridge status?

good point. I have changed it to the bridge status

fwolter · 2020-06-23T20:01:13Z

...ain/java/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java

+    /**
+     * Reset communication status to ONLINE if we're in an OFFLINE state
+     */
+    protected void resetCommunicationError() {


Is this method used anywhere?

I have inserted the method call in the polling data handlers

fwolter · 2020-06-23T20:07:01Z

...enhab.binding.modbus.stiebeleltron/src/main/resources/ESH-INF/config/config-descriptions.xml

+
+	<config-description uri="thing-type:stiebeleltron:modbusconfig">
+
+		<parameter name="refresh" type="integer" min="0" unit="s">


Did you test 0? If I see correctly it will be passed to the modbus code, which will throw an IllegalArgumentException.

changed it to one

TravisBuddy · 2020-06-24T19:35:12Z

Travis tests were successful

Hey @pail23,
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 · 2020-06-24T19:37:59Z

Travis tests were successful

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

pail23 · 2020-06-24T20:08:51Z

@fwolter Thanks once again. All your comments should be reflected, except the one regarding the Status Update

fwolter

LGTM

Since this is a new binding, another maintainer needs to review your code. This can take some time, because the backlog is quite extensive. Please be patient.

cpmeister · 2020-06-24T21:29:08Z

Since PR #7994 makes improvements to the modbus api that this PR would use, I'll hold back on reviewing this PR until the API changes have gone through.

TravisBuddy · 2020-07-10T15:05:19Z

Travis tests have failed

Hey @pail23,
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 · 2020-07-12T08:23:57Z

Travis tests were successful

Hey @pail23,
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

Thanks for implementing the new API.

fwolter · 2020-07-13T19:01:05Z

...ain/java/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java

+                if (getThing().getStatus() != ThingStatus.ONLINE) {
+                    updateStatus(ThingStatus.ONLINE);
+                }
+            }, failure -> {StiebelEltronHandler.this.handleReadError(failure);});


Syntactical sugar. Same for below.

Suggested change

}, failure -> {StiebelEltronHandler.this.handleReadError(failure);});

}, StiebelEltronHandler.this::handleReadError);

Changed to your proposal

fwolter · 2020-07-13T19:10:04Z

...ain/java/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java

+                String msg = "";
+                String cls = "";
+                cls = error.getClass().getName();
+                msg = error.getMessage();


Initialization with empty strings has no effect, since they are overwritten below. You can initialize them with the intended values at once.

Removed the initialization with the empty strings

pail23 · 2020-07-13T19:35:44Z

Thanks @fwolter for your review. I have implemented the 2 proposed changes.

TravisBuddy · 2020-07-13T20:20:23Z

Travis tests were successful

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

Hilbrand

Thanks for your contribution. Just some minor comments.

Hilbrand · 2020-08-26T13:20:01Z

bundles/org.openhab.binding.modbus.stiebeleltron/README.md

+
+This extension adds support for the Stiebel Eltron modbus protocol.
+
+An Internet Service Gateway (ISG) with an installed modbus extension is required in order to run this binding. In case the modbus extension is not yet installed on the ISG, the ISG Updater Tool for the update can be found here: https://www.stiebel-eltron.de/de/home/produkte-loesungen/erneuerbare_energien/regelung_energiemanagement/internet_servicegateway/isg_web/downloads.html


Can you put each line on a new line so it makes it easier to review (future) improvements to the documentation.

It think I implemented your feedback. Please have a look and let me know in case I didn't understand your review comment properly.

Hilbrand · 2020-08-26T13:22:07Z

bundles/pom.xml

@@ -230,7 +231,7 @@
    <module>org.openhab.binding.snmp</module>
    <module>org.openhab.binding.solaredge</module>
    <module>org.openhab.binding.solarlog</module>
-	<module>org.openhab.binding.somfymylink</module>
+	  <module>org.openhab.binding.somfymylink</module>


Please revert this indentation change.

Hilbrand · 2020-08-26T13:25:11Z

bundles/org.openhab.binding.modbus.stiebeleltron/pom.xml

+  <parent>
+    <groupId>org.openhab.addons.bundles</groupId>
+    <artifactId>org.openhab.addons.reactor.bundles</artifactId>
+    <version>2.5.7-SNAPSHOT</version>


Please rebase/update to 2.5.8-SNAPSHOT

Signed-off-by: Paul Frank <[email protected]>

Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Paul Frank <[email protected]>

…/org/openhab/binding/modbus/stiebeleltron/internal/dto/SystemInformationBlock.java Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Paul Frank <[email protected]>

…/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Paul Frank <[email protected]>

Signed-off-by: Paul Frank <[email protected]>

TravisBuddy · 2020-08-27T06:01:59Z

Travis tests have failed

Hey @pail23,
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.

Signed-off-by: Paul Frank <[email protected]>

TravisBuddy · 2020-08-27T06:22:48Z

Travis tests were successful

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

Signed-off-by: Paul Frank <[email protected]>

TravisBuddy · 2020-08-27T06:34:32Z

Travis tests were successful

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

pail23 · 2020-08-27T07:16:52Z

Thanks @Hilbrand for the review! I tried to implement your feedback.

Hilbrand

LGTM

Signed-off-by: Paul Frank <[email protected]>

Signed-off-by: Paul Frank <[email protected]> Signed-off-by: Daan Meijer <[email protected]>

Signed-off-by: Paul Frank <[email protected]>

pail23 requested a review from a team as a code owner May 21, 2020 18:43

cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label May 21, 2020

fwolter requested changes Jun 15, 2020

View reviewed changes

pail23 added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jun 17, 2020

fwolter requested changes Jun 17, 2020

View reviewed changes

kaikreuzer force-pushed the 2.5.x branch from 536fc42 to 3ab1af1 Compare June 21, 2020 16:12

fwolter requested changes Jun 23, 2020

View reviewed changes

fwolter approved these changes Jun 24, 2020

View reviewed changes

fwolter added the cre Coordinated Review Effort label Jun 24, 2020

fwolter reviewed Jul 13, 2020

View reviewed changes

ssalonen mentioned this pull request Jul 27, 2020

[e3dc] E3DC Home Power Plant Binding contribution #8172

Merged

pail23 requested review from cpmeister, Hilbrand and ssalonen August 21, 2020 05:25

Hilbrand reviewed Aug 26, 2020

View reviewed changes

pail23 and others added 13 commits August 27, 2020 07:58

renamed THING_TYPE_SAMPLE

b2f08c4

Signed-off-by: Paul Frank <[email protected]>

fix remaining review comment

9ad3c45

Signed-off-by: Paul Frank <[email protected]>

removed warnings

696c6ba

Signed-off-by: Paul Frank <[email protected]>

Added handling for REFRESH and changed exception

b69c402

Signed-off-by: Paul Frank <[email protected]>

Update bundles/org.openhab.binding.modbus.stiebeleltron/README.md

9255cf2

Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Paul Frank <[email protected]>

Update bundles/org.openhab.binding.modbus.stiebeleltron/src/main/java…

95fbb82

…/org/openhab/binding/modbus/stiebeleltron/internal/dto/SystemInformationBlock.java Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Paul Frank <[email protected]>

Update bundles/org.openhab.binding.modbus.stiebeleltron/src/main/java…

b7befb6

…/org/openhab/binding/modbus/stiebeleltron/internal/handler/StiebelEltronHandler.java Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Paul Frank <[email protected]>

fixed review findings

b8d80b4

Signed-off-by: Paul Frank <[email protected]>

mvn spotless:apply

9790165

Signed-off-by: Paul Frank <[email protected]>

rebased from upstream 2.5.x and upgraded 2.5.7

f9528e3

Signed-off-by: Paul Frank <[email protected]>

Adapted to simplified modbus api

9c76020

Signed-off-by: Paul Frank <[email protected]>

fixes

1f48432

Signed-off-by: Paul Frank <[email protected]>

review findings

73f0e28

Signed-off-by: Paul Frank <[email protected]>

update version

875e04c

Signed-off-by: Paul Frank <[email protected]>

updated documentation

c078020

Signed-off-by: Paul Frank <[email protected]>

Hilbrand approved these changes Aug 27, 2020

View reviewed changes

Hilbrand merged commit 1857a42 into openhab:2.5.x Aug 27, 2020

Hilbrand added this to the 2.5.9 milestone Aug 27, 2020

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[stiebeleltron] Initial contribution (openhab#7718)

1c0eb49

Signed-off-by: Paul Frank <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[stiebeleltron] Initial contribution (openhab#7718)

c2dfcf2

Signed-off-by: Paul Frank <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[stiebeleltron] Initial contribution (openhab#7718)

e2daf5b

Signed-off-by: Paul Frank <[email protected]>

andrewfg pushed a commit to andrewfg/openhab-addons that referenced this pull request Aug 31, 2020

[stiebeleltron] Initial contribution (openhab#7718)

f50b882

Signed-off-by: Paul Frank <[email protected]>

DaanMeijer pushed a commit to DaanMeijer/openhab-addons that referenced this pull request Sep 1, 2020

[stiebeleltron] Initial contribution (openhab#7718)

359a190

Signed-off-by: Paul Frank <[email protected]> Signed-off-by: Daan Meijer <[email protected]>

CSchlipp pushed a commit to CSchlipp/openhab-addons that referenced this pull request Sep 12, 2020

[stiebeleltron] Initial contribution (openhab#7718)

0a621e0

Signed-off-by: Paul Frank <[email protected]>

markus7017 pushed a commit to markus7017/openhab-addons that referenced this pull request Sep 19, 2020

[stiebeleltron] Initial contribution (openhab#7718)

28be91a

Signed-off-by: Paul Frank <[email protected]>

ssalonen mentioned this pull request Sep 22, 2020

[modbus.e3dc] [modbus.helioseasycontrols] [modbus.stiebeleltron] [modbus.studer] Missing in features xml? #8538

Closed

		@@ -0,0 +1,17 @@
		# FIXME: please substitute the xx_XX with a proper locale, ie. de_DE

		@@ -1,501 +1,982 @@
		<?xml version="1.0" encoding="UTF-8" standalone="no"?><project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">

		<?xml version="1.0" encoding="UTF-8" standalone="no"?>


		<config-description uri="thing-type:stiebeleltron:modbusconfig">

		<parameter name="refresh" type="integer" min="0" unit="s">

	}, failure -> {StiebelEltronHandler.this.handleReadError(failure);});
	}, StiebelEltronHandler.this::handleReadError);


		This extension adds support for the Stiebel Eltron modbus protocol.

		An Internet Service Gateway (ISG) with an installed modbus extension is required in order to run this binding. In case the modbus extension is not yet installed on the ISG, the ISG Updater Tool for the update can be found here: https://www.stiebel-eltron.de/de/home/produkte-loesungen/erneuerbare_energien/regelung_energiemanagement/internet_servicegateway/isg_web/downloads.html

[stiebeleltron] Initial commit #7718

[stiebeleltron] Initial commit #7718

Conversation

pail23 commented May 21, 2020 • edited Loading

TravisBuddy commented May 21, 2020

Travis tests were successful

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pail23 commented Jun 17, 2020

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pail23 commented Jun 18, 2020

fwolter commented Jun 18, 2020

pail23 commented Jun 19, 2020

pail23 commented Jun 22, 2020

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisBuddy commented Jun 24, 2020

Travis tests were successful

TravisBuddy commented Jun 24, 2020

Travis tests were successful

pail23 commented Jun 24, 2020

fwolter left a comment

Choose a reason for hiding this comment

cpmeister commented Jun 24, 2020

TravisBuddy commented Jul 10, 2020

Travis tests have failed

TravisBuddy commented Jul 12, 2020

Travis tests were successful

fwolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pail23 commented Jul 13, 2020

TravisBuddy commented Jul 13, 2020

Travis tests were successful

Hilbrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisBuddy commented Aug 27, 2020

Travis tests have failed

TravisBuddy commented Aug 27, 2020

Travis tests were successful

TravisBuddy commented Aug 27, 2020

Travis tests were successful

pail23 commented May 21, 2020 •

edited

Loading