[siemenshvac] Initial contribution - interface (OZW672) #14263

lo92fr · 2023-01-22T19:28:05Z

Title

Add a binding for siemens Hvac interface (OZW672)

Description

This pull request is about adding a binding for siemens Hvac interface OZW672 (https://hit.sbt.siemens.com/RWD/app.aspx?rc=FR&lang=fr&module=Catalog&action=ShowProduct&key=BPZ:OZW672.01)

The OZW672 is a ethernet bridge for the LSB bus of Hvac device.
It implements a web interface that enable reading and setting of differents hvac parameters.
OZW672 also support a REST api.

This binding use this REST api to integrate OZW672 into openhab.

Testing

The binding support autodiscovery of the OZW672 interface using upnp.
After a few minutes, you should see the OZW672 bridge into your Inbox.
If not, power off and up OZW672.
You can also add the bridge manually.

After adding the bridge, you will have to go to bridge parameters, setting the password for the OZW672, and then restart openhab.
On restart, OZW672 will do autodiscovery phase from you installation during initialization.
This can be a little long : ~2 to 5 minutes depending of your installation.

After OZW672 bridge come up again, go back again to the "things" page of openhab.
Click on the add button, then click on the "siemensHvac" binding, and on the next page click on the "scan" button.
After a few second, openhab should list all you devices connect to the LSB bus.
If you have a simple home Hvac installation, it will generally find only one device.
Most common name for this device are something like "RVS41xxx.yyy".
This device represents the main controler inside your Hvac device.

After you add this RVS to your system, you can go into it, and Channels tabs will expose all your hvac settings.
Structure of the Channels should be the same as the one on the LCD screen of your Hvac device.
You may have a lot more of items that on your LCD screen because the device exposte all items, even the one for "specialist" guys.

lo92fr · 2023-01-22T19:51:42Z

Hello,

Not sure to understand the error message on the build.
The compilation is working on my side, if you can give me some advice about this error.

lsiepel · 2023-01-23T20:31:48Z

Hope the rebuild helps, don't see anything obvious wrong with the POM etc. You might want to look at the readme.md and the i18n files as they seem to be just the placeholders.

clinique

Partial review

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

...ain/java/org/openhab/binding/siemenshvac/internal/Metadata/SiemensHvacMetadataDataPoint.java

bundles/org.openhab.binding.siemenshvac/src/main/resources/OH-INF/binding/binding.xml

...ain/java/org/openhab/binding/siemenshvac/internal/type/SiemensHvacThingTypeProviderImpl.java

...n/java/org/openhab/binding/siemenshvac/internal/type/SiemensHvacChannelTypeProviderImpl.java

...a/org/openhab/binding/siemenshvac/internal/type/SiemensHvacChannelGroupTypeProviderImpl.java

...c/main/java/org/openhab/binding/siemenshvac/internal/network/SiemensHvacRequestListener.java

clinique

Partial review.

lo92fr · 2023-01-24T18:22:59Z

Hello Gael,

Thanks for your review.
I've just updated the pull request with the requested modifications.

Laurent.

lo92fr · 2023-01-24T18:57:50Z

I also manage to fix the build.
Was missing a reference in bom/openhab-addons/pom.xml.

Laurent.

clinique

Second review

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

...c/main/java/org/openhab/binding/siemenshvac/internal/Metadata/RuntimeTypeAdapterFactory.java

...ain/java/org/openhab/binding/siemenshvac/internal/Metadata/SiemensHvacMetadataDataPoint.java

...src/main/java/org/openhab/binding/siemenshvac/internal/network/SiemensHvacConnectorImpl.java

...a/org/openhab/binding/siemenshvac/internal/type/SiemensHvacChannelGroupTypeProviderImpl.java

...n/java/org/openhab/binding/siemenshvac/internal/type/SiemensHvacChannelTypeProviderImpl.java

.../org.openhab.binding.siemenshvac/src/main/resources/OH-INF/i18n/siemenshvac_xx_XX.properties

freichen · 2023-04-04T03:01:02Z

@lo92fr Amazing binding - thank you ! I tested it with an OZW672.01 connected to a RVP350 and the only issue I found was that Enumeration channels are auto-discovered as Number - for example:

lo92fr · 2023-04-16T07:31:05Z

hannels are auto-discovered as Number - for example:

Hello Freichen,

Not sure what are the good way to fix this.
Do you have some guidlines, or some other example binding I can look to fix this.

lo92fr · 2023-04-16T07:32:18Z

Hello Gaël,

@clinique : I'm not sure what are the current state of this pull request.
Do you wait more change from my side ?

larnalcegid · 2023-04-28T16:17:38Z

Hello,

Can some one can give an update about the integration of this pull request in main stream.

Thanks,
Laurent.

larnalcegid · 2023-04-28T16:18:56Z

@lo92fr Amazing binding - thank you ! I tested it with an OZW672.01 connected to a RVP350 and the only issue I found was that Enumeration channels are auto-discovered as Number - for example:

I've commit a fix for this, should be working as excepted now.

larnalcegid · 2023-05-18T05:41:56Z

Hello Gaël, Hello Freichen,

Any update on this ?
Let me know if any more changed is need.

Laurent.

lo92fr · 2024-05-07T07:56:39Z

@lo92fr - I see a few commits here and there, but I haven't seen you respond to any of my comments for months. I'm starting to wonder if you somehow got into a state where your own responses by GitHub are considered as a review comments not yet submitted? See https://github.com/orgs/community/discussions/10369

Hello Jacob,

Yes, sorry for this. I have miss the step of submiting the review, so you don't see my comment.
It explains why you ask me many time to complete the review !

I do it this morning, so you shoudl see them now (please confirm if it's ok now).
I will also do a pass this morning to verify if there is some unresolved comment.

Thks,
Laurent.

Signed-off-by: Laurent ARNAL <laurent@clae.net>

lo92fr

I finish the review on my side

Signed-off-by: Laurent ARNAL <laurent@clae.net>

jlaur

I have checked most comment resolutions with some follow-ups on some of them, and a few new comments.

...ain/java/org/openhab/binding/siemenshvac/internal/converter/type/TimeOfDayTypeConverter.java

jlaur · 2024-05-07T18:02:12Z

...main/java/org/openhab/binding/siemenshvac/internal/converter/type/DateTimeTypeConverter.java

+
+                    return new DateTimeType(zdt);
+                } catch (ParseException ex) {
+                }


If you throw the exception after the for loop, you will know that parsing failed for all formats.

...main/java/org/openhab/binding/siemenshvac/internal/converter/type/DateTimeTypeConverter.java

jlaur · 2024-05-07T18:09:03Z

...inding.siemenshvac/src/main/java/org/openhab/binding/siemenshvac/internal/type/UidUtils.java

+            result = result.replaceAll("\\p{M}", "");
+        }
+
+        result = result.replace(' ', '_');


My suggestion was specifically for the line where it was put, i.e. line 51 just after the normalization. It was meant to replace the lines below.

...c/src/main/java/org/openhab/binding/siemenshvac/internal/handler/SiemensHvacHandlerImpl.java

...va/org/openhab/binding/siemenshvac/internal/handler/SiemensHvacOZW672BridgeThingHandler.java

.../java/org/openhab/binding/siemenshvac/internal/metadata/SiemensHvacMetadataRegistryImpl.java

...src/main/java/org/openhab/binding/siemenshvac/internal/network/SiemensHvacConnectorImpl.java

jlaur

I'm working my way through all the files, only a few left. I have posted many minor comments, but also a few crucial ones about the model.

At this point I think it would help me if you could post screenshots of a few Things and corresponding channels to gain more understanding of the results of the dynamic creation of everything.

jlaur · 2024-05-11T11:56:08Z

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

+controlBoilerApproval  | Set Boiler Approval (`AUTO`, `OFF`, `ON`)                | String        |          |                         |  R/W      | true
+controlProgram         | Set Program (`OFF`, `NORMAL`, `WARMWATER`, `MANUAL`)     | String        |          |                         |  R/W      | true


These documented channels do not comply to the naming convention - are they auto-generated like that?

.../java/org/openhab/binding/siemenshvac/internal/metadata/SiemensHvacMetadataRegistryImpl.java

jlaur · 2024-05-13T12:10:43Z

.../main/java/org/openhab/binding/siemenshvac/internal/converter/type/NumericTypeConverter.java

+            Number num = (quantityType);
+            valUpdate = num.doubleValue();


Is this the converter used for temperatures? It seems that units are stripped here, is that intentional? I suspect that temperatures are supported based on the README examples and this SiemensHvacMetadataRegistryImpl.getCategory:

} else if (dptUnit.contains("°C")) {

Yes, it is used for temperature.
I'm just handling it as a number as we already attach the unit to the meta data description of the ChannelType (see screenshoot below).

So i'm not sure we need to handle it one more time at this place, even if the API returns the unit as a separate field when reading the value.

For UoM support you would need to add dimension, i.e. Number:Temperature. This will allow the framework to convert to any temperature measurement unit. If you link a Number:Temperature item with unit °F to the Number channel without dimension, the result will be 14 °F for the example above, which is not expected. The binding needs to inform the framework about the unit.

For a Number:Temperature channel, you can update with QuantityType rather than DecimalType. Since the API actually carries the unit, you can even supply this directly when creating the QuantityType. Then the API could even start serving °F, and your item above would still be updated with °C (with correct conversion).

...rc/main/java/org/openhab/binding/siemenshvac/internal/converter/type/RadioTypeConverter.java

...ain/java/org/openhab/binding/siemenshvac/internal/constants/SiemensHvacBindingConstants.java

jlaur · 2024-05-13T12:19:34Z

...ain/java/org/openhab/binding/siemenshvac/internal/constants/SiemensHvacBindingConstants.java

+    public static final String CATEGORY_CHANNEL_PROPS_TEMP = "Temperature";
+    public static final String CATEGORY_CHANNEL_PROPS_TIME = "Time";
+
+    public static final String CATEGORY_CHANNEL_CONTROL_HEATING = "Heating";


Why are these named differently than the ones above ("WIDGETS" vs. "PROPS")?

no good reason, i changed them

CATEGORY_CHANNEL_CONTROL_HEATING is still without "WIDGETS", but perhaps it is better to drop that from all names?

Signed-off-by: Laurent ARNAL <laurent@clae.net>

lo92fr · 2024-05-14T11:00:52Z

Hello Jacob (@jlaur ),

I commit a bunch of fixes this morning about your last review.
I also see your question about screenshoot f the discovered things.
So I've put some screenshoot and explanation on your comment.
Tell me if it's sufficient, or if you're need more explanation.

Laurent.

Signed-off-by: Laurent ARNAL <laurent@clae.net>

lo92fr · 2024-05-14T12:37:16Z

Freichen

Hello @freichen,

I don't know if you see my last message (#14263 (comment)).
Do you have the time to verify if removing the json fix the issue ?

Thanks,
Laurent.

Signed-off-by: Laurent ARNAL <laurent@clae.net>

jlaur · 2024-05-14T13:36:42Z

...inding.siemenshvac/src/main/java/org/openhab/binding/siemenshvac/internal/type/UidUtils.java

+            result = result.replaceAll("\\p{M}", "");
+        }
+
+        result = result.replaceAll("[^a-zA-Z0-9_]", "_").toLowerCase();


I might have made a mistake in a previous suggestion based on the logic being replaced rather than the naming convention:

Suggested change

result = result.replaceAll("[^a-zA-Z0-9_]", "_").toLowerCase();

result = result.replaceAll("[^a-zA-Z0-9-]", "-").toLowerCase();

(not sure if "-" needs to be escaped, i.e. "[^a-zA-Z0-9\-]")

jlaur · 2024-05-15T07:31:01Z

bundles/org.openhab.binding.siemenshvac/src/main/resources/OH-INF/thing/ozw672.xml

+	xsi:schemaLocation="https://openhab.org/schemas/thing-description/v1.0.0 https://openhab.org/schemas/thing-description-1.0.0.xsd">
+
+	<!-- Bridge Thing Type -->
+	<bridge-type id="ozw672">


I was thinking about #14263 (comment) on my way to work and to provide some additional context. I then came also to think about the bridge type id. If the binding also works with OZW772.x, or might be brought to work with that as well, probably this id is too specialized. I'm thinking about two possible scenarios:

OZW772.x is very different code-wise and a new bridge type would have to be introduced.

OZW772.x is very similar code-wise, and it can be tweaked to work with the same bridge type, perhaps with some different configuration.

If actually it is expected to be similar, it would be strange to create a bridge type ozw672 for a OZW772.

My proposal in this case would be to rename to a more generic name, perhaps the common denominator "ozw"?

jlaur · 2024-05-15T10:07:57Z

...src/main/java/org/openhab/binding/siemenshvac/internal/network/SiemensHvacConnectorImpl.java

+        try {
+            c.add(new URI(baseUri), cookie);
+        } catch (URISyntaxException ex) {
+            throw new SiemensHvacException(String.format("URI is not correctly formated: %s", baseUri), ex);


Suggested change

throw new SiemensHvacException(String.format("URI is not correctly formated: %s", baseUri), ex);

throw new SiemensHvacException(String.format("URI is not correctly formatted: %s", baseUri), ex);

jlaur · 2024-05-15T10:17:04Z

...java/org/openhab/binding/siemenshvac/internal/handler/SiemensHvacBridgeBaseThingHandler.java

+            updateStatus(ThingStatus.ONLINE);
+        } catch (SiemensHvacException ex) {
+            updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
+                    String.format("@text/offline.error-gateway-init", ex.getMessage()));


Suggested change

String.format("@text/offline.error-gateway-init", ex.getMessage()));

"@text/offline.error-gateway-init[\"" + ex.getMessage() + "\"]");

See corresponding comment for the I18N text.

jlaur · 2024-05-15T10:17:30Z

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

+offline.waiting-bridge-initialization = Waiting bridge initialization, reading metadata in background
+
+offline.baseurl-mandatory = baseUrl is mandatory on configuration !
+offline.error-gateway-init = Error occurs during gateway initialization: %s


Suggested change

offline.error-gateway-init = Error occurs during gateway initialization: %s

offline.error-gateway-init = Error occurred during gateway initialization: {0}

jlaur · 2024-05-15T10:18:24Z

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

+
+offline.waiting-bridge-initialization = Waiting bridge initialization, reading metadata in background
+
+offline.baseurl-mandatory = baseUrl is mandatory on configuration !


Suggested change

offline.baseurl-mandatory = baseUrl is mandatory on configuration !

offline.baseurl-mandatory = baseUrl is mandatory on configuration.

jlaur · 2024-05-15T20:50:55Z

...vac/src/main/java/org/openhab/binding/siemenshvac/internal/network/SiemensHvacConnector.java

+public interface SiemensHvacConnector {
+
+    @Nullable
+    String doBasicRequest(String uri) throws Exception;


Suggested change

String doBasicRequest(String uri) throws Exception;

String doBasicRequest(String uri) throws SiemensHvacException;

jlaur · 2024-05-15T20:54:36Z

.../java/org/openhab/binding/siemenshvac/internal/metadata/SiemensHvacMetadataRegistryImpl.java

+                    }
+                }
+            }
+        } catch (Exception e) {


You can either wrap the exception from doBasicRequest:

Suggested change

} catch (Exception e) {

} catch (SiemensHvacException e) {

Or not catch it at all, letting the original exception be thrown to the caller of this method.

I don't think there are any other exceptions to handle here?

jlaur · 2024-05-15T21:35:44Z

...main/java/org/openhab/binding/siemenshvac/internal/converter/type/DateTimeTypeConverter.java

+                    }
+
+                    LocalDateTime parsedDate = LocalDateTime.parse(value.getAsString(),
+                            formatterBuilder.toFormatter(Locale.FRENCH));


Is this product only for the French market, or why do you need hardcoded French Locale here? Can you configure the OZW webserver language - if yes, how will such a change affect this method?

lo92fr requested a review from a team as a code owner January 22, 2023 19:28

lo92fr force-pushed the 14262-siemensHvac branch 2 times, most recently from dc1076f to 737f2f3 Compare January 22, 2023 19:41

lsiepel changed the title ~~Add a binding for siemens Hvac interface (OZW672)~~ [siemenshvac] Initial contribution - interface (OZW672) Jan 22, 2023

lsiepel added new binding If someone has started to work on a binding. For a new binding PR. rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 22, 2023

lsiepel linked an issue Jan 24, 2023 that may be closed by this pull request

Add a binding for siemens Hvac interface (OZW672) #14262

Open

clinique requested changes Jan 24, 2023

View reviewed changes

lo92fr force-pushed the 14262-siemensHvac branch 2 times, most recently from bc727ff to 66445a4 Compare January 24, 2023 18:50

clinique requested changes Jan 25, 2023

View reviewed changes

lo92fr force-pushed the 14262-siemensHvac branch from 2b83f89 to 0c8f02f Compare February 4, 2023 13:57

lo92fr force-pushed the 14262-siemensHvac branch from 0c8f02f to da5d4b8 Compare February 11, 2023 17:04

lo92fr requested a review from clinique February 11, 2023 17:07

lo92fr force-pushed the 14262-siemensHvac branch from 81d5e7d to 992ebe1 Compare April 16, 2023 11:00

larnalcegid approved these changes Apr 21, 2023

View reviewed changes

larnalcegid approved these changes Apr 28, 2023

View reviewed changes

lo92fr added 6 commits May 7, 2024 11:36

fixes for last jlaur review

d007a84

Signed-off-by: Laurent ARNAL <laurent@clae.net>

fix CI error

a237d99

Signed-off-by: Laurent ARNAL <laurent@clae.net>

spotless:apply

82eb3bf

Signed-off-by: Laurent ARNAL <laurent@clae.net>

fix CI error

a2fc00b

Signed-off-by: Laurent ARNAL <laurent@clae.net>

fix CI errors

74e5340

Signed-off-by: Laurent ARNAL <laurent@clae.net>

fix CI errors

9fd33f8

Signed-off-by: Laurent ARNAL <laurent@clae.net>

lo92fr commented May 7, 2024

View reviewed changes

add autodiscovering feature

6fa6aae

Signed-off-by: Laurent ARNAL <laurent@clae.net>

jlaur requested changes May 7, 2024

View reviewed changes

jlaur requested changes May 13, 2024

View reviewed changes

lo92fr added 2 commits May 14, 2024 12:29

spotless:apply

0fbec96

Signed-off-by: Laurent ARNAL <laurent@clae.net>

div fixes based on last JLaur review

0c3c6a7

Signed-off-by: Laurent ARNAL <laurent@clae.net>

lo92fr force-pushed the 14262-siemensHvac branch from 24944ab to 0c3c6a7 Compare May 14, 2024 10:29

lo92fr added 2 commits May 14, 2024 12:34

fix unit test

bcdfa37

Signed-off-by: Laurent ARNAL <laurent@clae.net>

fix code violations

5c1d1a6

Signed-off-by: Laurent ARNAL <laurent@clae.net>

lo92fr added 2 commits May 14, 2024 13:08

review upnp discovery

907373f

Signed-off-by: Laurent ARNAL <laurent@clae.net>

fix autodiscovery

b0957b7

Signed-off-by: Laurent ARNAL <laurent@clae.net>

change json cache file location

088cf37

Signed-off-by: Laurent ARNAL <laurent@clae.net>

jlaur reviewed May 14, 2024

View reviewed changes

jlaur reviewed May 15, 2024

View reviewed changes

		controlBoilerApproval \| Set Boiler Approval (`AUTO`, `OFF`, `ON`) \| String \| \| \| R/W \| true
		controlProgram \| Set Program (`OFF`, `NORMAL`, `WARMWATER`, `MANUAL`) \| String \| \| \| R/W \| true

	result = result.replaceAll("[^a-zA-Z0-9_]", "_").toLowerCase();
	result = result.replaceAll("[^a-zA-Z0-9-]", "-").toLowerCase();

	throw new SiemensHvacException(String.format("URI is not correctly formated: %s", baseUri), ex);
	throw new SiemensHvacException(String.format("URI is not correctly formatted: %s", baseUri), ex);

	String.format("@text/offline.error-gateway-init", ex.getMessage()));
	"@text/offline.error-gateway-init[\"" + ex.getMessage() + "\"]");

	offline.error-gateway-init = Error occurs during gateway initialization: %s
	offline.error-gateway-init = Error occurred during gateway initialization: {0}


		offline.waiting-bridge-initialization = Waiting bridge initialization, reading metadata in background

		offline.baseurl-mandatory = baseUrl is mandatory on configuration !

	String doBasicRequest(String uri) throws Exception;
	String doBasicRequest(String uri) throws SiemensHvacException;

[siemenshvac] Initial contribution - interface (OZW672) #14263

Are you sure you want to change the base?

[siemenshvac] Initial contribution - interface (OZW672) #14263

Conversation

lo92fr commented Jan 22, 2023

Title

Description

Testing

lo92fr commented Jan 22, 2023

lsiepel commented Jan 23, 2023

clinique left a comment

Choose a reason for hiding this comment

clinique left a comment

Choose a reason for hiding this comment

lo92fr commented Jan 24, 2023

lo92fr commented Jan 24, 2023

clinique left a comment

Choose a reason for hiding this comment

freichen commented Apr 4, 2023

lo92fr commented Apr 16, 2023

lo92fr commented Apr 16, 2023

larnalcegid commented Apr 28, 2023

larnalcegid commented Apr 28, 2023

larnalcegid commented May 18, 2023

lo92fr commented May 7, 2024

lo92fr left a comment

Choose a reason for hiding this comment

jlaur left a comment

Choose a reason for hiding this comment

jlaur May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlaur 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

jlaur May 14, 2024 • edited

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

lo92fr commented May 14, 2024

lo92fr commented May 14, 2024

jlaur May 14, 2024 • edited

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

jlaur May 7, 2024 •

edited

jlaur May 14, 2024 •

edited

jlaur May 14, 2024 •

edited