[ecotouch] add Waterkotte EcoTouch binding (ported from OH1) #10010

sibbi77 · 2021-01-31T17:36:27Z

It was developed at https://github.com/sibbi77/openhab2-addons/tree/ecotouch-3.0.0

Signed-off-by: Sebastian Held <[email protected]>

Skinah · 2021-02-21T11:05:46Z

bundles/org.openhab.binding.ecotouch/README.md

+
+| Channel ID          | Type               | Read-Only | Description |
+|---------------------|--------------------|-----------|-------------|
+| temperature_outside | Number:Temperature | yes       | The current outside temperature |


Suggested change

| temperature_outside | Number:Temperature | yes | The current outside temperature |

| temperatureOutside | Number:Temperature | yes | The current outside temperature |

Please change all channels to use lowerCamelCase

see

https://www.openhab.org/docs/developer/guidelines.html#java-coding-style

EDIT: I see this is a OH1 binding upgrade, does that mean the channels have been kept the same? ie will it be a breaking change if the channels are changed?

First, thanks for reviewing! Yes this is a OH1 binding upgrade and all channels have been kept compatible to OH1 to minimize the migration work for the users. What should I do now?

Leave it as is until someone with more knowledge steps in to make the call on what to do. I’ll help with what I can.

As far as I know underscores are still allowed in OH3.

Skinah · 2021-02-21T11:55:05Z

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/thing/thing-types.xml

+		<description></description>
+		<state readOnly="true" pattern="%.1f %unit%"/>
+	</channel-type>
+	<channel-type id="power_compressor">


Suggested change

<channel-type id="power_compressor">

<channel-type id="power_compressor" advanced="true">

Can you mark any channels which wont be used often as advanced? This way people can use the 'select all' button and only get the most useful channels without needing to go through a big list, or worse selecting them all and have a large amount of channels then using persistence and using system resources. To see the advanced channels you just tick 'show advanced' when linking channels.

EDIT just saw you have marked some already :) cool.

I'm unsure about the channels which should go into advanced. I tried to mark some as advanced, but that depends on what the users want to do. I'll try to put many more channels into advanced and only keep a dozen or so.

Sounds good as only you and people that use the binding can make that call. I usually ask two questions to myself when deciding

Can it be useful in a rule?

What are the channels that most users will want to add?

if it’s just a nice to have channel but not that useful to actually trigger an automation I tend to want it hidden under advanced. You can always hit the add all button and then tick show advanced and add a few more.

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/thing/thing-types.xml

Signed-off-by: Sebastian Held <[email protected]>

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/thing/thing-types.xml

...g.ecotouch/src/main/java/org/openhab/binding/ecotouch/internal/EcoTouchBindingConstants.java

...ding.ecotouch/src/main/java/org/openhab/binding/ecotouch/internal/EcoTouchConfiguration.java

....binding.ecotouch/src/main/java/org/openhab/binding/ecotouch/internal/EcoTouchConnector.java

Skinah · 2021-03-02T04:27:59Z

Please compile this and look at the lines that appear when compiling and solve any warnings. Also in the target folder after compiling is done is a code-analasis folder, open the report file and solve all the issues that it has listed. Once you have done that then feel free to re-request a review. A lot of the classes are not marked as nonNullByDefault currently.

sibbi77 · 2021-03-02T06:44:59Z

Thanks! I'll read about that "nonNullByDefault" thing.

Signed-off-by: Sebastian Held <[email protected]>

sibbi77 · 2021-03-07T18:57:18Z

I've a bunch of these "Potential null pointer access: this expression has a '@nullable' type" warnings left and cannot get rid of them, but I tried to address all other warnings and remarks.
For the final merge I would rebase and squash everything into one commit.

Signed-off-by: Sebastian Held <[email protected]>

Skinah · 2021-03-09T04:09:24Z

I've a bunch of these "Potential null pointer access: this expression has a '@nullable' type" warnings left and cannot get rid of them, but I tried to address all other warnings and remarks.
For the final merge I would rebase and squash everything into one commit.

Can you give the warning, the file and line number of where the warning is given? Happy to look at the latest code changes and give a suggestion on how to solve it so you don't bang your head on the wall.

bundles/org.openhab.binding.ecotouch/README.md

sibbi77 · 2021-03-09T07:05:28Z

....binding.ecotouch/src/main/java/org/openhab/binding/ecotouch/internal/EcoTouchConnector.java

+                if (cookies != null) {
+                    for (String cookie : cookies) {
+                        connection.addRequestProperty("Cookie", cookie.split(";", 2)[0]);
+                    }
+                }


Here it get the following warning:
[WARNING] org.openhab.binding.ecotouch/src/main/java/org/openhab/binding/ecotouch/internal/EcoTouchConnector.java:[201,42] Potential null pointer access: this expression has a '@Nullable' type
cookies is Nullable, but I check for null before use. I'm lost here.

Suggested change

if (cookies != null) {

for (String cookie : cookies) {

connection.addRequestProperty("Cookie", cookie.split(";", 2)[0]);

}

}

List<String> localCookies=cookies;

if (localCookies != null) {

for (String cookie : localCookies) {

connection.addRequestProperty("Cookie", cookie.split(";", 2)[0]);

}

}

does that mean, that the same class instance is executed by two different tasks at the same time? It is clear, that I can have more than one instance, if I have more than one heat pump. I thought serialization is performed by the framework.
If that is not true, I'll have to check all places for data races and have mutexes all around... That seems unreasonable and unlikely to me...
I would guess that the static checker is just not aware of the framework's serialization. Isn't there a magic attribute to provide that info?

I am not the best person to explain things to you as I am new to Java. Does the code suggested solve the compiler's warning for null? Your concerns about data races and serialization are not related to this, it is about potentially another thread making the handle to the object null if my understanding is correct.

The root of the problem is: the compiler thinks cookies can turn into null between line 200 and 201. That's only possible if multithreading is allowed AND the very same instance of the object is executed in two different threads. Your workaround would prevent the compiler from flagging the line 201 as a problem, because you created a local variable on the stack, which is bound to the thread. This will "fix" this particular problem (with an ugly workaround) but if the compiler is right in flagging it, then I have to check the complete code for data races.
Is there any documentation, how openhab implements multithreading? Again, I think the compiler warning is a false positive, because otherwise it would make binding implementation unnecessary complicated.

Yes it is a trade off, so where it is possible variables should be nonNullable and initialized at creation. Whilst it may be that you know your code 100% inside out, if someone else comes along to maintain it they may not know it as well. My knowledge is not very good, if you want to discuss how openHAB implements multithreading you should ask on the forum in the dev section. The suggestion I made to you, is what others have given me when I needed help.

Signed-off-by: Sebastian Held <[email protected]>

fwolter

Thanks for your contribution! I reviewed your code and here is my feedback.

Great that you already fixed all of the compiler warnings about null values with @Skinah's help!

fwolter · 2021-04-03T11:38:51Z

bundles/org.openhab.binding.ecotouch/.classpath

@@ -0,0 +1,49 @@
+<?xml version="1.0" encoding="UTF-8"?>


This file is generated automatically and should therefore not be part of your code.

fwolter · 2021-04-03T11:39:02Z

bundles/org.openhab.binding.ecotouch/.project

@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="UTF-8"?>


fwolter · 2021-04-03T11:39:37Z

bundles/org.openhab.binding.ecotouch/README.md

+## Binding Configuration
+
+No Binding configuration required.


This can be removed, then.

fwolter · 2021-04-03T11:40:40Z

bundles/org.openhab.binding.ecotouch/README.md

+```
+Thing ecotouch:geo:9a0ec0bbbd "Waterkotte Heatpump" @ "basement" [ ip="192.168.1.100", username="admin", password="wtkadmin", refresh=120 ]
+```


You could move this to the end of the file into the Examples section to be consistent with other bindings.

There should also be examples for the .items file. See other bindings for reference.

fwolter · 2021-04-03T11:42:11Z

bundles/org.openhab.binding.ecotouch/README.md

+
+## Channels
+
+All available channels can be seen inside PaperUI. Here is a small extract:


Can you express this a bit more abstract as PaperUI was replaced in OH3?

Actually, the readme should state all Channels as many users use textual configuration.

fwolter · 2021-04-03T12:09:46Z

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/i18n/ecotouch_de.properties

+thing-type.ecotouch.air.description = Waterkotte EcoTouch Air Luftw�rmepumpe
+
+# thing type config description (geo)
+thing-type.config.ecotouch.geo.ip.label = IP Adresse / Hostname der W�rmepumpe


Labels are expected to be as short as possible. Guideline is 2-3 words with max 20-25 chars. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions

Please check all.

Suggested change

thing-type.config.ecotouch.geo.ip.label = IP Adresse / Hostname der W�rmepumpe

thing-type.config.ecotouch.geo.ip.label = IP Adresse / Hostname

fwolter · 2021-04-03T12:10:59Z

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/i18n/ecotouch_de.properties

+channel-type.ecotouch.temperature_solar_flow.description = Solarkreis Vorlauftemperatur
+channel-type.ecotouch.position_expansion_valve.label = Ventil�ffnung el. Expansionsventil
+channel-type.ecotouch.position_expansion_valve.description = Ventil�ffnung elektrisches Expansionsventil
+channel-type.ecotouch.power_compressor.label = elektrische Antriebsleitung Verdichter


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

Please check all.

Can you comment on that?

fwolter · 2021-04-03T12:11:44Z

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/i18n/ecotouch_de.properties

+channel-type.ecotouch.ecovent_moisture_value.description = EcoVent Luftfeuchtigkeit
+channel-type.ecotouch.ecovent_output_y1.label = EcoVent L�fterdrehzahl
+channel-type.ecotouch.ecovent_output_y1.description = EcoVent L�fterdrehzahl
+


Please remove one empty line.

fwolter · 2021-04-03T12:13:04Z

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/thing/thing-types.xml

+	<channel-type id="temperature_source_in">
+		<item-type>Number:Temperature</item-type>
+		<label>Temperature Source Input</label>
+		<description></description>


You could drop the tag if it's empty.

fwolter · 2021-04-03T12:13:37Z

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/thing/thing-types.xml

+		<description></description>
+		<state min="0" max="5" step="1" readOnly="false"/>
+	</channel-type>
+


Please remove both empty lines.

Signed-off-by: Sebastian Held <[email protected]>

sibbi77 · 2021-04-11T18:15:09Z

tried to fix all mentioned problems

fwolter · 2021-04-19T16:35:06Z

bundles/org.openhab.binding.ecotouch/src/main/resources/OH-INF/i18n/ecotouch_de.properties

+channel-type.ecotouch.temperature_solar_flow.description = Solarkreis Vorlauftemperatur
+channel-type.ecotouch.position_expansion_valve.label = Ventil�ffnung el. Expansionsventil
+channel-type.ecotouch.position_expansion_valve.description = Ventil�ffnung elektrisches Expansionsventil
+channel-type.ecotouch.power_compressor.label = elektrische Antriebsleitung Verdichter


Can you comment on that?

sibbi77 · 2021-04-19T16:42:16Z

In file ecotouch_de.properties I've reduced the length of the *.label entries and added the longer description to the *.description tags.

fwolter · 2021-04-19T16:46:23Z

My orignal comment was:

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

sibbi77 · 2021-04-19T17:57:19Z

That may work for English, but other bindings also don't follow these rules in their German translation. The referenced document also states to have at most 25 characters in a label, that's what I did.

fwolter · 2021-04-19T18:00:39Z

You might be right, that doesn't work in German. But at least the first word in the label should be upper case, otherwise it looks odd, even in German.

sibbi77 · 2021-04-19T18:03:01Z

You're right, I'll fix that. If you think, new Bindings should follow the capitalization rule even in non English languages and ported bindings are not yet updated, I can even make that change, too. Your decision.

fwolter · 2021-04-19T18:14:27Z

I think it looks odd if every word is capitalized in German.

Signed-off-by: Sebastian Held <[email protected]>

fwolter

Thanks!

…#10010) Signed-off-by: Sebastian Held <[email protected]> Signed-off-by: John Marshall <[email protected]>

…#10010) Signed-off-by: Sebastian Held <[email protected]>

sibbi77 requested a review from a team as a code owner January 31, 2021 17:36

sibbi77 force-pushed the ecotouch-3.x branch from f9dd56b to d446a34 Compare January 31, 2021 17:44

add Waterkotte EcoTouch heat pump binding

596f93a

Signed-off-by: Sebastian Held <[email protected]>

sibbi77 force-pushed the ecotouch-3.x branch from d446a34 to 596f93a Compare January 31, 2021 17:47

fwolter added new binding If someone has started to work on a binding. For a new binding PR. oh1 migration Relates to migrating an openHAB 1 addon to openHAB 2 labels Jan 31, 2021

Hilbrand mentioned this pull request Jan 31, 2021

openHAB 1 addon compatibility overview with openHAB 3 #6179

Closed

Skinah reviewed Feb 21, 2021

View reviewed changes