[openthermgateway] Add support for Ventilation/Heat Recovery units #11203

ArjenKorevaar · 2021-09-04T07:46:12Z

Added support for Ventilation/Heat Recovery units to the OpenTherm Gateway binding.

More info at #11202

Solar Boiler units are currently only partially supported by the OpenTherm Gateway device itself, so this may be added to the binding at a later time.

openhab-bot · 2021-09-04T07:52:17Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/opentherm-gateway-binding/39160/294

ArjenKorevaar · 2021-09-04T12:04:08Z

Not sure why the build failed, it seems unrelated to the changes made to the binding.

Stacktrace

org.opentest4j.AssertionFailedError: Error cleaning up test (deleting table)
Caused by: java.util.concurrent.ExecutionException: software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException: Cannot do operations on a non-existent table (Service: DynamoDb, Status Code: 400, Request ID: 5c0b87d4-911e-4f17-a872-69038f24a1dd, Extended Request ID: null)
Caused by: software.amazon.awssdk.services.dynamodb.model.ResourceNotFoundException: Cannot do operations on a non-existent table (Service: DynamoDb, Status Code: 400, Request ID: 5c0b87d4-911e-4f17-a872-69038f24a1dd, Extended Request ID: null)

ArjenKorevaar · 2021-09-14T06:38:44Z

Rebased onto main, now PR build succeeds

ArjenKorevaar · 2021-11-27T09:09:12Z

Rebased onto main

fwolter

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html after building it with mvn clean install.

Are there any breaking changes like renamed channels or renamed Thing/Bridge UIDs?

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

fwolter · 2021-12-11T14:24:04Z

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

+
+| Channel ID | Item Type | Description | Access |
+|------------|-----------|-------------|--------|
+| vh_ventilationenable | Switch | Ventilation enabled | R |


What's the reason for adding a suffix to the channel names?

The names of the channels are mostly a copy/paste from the OpenTherm specifications. And those names do not always follow openHAB coding guidlines when it comes to the number of words, length in characters or things like using a suffix. So I find myself abbreviating or leaving words out.. just to comply with coding guidlines really but not making anything better in terms of readability imho or matching with the OT specs. So.. I don't know.. should I change them?

I'd remove the prefix as it seems redundant for all Channels.

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

...ermgateway/src/main/java/org/openhab/binding/openthermgateway/handler/BaseDeviceHandler.java

bundles/org.openhab.binding.openthermgateway/src/main/resources/OH-INF/thing/channels.xml

ArjenKorevaar · 2021-12-18T10:22:27Z

@fwolter thank you for the review! I will look into each comment but this may take some time. I'll let you know once it's ready to be reviewed again.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html after building it with mvn clean install.

Fixed most of them.. just the import of .* I haven't found a more elegant solution to

Are there any breaking changes like renamed channels or renamed Thing/Bridge UIDs?

Yes, definitely. The previous version had 1 Thing and that was used for boilers only. This version has a Bridge to communicate with the gateway and then separate Things for the actual devices, which allowed for support for ventilation/heat recovery units. In the future, more Things may be added such as solar panels and energy storage units.

Signed-off-by: Arjen Korevaar <[email protected]>

fwolter · 2021-12-27T20:18:27Z

Fixed most of them.. just the import of .* I haven't found a more elegant solution to

You can simply use the Organize Imports function of eclipse to resolve these.

I added the portentially not backward compatible label. The changes should be mentioned in the update.lst to show a warning when the user updates the system. See https://github.com/openhab/openhab-distro/blob/main/distributions/openhab/src/main/resources/bin/update.lst

andrewfg · 2022-02-02T17:30:43Z

Ok @ArjenKorevaar, this is going to take me more time than I thought, so I will do it in three (at least) steps..

Review of potential merge conflicts between the two files (OpenThermGatewaySocketConnector & OpenThermGatewayHandler) that are touched in my [openthermgateway] Rationalize exception handling & decouple tasks on different threads. #12185 and in this PR. I have good reasons why I think my fixes are better than yours, which I will mention in the review. And I will propose the correct respective core that would resolve the merge conflicts
Code review of all the other files in this PR.
Functional test on my operative system.

andrewfg

Review step 1 on the OpenThermGatewaySocketConnector and OpenThermGatewayHandler classes..

...main/java/org/openhab/binding/openthermgateway/internal/OpenThermGatewaySocketConnector.java

...eway/src/main/java/org/openhab/binding/openthermgateway/handler/OpenThermGatewayHandler.java

andrewfg · 2022-02-02T18:46:25Z

Code review of all the other files in this PR.

@ArjenKorevaar before I start work on the general code review, I would like to ask your opinion about how you prefer to handle it. There are two possible ways..

I could make review comments and proposed changes here in this PR; the advantage is that all proposals are discussed and formally tracked in this PR; and the disadvantage is that you have to make the requested changes in your IDE yourself and make respective commits, or
Alternatively I could derive a git branch on my repo from your own repo/branch, and make the changes directly in my IDE, and then make a 'sub-' PR from my repo/branch to your repo/branch; which if you would adopt my code changes, will then automatically flow into this PR here; the discussions and changes would then be tracked on your repo/branch, rather than here, but the advantage for you is that it saves you making the requested changes in your IDE yourself.

So it depends a bit on your workload and what you prefer. => Please let me know..

Signed-off-by: Andrew Fiddian-Green <[email protected]>

ArjenKorevaar · 2022-02-03T12:56:12Z

@andrewfg just a heads-up: I won't be able to spend time to look into this any time soon.

andrewfg · 2022-02-03T16:39:41Z

When I am done, I will make a PR from my repo/branch to your repo/branch, so that if you merge my changes, they will cascade into this PR

.. well that did not go quite as I expected .. I really hate Git! Instead of pushing my proposed changes to a PR on @ArjenKorevaar 's repo (so he could optionally choose whether to adopt them or not, and then cascade into this PR), it seems that all of my commits landed directly in this PR automatically! (I don't really know how that happened..) But anyway, @ArjenKorevaar if you are NOT happy with my suggestions, then you will need to unwind the 16 commits made by me today February 3rd, 2022.

Please accept my humble apologies for the unintended usurpation of your PR. The good news is that all of my code review suggestions, (and some of @fwolter 's still open suggestions), are now included in the PR, so I can now give you an LGTM on the final result!

andrewfg · 2022-02-03T17:13:16Z

PS I note the following..

the CI build failed, but I think it is not the fault of this code ??
it also reports potential merge conflicts on two files, but I am not clever enough to resolve them..

andrewfg · 2022-02-03T17:23:57Z

potential merge conflicts on two files

The two files OpenThermGatewayBindingConstants.java and DataType.java are ones that have been moved (to another package in the project) or re-moved entirely from the project. I have no idea why the CI build system chokes on that, but I can assure you that it all builds fine locally in Eclipse or MVN.

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg · 2022-02-03T18:37:26Z

^
I don't know if this helps, but regarding the merge conflicts (see screenshot below)..

OpenThermGatewayBindingConstants.java is tagged in Git as being a) deleted, and then b) added
DataType.java is tagged in Git as being renamed to ConnectionState.java

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

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg · 2022-02-07T18:42:29Z

One of my prior commits had broken the Thing initialization, so I pushed a fix just now.
Also I have installed the current JAR on my operative system, and will let it run fir a day or two.

...ermgateway/src/main/java/org/openhab/binding/openthermgateway/handler/BaseDeviceHandler.java

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg · 2022-02-11T15:31:36Z

@ArjenKorevaar there are a couple of open issues that you need to resolve concerning the review from @fwolter

use of logger.debug() in a couple of places
use of vh_ prefix on channel names

@fwolter I wonder if you can kindly help us with the merge conflict?

fwolter · 2022-02-20T18:42:23Z

Yes, how can I help?

andrewfg · 2022-02-20T18:49:56Z

how can I help?

@fwolter the merge conflict is due to two files where the Git change tracker is not clever enough to recognise the changes..

one is a renamed and repurposed class (was DataType, repurposed to ConnectionState) where it seems that the changes were so big that GitHub cannot recognise the change
the second is a file (OpenthermGatewayBindingConstants) was moved to a different sub directory

Unfortunately I dont know enough about Git commands to tell it how to unwind its confusion.

fwolter · 2022-02-20T18:58:51Z

There were other commits in the meantime, modifying the two files. That's why they are conflicting in this PR. @ArjenKorevaar you need to rebase your branch with git rebase upstream/main and resolve the conflicts by hand when executing the command.

andrewfg · 2022-02-23T16:24:50Z

you need to rebase your branch with git rebase upstream/main and resolve the conflicts by hand when executing the command.

@fwolter , @ArjenKorevaar told me that he is quite busy and does not have much time to work on this.

So as an alternative potential approach I created this new branch on my repo, to which I have copied all the files from this PR, and also also rebased it on the current main branch, thus removing the merge conflicts.

So if we would proceed with a PR based on my new branch rather than proceeding with this PR, we could potentially speed up the review and approval process since I myself would be able to do all the remaining work, without imposing further effort on @ArjenKorevaar.

However before I do this, can you please give me the Ok?

fwolter · 2022-02-23T16:28:12Z

Sounds good.

ArjenKorevaar · 2022-02-24T07:04:59Z

Sounds good to me too. I am currently abroad and won't be able to work on this for atleast another couple of weeks.

fwolter · 2022-02-27T16:16:08Z

Closed in favor of #12367.

Skinah added the enhancement An enhancement or new feature for an existing add-on label Sep 7, 2021

kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 9, 2021

ArjenKorevaar force-pushed the bridge_3.2.x branch from a0dd7a8 to 8ab3173 Compare September 13, 2021 17:39

ArjenKorevaar changed the title ~~[openthermgateway] Add support for Ventilation/Heat Recovery units~~ PTAL : [openthermgateway] Add support for Ventilation/Heat Recovery units Nov 12, 2021

ArjenKorevaar force-pushed the bridge_3.2.x branch from 8ab3173 to 7042170 Compare November 27, 2021 07:49

fwolter requested changes Dec 11, 2021

View reviewed changes

fwolter changed the title ~~PTAL : [openthermgateway] Add support for Ventilation/Heat Recovery units~~ [openthermgateway] Add support for Ventilation/Heat Recovery units Dec 11, 2021

ArjenKorevaar added 6 commits December 18, 2021 12:11

Added support for Ventilation/Heat Recovery units

18a6645

Signed-off-by: Arjen Korevaar <[email protected]>

Fixed vh_ventilationsetpoint issue and updated readme

887700e

Signed-off-by: Arjen Korevaar <[email protected]>

TSP and FHB indices are zero-based

45de4f4

Signed-off-by: Arjen Korevaar <[email protected]>

Updated README

77a1629

Signed-off-by: Arjen Korevaar <[email protected]>

Replaced == with equals()

3e06fa2

Signed-off-by: Arjen Korevaar <[email protected]>

Fixed a few code analysis messages and review comments

210066c

Signed-off-by: Arjen Korevaar <[email protected]>

ArjenKorevaar force-pushed the bridge_3.2.x branch from 19639a5 to 210066c Compare December 18, 2021 11:11

fwolter added the (potentially) not backward compatible label Dec 27, 2021

ArjenKorevaar mentioned this pull request Feb 1, 2022

[openthermgateway] Rationalize exception handling & decouple tasks on different threads. #12185

Closed

andrewfg requested changes Feb 2, 2022

View reviewed changes

andrewfg added 3 commits February 3, 2022 12:41

[openthermgateway] align communication fixes & synchronization

986298c

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[openthermgateway] fix potential NPE & decouple thread tasks

ebb99c8

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[openthermgateway] delete unnecessary method

b36ac99

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg approved these changes Feb 3, 2022

View reviewed changes

[openthermgateway] remove potential NPE warnings

269f4b9

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg requested changes Feb 3, 2022

View reviewed changes

bundles/org.openhab.binding.openthermgateway/README.md Show resolved Hide resolved

[openthermgateway] restore diag channel to boiler

47d4696

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg approved these changes Feb 5, 2022

View reviewed changes

andrewfg added 2 commits February 7, 2022 18:38

[openthermgateway] update readme for Number:Time items

d181d62

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[openthermgateway] fix thing initialization

344e89d

Signed-off-by: Andrew Fiddian-Green <[email protected]>

jlaur reviewed Feb 7, 2022

View reviewed changes

...ermgateway/src/main/java/org/openhab/binding/openthermgateway/handler/BaseDeviceHandler.java Outdated Show resolved Hide resolved

jlaur reviewed Feb 7, 2022

View reviewed changes

...ermgateway/src/main/java/org/openhab/binding/openthermgateway/handler/BaseDeviceHandler.java Show resolved Hide resolved

jlaur reviewed Feb 7, 2022

View reviewed changes

...ermgateway/src/main/java/org/openhab/binding/openthermgateway/handler/BaseDeviceHandler.java Outdated Show resolved Hide resolved

andrewfg added 2 commits February 10, 2022 18:40

[openthermgateway] sparser logger messages

20039f8

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[openthermgateway] adopt reviewer suggestion

93b9075

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg requested review from fwolter and jlaur February 11, 2022 15:22

andrewfg mentioned this pull request Feb 24, 2022

[openthermgateway] Add support for Ventilation/Heat Recovery units #12367

Merged

fwolter closed this Feb 27, 2022

ArjenKorevaar deleted the bridge_3.2.x branch April 7, 2023 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[openthermgateway] Add support for Ventilation/Heat Recovery units #11203

[openthermgateway] Add support for Ventilation/Heat Recovery units #11203

ArjenKorevaar commented Sep 4, 2021

openhab-bot commented Sep 4, 2021

ArjenKorevaar commented Sep 4, 2021

ArjenKorevaar commented Sep 14, 2021

ArjenKorevaar commented Nov 27, 2021 •

edited

Loading

fwolter left a comment

fwolter Dec 11, 2021

ArjenKorevaar Dec 18, 2021

fwolter Dec 27, 2021

ArjenKorevaar commented Dec 18, 2021 •

edited

Loading

fwolter commented Dec 27, 2021

andrewfg commented Feb 2, 2022

andrewfg left a comment

andrewfg commented Feb 2, 2022 •

edited

Loading

ArjenKorevaar commented Feb 3, 2022

andrewfg commented Feb 3, 2022 •

edited

Loading

andrewfg commented Feb 3, 2022

andrewfg commented Feb 3, 2022

andrewfg commented Feb 3, 2022 •

edited

Loading

andrewfg commented Feb 7, 2022

andrewfg commented Feb 11, 2022

fwolter commented Feb 20, 2022

andrewfg commented Feb 20, 2022 •

edited

Loading

fwolter commented Feb 20, 2022

andrewfg commented Feb 23, 2022

fwolter commented Feb 23, 2022

ArjenKorevaar commented Feb 24, 2022

fwolter commented Feb 27, 2022

[openthermgateway] Add support for Ventilation/Heat Recovery units #11203

[openthermgateway] Add support for Ventilation/Heat Recovery units #11203

Conversation

ArjenKorevaar commented Sep 4, 2021

openhab-bot commented Sep 4, 2021

ArjenKorevaar commented Sep 4, 2021

ArjenKorevaar commented Sep 14, 2021

ArjenKorevaar commented Nov 27, 2021 • edited Loading

fwolter left a comment

Choose a reason for hiding this comment

fwolter Dec 11, 2021

Choose a reason for hiding this comment

ArjenKorevaar Dec 18, 2021

Choose a reason for hiding this comment

fwolter Dec 27, 2021

Choose a reason for hiding this comment

ArjenKorevaar commented Dec 18, 2021 • edited Loading

fwolter commented Dec 27, 2021

andrewfg commented Feb 2, 2022

andrewfg left a comment

Choose a reason for hiding this comment

andrewfg commented Feb 2, 2022 • edited Loading

ArjenKorevaar commented Feb 3, 2022

andrewfg commented Feb 3, 2022 • edited Loading

andrewfg commented Feb 3, 2022

andrewfg commented Feb 3, 2022

andrewfg commented Feb 3, 2022 • edited Loading

andrewfg commented Feb 7, 2022

andrewfg commented Feb 11, 2022

fwolter commented Feb 20, 2022

andrewfg commented Feb 20, 2022 • edited Loading

fwolter commented Feb 20, 2022

andrewfg commented Feb 23, 2022

fwolter commented Feb 23, 2022

ArjenKorevaar commented Feb 24, 2022

fwolter commented Feb 27, 2022

ArjenKorevaar commented Nov 27, 2021 •

edited

Loading

ArjenKorevaar commented Dec 18, 2021 •

edited

Loading

andrewfg commented Feb 2, 2022 •

edited

Loading

andrewfg commented Feb 3, 2022 •

edited

Loading

andrewfg commented Feb 3, 2022 •

edited

Loading

andrewfg commented Feb 20, 2022 •

edited

Loading