openhab / openhab-core

Core framework of openHAB
https://www.openhab.org/
Eclipse Public License 2.0
925 stars 424 forks source link

Ranges (min,max,step) should be provided with a unit #4432

Open lsiepel opened 1 week ago

lsiepel commented 1 week ago

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 is hardcoded to a value and it is not known to what unit it relates.

To workaround this, bindings often choose to support the widest range. With that i mean, the min is set to the value corrosponding to the Celsius and max to the Fahrenheit representation of the operating range. It is obvious that this gives issues. as the actual operating range is more limited.

I would propose to add a xml attribute next to min, max, step to specify the unit for that range: "rangeUnit" It's value would be the actual unit just like unitHint. This attribute could be used to determine the min/max/step. This can happen at the same place, just with a little UoM logic added. If the attribute is missing it would fallback to the current behaviour. So an opt-in, full backwards comptable solution that handles all cases.

Alternatives: Use unitHint to determine the used unit. Rules out here: https://github.com/openhab/openhab-addons/issues/17638#issuecomment-2442371837 (in essence: this is a UI hint and should not be misused for something else) Use state pattern. This would not cover all bindings as many have it set to %unit%

Related to: https://github.com/openhab/openhab-core/pull/3132 Related to: https://github.com/openhab/openhab-addons/issues/17638 (solving this for just mireds/mireks)

boehan commented 6 days ago

Just my thoughts on this: Another alternative would be to allow units within the definition of min, max, step. Full state description definition could then, e.g., look like this: <state min="12 °C" max="30 °C" step="0.5 K" pattern="%.1f %unit%" readOnly="false"></state>

Potential advantages:

IMO, changes would be minimal:

lsiepel commented 6 days ago

Just my thoughts on this: Another alternative would be to allow units within the definition of min, max, step. Full state description definition could then, e.g., look like this: <state min="12 °C" max="30 °C" step="0.5 K" pattern="%.1f %unit%" readOnly="false"></state>

  • it would allow to define different units for min, max and step (may not be needed and probably doesn't make much sense in above example, even with °F instead of °C, but could be useful for wide ranges of max - min compared to step (e.g., min/max on m vs. step in cm)

Specifying the unit in each element seems very redundant and is error prone. It’s good to have all options on the table tough.

On a side note: having backwards compatibility is key, regardless of the solution. Having a first fallback step to the state pattern unit is breaking.

andrewfg commented 3 days ago

My thoughts..

  1. UnitHint cannot be used since it applies at the channel element and not on the state element.
  2. Adding UoM to all attributes min/max/step would be a breaking change with unknown consequences.
  3. Parsing the state pattern would not always work. Especially if min/max/step are hard coded across continents, and the state is soft coded for continent specific units. (e.g. Celsius vs Fahrenheit)
  4. THEREFORE the OP's suggestion to add one single attribute on the state element is by far the best proposal.
lsiepel commented 3 days ago

While experimenting with the code to come up with a PR, I have some questions. I experimente4d with this pattern as part of a temperature channel:

<state min="0.0" max="35.0" step="0.5" pattern="%.1f %unit%"/>

While using file based configuration, i was able to update the state to 50 C (outside of range) from the console and from basicui. In core i could only find a MinMaxValidator that applies to ConfigDescriptionParameter and not to the StateDescription Is there any validation to the min/max value at all?

JFTR: while looking at all this, i also realised there is a min/max/step setting for the config parameters. I was not aiming to provide UoM range validation for those configuration parameters at this moment.

andrewfg commented 3 days ago

^ I think there should be several PRs here. To introduce the attribute, change and upload the xml schema, to pass it to the items, include in the rest api, do the validation etc.

lsiepel commented 3 days ago

^ I think there should be several PRs here. To introduce the attribute, change and upload the xml schema, to pass it to the items, include in the rest api, do the validation etc.

Yes multiple. For now i'm experimenting and want to get a clear picture of how everything ties together. I'm using the core API's but have not yet had the need to looked into the core code in detail, so it is somewhat challenging.