Extend min/max/step with rangeUnit

[lsiepel]

A common scenario for a thermostat's is that they have a limited operation range. To support these ranges the xml thing type structure has min, max and step xml attributes. After the introduction of UoM, these attributes where not adapted. Binding's now typically have a state pattern of %unit% and a unitHint set. The min/max/step lack a unit so it is not known to what unit it relates.

This PR adds a unitRange xml attribute that holds the unit that represents used unit for min, max and step.

• Extend XML
• Extend AbstractStorageBasedTypeProvider.java
• Extend StateDescriptionFragmentImpl.java
• Adapt tests
• Validation

Fixes: #4432

[lolodomo]

@openhab/core-maintainers : having this enhancement in 4.3 would be really great. I would like to use it in Basic UI for a perfect working of the Colortemperaturepicker widget.

[mherwege]

Shouldn’t this be “rangeUnit” ?

[lsiepel]

Fixed

[mherwege]

Would it make sense to populate rangeUnit from pattern when it is not explicitly provided? That will make sure at least the pattern unit from the default is used.

[lsiepel]

Would it make sense to populate rangeUnit from pattern when it is not explicitly provided? That will make sure at least the pattern unit from the default is used.

1. For most UoM channels, the unit is not explicitly set in the state pattern (usually %unit%). So there is not much benefit doing this.
2. It introduces breaking changes / uncontrolled behavior. For instance: due to the current behavior of no provided unit, many bindings have the min/max set to the outer range of all Units.
   Maybe an example would make clear what i mean:
   Take a UoM Temperature channel. Commonly the users can switch between Fahenheit and Celsius. To limit the range, but also support both Units, the author sets the lower limit to a Celsius value and the upper limit to Fahrenheit value. With a fallback to the pattern unit, the min/max does no longer make any sense.

Really think this shit be opt-in and at some point in time, when all bindings have been adapted, we could look into making the attribute mandatory whenever a min/max is set. But that is for later.

[mherwege]

Fair enough. I only see @lolodomo and others going through addons and changing the default pattern to have a real unit to the benefit of the color temperature picker, and running into issues because the user configured pattern unit is used in the end, and not the pattern from the channel definition. This would avoid an extra round on this. If %unit# is set, it of course will not work and will still provide a null value without changing the channel definition.
While defining unitRange is really what should be done, it may avoid some issues in a few cases.
And I also think it still doesn’t hurt in your second example. It will not be worse than before.

I full agree this should ultimately be a required field when defining min/max/step on a number channel with a dimension. One, we cannot do that overnight as it will break all bindings at once, and two, it is difficult to enforce in code. An attribute is either mandatory, or it is not. But in this case it should only be mandatory for QuantityType channels.

[lsiepel]

Ok, what else do you (@florian-h05 and @lolodomo) need from core to get this working in the ui’s?

I think we should leave the validation as is (none) and leave it up to the UI. @openhab/core-maintainers wdyt?

[lolodomo]

I see no change in REST bundle so something is very probably missing in this PR.

There is also a notion of merge for state descriptions, the case of the new rangeUnit is not obvious to merge and could also have an impact on min/max. To be checked in your code.

PS: Main UI should then be enhanced to also propose the range unit for state description.

[lolodomo]

There is also a notion of merge for state descriptions, the case of the new rangeUnit is not obvious to merge and could also have an impact on min/max. To be checked in your code.

Imagine the developer sets for the channel a range 153-400 in range unit mirek. Now the user sets on his item a state description with range unit in Kelvin and min to 3000 and no max. What should be the final values for range unit, min and max ?

[lolodomo]

With your current code, I believe the result would be range Kelvin (K) min 3000 max 400 and that is wrong.

[florian-h05]

Ok, what else do you need from core to get this working in the ui’s?

For Main UI (and Basic UI I guess as well) we need the min/max/step values converted to the unit of the display state and provided through the REST API as we cannot do these unit conversions in the UI. This way the UI just needs to enforce min/max/step when controlling the Item (which is done or should be done if not already the case in the display state unit) without caring about the actual unit.

[mherwege]

For Main UI (and Basic UI I guess as well) we need the min/max/step values converted to the unit of the display state and provided through the REST API as we cannot do these unit conversions in the UI. This way the UI just needs to enforce min/max/step when controlling the Item (which is done or should be done if not already the case in the display state unit) without caring about the actual unit.

And wouldn't we need anything when defining State Description metadata? I would assume we need to have the rangeUnit field there as well.

[mherwege]

There is also a notion of merge for state descriptions, the case of the new rangeUnit is not obvious to merge and could also have an impact on min/max. To be checked in your code.

In my view, the min, max, step and rangeUnit fields should be treated as a whole. Don't mix these from 2 providers. I think pattern should still be set from any provider. I am undecided on options.

[lolodomo]

In my view, the min, max, step and rangeUnit fields should be treated as a whole. Don't mix these from 2 providers.

I agree with you, makes sense and simplifies.

I think pattern should still be set from any provider.

Of course.

I am undecided on options.

Probably same as pattern and no mix of options from different providers. To be checked what is currently implemented.

[lolodomo]

For Main UI (and Basic UI I guess as well) we need the min/max/step values converted to the unit of the display state and provided through the REST API as we cannot do these unit conversions in the UI.

Is it what's done currently in the item REST API?
For sitemap REST API, I will study.

[lolodomo]

@openhab/android-maintainers : I recently discovered that in BasicUI min/max/step from item state description was not used (until the Colortemperaturepicker element was added). Are we using them in the Android app?

[maniac103]

Are we using them in the Android app?

Yes. It's used as fallback if no widget level min/max/step is set.

[lsiepel]

I see no change in REST bundle so something is very probably missing in this PR.

There is also a notion of merge for state descriptions, the case of the new rangeUnit is not obvious to merge and could also have an impact on min/max. To be checked in your code.

PS: Main UI should then be enhanced to also propose the range unit for state description.

Could you point me towars the REST change you expect? I fail to find it. I searched Step (as i would assume that is also exposed by the API)in the whole repository and couldn't find it anywhere related to REST