openhab / openhab-core

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

RFC: Change Number item type for improved UoM handling #3282

Closed J-N-K closed 1 year ago

J-N-K commented 1 year ago

There have been various discussions (#3167, #3247, #3248, #3258, forum about the handling of UoM in Number items. Let me first summarize the current state, then the proposed state and finally the migration path for existing installations:

The 3.x situation

There are two "types" of Number items: plain Number and Number:dimension (like Number:Temperature). In contrast to the documentation they both accept DecimalType and QuantityType state updates and commands. The user needs to know the correct dimension, which is very likely the case if we talk about length or temperature but maybe not so obvious for other cases (take a second and ask yourself if you know the correct dimension for lx, in German it's "Helligkeit" which has more than ten different translations according to dict.leo.org and Google translator returns a wrong translation for our use-case).

State updates

State Number Number:Temperature
DecimalType as-is add default unit (see below)
QuantityType strip-unit as-is (if unit compatible and same measurement system)
convert (if unit compatible and other measurement system
reject (if unit incompatible

Command handling

Command Number Number:Temperature
DecimalType as-is as-is (if linked to a channel with accepted item-type Number)
add default unit (if linked to a channel with accepted item-type Number:dimension)
QuantityType as -is as-is

Persistence

Persistence only stores plain numbers

State stored restored
DecimalType as-is as-is
QuantityType convert to default unit and strip unit add default unit

The default unit is determined (in that order)

If the state description contains %unit% the unit is used as provided by the binding.

The 4.x proposal

The Number:dimension item type is removed. Instead a new metadata unit is introduced. Setting unit enables the item to handle UoM. The user does not need to care about the dimension, he just configures a unit he expects for this item which is much more convenient for those of us that are non-native speakers and don't deal with units in foreign languages in professional work.

State update

State Number without unit Number with unit
DecimalType as-is add configured unit
QuantityType strip-unit convert to configured unit (if unit compatible)
reject (if unit incompatible

Command handling

Command Number without unit Number with unit
DecimalType as-is add configured unit
QuantityType strip unit as-is (if unit compatible)

Persistence

State stored restored
DecimalType as-is as-is
QuantityType convert to configured unit and strip unit add configured unit

Examples

ItemStateEvent unit= metadata stateDescription internal value persisted value restored value displayed value
5.0 none none 5.0 5.0 5.0 5.0
5.0 none %.0f 5.0 5.0 5.0 5
5.0 none %.0f °C 5.0 5.0 5.0 5 °C
5.0 °C none 5.0 °C 5.0 5.0 °C 5.0 °C
5.0 °C %.0f K 5.0 °C 5.0 5.0 °C 278 K
5.0 °C %.0f %unit% 5.0 °C 5.0 5.0 °C 5 °C
5.0 °C none none 5.0 5.0 5.0 5.0
5.0 °C none %.0f 5.0 5.0 5.0 5
5.0 °C none %.0f °C 5.0 5.0 5.0 5 °C
5.0 °C °C none 5.0 °C 5.0 5.0 °C 5.0 °C
5.0 °C °C %.0f K 5.0 °C 5.0 5.0 °C 278 K
5.0 °C °C %.0f %unit% 5.0 °C 5.0 5.0 °C 5 °C
5.0 K °C none -268.15 °C -268.15 -268.15 °C -268.15 °C
5.0 K °C %.0f K -268.15 °C -268.15 -268.15 °C 5 K
5.0 K °C %.0f °C -268.15 °C -268.15 -268.15 °C -268.15 °C
5.0 K °C %.0f %unit% -268.15 °C -268.15 -268.15 °C -268.15 °C

The "internal value" is used in ItemStateChangedEvent and (if accepted ItemStateUpdatedEvent).

The migration

Plain Number items

There is nothing to to for plain Number items. They work like they did before.

Managed Number:dimension items

With #3268 (which could also be run automatically on upgrade, in contrast to what is written in the PR) the dimension could be removed from the item-type. If a dimension is found, the unit metadata is added. In case a state description is present that contains a unit, that unit is used, if not, the system default unit is used. IMO an automatic conversion is desired here, because the user expects plug-and-play for managed entities.

Unmanaged (textual) Number:dimension items

This needs to be discussed. @spacemanspiff2007 offered to provide a script that can be run during update to offer a similar functionality. The question is whether this should be run automatically or on demand, because usually users of textual configuration do not expect any changes to their configuration during upgrade.

UI

UI needs to be adapted to only present Number instead of a list of many items. It should show an additional field unit on creation only, later the unit should be editable like other metadata.

The decision

@openhab/maintainers and @openhab/architecture-council:

Since this requires quite some refactoring, I would like to hear your opinions on that before I start the implementation. IMO the new proposal is easier to understand and much more consistent than the current situation. It allows decoupling of internal state and presentation (because state description is no longer taken into account for determining the unit). To allow extensive testing we should reach a decision here within the next three weeks (i.e. until 2023-01-15) and have the PR merged latest 2023-03-31.

spacemanspiff2007 commented 1 year ago

@J-N-K: Thank you very much for the great summary! It would be nice if you could additionally edit it to provide a small example how the configuration would look like with real values. E.g. a kWh with W update and persistence and the corresponding item configuration. I think examples are great and improve understanding.

I think it would be sufficient to provide the script e.g. through a github repository with a step by step guide on how to run it. I would not expect openHAB to automatically change the textual configuration. If communicated properly it it might be okay to run it once, but I'll leave that up for debate.

rkoshak commented 1 year ago

This is a great summary and thanks for consolidating.

QuantityType convert to configured unit and strip unit add configured unit

Wouldn't the need to convert to configured unit prior to persisting no longer be required? We already convert when receiving a command or an update so the Item should already be in the right units, no conversion necessary (except in one case).

What happens if someone changes the unit on the fly? Is there anything we can do to limit the chaos that would result when (not if) a user decides to do that? Or do we just say "don't do that!" in the docs.

Finally, one more comment related to the call at @openhab/architecture-council. Should we open a thread on the AC discussion? I filed a discussion there a couple years ago to address some of the painful things having to do with UoM and this addresses it nicely. But it does take away a capability binding authors have today in setting the units. Of course they can still push a state description, but now the won't be able to dictate the units in the Item type.

I don't know what impact that will have across all the repos, particularly add-ons. But I think it's worth it. This is a big trouble point for end users and despite the size of the work, I think it's an elegant and consistent solution to most of the end user facing problems with using UoM.

J-N-K commented 1 year ago

What happens if the user changes the unit? The implementation needs to make sure that the internal value is converted. Display is not affected because that is handled by the state description. Restoring values will fail because openHAB has no knowledge of the correct unit for restoring. This is already the case now. It might be possible that persistence services store that information somehow and act accordingly. However, no one implemented that so far.

Regarding add-ons setting the item's unit: That has never been possible. The state description is just a proposal, if the user configured something different in the state description, that always took precedence.

It just comes to my mind that we might still need to keep the dimensions - at least in some way. They are currently used to propose the correct item-type when the item is created from the channel. But this seems to be doable. I'll think about that. Edit: This is more easy than I thought and will also improve the XML validation. We can restrict the item-type in the channel-description to the existing item-types and add a new dimension which can only have the available dimensions (this is already defined in the XSD but not used) as value. The UI can then pick a unit based on the measurement system as proposal when creating a new item.

rkoshak commented 1 year ago

Regarding add-ons setting the item's unit: That has never been possible.

The binding has been able to dictate the "measurement" type at least. It dictates the Item type must be Number:Length. It posts 1.23 m further narrowing down the unit. And it pushes a state description which until now was used, when present, to indicate the restoreOnStartup unit. By default, today, the binding has the ability to define the units from cradle to grave. That would go away.

The state description is just a proposal, if the user configured something different in the state description, that always took precedence.

To be clear, I'm not proposing that the binding should be able to set the units. I'm just pointing out that today, with the mechanisms currently in place, it essentially does and removing that option has implications outside of this repo and end user's expectations.

But if bindings were allowed to set a default unit, I'd have it work the same as with the state description. The binding pushes it's value and the end user can override it (note that fnot too long ago the user was not able to override the binding's suggestion which I think was ultimately determined to be a bug and fixed). I believe they currently push these at Link time and I hope that if it sees it's already set it leaves it alone. So it would only supply a unit in the case where it's linked to an Item that doesn't already have a unit defined. And the end user can override at will.

It just comes to my mind that we might still need to keep the dimensions - at least in some way. They are currently used to propose the correct item-type when the item is created from the channel. But this seems to be doable. I'll think about that.

With this proposal wouldn't the binding just need to know it's a Number. Assuming the binding isn't allowed to supply a default unit, that takes the bindings out of the business of messing with UoM entirely, right?

J-N-K commented 1 year ago

The binding developer still needs to give a hint to the UI what unit should be proposed when creating an item for a channel (otherwise this would be a step backward as that is currently possible). This is similar to the category or tags that can be defined in the channel description XML. From the binding POV this information is not needed.

jlaur commented 1 year ago

I'm wondering if this is something that would need to be considered also, even if just a small note on a checklist: https://www.openhab.org/addons/bindings/mqtt.generic/#channel-type-number

See openhab/openhab-addons#10727.

J-N-K commented 1 year ago

What issues do you expect there?

jlaur commented 1 year ago

Restoring values will fail because openHAB has no knowledge of the correct unit for restoring. This is already the case now. It might be possible that persistence services store that information somehow and act accordingly. However, no one implemented that so far.

I might be able to contribute this for JDBC persistence service if we can agree on a design. Do we want to persist original values to avoid rounding issues etc.? And is scaling still bound to the unit? For example, if a binding would update a channel with values in m3 (e.g. 0.001) and then switch to liters (1) (in a new version), should we persist the "raw" values provided by the binding and the unit:

Time Value Unit
2022.12.30 11:00:00 0.001 m3
2022.12.30 12:00:00 1 L

We would then have to store the unit in a new column in the same table, i.e. each row would contain the unit. This would result in a lot of redundancy because the unit is usually the same.

This could also get a little tricky when performing operations on the persisted data, as we would then need to convert to proper unit. I would need to check implementation though.

The advantage of course would be that there would be no need to migrate any existing data, since everything is persisted with the representation given at the time. But there are also disadvantages like redundancy, complexity on retrieval and possibly incompatible units.

See also: https://github.com/openhab/openhab-addons/issues/13929#issuecomment-1347316074

jlaur commented 1 year ago

What issues do you expect there?

Now briefly looking at the code, you have a point - probably this will not be a problem at all, since the binding will just provide QuantityType according to configured unit, so this should be completely decoupled from what we are discussing here. I will make sure to test it when we get that far.

J-N-K commented 1 year ago

@jlaur That was not my suggestions. I would stay with the way we persist now. But the persistence add-on could e.g. store for each item in which value it did persist when the storage (database, table) is created.

So essentially for JDBC you currently check if a table for that item already exists, if that is not the case you create one. At this point you could store the unit e.g. in a JSON Storage (or perhaps better item metadata) and don't call .getUnit each time you persist something, but always convert to that unit. The same unit is then used for restoring. Changing the internal unit of the item than does nothing regarding persistence.

jlaur commented 1 year ago

@J-N-K - that makes sense. So we would "lock" the unit initially, so in my example it would be m3. When asked to persist in liters, it would be converted to m3, and we would have full consistency. The user could then decide at some point to manually migrate the data and update the metadata at the same time.

spacemanspiff2007 commented 1 year ago

I think this can lead to rather unexpected behavior and obfuscates UoM handling again. The proposal in your first post is very nice because it's simple and consistent.

There is no need for an additional metadata persistenceUnit:

I fail to see the use case and the need for an additional abstraction layer. If the user wants to change the normalization value that's fine but then the user has to migrate the db data, too. (Or accept that it's from now on in a different scale & unit)

J-N-K commented 1 year ago

I would not advocate a general persistenceUnit. That is wrong. But if e.g. JDBC needs to know the unit in which it stored data (because it can't properly store data with units), then it should add something like jdbc=""[unit="m³"] for it's own internal usage.

spacemanspiff2007 commented 1 year ago

Can you elaborate a little bit further - I don't understand the issue. I configure an item to normalize in the unit and the value gets persisted accordingly. Restore works also as expected. jdbc stores only the plain value without a unit.

J-N-K commented 1 year ago

Exactly. And if you change the normalization unit, your persistence data will become worthless. If persistence services could remember what unit they stored the value in, they can properly restore the value.

There are persistence services (like MapDB) that store values together with their unit.Core is completely agnostic of the internal operation of the persistence services and we should not change that.

spacemanspiff2007 commented 1 year ago

Exactly. And if you change the normalization unit, your persistence data will become worthless.

It'll not be worthless, only the normalization is wrong. The majority of databases only stores values (e.g. postgres, mariadb, sqlite, mongodb, ...) so you have to pick a normalization when you start writing data to those. If you change the normalization you of course have to migrate the existing data.

If persistence services could remember what unit they stored the value in, they can properly restore the value.

They can - it's defined as the current UoM. If you want to properly remind the normalization then the persistence service should create a table in the database where the mapping is stored (additionally to the data).


Can you show the use case where an openhab user wants to change the already defined UoM? Because I currently don't see it.

J-N-K commented 1 year ago

Yes, you have to pick something when you create the storage. And afterwards you have to use the same otherwise you'll run into trouble. The solution is to store "additional information for this item, related to an add-on". This is the perfect description of what metadata is.

Why a user would like to change the unit? I don't know. I didn't bring this issue up. But there is a solution to it.

rkoshak commented 1 year ago

Can you show the use case where an openhab user wants to change the already defined UoM? Because I currently don't see it.

If it can be done, some user somewhere is going to do it and then complain mightily when it doesn't work. But the biggest use case will be those users who don't know what they want when getting started and choose the wrong units to start. Later on they figure out what they need and want to change it.

We can't always assume users know and understand the impact and import of their decisions, especially early on.

spacemanspiff2007 commented 1 year ago

I didn't bring this issue up.

If you look closely @jlaur 's question was what will happen if the state update from the binding changes the UoM. We have that fixed with the new UoM concept. He even said it himself:

so this should be completely decoupled from what we are discussing here.


Why a user would like to change the unit? I don't know. I didn't bring this issue up. But there is a solution to it.

Later on they figure out what they need and want to change it.

But we already have the solution and the data: the normalization definition of the UoM item. The normalization represents how the item state interacts with the outside world. It's irrelevant for rules/charting/gui so the only reason to change the normalization is if the outside world needs another normalization, e.g. a db normalization change. So even if I start with the "wrong" one I still can keep using it without penalty.

And beyond that: even the most exotic use case can be covered with a simple follow profile and a proxy item with a different normalization.

If it can be done, some user somewhere is going to do it and then complain mightily when it doesn't work.

Some user will also change the persistence unit metadata and complain that his charts won't work and db values are off. It's the same argument.

The added complexity will only lead to more confusion and strange behavior e.g. I changed the unit, why doesn't my db values change. Whereas it's clear for everyone if the unit gets changed the db will follow.

rkoshak commented 1 year ago

It's irrelevant for rules/charting/gui so the only reason to change the normalization is if the outside world needs another normalization, e.g. a db normalization change. So even if I start with the "wrong" one I still can keep using it without penalty.

Except if the user needs to change it to normalize with the outside world. Maybe they have Grafana charts of some external service that is consuming the values from different sources and they want that service to be consuming values with a different unit and/or scale.

I guarantee you it will happen that some users will want to change the units. I'm not arguing for or against any change recommended here. I'm just pointing out that it's something that will happen. It would be nice if there is a clear and concise documented way for those users to handle it. That's all.

spacemanspiff2007 commented 1 year ago

Except if the user needs to change it to normalize with the outside world. Maybe they have Grafana charts of some external service that is consuming the values from different sources and they want that service to be consuming values with a different unit and/or scale.

Grafana Charts use the value how it is persisted in the database. So this would be a normalization change of the db data I described it above.

it would be nice if there is a clear and concise documented way for those users to handle it. That's all.

Of course! The documentation should clearly state that the normalized state is how the item is reflected in the outside world. Change the normalization - outside value changes too.

spacemanspiff2007 commented 1 year ago

Yesterday another (imho important) thought crossed my mind: If the item changes it unit openHAB should issue an ItemChangedEvent

splatch commented 1 year ago

Out of curiosity, would it be worth to consider support for user controlled default units in first place? From design perspective, would it be worth to separate NumberItem and QuantityItem as to me fair chunk of troubles comes from basic fact that NumberItem tries to do too much and amount of supported cases exceeded its capability leading to situation where StateDescription become a driver for emulating QuantityItem.

Back to first point - the I18nProvider is being involved into selection of default units without possibility o adjust them. It could be separated given that we do have a UnitProvider (which doesn't have to be bound to i18n), cause i18n may not determine stable defaults across different continents. Some countries which are during transition between systems (ie. leaving imperial for metric system) would welcome user adjustable defaults.

rkoshak commented 1 year ago

Out of curiosity, would it be worth to consider support for user controlled default units in first place?

Unless I'm completely mistaken, that's exactly what's proposed here. By default, a Number Item will have no units. Only if the unit Item metadata is populated with a valid unit will the Item carry a QuantityType. This Item metadata is set by the end user. So the end user has to make a positive decision and take an action to use units in the first place and they get to choose the units when they do so. stateDescription is taken out of the picture entirely and relegated to just control how it appears on the UIs, not the Item's state nor how it's stored/restored in persistence.

I argued above (unsuccessfully I think) that the bindings ought to be able to suggest an overridable default for this unit metadata which would leave us a little closer to today's 3.x behavior. But I think the consensus remains that only the user should be allowed to set units on an Item.

By making these changes, most of the problems and surprises that arise when suddenly a Number Item is carrying a QuantityType unexpectedly goes away. The Item can only carry a QuantityType if the user has set the unit metadata. In all other cases, including if the binding sends a QuantityType, the Number Item will end up with just a DecimalType.

splatch commented 1 year ago

Rich, my proposal goes one step further, I suggest to bind NumberItem with DecimalType and do not accept QuantityType in there. Stop pretending that NumberItem can become a QuantityItem because it has unit label. It shouldn't even if its feed with QuantityType from binding or script. Similarly for QuantityItem would be cast only to Dimensionless which would be refused in most of the cases. Make it explicit and do not build assumptions. The Type casting logic embedded in core library should cover necessary conversation (quantity -> decimal and decimal -> dimensionless). Main problem with persistence is that it will always run into troubles because item definition can always be amended, but database contents not unless persistence services begin to listen for item changes, and even if they do listen for that, definitions can be changed when system is down leading to situation where newly loaded items do not match retained data.

spacemanspiff2007 commented 1 year ago

But I think with the new RFC things are clear and I don't understand the benefit the additional restriction might bring. Your seem to have the implementation point of view and I'm describing it from a user point of view. How the behavior is implemented internally is not important for the user, consistent and logical behavior is and I think with the RFC things are going to be very good:

User defines an item with a unit which:

The unit does not:

The openHAB user will typically only change the behavior of the second paragraph.

The only thing what is additionally missing is a profile which will append/set/overwrite a unit on a channel. Then we should have all use cases covered.

Main problem with persistence is that it will always run into troubles because item definition can always be amended, but database contents not unless persistence services begin to listen for item changes, and even if they do listen for that, definitions can be changed when system is down leading to situation where newly loaded items do not match retained data.

I think it would be very dangerous to automatically change persistence data retrospectively and a recipe for disaster. When persisting data one has to decide consciously for a format because almost all databases only store the numerical value (and not the scale and unit). This decision is reflected by the unit metadata and therefore this has to be a conscious decision made by the user. If one wants to change the scale and or unit how the data is persisted the user has to change the unit and eventually migrate the already existing data points in the persistence manually. OpenHAB will behave logically and it's easy to understand.

rkoshak commented 1 year ago

I completely agree with @spacemanspiff2007 . From the end user perspective we achieve everything you are asking for from the end user's perspective (much of what you are concerned about is internal implementation details which, like the intrepid space explorer, is not my main concern).

It shouldn't even if its feed with QuantityType from binding or script.

Covered. If the user doesn't supply a unit metadata, that Item can only ever carry a DecimalType. If a binding or script tries to send aQuantityType, it will be converted to aDecimalType` and the units ignored.

Similarly, if a binding or a script sends a DecimalType where the unit is defined, it will be converted to a QuantityType using unit as the units.

If a binding or a script sends a QuantiyType with different units from unit, if compatible the value is converted to unit. If not an error occurs.

If the user doesn't define a unit, that Item is always a DecimalType. If the user does define unit, that Item is always that unit no matter what is sent to it. The user never has to guess. The user never has to worry about converting or dealing with the fact that the value returned by REST API or seen in events.log may be different than expected. It will always either be the user defined unit or it will always be DecimalType.

There are no surprises and a side effect of this is that the use of QuantityTypes in the first place becomes opt-in instead of opt-out.

From the end user's perspective, I can't imagine anything that would be more explicit in any meaningful way.

The only thing what is additionally missing is a profile which will append/set/overwrite a unit on a channel. Then we should have all use cases covered.

I'm not sure we need a profile for this. If the unit is set and a value without units is provided, unit will be assumed. If a unit different from unit is provided, it will be converted to unit (if compatible). If no unit is provided, the units are thrown away.

What use case am I missing that needs a profile?

The SCRIPT profile (when done and merged) could handle this though so we are not wholly out of luck if there is a use case I can't think of where a profile is needed.

ccutrer commented 1 year ago

Where did we land at on how bindings can provide unit metadata? I've been working on the mqtt.homie binding, and I've fixed that it was always declaring the channel type as a plain Number (and now provides it as a Number: where dimension is the dimension inferred from the unit set on the channel). This is how Main UI chooses the default item type when it offers to create an item linked to a channel. Will ChannelDefinition and/or ChannelType be extended to have Unit on it, for similar functionality (and then the user can change the binding-provided-unit if they prefer as they create the item)?

J-N-K commented 1 year ago

I don't think that bindings should send metadata. They can provide a state description (since probably know what a good unit for displaying is and what precision is available), but that's it. There is no need for the binding to modify the item or the stored state itself.

splatch commented 1 year ago

Covered. If the user doesn't supply a unit metadata, that Item can only ever carry a DecimalType. If a binding or script tries to send a QuantityType, it will be converted to a DecimalType and the units ignored.

This is wrong, cause it will lead to inconsistent behavior known from 3.x where one source can send 10 kWh and other 10000 Wh making number item state go crazy. If you make changes to the behavior, do it once but for good.

Similarly, if a binding or a script sends a DecimalType where the unit is defined, it will be converted to a QuantityType using unit as the units.

This will lead to building up assumptions on users, effectively they will need to re-adjust their rules once again within next major release. Making a big fat warning that something is doing something wrong would let users clear their setups while transition to 4.x. More importantly it will lead to continuous creation of more bindings which will keep relying on that assumption and ie. miss handling quantities sent as commands.

@rkoshak I get above, but still any element which does processing of item itself will have to keep the awkward logic with whether it is a number-number item or number-quantity item. This leads to nested ifs/casts to work with number item state. See that the relying on unit metadata will mean item behavior (its state calculation) is still depend on piece of metadata just like it was with state description, but not on its type. In this regard I consider it no improvement as there is still no clear separation for user (same item type) and developer (same item class with variant state). Removal of Number:<Dimension> is overall good move as there are dimensions which do exist but are not explicitly listed in core and more importantly user interface.

A lot of places have to copy same logic:

Then on the restore:

Where with my proposal this chat is:

second case:

Its in theory one "if" less but we do have a lot of places where such "ifs" are distributed. Also the number item path gets much simpler as any code can assume that number item will return a decimal value and not quantity.

How the behavior is implemented internally is not important for the user, consistent and logical behavior is.

@spacemanspiff2007 It will not be consistent cause current approach still leaves a lot of processing to item caller. Behavior of item is one thing but behavior of addons is another. Looking at how different persistence services are (despite of doing same thing) you can be sure that they will never be aligned or aligned for short period of time. Until first edge case is detected in one of them.

Look at below places, they do fairly similar thing around number item which might or might not have a quantity/dimension specified: https://github.com/openhab/openhab-addons/blob/3.4.x/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcBaseDAO.java#L593L604 https://github.com/openhab/openhab-addons/blob/3.4.x/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcBaseDAO.java#L666L681 https://github.com/openhab/openhab-addons/blob/3.4.x/bundles/org.openhab.persistence.influxdb/src/main/java/org/openhab/persistence/influxdb/internal/InfluxDBStateConvertUtils.java#L126 https://github.com/openhab/openhab-addons/blob/3.4.x/bundles/org.openhab.persistence.dynamodb/src/main/java/org/openhab/persistence/dynamodb/internal/AbstractDynamoDBItem.java#L414L421 https://github.com/openhab/openhab-addons/blob/3.4.x/bundles/org.openhab.persistence.rrd4j/src/main/java/org/openhab/persistence/rrd4j/internal/RRD4jPersistenceService.java#L270L278 https://github.com/openhab/openhab-addons/blob/3.4.x/bundles/org.openhab.persistence.rrd4j/src/main/java/org/openhab/persistence/rrd4j/internal/RRD4jPersistenceService.java#L193L214

I think it would be very dangerous to automatically change persistence data retrospectively and a recipe for disaster. When persisting data one has to decide consciously for a format because almost all databases only store the numerical value (and not the scale and unit).

I haven't proposed that anywhere but outlined it as one of most common issues with evolving deployments.

spacemanspiff2007 commented 1 year ago

I'm not sure we need a profile for this. If the unit is set and a value without units is provided, unit will be assumed. If a unit different from unit is provided, it will be converted to unit (if compatible). If no unit is provided, the units are thrown away.

What use case am I missing that needs a profile?

You have a device that reports °F, but the binding has a unit-less channel (because it's not possible to have a channel with a unit, e.g. if the reported unit can be configured on the device). The item defines the unit as °C. Then you need a mechanism to indicate that the reported value was in °F.


This is wrong, cause it will lead to inconsistent behavior known from 3.x where one source can send 10 kWh and other 10000 Wh making number item state go crazy. If you make changes to the behavior, do it once but for good.

Imho none of these values should be sent to a unit-less item. The unit-less item should get the raw value (raw in not normalized) from the source). If that's not possible then the unit will just be stripped away and the value used. I agree there might be cases where this will not work (e.g. if I post an update through the RestAPI once with "10 kWh" and once with "1000 Wh"). Currently I think ignoring the unit is a okay because it's backwards compatible behavior but if we come to the conclusion this should not be possible it think that's good, too. If it's a Number item without a unit rejecting values with a unit would be consistent.

It will not be consistent cause current approach still leaves a lot of processing to item caller

I understand what are you trying to say. When I crated the first issues with ultimately lead to the RFC my idea was that the item should carry the unit as a field. @J-N-K thought metadata would be better which I didn't like (but for other reasons). He is the expert on the openHAB code and I described it from a user perspective.

And I think it's important that we change those two things: How things should behave and how would be a good way to implement things.

If it's good from an implementation side to make to separate item types - one with unit and one without - then that's fine with me because it ultimately doesn't change how things will behave from a user perspective. If for the code base it's better to have two item types (e.g. Number and Measurement) then go for it.

Now that I think about it it might even make things more clear for the user.

I haven't proposed that anywhere but outlined it as one of most common issues with evolving deployments.

Sorry - then that was a misunderstanding on my side.

rkoshak commented 1 year ago

This is wrong, cause it will lead to inconsistent behavior known from 3.x where one source can send 10 kWh and other 10000 Wh making number item state go crazy. If you make changes to the behavior, do it once but for good.

But that's a problem now. If you send 10 kWh to a Number Item and then send 10000 Wh to that same Item, the Item is going to bounce, of course, because a Number Item without units doesn't understand units. They would need to deploy a transform profile to convert the values from one of the links on the fly or use a rule to normalize the values since they have chosen not use units. TANSTAFL.

In those rare cases where this would happen (one Item linked to two different Channels?), if the user supplies unit (e.g. kWh) the Item will always have kWh no matter what is sent to it.

This will lead to building up assumptions on users, effectively they will need to re-adjust their rules once again within next major release.

Or adjust their Item configs. But this is a time for breaking changes. User only have to re-adjust their rules if they choose to do something different (i.e. they were using units before but are now deciding not to, or the other way around). If they want to continue on as before they may need to make some changes to their Items, not rules. And there might be some tool we can provide to make this transition easier.

@rkoshak I get above, but still any element which does processing of item itself will have to keep the awkward logic with whether it is a number-number item or number-quantity item. This leads to nested ifs/casts to work with number item state.

So you propose a solution that guarantees that users have to adjust all their rules. Which is your bigger concern?

But I don't think it needs to lead to that. We already have getStateAs on the Item. I've already been toying with the suggestion to add support for this to that.

  1. Have the Number Item's .state always return DecimalType.
  2. In those cases where the user knows they want a QuantityType, they can use getStateAs(QuantityType) and get the state with units.
  3. If one uses getStateAs and the Item doesn't have units, return null.

Another alternative could be to always return what ever the Item is carrying but the user can force it to one or the other using getStateAs.

A lot of places have to copy same logic:

I see it as this:

user: store item X state addon: hey X, are you a number Item? (does this actually happen? With the changes in profiles can the binding ever know the type of the end Item since you can use a profile to convert between Item types. item: yes I am! addon: here's 10.11 kWh (the addon doesn't need to know nor care whether there are units on the Item or not

Option 1: item: oops, I have unit of Wh, let's convert that to 10110 Wh

Option 2: item: oops, I don't have a unit, throw away the units and become 10.11

Option 3: item: oops, I have unit °F, throw an error

The bindings(or anything that sends updates) just send the value even as they send it now, with or without units as the binding author desires. Nothing is different from their perspective, so nothing needs to be done there.

Then on the restore:

This is a little closer to what I would expect to happen, but this could/should be centralized in one place, not spread across all the add-ons. The persistence add-ons shouldn't need to change much at all. They can just receive the value (maybe using getStateAs(DecimalType) if my proposal above is accepted and simply reuse the logic on Number Item that is already there to determine whether and what units to use on the restored states.

From my perspective, your proposal is more work spread across lots of repos to add a new Item type (assuming that would even be allowed).

You have a device that reports °F...

OK, that makes sense.

splatch commented 1 year ago

Looking at WIP pull request I do see that number and quantity items are note separated, so I guess that above discussion did not impact initial assumptions.

One thing which I did notice is that we do have unit label for channels but this piece of information can be utilized only by specific items. From that point of view, the channel descriptors are also impacted and inconsistent, because developer can specify unit, even if channel will deliver non-numeric values. Same thing applies to item builders which allow to specify unit, even for items which can't make use of it.

J-N-K commented 1 year ago

The unit attribute is the one thing I don't really like with the current solution, everything else works fine. In some way it's comparable to adding a command description to a TRIGGER channel, that also doesn't make sense but is in general possible. Unfortunately with XSD 1.0 we can't validate conditional attributes.

Item builders are not affected, since the unit is stored as metadata. You can always add metadata that is of no use for the item, it is ignored in that case.

kaikreuzer commented 1 year ago

I'm sorry for having ignored this RfC for so long - it really just came to my attention right now and it makes it difficult to get all the details of your long discussion here. In general, the OT proposal sounds good, I am just struggling a bit with grasping all of its consequences.

I am sharing @rkoshak's concern that we might be losing something if the binding does not provide the dimension anymore.

I argued above (unsuccessfully I think) that the bindings ought to be able to suggest an overridable default for this unit metadata

I do not think that a binding should provide a (default) unit - note that the whole idea of the dimensions is that the binding could provide values in any unit that is compatible with a dimension and the item is then always using units according to the measurement system. So a user with imperial measurement system will always see temperature values in °F, even if the sensor delivers them in °C - the user does not have to do anything for that. Ideally, the user should not have to choose anything when creating an item for a channel. Introducing a step where they need to choose a unit for the item feels wrong. Leaving them with no UoM support if they skip this step, feels wrong as well.

Stop pretending that NumberItem can become a QuantityItem because it has unit label.

I think @splatch has some point here as well and I think it could go together with the OT suggestion in some way. If we now discuss a breaking change, we might consider going the full way and making quantity items a primary citizen. "Hiding" UoM support in metadata and having "Number" again the paramount item type feels like a step back. We did the introduction of UoM onto Number items in 2.x to keep the full backward compatibility and to allow a smooth migration - but the goal was clearly that any measurement should one day be using only quantity types as values. Thinking this to the end means that the Number item type would only be used for discrete values (like enumerations etc.) and percentages, while everything else uses UoM (and if it is Dimensionless). In the end, the Number:<dimension> concept is nothing else than a Quantity item type, though (providing a dimension with it). So one option to do this "split" and simplify things could simply be to not try any type conversions between Numbers and Quantities at all and hence force people to use UoM at all places where this is possible.

J-N-K commented 1 year ago

What we found out during the discussion is that we need two different "units" for an item. One is the unit used in the state description, this is for displaying purposes. The other one is a sort of "internal" unit that is used to hold the item state and also used to store values in databases. What we currently do is mixing these two units and that causes a ton of issues.

I like the concept of "implicit dimensions". A unit clearly identifies the dimension and as I pointed out above, I believe getting the unit right is far easier than getting the dimension right. Everyone knows what is measured in lx, but I guess the correct dimension is far harder, especially for non-native speakers with a non-scientific background. I admit, that I - being a (nuclear) physicist - had to think a bit about the correct english term.

Another perfect example for this mess with far more common dimensions is kW/kWh. Go to the Facebook group of your favorite BEV and find out that your car has a "Ladegeschwindigkeit" ("charging rate") of 150 kW and the battery has a "Leistung" ("power" - maximum storable energy) of 77 kWh a "Ladung" ("charge" - currently stored energy) of 47 kWh. Do you think that these people will be able to figure out that they need to use Number:Power and Number:Energy? IMO it's far more likely they now that the units are kW and kWh.

That said, I'm fine with keeping the dimension.

I also agree that any implicit conversion should be dropped. This is especially true for the very unexpected conversion that happens in the CommunicationManager when a non-dimension item (Number) is linked to a dimension channel (accepted item-type Number:Temperature). By magic a unit gets added to a plain DecimalType command (20, which is still the state of the item) and a QuantityType (20 °C) arrives in the handleCommand method.

But we clearly need to be able to define the inner representation of the value. I think that metadata is the way to go here. Similar to what I originally implemented in #3248.

rkoshak commented 1 year ago

So one option to do this "split" and simplify things could simply be to not try any type conversions between Numbers and Quantities at all and hence force people to use UoM at all places where this is possible.

My concern with this idea of forcing people to use UoM is that actually using QuantityTypes in rules is not the greatest when needing to do math and boolean logic.

These create resistance in the flow for writing rules which some end users strongly object to, so much so that they go to great lengths to eliminate the units when and where ever possible. So I'm cautious about any idea that leads to forcing more use of QuantityTypes and at the same time making it more difficult to convert away from them.

Maybe if getStateAs() were made more flexible so that we could ask it to return the state as a DecimalType or a Number even if the Item carries units or is a different kind of Item that only carries units. JS Scripting just added this with it's myItem.numericState.

Another alternative could be for those languages that support operator overloading, perhaps some heuristic can be used to automatically add units to the operands that don't have them, assuming the value matches the units the QuantityType has, could ease some of the pressure on using QuantityTypes. For those that don't have operator overloading, it can be done in the methods of QuantityType themselves.

These are ideas I've been pondering for some time and have proposed in little ways over the past year or so.

Maybe a little bit of implicit conversion is OK if it's under the control and a deliberate choice of the end user?

Note, my only concern here is working with these units in rules. Everywhere else I'm fine with everything that's been proposed. But for rules, even today I helped someone who said that they have resisted moving to use Units since they were first introduced because doing so breaks all their rules (obviously since you can't do math with them the same way as you can with DecimalTypes). I'm not sure we have a solution for that, but we can maybe make it a little bit easier. But if we force these users to have to use units, they will be quite unhappy.

spacemanspiff2007 commented 1 year ago
  • one must use a QuantityType for both operands

But if the item unit is defined as e.g. kWh then the user should be able to compare with 100 resulting in a comparison against 100 kWh. As per our rules if the item unit is omitted the defined unit is assumed. So things should actually get easier and more consistent.


The user should not have to choose anything when creating an item for a channel. Introducing a step where they need to choose a unit for the item feels wrong. Leaving them with no UoM support if they skip this step, feels wrong as well.

The binding can make a suggestion (e.g. the gui could show a hint ) but the decision has come from the user because of the implications. Omitting this step obfuscates a too much important information. Also what happens if I e.g. link an item to multiple channels? What happens if a channel unit changes on binding update? There are too many possibilities where things can go wrong.

@kaikreuzer TL;DR:

Why:

ccutrer commented 1 year ago

I'm not sure how jRuby handles this

https://openhab.github.io/openhab-jruby/main/OpenHAB/Core/Types/QuantityType.html

In short... quite well. Constructing new QuantityTypes is relatively easy, and a full set of operator overloading and other methods is supported.

rkoshak commented 1 year ago

But if the item unit is defined as e.g. kWh then the user should be able to compare with 100 resulting in a comparison against 100 kWh.

Some Rules DSL cases using a Number:Temperature Item: (note the Item's current state is 15 °F

Simplest straight forward:

vWeather_Temp.state + 32
2023-02-22 07:06:19.645 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test' failed: vWeather_Temp.state +  ___ 32

   Type mismatch: cannot convert from int to String; line 1, column 22, length 2

Swap the order of operands:

32 + vWeather_Temp.state
2023-02-22 07:06:57.994 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test' failed: 32 +  ___ vWeather_Temp.state

   Type mismatch: cannot convert from State to byte; line 1, column 5, length 19

Cast the state to QuantityType<?>:

val result = vWeather_Temp.state as QuantityType<?> + 32
logInfo('test', result.toString)
2023-02-22 07:08:53.143 [INFO ] [org.openhab.core.model.script.test  ] - 295.8100000000000000000000000000000211048

What is that, Kelvin?

Use the | notation to define the 32 as a temperature:

val result = (vWeather_Temp.state as QuantityType<?>) + |32 °F
logInfo('test', result)
2023-02-22 07:10:48.746 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test' failed: val result = (vWeather_Temp.state as QuantityType<?>) + |32 °F
logInfo('test', result)

I expected that one to actually work. 🤷

Create a new QT for the 32:

val result = (vWeather_Temp.state as QuantityType<?>) + new QuantityType('32 °F')
logInfo('test', result)
2023-02-22 07:12:32.560 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test' failed: val result = (vWeather_Temp.state as QuantityType<?>) + new  ___ QuantityType('32 °F')
logInfo('test', result)
   1. Bounds mismatch: The type argument <Quantity<?>> is not a valid substitute for the bounded type parameter <T extends Quantity<T>> of the constructor QuantityType<T>(String); line 1, column 60, length 12
   2. Type mismatch: cannot convert from QuantityType<?> to String; line 2, column 98, length 6

Use the QT add method:

val result = (vWeather_Temp.state as QuantityType<?>).add(new QuantityType('32 °F'))
logInfo('test', result)
2023-02-22 07:14:02.261 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test' failed: val result = (vWeather_Temp.state as QuantityType<?>).add(new  ___ QuantityType('32 °F'))
logInfo('test', result)
   1. Bounds mismatch: The type argument <Quantity<?>> is not a valid substitute for the bounded type parameter <T extends Quantity<T>> of the constructor QuantityType<T>(String); line 1, column 62, length 12
   2. Type mismatch: cannot convert from QuantityType<? extends Quantity<?>> to String; line 2, column 101, length 6

Cast to QuantityType: Since I'm in the UI, I can't import it and have to use the full path.

val result = (vWeather_Temp.state as QuantityType<javax.measure.quantity.Temperature>) + 32
logInfo('test', result.toString)
2023-02-22 07:19:43.027 [INFO ] [org.openhab.core.model.script.test  ] - 295.750000000000000000000000000000021100

Cast to QT and | notation:

val result = (vWeather_Temp.state as QuantityType<javax.measure.quantity.Temperature>) + |32 °F
logInfo('test', result.toString)
2023-02-22 07:24:05.506 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test' failed: logInfo('test', vWeather_Temp.state.toString)
val result = (vWeather_Temp.state as QuantityType<javax.measure.quantity.Temperature>) + |32 °F
logInfo('test', result)

Create new QT through new:

val result = (vWeather_Temp.state as QuantityType<javax.measure.quantity.Temperature>) + new QuantityType('32 °F')
logInfo('test', result.toString)
2023-02-22 07:25:42.396 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test' failed: logInfo('test', vWeather_Temp.state.toString)
val result = (vWeather_Temp.state as QuantityType<javax.measure.quantity.Temperature>) + new QuantityType('32 °F')
logInfo('test', result.toString)
   Bounds mismatch: The type argument <Quantity<?>> is not a valid substitute for the bounded type parameter <T extends Quantity<T>> of the constructor QuantityType<T>(String); line 2, column 139, length 12

I can go on if kit useful. But all I want to do here is add 32 to a temperature. I've tried 9 different ways, all of which seem like they should work. Every single one either generated an error or resulted in a widely unexpected answer (why's it using °K?). And the error changes with almost every attempt.

It's a little better in JS because QuantityType is converted to a js-quantities and it's operations support Strings.

var result = items.vWeather_Temp.quantityState.add('32 °F');
console.log(result);
2023-02-22 07:32:19.273 [INFO ] [nhab.automation.script.ui.scratchpad] - 46.70200000000000000000000000000000517616 °F

But I cannot add a number without units and there is no operator overloading so I have to use the method.

If you are in Rules DSL, good luck figuring out what's going to work. In JS Scripting and Blockly it's more straight forward but awkward. Apparently because jRuby supports operator overloading so it's less awkward.

But this is what I mean by working with QuantityTypes in rules is miserable. Just for completeness, the following works in Rules DSL. There may be others.

val result = (vWeather_Temp.state as QuantityType<javax.measure.quantity.Temperature>).toUnit('°F').add(new QuantityType('32 °F'))

As per our rules if the item unit is omitted the defined unit is assumed.

That's for setting the state of the Item. Not for how the Item's state is used in math operations in rules as far as I'm aware. And even if it did, it doesn't solve most of the problems that are inherent in QuantityTypes.

mhilbush commented 1 year ago

I agree using QuantityTypes in rules is awkward.

Having written a few bindings that use QuantityTypes, I had to figure out how to deal with them in rules (DSL) in order to test my bindings. All this works except for the very last part.

    val QuantityType<Speed> speed1 = new QuantityType(25.0, SIUnits.KILOMETRE_PER_HOUR)
    val QuantityType<Speed> speed2 = speed1 + new QuantityType(20, SIUnits.KILOMETRE_PER_HOUR)
    val QuantityType<Speed> speed3 = speed1 + 10.0|mph
    val QuantityType<Speed> speed4 = speed1 + 2.0|"km/h"
    val QuantityType<Speed> speed5 = 0.85|"m/s"
    val QuantityType<Speed> speed6 = new QuantityType(new DecimalType(20), ImperialUnits.MILES_PER_HOUR)
    val QuantityType<Speed> totalSpeed = speed1 + speed2 + speed3 + speed4 + speed5 + speed6 + 5|"ft/s"

    logInfo("test", "speed1={}", speed1)
    logInfo("test", "speed2={}", speed2)
    logInfo("test", "speed3={}", speed3)
    logInfo("test", "speed4={}", speed4)
    logInfo("test", "speed5={}", speed5)
    logInfo("test", "speed6={}", speed6)
    logInfo("test", "totalSpeed={}", totalSpeed)

    val double speedDouble = totalSpeed.doubleValue
    logInfo("test", "speedDouble={}", speedDouble)

    // Number:Speed    TestItem_NumberSpeed       "Test Number:Speed [%.2f %unit%]"
    TestItem_NumberSpeed.postUpdate(totalSpeed)
    Thread::sleep(500)  // Wait for it to update
    logInfo("test", "TestItem_NumberSpeed.state={}", TestItem_NumberSpeed.state)

    val QuantityType<Speed> speedx = (TestItem_NumberSpeed.state as QuantityType<Speed>) + 5|mph
    logInfo("test", "speedx={}", speedx)

    val speedy = (TestItem_NumberSpeed.state as QuantityType<?>) + 5|mph
    logInfo("test", "speedy={}", speedy)

    val QuantityType<?> speedz = (TestItem_NumberSpeed.state as QuantityType<?>) + (TestItem_NumberSpeed.state as QuantityType<Speed>)
    logInfo("test", "speedz={}", speedz)

    val result = (TestItem_NumberSpeed.state as QuantityType<?>).add(9|mph).subtract(55|"km/h")
    logInfo("test", "result={}", result)

    TestItem_NumberSpeed.postUpdate(result)

    val halfSpeed = (TestItem_NumberSpeed.state as QuantityType<?>) / 2.0
    logInfo("test", "halfSpeed={}", halfSpeed)

    val tripleSpeed = halfSpeed * 3.0
    logInfo("test", "tripleSpeed={}", tripleSpeed)

    // I believe this should work, but it fails and I don't know why
    // https://www.openhab.org/javadoc/latest/org/openhab/core/library/types/quantitytype#divide(java.math.BigDecimal)
    val QuantityType<Speed> quarterSpeed = tripleSpeed.divide(4.0)
    logInfo("test", "quarterSpeed={}", quarterSpeed)
spacemanspiff2007 commented 1 year ago

You both describe how things currently are - not how they should actually behave.

With the new UoM concept it's now possible to convert plain numbers to UoM numbers because the user has supplied a default unit and normalization. Thus it's now possible to convert e.g. 100 to 100 kWh because the user supplied the information. Maybe with some clever operator overloading / type casting it's possible to make the comparisons work out of the box.

rkoshak commented 1 year ago

With the new UoM concept it's now possible to convert plain numbers to UoM numbers because the user has supplied a default unit and normalization. Thus it's now possible to convert e.g. 100 to 100 kWh because the user supplied the information.

My understanding is that everything that has been discussed thus far is how the units are applied or not to the Item. There is no change to how QuantityType itself works being discussed here as far as I can tell. It's still the same class with the same behaviors.

Indeed, it's possible to convert 100 to 100 kWh when you post that as an update to an Item. That doesn't help me add 100 kWh to a QuantityType in a rule though. It's still awkward. JS Scripting at least still doesn't support operator overloading (it's simply not supported by JS). And pushing us in the direction where end users are forced to use UoM in all cases where it's possible to is going to be a net negative to the end user's rule development experience.

Yes, I think there are some things that can be done to make the better, but beyond the couple of ideas I posted above, those are not being discussed as part of this thread. It's all been about managing the units at the Item level, not how units are used in rules.

mhilbush commented 1 year ago

You both describe how things currently are

Really? I didn't know that. 😉

I was just following up @rkoshak's examples with some that work (mostly), in case it would be helpful for others reading this to better understand how current state is applied in rules. And how the syntax can be perceived as quite arcane. And, as @rkoshak mentions, I guess this isn't going away. Is this correct?

In fact, I purposely used mph and km/h to expose the issues that can arise with units that contain slashes.

Personally, the vast majority of the time I've spent on UoM has been in rules, not items. But maybe that's just me.

spacemanspiff2007 commented 1 year ago

My understanding is that everything that has been discussed thus far is how the units are applied or not to the Item. There is no change to how QuantityType itself works being discussed here as far as I can tell. It's still the same class with the same behaviors.

There were some questions how the unit is applied to the item. In the beginning I suggest an additional field on the Item, J-N-K suggested metadata and provided an implementation and then Kai suggested maybe a new class. However this in an implementation detail and I'm confident the experts will find a good solution.

However the UoM RFC provides additional rules (see TL;DR; in previous post for a short summary) how things should behave. One of the rules is "If there is no unit provided the configured unit shall be assumed".

Up until this RFC it was not possible to compare an item state of a UoM item with a plain state because of the ambiguity. E.g. if you have Number:Energy and want to compare it with 100 it's not clear which unit should be assumed (J, Wh kWh, ...). But with this RFC we have now a defined behavior so that now this can be resolved and should be possible. That means it should no longer be necessary to supply a QuantityType in rules because the core/item/QuantityType should be smart enough a assume the configured unit.

There are countless possible solutions how this can be achieved programmatically e.g. add an additional field on the QuantityType with the name defaultUnit and set it to the corresponding configured unit when the QuantityType is set as an item state. The QuantityType can now extend the current comparison logic so when no unit is provided during the comparison the defaultUnit is assumed. However I am no Java expert so there sure is something more elegant.


I guess this isn't going away. Is this correct?

With this RFC it's possible that this will all disappear, however somebody still has to implement it.

JS Scripting at least still doesn't support operator overloading (it's simply not supported by JS)

Since this is part of the core it should also work out of the box with JS

rkoshak commented 1 year ago

Up until this RFC it was not possible to compare an item state of a UoM item with a plain state because of the ambiguity. E.g. if you have Number:Energy and want to compare it with 100 it's not clear which unit should be assumed (J, Wh kWh, ...).

Actually that works now it appears. It's just in my case it chose the wrong units (°K instead of my configured °F) but @mhilbush posted at least one example of doing an operation where one of the operands have no units (the half speed test).

That solves half the problem.

Since this is part of the core it should also work out of the box with JS

How? JS is never going to support using +, -, >, et. al. on something that isn't a JS Number. QuantityType is not and never can be a JS Number. It is and will forever be a Java Class. There's nothing you can do in core that is going to change that.

To aid things openhab-js now converts/wraps the Java QuantityType using the js-quantities library but even here, because there's no operator overloading, we still cannot use the simple math operators and have to resort to calling methods.

I want to be clear. I'm not arguing against the original proposal nor any variation of the original proposal. All I'm arguing against is @kaikreuzer's use of the word "force". If we force the end users to use UoM whether they want to or not without an alternative to not using UoM, it will negatively impact a number of end users and make doing any kind of math in JS Scripting ugly and frustrating.

kaikreuzer commented 1 year ago

My feeling is that we are now discussing two distinct things:

  1. How to define items with units and when and where to do conversions (that's what the RfC is about).
  2. How to deal with QuantityTypes within rules (all the recent comments here on the issue).

Wrt (2): I feel the pain @rkoshak describes, but you sound as if the solution to you is to not use UoM at all - I don't think that's what this RfC is about, it is rather about simplifying the use of UoM. I absolutely agree that handling of QuantityType in rules must be made simpler; but as you say, this RfC won't change anything about that as it is addressing a different problem. Most of your examples above (like that it is not possible to add a QuantityType to a DecimalType), I would actually consider a bug that could/should be solved within the RuleDSL syntax. Any QuantityType has a unit, so when doing arithmetic with a DecimalType, we could simply assume the same unit. JS scripting could be more tricky, but I isn't a DecimalType also already a problem there, since it is a class and not a JS Number? Couldn't QuantityTypes be stripped down to Numbers within scripts, so that in JS you would simply not have units at hand (but you have the advantage of simpler calculations)?

Wrt (1):

The binding can make a suggestion (e.g. the gui could show a hint ) but the decision has come from the user because of the implications.

My point was that the UI should make a sensible suggestion to the user when creating an item for a channel. This should be based on a dimension, not on a unit, since otherwise it contradicts the main reason why UoM had been introduced in the first place: To make metric and imperial systems both easily possible, independent of what the binding provides. If I have my system configured for the imperial system, I want the "suggested" units to be mph, yard, °F, inHg, gal, etc. For this to work, the binding needs to "suggest" the dimension (just as it is doing it right now) and the UI can then suggest the default unit for this dimension for the active measuring system. "Pinning" the unit on an item will actually destroy the possibility to change the measurement system of an installation and automatically see all values being updated automatically. But I agree that this is probably just an theoretical feature and besides some demo / show cases, it isn't really used by anyone - so I would be fine to give up on it.

Personally, the vast majority of the time I've spent on UoM has been in rules, not items. But maybe that's just me.

No, it is the same for me @mhilbush. I never had any issue with item definitions with UoM (especially if they are created through the UI, where it is "just a click"). So for me the handling of QuantityTypes within rules is also a more pressing issue - but as mentioned above, I guess we should move that discussion possibly to a separate issue as it won't be addressed here.

rkoshak commented 1 year ago

Wrt (2): I feel the pain @rkoshak describes, but you sound as if the solution to you is to not use UoM at all - I don't think that's what this RfC is about, it is rather about simplifying the use of UoM.

Not at all. But my desire is to not force the use on those who don't want to use it because of how awkward it can be to work with it. I personally use UoM everywhere I can despite the awkwardness but others should be able to choose otherwise.

JS scripting could be more tricky, but I isn't a DecimalType also already a problem there, since it is a class and not a JS Number?

Until relatively recently, the default way to get the state of an Item is as a String. If you want to do math with it, you had to call Number.parseFloat() on that String. Or you could get the rawState and call toFloat() and get the primitive float which GraalVM (as I understand it) manages the conversion so we get a JS primitive float instead of a Java primitive float.

Now in openhab-js 4.0 and later, Item (the JavaScript representation of an Item, not the OH Java class) implements a quantityState and numericState.

attribute what you get what you can do with it
items.MyItem.state String anything you can do with a String
items.MyItem.numbericState JS 'Number' or null if it's not a state that can become a Number anything you can do with a JS Number
items.MyItem.quantityState js-quantity Quantity or null if it's not a number with units anything you can do with a Quantity (it's not a JS Number so you can't us the math operators).
items.MyItem.rawState Java State Object be careful, because it's Java it doesn't interoperate with JavaScript Objects in predictable ways a lot of the time

The library converts, translates, and otherwise transforms the Java Objects to a JS type which is why this isn't a problem for DecimalType. But the Quantity can't because it still carries the units.

But now that I think about it, JS Scripting has already provided an option to not use the Quantity as if it's a QuantityType, the value without units will be available under numericState. So as long as the other languages are happy and Rules DSL is improved to work in predictable ways (note in the UI one cannot import things in Scripts so using QuantityType<Temperature> is going to be awkward there unless all the measurements are inserted into the default rule preset.

Though I wonder what complexities this might cause on Blockly.

For this to work, the binding needs to "suggest" the dimension (just as it is doing it right now) and the UI can then suggest the default unit for this dimension for the active measuring system.

One place where this currently breaks down is when the destination Item is a different type through some transformation Profile applied to the Link. I just a few minutes ago helped someone solve a problem because they were using the Scale transform to convert an angle to NNW (e.g.) but it was showing up as -- on the sitemap because %d ° is nonsense as a Pattern for a String Item. I would ask that this use case not be forgotten.

spacemanspiff2007 commented 1 year ago

My feeling is that we are now discussing two distinct things:

1. How to define items with units and when and where to do conversions (that's what the RfC is about).

2. How to deal with QuantityTypes within rules (all the recent comments here on the issue).

You're absolutely right. But the 1. has huge implications on 2. and only when 1. has been implemented it allows the simplifications for 2.. Imho it would make sense to split this discussion so we can have the RFC and one or more follow up issues how this can simplify and change things e.g. in rules.

Currently it really feels that we're running another circle (again). 😕


Any QuantityType has a unit, so when doing arithmetic with a DecimalType, we could simply assume the same unit.

In a rule you compare the state of an item > 100.

Current behavior:

Item with `Number:Energy`
State update `50J`   -> Unit is now `J`,   value = 50   Comparison > `100 J`    -> False
State update `50kWh` -> Unit is now `kWh`, value = 50   Comparison > `100 kWh`  -> False

As you can see these are two very different values and the rule will not work as expected.

New behavior:

Item with unit `Wh`
State update `50J`   -> Unit is now `Wh`,  value = 0.05.   Comparison > `100 Wh`  -> False
State update `50kWh` -> Unit is now `Wh`,  value = 50_000  Comparison > `100 Wh`  -> True

Rule will work as expected

You have to infer the unit from the item definition, not the current QuantitiyType. Or as of this RFC suggests normalize the item state to certain unit/scale. That way the comparison in the rule will also work with the QuantitiyType.

My point was that the UI should make a sensible suggestion to the user when creating an item for a channel. This should be based on a dimension, not on a unit, since otherwise it contradicts the main reason why UoM had been introduced in the first place: To make metric and imperial systems both easily possible, independent of what the binding provides. If I have my system configured for the imperial system, I want the "suggested" units to be mph, yard, °F, inHg, gal, etc. For this to work, the binding needs to "suggest" the dimension (just as it is doing it right now) and the UI can then suggest the default unit for this dimension for the active measuring system.

But I see no contradiction here. The binding can still make the suggestion based on a dimension and depending on e.g. the configuration the UI suggests the appropriate unit. E.g. for suggested dimension Temperature the system can suggest unit °F or °C as an item unit. The item creation is not (really) affected by this change. It's only once the item is created the values are always (internally) normalized to the configured unit/scale. But you can still pick a different unit/scale for visualization and charting.

"Pinning" the unit on an item will actually destroy the possibility to change the measurement system of an installation and automatically see all values being updated automatically. But I agree that this is probably just an theoretical feature and besides some demo / show cases, it isn't really used by anyone - so I would be fine to give up on it.

As soon as you work with persistence or interact with the outside world this causes endless problems. Item with 30°F, persisted as 30. Now you change the measurement system, same item is persisted as 0. Charts are broken, historical data is broken, restore is/might be broken. This unpredictable behavior is one of the reasons for this RFC.

J-N-K commented 1 year ago

Currently the CommunicationManager determines if units shall be added or stripped depending on the item and channel-type.

Channel/Item Number Number:dimension
Number as-is strip if present
Number:dimension strip if present add if missing and compatible

Shall we keep this logic or remove it? Removing it would be more clear, but could break setups where a Number:dimension item is linked to a channel that does not accept QuantityType. This could be mitigated by adding a system:strip-unit profile that user can add to support such links.

@openhab/maintainers @openhab/architecture-council Please put your opinion here. Time is running out to get such a big change merged for OH4 and I believe just staying where we are is not an option.