[mqtt.generic] Add UOM to inbound values for MQTT Channels

[jamesmelville]

I wanted to get the MQTT binding to apply units to incoming data before applying to an Item. I believe this fixes tickets #6751 and #10338, and implements what I think was intended to be delivered by #6763.

In my opinion this is a bug fix because it brings behaviour in line with the description of the unit parameter on the number type channel.

Despite the description, current behaviour was to only apply UoM to updates going from an item to an MQTT command topic. e.g. A channel with the unit W, linked to an item with the unit mW would output 1 when the item was updated with the value 1000. A value of 1000 received from the MQTT state topic would result in the value of 1000 being applied to the item, despite any configuration of UoM on the channel or item.

When a valid unit symbol is applied to the channel definition, this change results in QuantityTypes and expected conversions being applied to states and commands going back and forth between items and MQTT topics, except for dimensionless types where the previous behaviour of using decimals is retained to avoid conversions on types like percentages. Where a unit is not present, DecimalTypes continue to be used.

I'm concerned that this change will cause unexpected behaviour for existing users with command only channels which have a unit configured. I could see an argument to put this change behind a flag on the broker / thing / channel configuration, but haven't implemented this unless it's decided this is an enhancement rather than a bug fix.

I've followed the coding guidelines, the commit has been signed off, tests have been extended and run and the code has been formatted etc. I don't believe any README updates are required at this point.

[jochen314]

I would always assume, that a device would sent the data with a fixed UOM. So there woul dbe not need for a convertion on the receiving side.
But when you want to display it in the UI, you might want to use different UOM (maybe even for the same channel).
But that would still not work. The state is still forwarded as a DecimalType unless we are aable to tell the system the dimension of the value. And this is normaly done by adding the dimension name to the type AFAIK

[jamesmelville]

I would always assume, that a device would sent the data with a fixed UOM. So there woul dbe not need for a convertion on the receiving side.
But when you want to display it in the UI, you might want to use different UOM (maybe even for the same channel).
But that would still not work. The state is still forwarded as a DecimalType unless we are aable to tell the system the dimension of the value. And this is normaly done by adding the dimension name to the type AFAIK

Sorry - I don't exactly follow - I'm setting the state to a QuantityType without specifying the dimension, but with units which infer the dimension state = new QuantityType<>(newValue, unitType);. So for items which are configured as a dimensioned number with specified units, any conversions are carried out automatically. What are you saying won't work?

I think an easy way to see what I'm doing is to configure something like the following and compare the binding behaviour before and after my PR:

Thing

Thing mqtt:topic:myTopic "My Topic" (mqtt:broker:myBroker) {
    Channels:
        Type number   : temp     "A Kelvin Temperature"           [stateTopic="test/in", commandTopic="test/out", unit="K"]
}

Items.

Number:Temperature kelvinTemp "Temperature Kelvin" {channel="mqtt:topic:myTopic:temp"}
Number:Temperature celsiusTemp "Temperature Celsius [%.1f °C]" {channel="mqtt:topic:myTopic:temp"}
Number:Temperature farenheitTemp "Temperature Farenheit [%.1f °F]" {channel="mqtt:topic:myTopic:temp"}

Sitemap

sitemap demo label="Demo Sitemap" {
	Frame label="Demo Items" {
		Default item=kelvinTemp 
		Default item=celsiusTemp 
		Default item=farenheitTemp 
		Default item=kelvinTemp label="Kelvin Converted to Celsius [%.1f °C]"
	}
}

[jochen314]

Ah, I see.
I am using the UI and i only now found the correct way to set the "State Description" -> Pattern for an item.
Great, is looks good like this :-)

[jochen314]

Could it be enough to just do

         return builder.withStep(step).withPattern("%s %unit%");

now, that we switched to QuantityType?

[jamesmelville]

I have to say I don't know much about StateDescriptionFragmentBuilder and I've struggled to track down any docs on it. It seems to me that most bindings don't implement it at all. I think we have two options, what you suggest (which improves the PR, and I think maintains compatibility with existing configurations), or to remove all calls to withPattern (don't call the super method either), which seems to leave rendering up to the default behaviour for the state. In that scenario everything appears as I expect (with units) in the MainUI, but Sitemaps expect a state presentation format in order to display anything, so existing users might be confused by values disappearing. The only benefit I can think of is some localisation where (e.g.) units are presented before the number, but can't find any evidence that exists, so probably easiest to go with your proposal?

[openhab-bot]

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

https://community.openhab.org/t/mqtt-binding-and-units-of-measurement/127026/3

[jamesmelville]

@jochen314 @davidgraeff please could I get this re-reviewed? It went stale so I've updated it for changes against the underlying code, but I think all the issues have been addressed. One concern is I don't use the mqtt.homeassistant binding but have made some minor changes to it, @antroids it looks like I've made small tweaks to code you've written, would you mind taking a look?

[antroids]

@antroids it looks like I've made small tweaks to code you've written, would you mind taking a look?

The idea of this PR is looks good to me.
But please don't try to fix tests by changing test resources used as input. According with the climate.mqtt specification temperature_unit value can be "C" or "F".
Could you please define temperature_unit field type as enum TemperatureUnit {CELSIUS(SIUnits.CELSIUS), FAHRENHEIT(SIUnits.FAHRENHEIT)} and annotate enum members with @SerializedName.

[jamesmelville]

@antroids thank you for flagging that, glad I @ ed you. I've made the changes you suggested, hope that resolves it

[jochen314]

Sorry, I did not spot this before: The only use of precision is to convert it to BigDecimal.
Converting from decimal (0.1) to float and then back to decimal is never a good idea.
We could make default precision a BigDecimal from the beginning.
BigDecimal.ONE is already defined. the other is best converted from string as new BigDecimal("0.1")

[jamesmelville]

Based on @antroids' 👍 I've swapped all the floats for BigDecimals

[antroids]

Thanks! MQTT HomeAssistant part LGTM. I'm unable to test it, but this should be covered by Unit tests.
I don't think that deserialization to BigDecimal except Float can broke something and this makes code cleaner.
GSON is using constructor with single string argument.

[jochen314]

Great! you also converted the other usages of Float to BigDecimal

LGTM

[openhab-bot]

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

https://community.openhab.org/t/mqtt-and-converting-c-to-f/122570/10

[fwolter]

@jamesmelville Your code doesn't build as there is one SAT error:

[ERROR] org.openhab.binding.mqtt.generic.values.NumberValue.java:[115]
Use equals() to compare object references.

Should be fixable easily. If you fix it on this weekend, your PR will make it into openHAB 3.2, which will be feature-freezed on monday.

[jamesmelville]

@fwolter sorry for the delay, fixed the SAT issue and hope this can make it to 3.3!

[fwolter]

Seems like @jlaur comment is addressed.

LGTM

[openhab-bot]

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

https://community.openhab.org/t/oh3-3-0-the-units-configured-for-things-for-items-have-ceased-to-be-displayed/137013/7