-
-
Notifications
You must be signed in to change notification settings - Fork 429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ranges (min,max,step) should be provided with a unit #4432
Comments
Just my thoughts on this: Potential advantages:
IMO, changes would be minimal:
|
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. |
My thoughts..
|
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:
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 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. |
^ |
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. |
Just created a single PR to get early feedback, there is still something to discuss about the validation. Where do we validate this range?
I think the UI's need to be adapted anyway. |
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 aunitHint
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: openhab/openhab-addons#17638 (comment) (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: #3132
Related to: openhab/openhab-addons#17638 (solving this for just mireds/mireks)
The text was updated successfully, but these errors were encountered: