Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kostalinverter] Fix using binding together with z-wave binding #11002

Merged

Conversation

chris922
Copy link
Contributor

Fixes using the kostalinverter binding together with the z-wave binding (#7090)

According to the example in the documentation the kostalinverter binding seems to use an invalid format for the config-description URI. The value must consist of three parts separated by : and not just two.

The z-wave binding expects this format and crashes when an invalid one is given.

I verified the fix by building a new JAR and dropping it into my openHAB 3.1.0 instance.

This solution is similar to the one proposed in #7090, but I used the binding name as the first identifier and not already a name related to the config or thing itself.

Signed-off-by: Christian Bandowski <christian@myvm.de>
@lolodomo
Copy link
Contributor

lolodomo commented Jul 17, 2021

By curiosity, using a grep command, I checked if other bindings are not defining 3 parts and I found that:

org.openhab.binding.mqtt.homeassistant/src/main/resources/OH-INF/config/homeassistant-channel-config.xml:       <config-description uri="mqtt:ha_channel">

org.openhab.binding.mqtt.homie/src/main/resources/OH-INF/config/homie-channel-config.xml:       <config-description uri="mqtt:homie_channel">

org.openhab.binding.pulseaudio/src/main/resources/OH-INF/config/config.xml:     <config-description uri="binding:pulseaudio">

org.openhab.binding.shelly/src/main/resources/OH-INF/binding/binding.xml:       <config-description uri="binding:shelly">

For pulseaudio, the config seems to be unused !
For shelly, this is simply the binding configuration. Setting an URI is not required in this case.

And for other bundles:

org.openhab.io.homekit/src/main/resources/OH-INF/config/config.xml:     <config-description uri="io:homekit">

org.openhab.io.hueemulation/src/main/resources/OH-INF/config/config.xml:        <config-description uri="io:hueemulation">

org.openhab.io.imperihome/src/main/resources/OH-INF/config/config.xml:  <config-description uri="io:imperihome">

org.openhab.io.metrics/src/main/resources/OH-INF/config/config.xml:     <config-description uri="io:metrics">

org.openhab.io.neeo/src/main/resources/OH-INF/config/config.xml:        <config-description uri="io:neeo">

org.openhab.io.openhabcloud/src/main/resources/OH-INF/config/config.xml:        <config-description uri="io:openhabcloud">

org.openhab.persistence.dynamodb/src/main/resources/OH-INF/config/config.xml:   <config-description uri="persistence:dynamodb">

org.openhab.persistence.influxdb/src/main/resources/OH-INF/config/config.xml:   <config-description uri="persistence:influxdb">

org.openhab.persistence.jdbc/src/main/resources/OH-INF/config/config.xml:       <config-description uri="persistence:jdbc">

org.openhab.voice.googletts/src/main/resources/OH-INF/config/config.xml:        <config-description uri="voice:googletts">

org.openhab.voice.pollytts/src/main/resources/OH-INF/config/config.xml: <config-description uri="voice:pollytts">

org.openhab.voice.voicerss/src/main/resources/OH-INF/config/config.xml: <config-description uri="voice:voicerss">

So potentially a lot of fix to apply....

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks for the fix!

@kaikreuzer kaikreuzer added the bug An unexpected problem or unintended behavior of an add-on label Jul 18, 2021
@kaikreuzer kaikreuzer added this to the 3.2 milestone Jul 18, 2021
@kaikreuzer kaikreuzer merged commit cafcdb6 into openhab:main Jul 18, 2021
@lolodomo
Copy link
Contributor

@chris922 : to clearly understand, what was the link with the Z-Wave binding?

@lolodomo
Copy link
Contributor

lolodomo commented Jul 18, 2021

I have now standardized the URI everywhere in bindings. I am just waiting an answer from the maintainer of the pulseaudio binding (unused config to be removed IMHO).

Now remains all the io/persistence/voice bundles with 2 segments config URI. @kaikreuzer : do you thing we should update them ?

@kaikreuzer
Copy link
Member

@lolodomo I am not sure - we don't have concepts like "thing-type" etc there what could be used for the URIs, so I wouldn't know how we should update them. Using the addon id as a first segment does not look too bad imho. If this causes issues with the Z-Wave binding because it fails parsing those, I guess that should then rather be fixed in the Z-Wave binding. Wdyt?

@lolodomo
Copy link
Contributor

First, I would like to understand why exactly this leads to problems with the Z-Wave binding. I hope @chris922 can explain that.

Then, I used myself the openhabcloud and voicerss bindings in combination with the Z-Wave binding without problem, so I really doubt we need to change these URI.

In case we would like to change them anyway, I could propose to add "config" as thrid segment, like for example "voice:voicerss:config" or "io:openhabcloud:config".

@chris922
Copy link
Contributor Author

chris922 commented Jul 21, 2021

@lolodomo : When the z-wave binding is active adding a kostal-inverter thing failed. An exception occurred here:

https://github.com/openhab/org.openhab.binding.zwave/blob/b4438148bfdbf8007684f66516b7df5e0aae79b2/src/main/java/org/openhab/binding/zwave/internal/ZWaveConfigProvider.java#L129

Seems that the z-wave binding has a hooks into a more general place and tries to parse the thing-type URI to get the information which type of binding the thing belongs to as this information is usually available in the thing-type URI... but it fails to parse the URI if it doesn't consist of three parts.

Few lines before the mentioned one there is an if-clause that restricts parsing the URIs for thing and thing-type entries.. That's probably the reason why not all config-description URIs will lead to errors together with the z-wave binding.

@lolodomo
Copy link
Contributor

In this case, it looks to me like a bug in the Z-wave binding. The binding should first check that uri.getSchemeSpecificPart() starts by "zwave:".

dw-8 pushed a commit to dw-8/openhab-addons that referenced this pull request Jul 25, 2021
…hab#11002)

Signed-off-by: Christian Bandowski <christian@myvm.de>
Signed-off-by: dw-8 <davy.wong.on+github@gmail.com>
dw-8 added a commit to dw-8/openhab-addons that referenced this pull request Jul 25, 2021
dw-8 pushed a commit to dw-8/openhab-addons that referenced this pull request Jul 25, 2021
…hab#11002)

Signed-off-by: Christian Bandowski <christian@myvm.de>
Signed-off-by: dw-8 <davy.wong.on+github@gmail.com>
dw-8 added a commit to dw-8/openhab-addons that referenced this pull request Jul 25, 2021
…ng (openhab#11002)"

This reverts commit 6ae30cd.

Signed-off-by: dw-8 <davy.wong.on+github@gmail.com>
@Thorte007
Copy link

Hi, I still have the problem. ZWave is working fine but if I try to add any kostal-plenticore binding, the system is going into a infinite loop.

@basse04
Copy link
Contributor

basse04 commented Sep 10, 2021

Hi, just to sort things out, so no solution found so far.
Before #7090 is PIKOIQ42.xml as , and in config/ThirdGeneration.xml as , and this doesn't work

After #7090 is PIKOIQ42.xml as , and in
config/ThirdGeneration.xml as and as far I have understand will this work.

After #11002 is PIKOIQ42.xml as , and in
config/ThirdGeneration.xml as and there is same problems as before #7090

Just a guess, will this (from XML Structure for Binding Definition) describe that if you choose to use binding will you then use bindingID, but if you choose to use thing-type will you then use thingTypeID(or anything likely but not bindingID) as parameter ?

@Thorte007
Copy link

Hi, the problem appears when I try to add any Plenticore binding. After clicking the button in the menu, the system is going into a infinite loop. I'm not able to configure thingTypeIDs or something else because I'm not able to get to this point at any time.

@basse04
Copy link
Contributor

basse04 commented Sep 20, 2021

Hi, I can also see this problem, and it's regarded to as described above. So I assume we have to figure out how the syntax in PIKOxxx.xml will look like, and then use the right working syntax for all different invertertypes.

frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
…hab#11002)

Signed-off-by: Christian Bandowski <christian@myvm.de>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
…hab#11002)

Signed-off-by: Christian Bandowski <christian@myvm.de>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
…hab#11002)

Signed-off-by: Christian Bandowski <christian@myvm.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants