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

conversion to base unit on persisting #3167

Closed
mstormi opened this issue Nov 20, 2022 · 19 comments
Closed

conversion to base unit on persisting #3167

mstormi opened this issue Nov 20, 2022 · 19 comments

Comments

@mstormi
Copy link

mstormi commented Nov 20, 2022

Persistence so far does not store/restore UoM but only the value.
I suggest to now forcefully convert a QuantityType item to its base unit before we persist it to statically work around our current lack to store the unit and to avoid having to refactor all of our persistence implementation.
Following up on #3121, now with #3143 merged we have default units for all item UoM types.

This added conversion will break some installations where users chose to apply units other than the one that science defined to be the base unit for a physical dimension. It's a philosophy discussion whether that's breakage or rather as I see it the opposite, overdue fixing of a long-standing problem, but either way it's a breaking change.
From a user pov it breaks e.g. their OH charts.

I suggest to make it an option so users can choose for themselves whether to apply it to their environment or not.
For details and discussion see #3121.

@rkoshak
Copy link

rkoshak commented Nov 21, 2022

Consider the following scenario.

I live in the US and use °F for temperature. All my sensors report °F and all but let's say I've one device that only supports °C. Let's consider the Setpoint channel on that one device. I've linked that to a Number:Temperature Item and I've configured the state description to °F and my locale defaults are °F.

My understanding is that the Item would carry °C but show me °F and ultimately store °F in persistence. What's going to happen on restoreOnStartup? Will I have to worry about calculation errors creeping in if this Item is constantly being converted back and forth between °C and °F?

What if I change the scenario a little and instead of being delivered °C from the binding, for this one Item I set the state description of °C but my locale still defaults to °F. Same question, will conversion errors potentially accumulate as the value is converted back and forth?

My main point is, as this gets implemented, consider some of the edge cases and make sure we have a reasonable answer for them.

@mherwege
Copy link
Contributor

I think I am already running into the issue.
I receive an electricity meter reading through mqtt in kWh. The channel in mqtt has unit kWh applied to it.
I had state description %.0f %unit%, and after the upgrade to 3.4M5, the values were persisted in J, giving a huge increase. Setting the state description to %.0f kWh brought it back to the previous situation.

@rkoshak
Copy link

rkoshak commented Nov 28, 2022

I think I am already running into the issue.

That's a different issue. It might be worth filing a separate issue as this one is strictly dealing with persistence, and it's not yet been implemented anyway.

I'm not sure whether it belongs here or on the MQTT binding though. Either the MQTT binding is not supplying the kWh the way it should, or somehow the system default is taking precedence over the binding provided units.

Your fix, by using the stateDescription works because that takes precedence over the system default.

@mstormi
Copy link
Author

mstormi commented Dec 21, 2022

For almost any item type, physics define the base unit to be used.
Length base is m (meters) no matter if your use case predominantly uses nm, m, km or light-years, you can represent any of those as multiples or fractions of m, too.

My understanding is that the Item would carry °C but show me °F and ultimately store °F in persistence.

I did not check but assume that C is the default unit for temps introduced in #3121 (I know it's American to believe you are the reference but this time the Old World reigns physics, sorry :-) :-)).
With my proposal, any of your °F values would be converted to °C before storing in persistence, then on restore we would read the value back and apply the default unit i.e. set it to C. You would have a stateDescription pattern of %.1f °F so everywhere you display it it's converted on the fly to °F but in the number item it would remain to be stored as a °C value.
Same for ALL of your °F items.
Your example sensor updates the item with a °C value and would not be converted but remains. If the binding does not provide the unit, the default from #3121 will be applied (C that is, too).

@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/uom-default-units-and-consequences/142360/42

@spacemanspiff2007
Copy link
Contributor

I can't emphasize enough how strongly I am against this!
It would render many database functionalities other than openHAB internal persistence useless.

Let's stick to the Fahrenheit example:
@rkoshak 's Sensor reports reports 25°C which then gets transformed to 77°F. The persisted value in the db will be in the base unit which we assume to be °C.
So Sensor 25°C -> Item: 77°F -> Persistence: 25°C

The DB is also connected e.g. with Graphana which is used for charting.
The chart for the item will be gibberish and useless because the values doesn't make any sense for him.

Other example:
The default unit for energy consumption was Joule, so every energy meter would store e.g. a daily energy consumption of 18000000J instead of 5kWh. If you want to run a Query that selects all the days with an energy consumption >= 7kWh you have to manually calculate kWh -> J, run the Query and then manually calculate all values back from J -> kWh.

If you only use openHAB and openHAB internal charting all this is no problem but as soon as you want further work with the data in database it'll get a huge pain.

The only correct way is to respect the unit of the item and persist the value how it's represented in openHAB.
The value you see (as in see as an item state) is the value that will be persisted.

@pacive
Copy link
Member

pacive commented Dec 22, 2022

I agree with @spacemanspiff2007, this would make visualization using e.g. grafana a real pain.

Another example: the SMHI binding (and perhaps other weather bindings?) report precipitation intensity in mm/h, which is a unit of speed. The default unit for speed according to SI is m/s, so in my grafana graphs a precipitation intensity of 5 mm/h would be displayed as 5 / 3 600 000 = 1.3888888888889E-6 m/s (0.00000139 m/s), which would be very hard to interpret.

Either use the state description and store the value in that unit, or create a new configuration for the base unit for each Item (which falls back to the default if not set), and use that unit when restoring from persistence.

@rkoshak
Copy link

rkoshak commented Dec 22, 2022

Either use the state description and store the value in that unit, or create a new configuration for the base unit for each Item (which falls back to the default if not set), and use that unit when restoring from persistence.

See #3248

@mstormi
Copy link
Author

mstormi commented Dec 22, 2022

Let's stick to the Fahrenheit example:

Better not use this as an example because this is not about normalization. It is a special case to convert to handle changes across measurement system borders.
See https://github.com/openhab/openhab-core/pull/3143/files#diff-fe824d8b8b1153ce77e551aa832a8a4fca9066fa6480ca7a4bdb2f9851d3765bR403
It means "the" default unit for temp is not C everywhere but depends on locale so would be F in Rich' setup.
So unlike you and I as well have been thinking, the value will not be persisted in C but in F.

The default unit for energy consumption was Joule, so every energy meter would store e.g. a daily energy consumption of 18000000J instead of 5kWh. If you want to run a Query that selects all the days with an energy consumption >= 7kWh you have to manually calculate kWh -> J, run the Query and then manually calculate all values back from J -> kWh.

And it's only breaking if you 1) have been using a non-default unit so far 2) you want to keep working with historic data across the point in time you apply the change (rather than make a simple cut) 3) don't want to convert your pre-change historic data (which would be a simple one-time action, particularly when you use an external DB. That's a low-cost effort, isn't it.)
Given any fix will be a breaking change in one way or another, I would consider that acceptable even for edge cases like this is one.
Granted, it was physically the "right" base unit to use J (= Ws) rather than Wh but probably not an ideal choice as Wh is more widely used.
@J-N-K would you consider changing it ?

The default unit for speed according to SI is m/s, so in my grafana graphs a precipitation intensity of 5 mm/h would be displayed as 5 / 3 600 000 = 1.3888888888889E-6 m/s (0.00000139 m/s),

I think that's not the right view angle on the problem.
You would only need to add a display conversion in Grafana just like you have to do with the stateDescription pattern inside OH.

And you don't have to use m/s.
Isn't there a separate Number:Precipitation UoM item type you can use rather than Number:Speed ?
If not I think there should be because while both units use the same dimensions, their use case and meaning is really different and noone ever said that only one UoM type may exist for a specific combo of physical dimensions.

@mstormi
Copy link
Author

mstormi commented Dec 22, 2022

See https://community.openhab.org/t/uom-default-units-and-consequences/142360/98

Someone should test this out with 3.4.
Sturm im Wasserglas ?

@spacemanspiff2007
Copy link
Contributor

And it's only breaking if you 1) have been using a non-default unit so far 2) you want to keep working with historic data across the point in time you apply the change (rather than make a simple cut) 3) don't want to convert your pre-change historic data (which would be a simple one-time action, particularly when you use an external DB. That's a low-cost effort, isn't it.)
Given any fix will be a breaking change in one way or another, I would consider that acceptable even for edge cases like this is one.

Please read carefully.
My issue raised is that when working with a database I first have to manually convert to the query to the unit and scale picked by openHAB, then run the query and then manually convert the returned values back into the unit and scale that is desired.

You would only need to add a display conversion in Grafana

That would imply that everything that interacts with databases from openHAB has support for these display conversions.
Additionally for every interaction with the database I would have to set these conversion up.


It would be much more elegant if there is one place to define once which unit and scale is used for an item and that should be on the item itself.

@rkoshak
Copy link

rkoshak commented Dec 23, 2022

It would be much more elegant if there is one place to define once which unit and scale is used for an item and that should be on the item itself.

I've posted it before but I think it's worth saying a little more. PR #3248 is super relevant to this conversation. The reason why is that it implements exactly this. A way to define a default base unit on persisting. You'll have metadata to define on the Item (simple, something like { defaultUnit='Wh' } to specify what unit the value is saved to persistence as. If the Item happens to not match that unit, it will be converted prior to being saved.

Only in cases where there is no defaultUnit defined will a conversion take place to the system default unit for the Item type, if the Item isn't already in that unit.

In short PR #3248 gives the end user a way to control what units/scale the values have when they get saved. And this way is independent from your display on the sitemap/MainUI which is still controlled through the State Description pattern.

@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/uom-default-units-and-consequences/142360/115

@spacemanspiff2007
Copy link
Contributor

I'll try again to explain my thoughts again because of all the default discussion I still have the feeling that my point is lost.

I've posted it before but I think it's worth saying a little more. PR #3248 is super relevant to this conversation. The reason why is that it implements exactly this.

From what I understand from the issue the PR implements the following:
If a binding or a Command/State Update does not provide a Unit the Unit specified with defaultUnit will be assumed and used for the command.
E.g. postUpdate("33") will result in postUpdate("33 kW")

Additionally the persistence service will use the defaultUnit to save the item value.
An item Value of "1000W" will be persisted as "1kW" if the defaultUnitis set to kW.

Both things are a good start but I there are two things I think are sub optimal:

  • defaultUnit as a parameter is wrong since "1000W" already has a Unit so one would think it doesn't apply here
    (but that's just nitpicking)
  • The logfiles and events on the EvenBus will show an Event with "1000W" yet that's not what will be persisted.
  • The RestAPI will show a state with "1000W" yet that's not what was persisted.

Especially the second/third point can be very confusing because there is a discrepancy on what is happening in openHAB and what is shown in the external system.

If one could enforce a unit and scale on an internal level things would be much more consistent.
First I define
"This item state should always be in kW"

Then

  • postUpdate("33") will result in postUpdate("33 kW")
    → Makes sense since the item state is defined to be in kW
  • postUpdate("3000W") will result in an item state of 3 kW)
    → Makes sense since the item state is defined be be in kW
  • Persistence will persist the state 33 kW as 33
    → Makes sense since that is exactly the state that was reported in the log files / RestApi / Event Files
    → Makes sense since I've defined that this item is in kW

As you see if the unit and scale is part of a number item it's immediately clear what is happening throughout openHAB and all ambiguity is removed.
I've learned that the internal representation of a value doesn't matter because of BigDecimal so why not give the user an option to chose both unit and scale for the internal representation of values.
What way the item state is consistent across systems.
This will be a huge improvement over how things are now with little to no downsides!

Additional Nodes:

  • If no value for the state unit and scale is provided a default is picked
  • The state unit and scale is independent of the state description which is used for the GUI

@J-N-K
Copy link
Member

J-N-K commented Dec 24, 2022

See also #2253

@rkoshak
Copy link

rkoshak commented Dec 28, 2022

postUpdate("3000W") will result in an item state of 3 kW)
→ Makes sense since the item state is defined be be in kW

This is the only part that's missing with what's currently being proposed. Right now, if you post "3000W", the Item carries "3000W". It'll be converted upon saving to persistence to kW and restored as kW.

Personally, I'm a little uncomfortable with this type of conversion, but I'm also uncomfortable with the conversion on persistence. But I can see how it can be useful to normalize the units everywhere when a defaultUnit is defined on an Item.

But what of the case where a defaultUnit isn't defined. There's two cases with two subcases:

  1. unit is provided in the update
    a. provided unit is the same as the unit the Item already has
    b. provided unit is different from the unit the Item already has
  2. no unit is provided

1.b. is troubling when it comes to persistence. But this should be exceedingly rare I would hope.

@spacemanspiff2007
Copy link
Contributor

If we do it the way I proposed it's a non issue:
Then there are only two kind of numeric items:

  1. plain number
  2. number with unit and scale (and normalization)
    (If the unit and scale is defined through the user or through the system locale is out of scope here)

Then the cases become easy:

  1. Unit is provided with a state update:
    1.1. Unit will be stripped
    1.2. State will be converted to the configured unit and normalized

  2. No unit is provided with a state update:
    2.1. Nothing to do
    2.2. The currently configured unit is assumed for the state update (no conversion and no normalization happens)

@rkoshak
Copy link

rkoshak commented Dec 29, 2022

If we do it the way I proposed it's a non issue:

You are missing a case and it's the case I'm worried about.

Number Item Type Update defaultUnit Item State Comment
1 Number 1 1
2 Number 1 kWh 1
3 Number 1 kWh kWh 1 Default unit is ignored
4 Number:Energy 1 1 Wh System default is Wh
5 Number:Energy 1 kWh 1 kWh Item was NULL or already kWh
6 Number:Energy 1 kWh 1 kWh. Item state was 1 Wh before
7 Number:Energy 1000 Wh kWh. 1 kWh

It's case 6 I'm worried about. When the user doesn't supply a default unit but the Item is defined to have units and a unit was supplied in the update.

But given the direction of #3248 is going case 6 is also solved. If we remove the Item type from the Number and require the use of defaultUnit to indicate what units the Number carry, cases 3, 4, 5, and 6 become impossible.

@J-N-K
Copy link
Member

J-N-K commented Dec 29, 2022

Closed, superseded by #3282.

@J-N-K J-N-K closed this as completed Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants