eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 786 forks source link

Converted decimal commands on items with %unit% should fall back to current unit of item state #5761

Open wborn opened 6 years ago

wborn commented 6 years ago

PR https://github.com/eclipse/smarthome/pull/5734 introduced the conversion of decimal commands to QuantityType for UoM channels.

This works well for items that have a unit in their state description or channel type. When both have the unit placeholder (%unit%) as unit, the UnitProvider is used as fall back which may result in unexpected behavior.

Hypothetical example

This hypothetical example uses the following item definition:

Number:Temperature Spacecraft_Temperature_SP "Spacecraft Temperature [%.1f %unit%]"  { channel="nasa:spacecraft:voyager1:setpoint" }

The setpoint channel of the spacecraft allows for monitoring the current temperature setpoint as well as changing it with commands.

It has the following channel type:

<channel-type id="SetPoint">
    <item-type>Number:Temperature</item-type>
    <label>Set Point</label>
    <description>The spacecraft set point temperature</description>
    <category>Temperature</category>
    <state pattern="%.1f %unit%" />
</channel-type>

I think it would make more sense to use the current unit of the item state as default unit for such items instead of the default unit provided by UnitProvider.

It would also fix the issue that no unit conversion takes place when the UnitProvider has no default unit (e.g. with the Power dimension).

The UnitProvider could still be used for providing units as fall back when the item has no state.

For an example with the Nest Binding see https://github.com/eclipse/smarthome/pull/5734#discussion_r195571095.

wborn commented 6 years ago

Should the UIs that support UoM in this example actually show the state in Kelvin or the system default Fahrenheit @htreu?

When I check Basic UI it shows Kelvin values but the unit in the HTML is the system default:

<div class="mdl-form__control mdl-form__setpoint" 
 data-control-type="setpoint" 
 data-item="Spacecraft_Temperature_SP"
 data-max="100" 
 data-min="0" 
 data-step="1" 
 data-value="23+K" 
 data-unit="°F" 
 data-widget-id="0000">

The first command is done with Fahrenheit but after the state has been updated via the EventStream Basic UI sends commands in Kelvin.


The Classic UI shows the item in Kelvin and the commands are always done with a Kelvin unit.


This Javadoc suggests an item with %unit% should either get the unit of the state description or fallback to the system default:

https://github.com/eclipse/smarthome/blob/98686f2539f623bc83369e0bedafae61d7c6f16b/bundles/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/library/items/NumberItem.java#L146-L157

htreu commented 6 years ago

My main argument for not using the item state's unit is that when the UI does not know about units and just sends plain numbers, we will never know for sure how these numbers should be interpreted. Staying in your example above:

When Spacecraft_Temperature_SP is changed in HABPanel (not supporting UoM) the command is converted to a QuantityType command with Fahrenheit as unit before it is sent to the binding.

This may be accurate and the binding has to deal with different temperature units anyway. Your assumption is that the user of HABPanel in this situation knows that kelvin should be sent. In this situation the conversion to Fahrenheit is wrong. But if the user of HABPanel meant to send Fahrenheit, everything would be fine.

The point I want to make is: A UI not supporting UoM is the main problem here and I don't think we can make up for that in the backend.