default UoM missing

[mstormi]

There's operational states when an UoM item (say Number:Power) has a value but no unit such as after mapdb restoreOnStartup (as persistence generally does not store the unit).
This state might go unnoticed until e.g. the item is cast to the proper type in rulesDSL. rulesDSL refuses to cast and the rule crashes. Sad but correctly so because it does not know which unit to apply (in the Number:Power example whether to apply W or kW or mW).

We need a default unit to apply whenever a UoM item is updated and does not have a unit yet. It is imho not acceptable to leave that problem to the rule writer (incredibly hard to catch and work around everywhere it could surface).
The default unit to apply can be derived from the UoM (when there's none it is reasonable to assume the base unit i.e. W in the example).

[rkoshak]

It gets challenging though for Dimensionless because the units don't all mean the same sort of thing. Dimensionless can be a percentage, parts per million (or billion or what ever), or decibels. What makes sense in this case to be the default? Percent is going to be the most used so maybe that makes sense?

But my understanding is the units defined in the State Description Pattern will be applied when the value is restoredOnStartup so that could be a work around for the time being. And that might ultimately be the right solution over all. Is it better to leave the Item without units or to apply a potentially erroneous one? If the value stored in the database is kW, your rule is going to be off by a thousand when the value is restored from MapDB and because it doesn't have units, W is applied. I'm not sure that isn't even worse than getting an error and having the rule exit, especially since these rules are making decisions based on the results of the calculations.

Personally, were it me, I'd see a complete overhaul of UoM and come to a wholistic design that addresses how it's used everywhere. About the only place where UoM provides a good end user experience is the UI. In rules and in persistence it falls short.

[mstormi]

It gets challenging though for Dimensionless because the units don't all mean the same sort of thing.

Dimensionless is probably the only one where we do not need this. Remember it's a dimensionless value, literally.

But my understanding is the units defined in the State Description Pattern will be applied when the value is restoredOnStartup so that could be a work around for the time being.

No it isn't, at least not in a number of situations, possibly depending on startup order .

Is it better to leave the Item without units or to apply a potentially erroneous one?

I agree it would be better to overhaul UoM, but short of that yes the base unit is applicable in most case so that's better than having to fix it in every code use location.
Maybe on calling item.persist, we should convert what is persisted to the base unit first.

[rkoshak]

Maybe on calling item.persist, we should convert what is persisted to the base unit first.

I'm pretty sure that will muck up MainUI charting which doesn't have the facility to convert between UoM. Since all it gets from persistence is a raw number it depends on the State Description to supply the units. If the numbers in the database do not match the units in the State Description the cart will not be labeled correctly.

I really think we are dancing around the issue though. Persistence should support saving the units somehow. I'm not sure how/if that would work for rrd4j but the other databases should be able to manage it.

I just hate to see another half measure of a patch applied to the use of UoM which only sort of addresses the problem half way. We'd be trading an explicit error that needs to be fixed in rules to a silent error that happens some of the time for no obvious (to most users) reason.

I can already see "my rule works except when I reload my .items file and then the math doesn't come out right" postings to the forum.

[mstormi]

Maybe on calling item.persist, we should convert what is persisted to the base unit first.

I just hate to see another half measure of a patch applied to the use of UoM which only sort of addresses the problem half way.

Would it really be a half way solution? I don't think so.
We have a silent error today in that it is undefined what the unit of a UoM Number item is. That error is independent of the display pattern (which btw is optional) and independent of what's stored in persistence (btw also optional).
Even a change to also persist units would not fix that error. I could still xxx.postUpdate(1234) and we wouldn't know if that means W or kW.
The only proper fix I can think of at the moment is to introduce a mandatory (ugly) or implicit (better) default unit for every UoM item, to apply whenever no unit is given.
Conversion into the base unit before persisting would need to be part of that fix and would even avoid the need to extend the persistence implementation to include unit storage.

Sure we would get quite some posts, but essentially even if you're right with the assumption that it affects charting, that is a one-time event and as such a breaking change that cannot be avoided anyway, no matter what any other solution would look like and I see much more work and impact for any such.

[rkoshak]

Conversion into the base unit before persisting would need to be part of that fix and would even avoid the need to extend the persistence implementation to include unit storage.

I like this idea as it would work in rrd4j as well as the other DBs and it shouldn't impact any of the add-ons themselves. There still needs to be some sort of special handling for Dimensionless I think because I'm not sure it make sense to convert decibels to a percentage (for example).

Charting is going to need a bunch of changes to accommodate something like this and I can imagine the persistence actions will need to be overhauled as well. But this fixes the problem where Items end up with values that are orders of magnitude off of their "real" value because a default unit was blindly applied.

But if I were king, I would do the mandatory unit approach. If I have a Number:Temperature, I shouldn't be able to update that to a value without a Temperature unit. Likewise, if I have just a plain Number, I shouldn't be able to update that to a value with any units. This causes all sorts of problems and confusion.

I'd also make it so that doing math with numbers that have and do not have units more seamless. I've been working on something to help with that in JS Scripting but it would be even better if it were supported across all of OH. I'm experimenting with automatically adding units to a plain number using the units of the other operand of the calculation. That may not make sense all the time but it should most of the time and one can specify the units on their "plain number" to handle the rest.

[J-N-K]

So the problem is the missing unit when a rule is triggered via an ItemStateTrigger? Then this will probably be solved by #3141 because it ensures that really the item state (which usually has a unit) is used.

[mstormi]

So the problem is the missing unit when a rule is triggered via an ItemStateTrigger?

No. It's whenever the item is accessed from rules DSL code (JS and others too, probably) and item was never assigned any unit before (like it is the state when restoreOnStartup was applied).
If I understand your #3141 right, that won't help because there you assume you already know the item's unit which you actually don't. Freshly created and restored items just don't have any unit until you (re)assign one by updating it with a value+unit from rules or channels.

[mstormi]

But if I were king, I would do the mandatory unit approach. If I have a Number:Temperature, I shouldn't be able to update that to a value without a Temperature unit.

Ugh, that now would be a breaking change to result in a lot of "update broke my rules" posts I guess.

[J-N-K]

GenericItem has an optional UnitProvider which - if set - is queried for the unit of no state description is found. The UnitProvider is passed to the item once it is available, I believe this is very early in the startup process, so that should not be an issue.

Currently the only implementation of UnitProvider is I18nProviderImpl. In there we find initDimensionMap and "default" dimensions are set based on the configured measurement system (SI/imperial) for Temperature, Pressure, Speed, Length, Intensity, Angle and Dimensionless (percent).

So it boils down to a limited set of dimensions which have a default unit. I think it leads to very ugly code if we just extend that and we should think about a better way of providing the set to the UnitProvider, maybe a .properties file.

Edit: the mandatory approach is not necessary and would, as @mstormi points out, break tons of existing installations.

[J-N-K]

Maybe on calling item.persist, we should convert what is persisted to the base unit first.

I'm pretty sure that will muck up MainUI charting which doesn't have the facility to convert between UoM. Since all it gets from persistence is a raw number it depends on the State Description to supply the units. If the numbers in the database do not match the units in the State Description the cart will not be labeled correctly.

You'll run into trouble if you change the state description if you use the state description to determine the unit. While this is unlikely for temperatures (I don't think that someone switches e.g. from °C to °F), it might happen for other dimensions like data amount (I'm thinking of internet traffic in MB, GB, TB) or volume (water consumption in l or m^3). Yes, that is the case now, but IMO it's not ideal.

[mstormi]

So it boils down to a limited set of dimensions which have a default unit. I think it leads to very ugly code if we just extend that and we should think about a better way of providing the set to the UnitProvider, maybe a .properties file

Granted I don't know the code but it would only need to be a single table of definitions, one for each UoM type, just the table is now larger than the current one. But it's static, defined as per physics behind, no need for anybody to change (unless you want to make that available as a feature to users).
So no need to map so no need for a .properties, right ?

[J-N-K]

I thought of a .properties file inside the bundle, not externally. But that idea is even better, it allows the user to override the settings (e.g. use mbar instead of hPa for pressure or cm instead of m for length). The distribution could then provide a default and install that in the user directory.

[rkoshak]

Ugh, that now would be a breaking change to result in a lot of "update broke my rules" posts I guess.
Edit: the mandatory approach is not necessary and would, as @mstormi points out, break tons of existing installations.

I'm not really arguing to implement this.

But we already get a lot of posts about people struggling because all of a sudden they have a Number Item that is carrying units. I can't say how many times I or rossko57 have had to say "I know it's defined as a Number, but look in events.log. It might carry units anyway and you have to do the math differently." We set the expectation that Number doesn't carry units then violate that expectation. If you have a Number, you can never be sure whether or not it's carrying units until your rule breaks (or you notice it elsewhere).

I don't see how enforcing the Number types would be any worse than that. What's the point of of having the different Number types if plain old Number can be any one of them at any time, as we have now?

[J-N-K]

We must be careful not to mix different issues:

• Number items with dimension. The should accept DecimalType and QuantityType. If the unit is missing, the default unit should be added. This is what I implemented in the attached PR.
• Number items without dimension. Of course they accept DecimalType. Currently they also accept QuantityType and I feel this is fine. However - and that is what you added in your last comment - the unit should be stripped, so a plain Number item always has a dimensionless state. This is currently not the case.

[rkoshak]

Number items with dimension. The should accept DecimalType and QuantityType. If the unit is missing, the default unit should be added. This is what I implemented in the attached PR.

👍

Number items without dimension. Of course they accept DecimalType. Currently they also accept QuantityType and I feel this is fine. However - and that is what you added in your last comment - the unit should be stripped, so a plain Number item always has a dimensionless state. This is currently not the case.

That would make me happy too (I should have thought of that). It's that surprise when a Number has units. Stripping them seems reasonable.

[mstormi]

• This is what I implemented in the attached PR.

👍

Remember conversion into the base unit before persisting should be part of a comprehensive fix, as
should be stripping units on assignments to Number item, like you said.
Will you be extending #3143 or put up more PRs?

[mstormi]

@J-N-K do you still plan to intro a unit.properties mapping file?
plus as suggested above conversion to base unit before persisting ?

With 3.4 and code freeze now in sight it would be half-hearted to have #3141 implemented but having to wait for the rest until 3.5.

[J-N-K]

I think the conversion to base unit before persisting is a big breaking change and essentially makes "old" persisted data useless, because you can't convert it. This is not something that should happen in a minor release, where mostly new features are expected and only minor breaking changes should occur.

The stripped units and the base units are more feature/bugfix than breaking. They are only breaking if you relied on the undocumented "we keep units even for dimensionless items".

I still have no good idea how to implement unit overrides in a way that they can be configured via UI. We should not again deliver features that can't be configured from UI and later find out that we have to change everything again because it's needed for UI.

[mstormi]

We should not again deliver features that can't be configured from UI

We don't need full override capabilities just defaults to apply if no unit is given, do we ? Which is what #3143 does.

Or do you mean that a defaultunit.properties file is not sufficient for that but needs a GUI equivalent ?
I wouldn't think so as the need to really change any mapping is very rare. Even GUI apologists should be fine with editing a file in that case.

On conversion to base unit, I agree it's a breaker albeit not that big.
Remember it's just breaking where you used non-default bases so far - which probably not many users did.
And it does not mean we have to wait for whatever next release.
At some point the user has to take a conscious decision to invalidate his old data (if applicable), and that's unrelated to the OH version. So why not offer that to him immediately ? Waiting for OH4 is just delaying a solution, that does not improve the situation.
BTW it won't affect new installs as with #3143 those will get the base unit assigned as their default right from the beginning.
Conversion could be made an option, off by default. wdyt?

[J-N-K]

I'm not fine with editing files.

It's breaking for every item where the unit was set in the state description to something different than what we now set as default. Since we did not provide defaults for most dimensions my guess would be that the state description was used quite often.

[mstormi]

Frankly, your post completely confused me.

I'm not fine with editing files.

I'm talking about changing default units per physical dimension. The base unit to use if no unit is given.
That's not per item. That you'll do essentially just once, after installation. You would only do that anyway if you want special stuff like mbar instead of hPa.
Cannot believe you're not fine with editing a file once in an installation lifetime. Most won't want to anyway, I'd be fine if you cannot, BTW it was your own suggestion to make that user accessible.

It's breaking for every item where the unit was set in the state description to something different than what we now set as default

I don't understand.
What do you mean by "It" and what does that break ?

Adding or changing state description metadata won't change the value. Neither will changing the (just introduced) physical default unit. Where do you think that breaks anything ?

Say we have a Number:Power item, with #3143 merged that now will have a default unit W.
If the value was 1W before, nothing will change.
If the user had NO unit set and the value is 5 and stateDescription on that item is set to kW, it used to be displayed as 5kW.
Now on next value assignment "1" (again w/o unit), unit will be set for the first time, applying the new defaults.
The value will be 1 W and item will be displayed as 0.001kW from there on and the user will think this is an error.
But the value remains 1 and keeps being stored to persistence etc as such. Its meaning (if that value is supposed to mean 1W or 1kW) has always been determined either by the device channel or for purely virtual items by the programmer. That meaning doesn't change and never has. So if the meaning was W before, the previous display 5kW or 1kW was wrong anyway.
Granted hard to understand for an affected user but the only proper way to get outta this pithole, and will only affect people that misconfigured stuff because they were not understanding what they're doing such as misusing/mislabelling a W channel as kW.

[J-N-K]

I'm not against having it user-configurable. I'm against a solution that can only be achieved by editing files. Because the same argument applies to nearly all settings on the right side of the settings page. I believe changing the timezone or the API security settings is also not something you would do every day.

I overlooked a ! in the code when checking if conversion should be done:

      // QuantityType update, check unit and convert if necessary:
        if (state instanceof QuantityType) {
            Unit<?> itemUnit = getUnit();
            Unit<?> stateUnit = ((QuantityType<?>) state).getUnit();
            if (itemUnit != null && (!stateUnit.getSystemUnit().equals(itemUnit.getSystemUnit())
                    || UnitUtils.isDifferentMeasurementSystem(itemUnit, stateUnit))) {
                QuantityType<?> convertedState = ((QuantityType<?>) state).toInvertibleUnit(itemUnit);
                if (convertedState != null) {
                    super.setState(convertedState);
                    return;
                }

                // the state could not be converted to an accepted unit.
                return;
            }
        }

However, this is a different discussion. I would suggest to open a new issue for "always persist base unit". I'm willing to implement that (and it's not very difficult to do so) if a majority of the maintainers agrees that we should take this breaking change.

[mstormi]

so by "It" you refer to implmeneting the conversion to base unit ?

[mstormi]

I'm not against having it user-configurable. I'm against a solution that can only be achieved by editing files.

I wouldn't want to apply dogmatics at this stage. If it was "Beifang" to expose a capability that exists anyway (as you implement it with a mapping file, you can easily expose that to users, too), I would do it.
But if you also insist on GUI-configurability I'd say that the whole "non-default default units" feature (which after all is just required when you want to bend physics ;-)) would be over-engineering, it's not worth the effort so then better don't expose the interface at all.

I would suggest to open a new issue for "always persist base unit"

Opened #3167