[tacmi] Initial push of OpenHAB 2 ported tacmi binding #7768

marvkis · 2020-05-24T10:36:04Z

Signed-off-by: Christian Niessner [email protected]

TA C.M.I. binding - ported legacy 1.xx addon to 2.xx

Hi. As I was helping a friend connecting his TA system with openhab we had some issues getting the old OpenHAB 1.xx addon to work and there have been some issues within the plugin itself I've started porting it to 2.xx.

Currently this binding should deliver all functionality as the 1.xx binding had with some additional bugfixing and removal of some limitations. Especially each output id should work and not only a few as the cave cat with bundled transfer of multiple values was solved by adding some internal caching so the binding is aware of the current values of the neighboring outputs and can send a complete set of updates.

I've uploaded a pre-compiled jar file to here for testing: https://github.com/marvkis/openhab-addons-dist/raw/master/org.openhab.binding.tacmi-2.5.6-SNAPSHOT.jar

And I've created a thread in the forum where the new version is announced and I asked for testing: https://community.openhab.org/t/tacmi-technische-alternative-ta-c-m-i-binding-upgrade-to-1-xx-2-xx/99416

I've marked the PR as WIP as I have some further idea's for this plugin. Never the less it should provide full functionality as the 1.xx version plugin had, so it might be an idea to merge this one as replacement for the 1.xx plugin and add the new ideas later on.

btw: I've developed this plugin via VSC as OpenHAB in eclipse kills my quite old 2015 mb (non-pro) - and i'm not sure if all formatting rules are applied correctly. So when you see issues here please give me the right pointers what & how to fix it.

Thanks & Bye,
Chris

TravisBuddy · 2020-05-24T10:39:49Z

Travis tests have failed

Hey @marvkis,
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-05-24T15:03:35Z

Travis tests have failed

Hey @marvkis,
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-05-24T15:34:38Z

Travis tests have failed

Hey @marvkis,
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-05-24T15:39:17Z

Travis tests were successful

Hey @marvkis,
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-05-24T16:22:28Z

Travis tests were successful

Hey @marvkis,
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-08-02T14:04:18Z

Travis tests have failed

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

fwolter

Thank you for migrating this binding to OH2, really appreciate it! I know, many of the source code files you took over from the OH1 binding and some of them have legacy code which doesn't fit to our current coding guidelines. But this is the chance to clean up the old code and make a nice OH2 binding! So, I reviewed the whole code and here is my feedback.

The scaling of the picture sample-with-heating-circuit.png seems a bit large.

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

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

CODEOWNERS

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

bundles/org.openhab.binding.tacmi/src/main/resources/ESH-INF/thing/bridge.xml

bundles/org.openhab.binding.tacmi/src/main/resources/ESH-INF/thing/thing-types.xml

fwolter · 2020-08-03T20:54:34Z

bundles/org.openhab.binding.tacmi/src/main/resources/ESH-INF/thing/thing-types.xml

+	</channel-type>
+
+	<thing-type id="cmiSchema" extensible="schema-switch-ro,schema-switch-rw,schema-numeric-ro,schema-state-ro">
+		<label>TA C.M.I. schema API connection</label>


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

fwolter · 2020-08-03T20:55:59Z

bom/openhab-addons/pom.xml

+    <dependency>
+      <groupId>org.openhab.addons.bundles</groupId>
+      <artifactId>org.openhab.binding.cacmi</artifactId>
+      <version>${project.version}</version>
+    </dependency>


marvkis

Hi @fwolter ,

Thank's a lot for your Review! Sorry for the initial flood of messages, it's the first time I'm using github's review function.
I have some questions on some points, but the most of them sound clear and straight forward. I Hope I'll find the time next weekend to incorporate the changes into the code.
Would be great when you give me some hints on the things that are not clear !

Thanks a lot & Bye,
Chris

marvkis · 2020-08-04T19:36:09Z

...org.openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/TACmiHandler.java

+        if (command instanceof RefreshType) {
+            // this is not supported - we cannot pull states...


Yes, this would be possible. But from my observations theses Refresh-Messages only occur during startup when we don't have data received from the TA equipment. TA only sends data periodically or on change, but there is no known way of polling current values.

...hab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/ApiPageParser.java

marvkis · 2020-08-04T19:54:15Z

...inding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/TACmiSchemaHandler.java

+            // TODO how to debounce this? we could trigger refreshData() but during startup
+            // this issues lots of requests... :-/


Okay, I definitively have to have a look on the cache classes. I'm not aware of them.

marvkis · 2020-08-04T19:59:15Z

....openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/message/Message.java

+        setPodNumber(podNumber);
+    }
+
+    public abstract void debug(Logger logger);


good question. Is from the original binding. Might be stale - I already removed a bunch of stale methods. Will dig into it...

TravisBuddy · 2020-08-04T20:22:53Z

Travis tests were successful

Hey @marvkis,
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-08-09T15:34:55Z

Travis tests were successful

Hey @marvkis,
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-08-09T15:39:03Z

Travis tests were successful

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

marvkis · 2020-08-09T15:57:46Z

Hi @fwolter,

I just pushed some updates. There are currently 3 open things left I'm aware:

The 'persistence service': I had the idea to make it configurable and disabled it by default, as I assume most users could life without it. The few users who really need this feature could enable it. This comes with this commit: 0287bcf0d . Maybe this is a compromise for now. Never the less I'll start a thread in OH's developer forum on this when I have some time left this evening.
I'm still struggling with the @NonNullByDefault in ApiPageParser and ChangerX2Parser and this wired inheritance error message. No Idea how to solve it and it seems i also have the wrong keywords for my favorite search machine ;). Do you have me a hint on this or any other ideas how to proceed?
You asked me to narrow down the exception caught in TACmiCoEBridgeHandler::ReceiveThread::run(). I'm catching RuntimeException here now as I want to keep the thread running even if some wired situation occurs that could trigger things like NumberConversionFailed or IllegalArgumentException, 'IllegalStateException' and so on. As the TA stuff is very flexible I have no idea what people could do so I would prefer to catch and log this here and also try to keep things running.

Everything else should be done. I hope I got everything form your valuable review and applied it accordingly.

Thanks & Bye,
Chris

fwolter · 2020-08-11T21:22:26Z

* I'm still struggling with the `@NonNullByDefault` in `ApiPageParser` and `ChangerX2Parser` and this wired inheritance error message.

I didn't manage to fix the compiler error either.

@openhab/add-ons-maintainers There is an overriden method from a non-annotated class. A String argument works fine, but the Map argument fails with "Illegal redefinition of parameter attributes, inherited method from AbstractSimpleMarkupHandler declares this parameter as 'Map<String,String>' (mismatching null constraints)". Here is the signature:

    public void handleStandaloneElement(final @Nullable String elementName,
            final @Nullable Map<String, String> attributes, final boolean minimized, final int line, final int col)
            throws ParseException {

When using the raw type Map without generics, the error is gone. Any idea what is wrong here?

fwolter · 2020-08-14T19:29:34Z

@marvkis Did you take a look at https://www.openhab.org/docs/configuration/persistence.html#restoring-item-states-on-restart ? You could leave it up to the user to configure this. Then, you don't need to handle it in the binding.

marvkis · 2020-08-19T13:14:54Z

@marvkis Did you take a look at https://www.openhab.org/docs/configuration/persistence.html#restoring-item-states-on-restart ? You could leave it up to the user to configure this. Then, you don't need to handle it in the binding.

Hi @fwolter ,
Thank you for pointing this out. I have this configured on my system, and this is primarily for restoring the item states - but I need it in the channel state. Later in the article there is a discussion on how to handle rules during the startup phasis as there seems to be a time gap between triggering "Startup started" rules and the real "item value restore". Never the less, there might be a way to trigger some rules that update the channel states from the item state after the restore has completed. But this leads me to the next issue: As already stated the communication to the TA updates a bunch of values. When the rule updates the first channel the data update is sent to the TA - with one channel holding the valid data and all other channels having a wrong data. Okay, all the other channels will be updated within a small time frame - but here we hit an other issue: The TA system has some integrated de-bouncing & validity mechanism that will be triggered in this situation and so the system sees the wrong values for the other channels for quite a long time...

So, when using a external persistence it seems we have to find a way to update all channels 'at once'...

I took me the time to open a post in the forum: https://community.openhab.org/t/channel-state-persistence-in-a-binding/103868 - I hope somebody will come up with a neat idea how to solve it ;)

marvkis · 2020-08-19T13:17:51Z

Hi @fwolter,

how should we proceed on this point:

I'm still struggling with the @NonNullByDefault in ApiPageParser and ChangerX2Parser and this wired inheritance error message.

I didn't manage to fix the compiler error either.

Shall I also open a thread in developers forum or shall we go without @NonNullByDefault - what do you thing?

Thanks & Bye,
Chris

fwolter · 2020-08-20T21:35:26Z

Shall I also open a thread in developers forum or shall we go without @NonNullByDefault - what do you thing?

You can disable the null check for the specific methods by annotating them with @NonNullByDefault({}).

fwolter · 2020-09-05T10:28:52Z

As the merge window for 2.5.9 comes closer and closer, do you want this get merged before OH3?

TravisBuddy · 2020-09-05T12:03:37Z

Travis tests have failed

Hey @marvkis,
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: Christian Niessner <[email protected]>

…ing stuff... Signed-off-by: Christian Niessner <[email protected]>

TravisBuddy · 2020-09-07T19:51:37Z

Travis tests were successful

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

Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Christian Niessner <[email protected]>

…ging output, Exception handling Signed-off-by: Christian Niessner <[email protected]>

…xception improvements Signed-off-by: Christian Niessner <[email protected]>

TravisBuddy · 2020-09-07T19:56:26Z

Travis tests were successful

Hey @marvkis,
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 · 2020-09-08T18:05:17Z

...inding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/TACmiSchemaHandler.java

+            updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR, "Error: " + e.getMessage());
+            this.online = false;
+        } catch (final TimeoutException | ExecutionException e) {
+            logger.warn("Error loading API Scheme: {} ", e.getMessage());


You don't need to log this, as updateStatus() already does it.

Damn, I didn't learn it. Sorry bothering you with this again... Having the logs from updateStatus() I changed the level to debug for the parser errors so when the stack trace is needed the user has to enable debug logging...

Signed-off-by: Christian Niessner <[email protected]>

TravisBuddy · 2020-09-08T21:26:45Z

Travis tests were successful

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

LGTM

Hilbrand

Thanks for migrating this binding to openHAB 2. I've done my review, mostly style improvements next to 3 main topics: Use of log to warn, use of specific detailed state information with state updates and suggestion about if you considered making type specific channels besides the generic output channel. These topics are covered in the review comments. The first 2 of these topcs cover multiple places, I haven't marked every occurrence.

bundles/org.openhab.binding.tacmi/NOTICE

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

...openhab.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/coe/TACmiHandler.java

Hilbrand · 2020-09-09T15:01:08Z

...b.binding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/ChangerX2Parser.java

+    }
+
+    @Override
+    @NonNullByDefault({})


NonNullByDefault should normally not be set on methods.

We had trouble finding a proper way to annotate the Map<> in the (inherited) method definition properly with @Nullable - all our testes ended with errors like "Illegal redefinition of parameter attributes, inherited method from AbstractSimpleMarkupHandler declares this parameter as 'Map<String,String>' (mismatching null constraints)". When you review the discussion here in the PR it was quite a topic. Key comments on this are here and here. If you have a better idea to solve this just enlighten me ;)

Regarding the map I suspect it might need to be some combination of Nullable placed somewhere:
@Nullable Map<@Nullable String, @Nullable String>

I tried quite every variant making sense, but I wasn't able to get it working. This is for the variant you suggested:

When compiling on the command line with mvn it shows the same error...

...inding.tacmi/src/main/java/org/openhab/binding/tacmi/internal/schema/TACmiSchemaHandler.java

Hilbrand · 2020-09-09T15:26:42Z

bundles/org.openhab.binding.tacmi/src/main/resources/ESH-INF/thing/thing-types.xml

+			<parameter name="type" type="integer" min="0" max="21" required="true">
+				<label>Measurement Type</label>
+				<description>Measurement type for this channel</description>
+				<options>


Did you consider creating type specific channels for measurement type? Like a Number:Temperature channel for value 1 type channels. That way the user can use it as an actual temperature channel.

Not yet - but yes, it's quite obvious and sounds like a good idea. Would it be okay to do this in a later PR? I also plan to find out all "UNKNOWN" types from TACmiMeasureType.java to map to the real units. During this I also could create separate channels for all these different types.

…framework usage cleanups Co-authored-by: Hilbrand Bouwkamp <[email protected]> Signed-off-by: Christian Niessner <[email protected]>

Signed-off-by: Christian Niessner <[email protected]>

TravisBuddy · 2020-09-09T19:33:31Z

Travis tests were successful

Hey @marvkis,
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: Christian Niessner <[email protected]>

TravisBuddy · 2020-09-09T20:29:48Z

Travis tests were successful

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

marvkis · 2020-09-09T20:29:49Z

Hi @Hilbrand I just reviewed my logger.warn messages. Set a bunch of it to .debug. I left messages as warn when they are related to communication errors and configuration problems on TA side. Is this okay?

Hilbrand

LGTM

Migrated from the openHAB 1 version. Signed-off-by: Christian Niessner <[email protected]>

marvkis requested a review from a team as a code owner May 24, 2020 10:36

cpmeister 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 May 25, 2020

Hilbrand mentioned this pull request May 25, 2020

openHAB 1 addon compatibility overview with openHAB 3 #6179

Closed

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

marvkis force-pushed the addon/TA-CMI branch from 434d40d to 20d8d39 Compare August 2, 2020 14:01

marvkis force-pushed the addon/TA-CMI branch from 20d8d39 to 066f08d Compare August 2, 2020 18:10

fwolter requested changes Aug 3, 2020

View reviewed changes

marvkis commented Aug 4, 2020

View reviewed changes

marvkis force-pushed the addon/TA-CMI branch from 261a3da to 16574f0 Compare August 9, 2020 15:36

marvkis added 3 commits September 5, 2020 22:57

[tacmi] Initial push of OpenHAB 2 ported tacmi binding

a8d9a30

Signed-off-by: Christian Niessner <[email protected]>

[tacmi] fixed travis errors & added some @NonNullByDefault

eb8ee35

Signed-off-by: Christian Niessner <[email protected]>

[tacmi] fixed further travis errors & warnings, especially Null handl…

fe7360d

…ing stuff... Signed-off-by: Christian Niessner <[email protected]>

marvkis force-pushed the addon/TA-CMI branch from fce64eb to ddf0e81 Compare September 7, 2020 19:48

marvkis and others added 3 commits September 7, 2020 21:53

[tacmi] Apply suggestions from code review

14893ca

Co-authored-by: Fabian Wolter <[email protected]> Signed-off-by: Christian Niessner <[email protected]>

[tacmi] Additional changes from code review: Thread name, reduced log…

6289423

…ging output, Exception handling Signed-off-by: Christian Niessner <[email protected]>

[tacmi] Additional changes from code review: Thread name, logging & e…

5f5bbed

…xception improvements Signed-off-by: Christian Niessner <[email protected]>

marvkis force-pushed the addon/TA-CMI branch from ddf0e81 to 5f5bbed Compare September 7, 2020 19:53

fwolter requested changes Sep 8, 2020

View reviewed changes

[tacmi] Reduced logging output...

c79ed2a

Signed-off-by: Christian Niessner <[email protected]>

fwolter approved these changes Sep 8, 2020

View reviewed changes

fwolter changed the title ~~[tacmi] [WIP] Initial push of OpenHAB 2 ported tacmi binding~~ [tacmi] Initial push of OpenHAB 2 ported tacmi binding Sep 8, 2020

fwolter added the cre Coordinated Review Effort label Sep 8, 2020

fwolter requested a review from Hilbrand September 8, 2020 21:29

Hilbrand requested changes Sep 9, 2020

View reviewed changes

marvkis and others added 3 commits September 9, 2020 21:29

[tacmi] Apply suggestions from code review: code formatting and some …

6f5f9a1

…framework usage cleanups Co-authored-by: Hilbrand Bouwkamp <[email protected]> Signed-off-by: Christian Niessner <[email protected]>

[tacmi] TACmiHandler - improved thing states and transitions

222858f

Signed-off-by: Christian Niessner <[email protected]>

[tacmi] Additional cleanups & improvements from review

92eb6cf

Signed-off-by: Christian Niessner <[email protected]>

marvkis force-pushed the addon/TA-CMI branch from 3a1f227 to 92eb6cf Compare September 9, 2020 19:30

marvkis requested a review from Hilbrand September 9, 2020 19:59

[tacmi] logging: changed some more loggings from warn to debug

cd25457

Signed-off-by: Christian Niessner <[email protected]>

Hilbrand approved these changes Sep 11, 2020

View reviewed changes

Hilbrand merged commit 623eb57 into openhab:2.5.x Sep 11, 2020

Hilbrand added this to the 2.5.9 milestone Sep 11, 2020

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

[tacmi] Initial contribution tacmi binding (openhab#7768)

56b6e56

Migrated from the openHAB 1 version. Signed-off-by: Christian Niessner <[email protected]>

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

[tacmi] Initial contribution tacmi binding (openhab#7768)

afd5f55

Migrated from the openHAB 1 version. Signed-off-by: Christian Niessner <[email protected]>

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

[tacmi] Initial contribution tacmi binding (openhab#7768)

3402afb

Migrated from the openHAB 1 version. Signed-off-by: Christian Niessner <[email protected]>

		if (command instanceof RefreshType) {
		// this is not supported - we cannot pull states...

		// TODO how to debounce this? we could trigger refreshData() but during startup
		// this issues lots of requests... :-/

[tacmi] Initial push of OpenHAB 2 ported tacmi binding #7768

[tacmi] Initial push of OpenHAB 2 ported tacmi binding #7768

Conversation

marvkis commented May 24, 2020

TravisBuddy commented May 24, 2020

Travis tests have failed

TravisBuddy commented May 24, 2020

Travis tests have failed

TravisBuddy commented May 24, 2020

Travis tests have failed

TravisBuddy commented May 24, 2020

Travis tests were successful

TravisBuddy commented May 24, 2020

Travis tests were successful

TravisBuddy commented Aug 2, 2020

Travis tests have failed

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

marvkis 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

TravisBuddy commented Aug 4, 2020

Travis tests were successful

TravisBuddy commented Aug 9, 2020

Travis tests were successful

TravisBuddy commented Aug 9, 2020

Travis tests were successful

marvkis commented Aug 9, 2020

fwolter commented Aug 11, 2020

fwolter commented Aug 14, 2020

marvkis commented Aug 19, 2020

marvkis commented Aug 19, 2020

fwolter commented Aug 20, 2020

fwolter commented Sep 5, 2020

TravisBuddy commented Sep 5, 2020

Travis tests have failed

TravisBuddy commented Sep 7, 2020

Travis tests were successful

TravisBuddy commented Sep 7, 2020

Travis tests were successful

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TravisBuddy commented Sep 8, 2020

Travis tests were successful

fwolter left a comment

Choose a reason for hiding this comment

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 Sep 9, 2020

Travis tests were successful

TravisBuddy commented Sep 9, 2020

Travis tests were successful

marvkis commented Sep 9, 2020

Hilbrand left a comment

Choose a reason for hiding this comment