Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
J-N-K opened this issue Dec 29, 2022 · 89 comments
Closed

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

J-N-K opened this issue Dec 29, 2022 · 89 comments
Assignees

Comments

@J-N-K
Copy link
Member

J-N-K commented Dec 29, 2022

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)

  • from the state description
  • from the list of system default units

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.

@J-N-K J-N-K pinned this issue Dec 29, 2022
@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Dec 29, 2022

@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
Copy link

rkoshak commented Dec 29, 2022

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
Copy link
Member Author

J-N-K commented Dec 29, 2022

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
Copy link

rkoshak commented Dec 29, 2022

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
Copy link
Member Author

J-N-K commented Dec 29, 2022

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
Copy link
Contributor

jlaur commented Dec 30, 2022

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
Copy link
Member Author

J-N-K commented Dec 30, 2022

What issues do you expect there?

@jlaur
Copy link
Contributor

jlaur commented Dec 30, 2022

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:
openhab/openhab-addons#13929 (comment)

@jlaur
Copy link
Contributor

jlaur commented Dec 30, 2022

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
Copy link
Member Author

J-N-K commented Dec 30, 2022

@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
Copy link
Contributor

jlaur commented Dec 30, 2022

@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
Copy link
Contributor

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:

  • Charting should use the GUI state representation (from the label) so this doesn't depend on the internal normalized state.
  • You can use a simple follow profile to have the same item represented in two different normalizations and only persist one of them
  • Logged values are different than the values that are persistence
  • See inconsistencies from my first issue.

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
Copy link
Member Author

J-N-K commented Dec 30, 2022

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
Copy link
Contributor

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
Copy link
Member Author

J-N-K commented Dec 30, 2022

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
Copy link
Contributor

spacemanspiff2007 commented Dec 30, 2022

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
Copy link
Member Author

J-N-K commented Dec 30, 2022

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
Copy link

rkoshak commented Dec 30, 2022

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
Copy link
Contributor

spacemanspiff2007 commented Dec 30, 2022

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
Copy link

rkoshak commented Dec 30, 2022

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
Copy link
Contributor

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.

@J-N-K J-N-K closed this as completed Apr 18, 2023
@J-N-K J-N-K unpinned this issue Apr 18, 2023
@spacemanspiff2007
Copy link
Contributor

It seems no consensus can be reached

Is that a hint in my direction? Because I especially stated above that the PR will be better than what we have now.

@J-N-K
Copy link
Member Author

J-N-K commented Apr 18, 2023

I have spent far more than 100 hours on discussions in issues (this one and related) and the creation of the associated PRs (#3248, #3268, #3367, #3481 and additions in openhab-addons and openhab-webui). Still it seems that no one is satisfied and we are discussing the same issues over and over again (and sorry, that also includes you) and every change is considered a bad thing. I'll not again be the bad guy that "breaks everything" and "recklessly moves forward much too fast" like with the Java 17 change.

Maybe we can give it another try for OH5 in some years.

@spacemanspiff2007
Copy link
Contributor

(and sorry, that also includes you)

No need to say sorry because it's a fact.
Obviously I'm also heavily invested and want OH to have good concept so it can have a sustainable development.
Nonetheless I understand that you are frustrated because I would be (and am) too.
Anyway - thanks for you effort.

we are discussing the same issues over and over again

It's time consuming to read the whole history and as I stated it seems (almost) nobody is willing to read and understand the issue, yet has an opinion and makes suggestions. That in evidently leads to discussions which run in circles.
Imho this issue should have been at some point a call/conference which freezes the concept.

At least we now have a decision and can move forward from that.

I've been in limbo what to do about the UoM items in HABApp but now I have something I can work with.

Maybe we can give it another try for OH5 in some years.

Let's see if we'll still be motivated and have the time and energy. 😉

@lolodomo
Copy link
Contributor

lolodomo commented Apr 18, 2023

iMHO, last @J-N-K proposal keeping dimension was a good compromise, it was not breaking current setups too much and if my understanding is good, it was not requiring a code migration in all existing bindings. And it was apparently adding the features requested by few advanced users.

The question is not whether we want or not to move. Just be pragmatic. If a change in core framework would break all exishing bindings (I suppose removing dimension from Number will?), who will be volunteer to fix all of them? Probably neither @J-N-K nor any existing addon maintainer, this will be done only on well maintained bindings. This is acceptable when the changes are bringing a lot of new things like when we moved from OH1 to OH2. Doing that just for the fun to remove dimension from Number, I am not sure at all it's worth the required effort.
Just my opinion.

@mherwege
Copy link
Contributor

I have been following this discussion for quite a while and looked forward to the outcome of it.
I think the proposal was a good compromise. While I do understand all the arguments for dropping the dimension, I do see dropping it has a major impact on bindings. And it will require more explaining to the existing users (even it is a simplification).
I do not consider doing anything at all an option. At least a separation between default unit and unit as conveyed in a state description should be made. It has hunted me a number of times and is difficult to explain/understand to someone that is less in the details. I still believe the logic: dimension has a default unit and the default unit can be overriden in the state description for representation is easy enough to understand, even when dimension is not strictly required. But the current logic: dimension's default unit is a bit of black magic based on the presence or not of state descriptions, is not acceptable. I call it black magic, and I do unstand there is clear logic behind it, but it is not always easy to understand the consequences of changes. So I would vote for implementing @J-N-K proposal.

@J-N-K
Copy link
Member Author

J-N-K commented Apr 18, 2023

@lolodomo The question is exactly that: If nothing can be touched without people complaining, then the decision is not to move. And when I originally suggested to remove the dimension from the item and channel (#3367) and let it automatically determine the correct dimension by the unit, I also added a PR in addons (openhab/openhab-addons#14389) that would have solved far more than 90% of all issues, except those that use dynamic channel types (channel types not channels!).

@mherwege It would have been good to have read some kind of support for that proposal before. All I read is either "that's too much" or "that's not enough". Feel free to pick up #3481, maybe you'll be more successful in finding someone who reviews and merges it.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Apr 18, 2023

And it was apparently adding the features requested by few advanced users.

This is again a misunderstanding.
The PR was a fix for an issue that affected users who used UoM items and persistence (or rest api events).
If you used those you were affected and if things worked as expected it was just by chance.
This is by no means a feature for super professional users (and I should know since I've requested those) but rather a feature that would have affected almost all users because it would have brought huge simplifications.
This is why I think such a huge change would have benn justified because it would have been a huge gain (e.g. this would also have simplified the use of units in dsl-rules, etc.).

I call it black magic, and I do unstand there is clear logic behind it, but it is not always easy to understand the consequences of changes.

There's always one thing you can do: Not use UoM items.
UoM provides benefits when switching units/scale between states but it provides nothing that can't be done with a rule and a proxy item (albeit not as elegant).
With this PR closed UoM is dead for OH4 and I can (and will) only recommend any user not to use it because the dangers outweigh the benefits by a huge margin.

@rkoshak
Copy link

rkoshak commented Apr 18, 2023

As someone who fights with UoM problems more than anything when helping users in the forum I'm very much saddened to see it come to this. Fighting with UoM "black magic" as @mherwege is probably the number one issue even advanced rules writers have to fight against.

I thought we had come to a good compromise too with keeping the dimensions part of the Number Item type and adding the unit metadata. I was just waiting for a review, a merge, and a party. This was a huge missed opportunity I think and it's very unfortunate.

@lolodomo
Copy link
Contributor

Except @spacemanspiff2007 , I understand all others discussing here were ok with @J-N-K proposal. So why not validating @J-N-K 's simplest option?

@J-N-K
Copy link
Member Author

J-N-K commented Apr 18, 2023

@jimtng Also has a different opinion, from what I read above. Even after reading the whole issue again, I did not find the broad support for my proposal that is now expressed. If we agree not to discuss the concept and concentrate on the code, I can re-open the PR.

@lolodomo
Copy link
Contributor

lolodomo commented Apr 18, 2023

@jimtng Also has a different opinion, from what I read above. Even after reading the whole issue again, I did not find the broad support for my proposal that is now expressed.

Ok, I haven't read all the discussion again.
So maybe you re right and there are more disagreement with your proposal than I imagine.

@splatch
Copy link
Contributor

splatch commented Apr 18, 2023

Well, my points weren't taken into consideration (making clear separation of Decmial vs Quantity types), as I think it will sooner or later become a problem. it is part of problem right now, however way you go is up to you.
I am not a core contributor, actually I am not a nominated contributor to any of repos, so I'm perfectly fine if you treat my voice as an independent observer and ignore it with final implementation. ;-)

@rkoshak
Copy link

rkoshak commented Apr 18, 2023

If we agree not to discuss the concept and concentrate on the code, I can re-open the PR.

I've been satisfied with the concept for some time, which is why I've not been commenting (perhaps I should have been more vocal in my support). Keeping Number:Dimension around doesn't give me too much concern (would it be better to get rid of it, or create a new QuantityType Item, perhaps, but that's not where users are having problems in my experience and the disruption across repos could be huge). Separating the unit of the Item from the state description makes me very happy. Making the unit consistent in all the places the Item is used (REST API, persistence, etc) makes be very happy. The rest (as far as I can tell) are arguments over implementation details which as a non-developer/maintainer on OH I don't have an informed opinion to provide.

And even were I not happy with the current approach, it's a heck of a lot better than what we have now. I'd rather see that and readdress the remaining concerns for OH 5 than abandon all attempts to make UoM work better.

@J-N-K
Copy link
Member Author

J-N-K commented Apr 18, 2023

@splatch Regarding data types DecimalType and QuantityType are already separated. IMO we should probably add some sort of deprecation warning if the "implicit conversion" logic (i.e. adding/stripping units) is called. Users can then add the UoM profile (also in #3481) to the link if necessary and we allow for a smooth transition when we remove these implicit conversions later. This seems to be a much less disruptive approach than breaking these conversions immediately (IMO that should never have been introduced at all, but that decision has been made in OH2 days).

@ccutrer
Copy link
Contributor

ccutrer commented Apr 18, 2023

I've been satisfied with the concept for some time, which is why I've not been commenting

This has basically been my position for some time as well. I've only been tangentially following, since I don't have enough time to get super involved with this. I figured once it all shakes out, I would make any changes (if necessary) to improve support for the new approach in the addons I maintain.

@J-N-K J-N-K reopened this Apr 18, 2023
@splatch
Copy link
Contributor

splatch commented Apr 18, 2023

@splatch Regarding data types DecimalType and QuantityType are already separated. IMO we should probably add some sort of deprecation warning if the "implicit conversion" logic (i.e. adding/stripping units) is called

@J-N-K My point was that same separation should follow up with NumberItem and QuantityItem. Then unit handling would be specific only to QuantityItem.
Personally - effectively any improvement, even if its partial, its better than what we have now with state description driving everything. From my point of view, while I consider problem still be present, its good to go as-is and make a follow up changes and clarifications after 4.0. I understand necessity to provide smooth transition, even if it will double the pain, but span it over longer time.

@J-N-K
Copy link
Member Author

J-N-K commented Apr 18, 2023

But that is only an implementation detail, not a user-facing change. If we have a clear separation Number is for DecimalType and Number:.... is for QuantityType we can refactor the code so that Number is handled by NumberItem and Number:.... is handled by QuantityItem. This is nothing the user will ever notice, so it's far less critical.

I'm far less concerned about changes that require developers to adjust something than changes that require the end-user to adjust something. If we change that in core, most add-on developers (except probably scripting and persistence) will not notice.

@rkoshak
Copy link

rkoshak commented Apr 18, 2023

IMO we should probably add some sort of deprecation warning if the "implicit conversion" logic

I really like that! If you add a warning log statement, users will freak out and move mountains to get rid of it (especially if it happens a lot like on every update/change to the Item). If the warning tells them what to do to fix it, so much the better! :-D

@jimtng
Copy link
Contributor

jimtng commented Apr 18, 2023

I'm fine with the latest pr keeping the dimension. Tbh I don't know what's best so I was just asking questions trying to understand. I'm sorry if it came across as being negative.

I think the general consensus, including my own is to support the proposal.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Apr 19, 2023

Except @spacemanspiff2007 , I understand all others discussing here were ok with @J-N-K proposal. So why not validating @J-N-K 's simplest option?

@J-N-K is doing all the work so he gets to say what he is implementing.
I reported the initial issues and have a pretty good understanding what the feature (imho) should look like however I am not the one to decide what and how. I've tried to make my arguments clear countless times.

Besides - and correct me if I am wrong - even @J-N-K thought that it would be a good idea to remove the dimension from the NumberItem (see #14389). However some people made some of out of context claims or made misleading statements because the misunderstood the issue/PR which finally caused him to backtrack and close the PR.

The problem with this issue has never been the lack of feedback and opinions, it has been the lack of qualified feedback.
It's a reoccurring theme if you skim through the issues and PR and you'll often read "I didn't fully understand the issue, but ..." or "I haven't read all the issues, but ...".
Obviously it has been the main reason why we are running in circles and I would have expected someone from the openHAB core maintainers (don't we have some volunteers for architecture, not sure) to provide feedback on the core concept - even if it would have been only a "I don't fully understand it - do what you think is best". Instead this issue and the concept has been left in limbo so even more opinions accumulated.

So now the confusion is perfect and we're already behind M2 and time is running out.
Yet trying to bring clarity still feels like the fight against windmills.
Even I don't know the current state of the PR any more and if it aligns to a consistent user experience.

However pulling at @J-N-K to steer him in my envisioned direction has been very exhausting both for me and him because I'm not the only one pulling him. So I'll stop meddling with him and this issue because after creating the issue and countless comments trying to get everyone onboard now I am the one preventing a solution?
Nothing can be further from the truth.

@mhilbush
Copy link
Contributor

@J-N-K the time you spent on this (and other) PRs is not lost on me. I very much appreciate your contributions.

I thought this PR had reached a good point, which is why I remained silent. Anything than can make the internal representation consistent while allowing the user to define the external representation is a step in the right direction. It's also a good thing that this PR minimizes the impact on bindings.

My earlier post simply pointed out that when it comes to dealing with units, the time-consuming part for most users is when writing rules. And I thought it was important to point out that this PR will not change that much, if at all. I hope that was not considered misleading or out of context.

It would be good to get this finalized and merged soon as we are getting pretty deep into the release cycle for 4.0.

@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/dimensionless-state-description-pattern-unit-vs-rpm-ppm-etc/148723/5

@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/dimensionless-state-description-pattern-unit-vs-rpm-ppm-etc/148723/6

@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/how-to-change-unit-of-quantity-types/150772/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.