jlaur commented 1 year ago

3481 was quite a leap forward in terms of making UoM predictable and reliable.

Now that the smoke has cleared, I see a small opportunity to improve/fine-tune the user experience without changing anything in the UoM logic itself.

A few examples where user experience could improve:

A binding integrating a washing machine or dishwasher provides water consumption for a Number:Volume channel. Default metric unit m³ does not make much sense in this context. In most cases it should be either litres or gallons.
A binding integrating a car provides distances for a Number:Length channel. Default metric unit m is not ideal. In most cases km would be preferred (similar for imperial).
A binding provides relative humidity in percent for a Number:Dimensionless channel. Default unit one is not ideal, % would be expected.
A binding provides loudness in decibel for a Number:Dimensionless channel. Default unit one is not ideal, dB would be expected.

Since the decision of which unit to use is (and should be) entirely up to the user, my only suggestion would be to add a possibility for bindings to give a hint about recommended units. The only usage of this hint would be for the UI to suggest reasonable default units when creating new items for channels.

In other words, this will not control or override anything, or make the inner UoM logic more complicated in any way. It would exist in parallel as something exposed by the relevant REST methods to be consumed by the UI in order to better assist the user. Additionally, it should be easy/light-weight for bindings to provide such hints, and the bindings should not have to deal with LocaleProvider etc. or duplicate any logic that could be implemented in core and webui.

The way I see this could be hinted by bindings would be to extend channel and channel-type definitions with an optional new tag. For example:

<channel-type id="humidity">
    <item-type>Number:Dimensionless</item-type>
    <label>Humidity</label>
    <description>Relative humidity</description>
    <category>Humidity</category>
    <state readOnly="true"/>
    <unit-hint>%</unit-hint>
</channel-type>

Or perhaps:

<channel-type id="humidity">
    <item-type unitHint="%">Number:Dimensionless</item-type>
    <label>Humidity</label>
    <description>Relative humidity</description>
    <category>Humidity</category>
    <state readOnly="true"/>
</channel-type>

And likewise for channel:

<channel id="humidity" typeId="system.atmospheric-humidity">
    <label>Humidity</label>
    <description>Relative humidity</description>
    <unit-hint>%</unit-hint>
</channel>

I'm not sure if it makes sense for channels. Perhaps it would be better to require a custom channel type in order to provide such hints, as being able to override at channel level will increase the complexity. I can't think of any use cases since channel types needing specialized hints are probably too generic anyway.

Now, a more complex example - the washing machine:

<channel-type id="water-consumption">
    <item-type unitHint="l,gal">Number:Volume</item-type>
    <label>Water Consumption</label>
    <description>Water consumption by the currently running program on the appliance</description>
    <category>Water</category>
    <tags>
        <tag>Measurement</tag>
        <tag>Water</tag>
    </tags>
    <state readOnly="true" pattern="%.1f l"/>
</channel-type>

In this case two units are provided. This is because the hints should be interpreted in the context of the regional settings. So for a US user, gal should be suggested, and for a European user, l should be suggested. I think this distinction is important if we want to get it right. The hint doesn't dictate anything, but in order to be truly helpful, we should still respect those settings, like we also do when item unit is completely missing and must be defaulted.

Calling a method like this:

http://openhab:8080/rest/channel-types/miele%3AwaterConsumption

should then provide this hint (if this can be added while maintaining backwards compatibility):

{
  "parameters": [],
  "parameterGroups": [],
  "description": "Water consumption by the currently running program on the appliance",
  "label": "Water Consumption",
  "category": "Water",
  "itemType": "Number:Volume",
  "unitHint": "l,gal",
  "kind": "STATE",
  "stateDescription": {
    "pattern": "%.1f l",
    "readOnly": true,
    "options": []
  },
  "tags": [
    "Measurement",
    "Water"
  ],
  "UID": "miele:waterConsumption",
  "advanced": false
}

I'm not sure where the processing of this list of units should happen. Ultimately, for regional settings in Europe, the unit to default would be "l". I'm not into the architecture here, just outlining my proposal as detailed as possible.

See also:

https://community.openhab.org/t/oh-4-units/148410/1

openhab-bot commented 1 year ago

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

https://community.openhab.org/t/oh-4-units/148410/20

spacemanspiff2007 commented 1 year ago

This sounds like a reasonable enhancement. What about use cases where the unit is determined by a 3rd party system. E.g. I have a thing that connects to a webpage. In the webpage I can configure how the values will be reported, either as l or gal. The unitHint should then reflect the configuration of the webpage and dynamically change. Would that also work?

rkoshak commented 1 year ago

The unitHint should then reflect the configuration of the webpage and dynamically change. Would that also work?

I don't think so because this XML is part of the add-on config and, as I understand it, not very dynamic in nature.

Maybe the servlet behind the REST API call above could be enhanced to query the binding for the configured unit, assuming the binding knows it?

Personally, I'd not have this somewhat edge case hold up the overall approach. I'd split it into two issues. First let's handle the case where the binding absolutely knows from the start which units are appropriate. Then later come back with a new issue/pr and handle the cases like HTTP, MQTT, modbus, etc. where the unit isn't known until the Thing is created and configured. The latter is much more complex and will likely require a brand new API I would think.

obones commented 10 months ago

This would indeed be very practical for binding writers like me, in particular for humidity which will never actually change its unit. What I have a hard time understanding, though, is why it's not already working properly, as an order of preference was mentioned here:

In OH 4 the order of precedence is now:

the unit defined in the unit Item metadata the unit in the QuantityType posted to the Item the system default

But when my binding posts this state: new QuantityType<>(45, Units.PERCENT);

it does get displayed with a % sign, but the value stored in the persistence layer and shown in the log is 0.45 and not 45% Only when the Item gets a "Unit" metadata set to % does it use the proper unit everywhere.

Having a unit hint set to % would surely prefill the item with % when created, but this does not help migrating from OH3 where the same code was working properly.

openhab-bot commented 10 months ago

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

https://community.openhab.org/t/oh-3-to-oh-4-most-percentage-values-are-now-divided-by-100/153460/12

rkoshak commented 10 months ago

mentioned here:

In OH 4 the order of precedence is now:

the unit defined in the unit Item metadata the unit in the QuantityType posted to the Item the system default

That middle one is not correct. I don't know if that was correct at the time I wrote it or if it was always incorrect.

It is true that if you have managed Items, the upgrade tool will add unit metadata to your Items based on the unit sent by the Thing when upgrading from OH 3 to OH 4.

But as currently implemented, if a unit different from the unit metadata or system default if unit is not defined, the value will be converted to the unit or system default before the Item is updated.

it does get displayed with a % sign

That's because of the State Description Pattern which can display the value with a different unit, but it no longer controls the unit carried by the Item's state.

but the value stored in the persistence layer and shown in the log is 0.45 and not 45%

That's because the 45% is being converted to the system default 0.45 ONE on the update. Persistence, the REST API, and Rules will all see 0.45. Places that use the State Description Pattern will see 45 % because the unit is changed for display.

Having a unit hint set to % would surely prefill the item with % when created, but this does not help migrating from OH3 where the same code was working properly.

If you had managed Items most of the cases should have been caught and handled by the upgradeTool. If you are using .items files, well you wanted all that control so you'll have to make the changes manually.

IIRC the upgradeTool, which runs as part of the upgrade process creates unit metadata using the following in order:

State Description Pattern that is different from %UNIT% meaning the user has overridden the default
The unit the Channel reports it delivers

If 1 or 2 doesn't apply, no unit metadata is assumed and the system default is applied.

And, this was clearly explained with a lengthy description in the list of breaking changes between OH 3 and OH 4. It's not and never was expected to work without any end user changes. It's a breaking change after all. Everyone did their best to make the transition as seamless as possible but there will be cases that require manual intervention.

obones commented 10 months ago

Things is, I never defined any unit nor state description pattern on any of my items, which I believe is standard behavior for most users. And as a result, the "percent" ones still have no unit and description pattern. Yes, I agree, I should have read the release notes more carefully, but considering the sheer amount of discussion on "humidity" things, I find I must not be the only one.

Sorry for the digression on that issue, but I had to get a clarification on the state of things, and I must say, as a binding writer, I would very much welcome the "unit hint" along with a default pattern.

Also, you mention this:

later come back with a new issue/pr and handle the cases like HTTP, MQTT, modbus, etc. where the unit isn't known until the Thing is created and configured. The latter is much more complex and will likely require a brand new API I would think.

It's actually already possible to completely tweak an Thing creation via code by using ChannelBuilder and its withXXX methods like it's already done in a number of bindings, mine included. All that would be needed would thus be a new method withUnitHint on the ChannelBuilder which I believe would actually be needed for the XML parser anyway.

rkoshak commented 10 months ago

Things is, I never defined any unit nor state description pattern on any of my items, which I believe is standard behavior for most users.

That used to work sometimes and it never was consistent. The State Description Pattern was used to figure out the unit. But it lead to all sorts of side effects, edge cases, etc. I've spent way more time dealing with those than I have dealing with people who don't have a unit defined on their Items.

But since OH 4.1, it is standard behavior for most users to define the unit. The UI makes it so you almost have to go out of your way to avoid it.

If you are a .items file users, well, watching the announcements for breaking changes is part of the job you signed up for.

Sorry for the digression on that issue, but I had to get a clarification on the state of things, and I must say, as a binding writer, I would very much welcome the "unit hint" along with a default pattern.

To play devil's advocate (if you look at the original issue you'll see I advocated for a way for binding authors to push a unit to the Item) what do you as a binding author know about what units I as an end user prefer?

As a binding author, wouldn't it even be easier if you just pick whatever unit you want to publish, safe in the knowledge that the end user will get that value converted to the unit they want. For example, now there really is no reason for any binding author to mess with discovering the system default units and deciding whether to publish °C or °F (for example). Those of us in the US that use °F will have that as our default unit and the published °C will be converted automatically. The binding author just needs to publish °C or even Kelvin if that's what they want to.

All of the confusion on the forum and problems encountered revolve around two different issues:

The default unit for Number:Dimensionless is ONE and not %. Change that default and the whole percentage issue goes away. In practice, the number of Number:Dimensionless Items that are any unit other than % is small. The number that should be ONE is vanishingly small. We have the wrong default unit for this Item type.
The scale of the unit isn't ideal. For example, IIRC the default unit for length in SI is m but the default unit for Imperial is ft. If you have an Item that is measuring the range of an EV, neither meters nor feet makes any sense. You'd want km or mi. This is an area where providing some sort of scaling hint would be useful.

spacemanspiff2007 commented 10 months ago

I agree with @rkoshak . Why should a binding author dictate e.g. how my items are persisted. Implicitly applying units from the binding obfuscates how the UoM logic works. Just set the unit and everything works fine or define it as a normal non UoM item and everything will work consistent, too. By the way - HABApp even issues a warning if you have an UoM item without a unit set since a UoM item only makes sense together with unit.

We have the wrong default unit for this Item type.

I agree. Even though percent is not unit given how prevalent Percent values are there should have been a Number:Percent.

obones commented 10 months ago

I agree with @rkoshak . Why should a binding author dictate e.g. how my items are persisted. Implicitly applying units from the binding obfuscates how the UoM logic works.

The point of this issue is not to force a unit on the user but rather suggest one as the default value shown on the MainUI page when creating an item. The current interface would still allow to change it and the way I see it, the suggestion would only be provided by bindings for percentage channels anyway.

Even though percent is not unit given how prevalent Percent values are there should have been a Number:Percent

Yes, that would have been nice, but I don't see how one would migrate the dimensionless to percent automatically when upgrading to OH4+ This is why I think that the current proposal is a viable middle ground to somewhat mitigate the current situation.

rkoshak commented 10 months ago

Yes, that would have been nice, but I don't see how one would migrate the dimensionless to percent automatically when upgrading to OH4+

For managed Items, the upgradetool could handle that fairly easily. Just change the type for any Number:Dimensionless that doesn't have a unit metadata already defined. That might trip up a tiny tiny population of users who are indeed using ONE for some reason though, so maybe make it an optional upgrade step.

For text based .items files, well you'll always have to adjust to breaking changes manually anyway.

But I don't think a new UoM type is the way to go. I think simply changing the default for Number:Dimensionless would be sufficient. Everyone is already used to using Number:Dimensionless for percentages so that's not the confusing part.

The confusion comes from:

I never had to define the unit before, but now all my percent Items are < 1.
I never had to define the unit before, my Item is showing as a % in the UI but my charts are all < 1.

2 is particularly pernicious because of the way bindings can push state description patterns. Without the unit metadata the Item's state will be ONE but everywhere they look at the Item in the UI they will see %.

obones commented 10 months ago

And point 2 is exactly the reason why I got "beefed up" at this situation. If this could be addressed in 4.2 considering all the migrations paths (3.x, 4.0, 4.1), that would be perfect but I know this is not a simple thing to do

rkoshak commented 10 months ago

If this could be addressed in 4.2 considering all the migrations paths (3.x, 4.0, 4.1), that would be perfect but I know this is not a simple thing to do

The challenge there was it was a deliberate and definitive decision to not allow bindings to influence the unit metadata. There really haven't been any new arguments put forward that were not already considered when that decision was made. Without some new argument 🤷‍♂️. These arguments already lost.

That's a big reason why I keep pushing changing the default unit for Number:Dimensionless instead. It solves the most common instance of this problem with minimal impact to both end users, core, and bindings. And I think the arguments for doing so are pretty good.

seime commented 10 months ago

I must say, as a binding writer, I would very much welcome the "unit hint" along with a default pattern.

@obones I second this. Had to support a user with this exact problem just last weekend. https://github.com/seime/openhab-esphome/issues/12#issuecomment-1913663753 .

mherwege commented 10 months ago

The one thing being overlooked in all these discussions is that an item can be linked to two or more channels (and things) at the same time. I know this is not very common, but it is possible. So, suppose hints are implemented, which hint from which binding takes precedence? This already leads to issues today with state descriptions as these can be defined on the channel (thing). But that is just a representation thing. It is not predictable which one takes precedence. In general, item configurations (both unit and state description) should be with the item, nowhere else. And it is logical to allow overriding state descriptions in the UI configuration (sitemap or mainUI). I could agree with making the Number:Dimensionless default unit to be % as that would solve most issues.

jlaur commented 10 months ago

I could agree with making the Number:Dimensionless default unit to be % as that would solve most issues.

Just to be clear, you mean the UI would suggest % when creating a new item linked to a Number:Dimensionless channel? So in case of dB, for example, the user will just need to change it from % to dB (rather than from nothing to dB)?

mherwege commented 10 months ago

I could agree with making the Number:Dimensionless default unit to be % as that would solve most issues.

Just to be clear, you mean the UI would suggest % when creating a new item linked to a Number:Dimensionless channel? So in case of dB, for example, the user will just need to change it from % to dB (rather than from nothing to dB)?

Yes

openhab-bot commented 10 months ago

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

https://community.openhab.org/t/thoughts-on-upgrading-bindings-to-use-new-uom-features-for-humidity/153615/7

andrewfg commented 10 months ago

@jlaur in your OP you suggested two variants for setting the unit-hint on a channel type -- namely either as new xml element within the channel-type schema, or as a new attribute on its respective item-type element.

And so I was thinking to suggest a third option -- namely adding a new attribute on its respective state element .. e.g. as follows.

<channel-type id="humidity">
    <item-type>Number:Dimensionless</item-type>
    <label>Humidity</label>
    <description>Relative humidity</description>
    <category>Humidity</category>
    <state readOnly="true" pattern="%.0f %%r.H." unit-hint="%">
</channel-type>

HOWEVER as I was starting to write this post, it occurred to me in a sudden flash, that the existing 'state:pattern' attribute is in fact already the 'unit-hint' that you allude to in the OP. So perhaps it is not necessary to add anything new at all. Or??

obones commented 10 months ago

HOWEVER as I was starting to write this post, it occurred to me in a sudden flash, that the existing 'state:pattern' attribute is in fact already the 'unit-hint' that you allude to in the OP. So perhaps it is not necessary to add anything new at all. Or??

Except that this state is only for display and the whole issue here is that percentage values, while displayed as 54.3% thanks to the state pattern are stored as 0.543 in the persistence layer, in effect making any new history useless for many migrating users. Note that the rules that access the QuantityState or NumericState for an item will also see 0.543 and not the expected 54.3

andrewfg commented 10 months ago

this state is only for display and the whole issue here is that percentage values, while displayed as 54.3% thanks to the state pattern are stored as 0.543 in the persistence layer,

Ok. I get your point -- so my original suggestion to add an attribute to the 'state' element -- therefore -- applies. IMHO extending the 'state' element is more consistent than adding a new element, or adding a new attribute on another element.

<state readOnly="true" pattern="%.0f %%r.H." unit-hint="%">

mherwege commented 10 months ago

Please read the whole discussion. An item can be linked to multiple things and channels. If each binding gives different suggestions, this again creates conflict and confusion. I would even argue that the binding defining a state description is a mistake for the same reason, but it is not as bad as defining a unit. Decoupling unit from state description has been the right thing to do, so let’s not introduce it again. I just think that Unit.ONE is not the best default unit for QuantityType:Dimensionless.

rkoshak commented 10 months ago

The one thing being overlooked in all these discussions is that an item can be linked to two or more channels (and things) at the same time.

Or the type for the Item might be changed by a profile. This has actually caused some very challenging problems for users.

It is not predictable which one takes precedence.

Since the metadata is passed to the Item at Link time, if I understand correctly, wouldn't it be the Link that was created last that takes precedence?

Just to be clear, you mean the UI would suggest % when creating a new item linked to a Number:Dimensionless channel? So in case of dB, for example, the user will just need to change it from % to dB (rather than from nothing to dB)?

I would take it one step further and if no unit metadata is applied to the Number:Dimensionless Item, % is assumed to be the unit instead of ONE.

Note that it isn't "nothing". Right now the default unit for Number:Dimensionless is ONE which is how the upstream library denotes a basic ratio. This is the only unit that is not represented with the Item, but in all other ways it is treated as a unit.

Note that the rules that access the QuantityState or NumericState for an item will also see 0.543 and not the expected 54.3

I have been advocating for making it a best practice in rules to always explicitly declare the units being used and if you have units, don't convert them to raw numbers. This results in more robust and self documenting rules. For example in JS Scripting

if(items.ThermostatTemp.quantityState.greaterThan(Quantity('65 °F')){

as opposed to

if(items.ThermostatTemp.numericState > 65) {

While the latter one is the shortest, we've completely thrown away the unit information and are just assuming that the units match. If the Item happens to be carrying °C or K the rule will completely but silently fail. Thanks to the very changes made to UoM that generated this thread, that's less likely to happen than it used to be, but it's still a possibility.

The former example will still work, and it's clear in the rule what units you are using. This is one of the main purposes of UoM in the first place.

That won't help those who are using the latter approach but frankly, I don't think it's a super bad thing for those users to reconsider their approach. If they don't want to use UoM, then don't use them and change everything to be a plain Number (you can link a plain Number to a Number:X Channel now without error and the unit is thrown away). If you do want to use UoM, then actually use them.

A lot of bad habits were formed with the slightly broken earlier implementations of UoM and those habits should be broken.

If each binding gives different suggestions, this again creates conflict and confusion.

One thing I wish was that it was more clear in MainUI when this actually happens. Right now Items just seem to magically have State Description metadata even though none is defined. And even if you create some manually, it doesn't seem to override all of the State Description. But unless it's in the add-on docs, the end users really have no idea what it is.

I would even argue that the binding defining a state description is a mistake for the same reason,

The way it's currently implemented I agree. But I also know binding authors really want to control this stuff for the end users.

I just think that Unit.ONE is not the best default unit for QuantityType:Dimensionless.

I'll open a new issue. I think opening a new issue would get more attention and the discussion can be more focused. Or if anyone here has the skills to submit a PR, that would be even better. I suspect the change would be relatively simple.

4077

jlaur commented 10 months ago

The one thing being overlooked in all these discussions is that an item can be linked to two or more channels (and things) at the same time. I know this is not very common, but it is possible. So, suppose hints are implemented, which hint from which binding takes precedence?

The idea was that the hint is used by the UI when creating a new item linked to a channel, i.e. here:

So I imagined the UI would be able to pick up the hint for the given channel and use it to default to unit of the item being created. Since it's a new item being created from a specific channel, I don't think there would be any issues?

mherwege commented 10 months ago

So I imagined the UI would be able to pick up the hint for the given channel and use it to default to unit of the item being created. Since it's a new item being created from a specific channel, I don't think there would be any issues?

Fair enough, if we agree it is not used anywhere else. But I can see the expectation from users coming that linking any existing item also updates this.

jlaur commented 10 months ago

Fair enough, if we agree it is not used anywhere else. But I can see the expectation from users coming that linking any existing item also updates this.

I'm not sure if there are other relevant places, but the idea is only to assist users by providing a better UI default.

I do not suggest to break the decoupling in any way or influence the actual default unit for an item when there is no unit configured.

openhab-bot commented 10 months ago

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

https://community.openhab.org/t/thoughts-on-upgrading-bindings-to-use-new-uom-features-for-humidity/153615/9

andrewfg commented 8 months ago

@jlaur I believe that #4079 would close this, and it is ready to go..

jlaur commented 8 months ago

@jlaur I believe that #4079 would close this, and it is ready to go..

Yes, I believe so too.

jlaur commented 7 months ago

@mherwege - many thanks for implementing this proposal. 👍

openhab-bot commented 7 months ago

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

https://community.openhab.org/t/solar-forecast-pv/137681/208

openhab / openhab-core

[RfC] Make it possible for bindings to provide unit hints #3854

3481 was quite a leap forward in terms of making UoM predictable and reliable.

4077