[luxtronikheatpump] Fix control signal circulating pump

[jamietownsend]

Fixes #15836

[sgiehl]

Hey @jamietownsend
Thanks for providing this pull request. The Energy definition was for sure added by accident.

I'm actually not using this channel on my heatpump as it doesn't provide a value in my set up. Otherwise I might have noticed it.

I've added some comments, as some changes might be required to have that working correctly.

[jamietownsend]

Thanks for the fix! Indeed this is the correct dimension which is used for updating the channel:

openhab-addons/bundles/org.openhab.binding.luxtronikheatpump/src/main/java/org/openhab/binding/luxtronikheatpump/internal/enums/HeatpumpChannel.java

Lines 937 to 941 in 24f4234

	/**
	* Control signal circulating pump
	* (original: Steuersignal Umwälzpumpe)
	*/
	CHANNEL_HZIO_PWM(183, "controlSignalCirculatingPump", NumberItem.class, Units.PERCENT, false, null),

Since the channel type changed, you need to provide update instructions for the channel. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

A similar example can be found here:

openhab-addons/bundles/org.openhab.binding.miele/src/main/resources/OH-INF/update/instructions.xml

Lines 11 to 13 in 24f4234

	<update-channel id="waterConsumption">
	<type>miele:water-consumption</type>
	</update-channel>

There is a slight doubt for me though. I see statically declared channels:

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

Line 168 in 24f4234

<channel id="controlSignalCirculatingPump" typeId="controlSignalCirculatingPump"/>

But also code that seems to dynamically add channels:

openhab-addons/bundles/org.openhab.binding.luxtronikheatpump/src/main/java/org/openhab/binding/luxtronikheatpump/internal/LuxtronikHeatpumpHandler.java

Lines 274 to 293 in 24f4234

	// create list with available channels
	for (HeatpumpChannel channel : HeatpumpChannel.values()) {
	Integer channelId = channel.getChannelId();
	int length = channel.isWritable() ? heatpumpParams.length : heatpumpValues.length;
	ChannelUID channelUID = new ChannelUID(thing.getUID(), channel.getCommand());
	ChannelTypeUID channelTypeUID;
	if (channel.getCommand().matches("^channel[0-9]+$")) {
	channelTypeUID = new ChannelTypeUID(LuxtronikHeatpumpBindingConstants.BINDING_ID, "unknown");
	} else {
	channelTypeUID = new ChannelTypeUID(LuxtronikHeatpumpBindingConstants.BINDING_ID, channel.getCommand());
	}
	if ((channelId != null && length <= channelId)
	|| (config.showAllChannels == Boolean.FALSE && !channel.isVisible(visibilityValues))) {
	logger.debug("Hiding channel {}", channel.getCommand());
	} else {
	channelList.add(callback.createChannelBuilder(channelUID, channelTypeUID).build());
	}
	}
	thingBuilder.withChannels(channelList);

The update instructions are not needed if the channel is actually added dynamically.

You can probably tell me where I am wrong. 🙂 But otherwise you could test this by first running the bundled version of the binding. Then create a managed Thing using the UI. Then uninstall the binding and drop your JAR with the fix. Then verify if the channel type is automatically updated after reinitialization, so that a new item will automatically be suggested as Number:Dimensionless. If it does, it looks like the channel is created dynamically. If not, most likely update instructions are needed. In this case you could add them and run the test again.

I'm not sure what needs to be done here. After following your suggested approach, I see that the Channel definition under the Thing shows "(Number:Dimensionless)", but the Item shows "Number:Energy". Deleting the Item and recreating it doesn't fix the problem. I notice the following while adding the Item from the Thing:

Notice that the types are not the same. I also notice that the file .../openhab/userdata/jsondb/org.openhab.core.thing.Thing.json doesn't contain the correct value, even though the channel appears right on the Thing in the UI.

Deleting the Item and the Thing and then re-creating them solves the conflict.

@jlaur, any ideas how to proceed? (@sgiehl, FYI)

[jamietownsend]

I added an update.xml to delete and re-create the controlSignalCirculatingPump channel. This enables the correct creation of a new Item. Old items will still have issues and will have to be manually deleted and re-created.

[jlaur]

I added an update.xml to delete and re-create the controlSignalCirculatingPump channel. This enables the correct creation of a new Item. Old items will still have issues and will have to be manually deleted and re-created.

Yes, the update instructions will only update the channel definitions in jsondb, but will touch linked items.

Did you notice a difference between having the update instructions when upgrading the binding and not having them? Since I'm confused about whether this binding provide static or dynamic channels (or both), I would like to figure out if the instructions are needed in this case.

[jlaur]

Did you notice a difference between having the update instructions when upgrading the binding and not having them? Since I'm confused about whether this binding provide static or dynamic channels (or both), I would like to figure out if the instructions are needed in this case.

If the channel was automatically updated for your existing Thing when you updated the binding, the instructions are not needed. You have not added the thingTypeVersion property, so the instructions haven't been triggered.

[jamietownsend]

If the channel was automatically updated for your existing Thing when you updated the binding, the instructions are not needed.

This is the case. The Thing was updated automatically, even without the update.xml.

You have not added the thingTypeVersion property, so the instructions haven't been triggered.

Thanks for this clarification. I tested what happened when the thingTypeVersion property was correctly configured and I saw in the log that the update was correctly triggered and the thingTypeVersion changed from 0 to 1, but otherwise, there was no difference in what was changed. The Thing was updated, but the Item was not.

As such I'll remove the update.xml again to keep things simple.

Question: is there any way that we can automatically have the Items updated or should we write something in the docs to explain what has to be done?

[jamietownsend]

Not sure why this was closed. I'll try to re-open it...

[jamietownsend]

Ok, re-opening 🙂

[jlaur]

Question: is there any way that we can automatically have the Items updated or should we write something in the docs to explain what has to be done?

There is no way to automatically update the items, but you could add a note somewhere around here:

https://github.com/openhab/openhab-distro/blob/b431198b21b0f435f62f5ebe671007dfafa7b53a/distributions/openhab/src/main/resources/bin/update.lst#L120-L122

These messages will be displayed during an upgrade.

[jamietownsend]

Thanks @jlaur. See related pull request: openhab/openhab-distro#1606

I hope both pull requests can now me merged 🙂

[sgiehl]

To me this looks good now.

Note: I'm unable to say if the new label matches what the channel actually provides. The previously used label was defined by "reverse engineering" and looking up what the java applet on the heatpump was using for it.

[jlaur]

LGTM