Convert units for bounds when compiling full state descriptions #3132

ccutrer · 2022-10-24T22:52:24Z

See the comments in StateDescriptionFragmentImpl.merge for a full description of the problem and solution.

So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>

So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>

J-N-K · 2022-11-13T10:34:08Z

I feel that - especially considering #2283 - we should discuss what get's merged and what SHOULD take precedence (not necessarily what we have now).

ccutrer · 2022-11-13T15:34:05Z

+1. I hadn't even considered how multiple-linking would affect this. Definitely needs thought through

Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer · 2022-11-29T16:20:52Z

I force-pushed and rebased since part of this PR was extracted to its own PR and merged. remaining files have not changed since the last version of this PR, and discussion is still pending. Just wanted to clean it up so when we get back to it in the future it won't be as confusing.

openhab-bot · 2022-12-22T14:48:11Z

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

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

So that other pieces of openhab can know what unit it's going to be, without it having a value yet. Importantly, any necessary conversion that need to be applied to the other portion of the state description - min, max, and step. See also openhab/openhab-core#3132 Signed-off-by: Cody Cutrer <cody@cutrer.us>

openhab-bot · 2023-05-13T18:17:54Z

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

https://community.openhab.org/t/binding-improvements-best-practice/146664/2

J-N-K · 2023-05-13T18:18:55Z

@ccutrer Now that we have proper UoM support, could you rebase and check if this is still working as you expect? If so, I can review it.

J-N-K

Thanks, mostly looks good. Can you also add. Test that includes Number:Dimensionless? We have seen a lot of unexpected issues with the in the past, it would be good to have that covered.

It would also be great if you could rebase, so we are sure there is nothing wrong with the new unit handling.

J-N-K · 2023-05-23T17:41:09Z

...openhab.core/src/main/java/org/openhab/core/internal/types/StateDescriptionFragmentImpl.java

+
+                    if (minimum == null && (newValue = fragment.getMaximum()) != null) {
+                        minimum = new QuantityType(newValue, newUnit).toInvertibleUnit(oldUnit).toBigDecimal();


While this works, I would prefer a more explicit style here like:

Suggested change

if (minimum == null && (newValue = fragment.getMaximum()) != null) {

minimum = new QuantityType(newValue, newUnit).toInvertibleUnit(oldUnit).toBigDecimal();

BigDecimal newMinimum = fragment.getMaximum();

if (minimum == null && newMinimum != null) {

minimum = new QuantityType(newMinimum, newUnit).toInvertibleUnit(oldUnit).toBigDecimal();

The same of course also applies to the other locations.

boehan · 2023-07-05T19:57:58Z

If I understand this correctly, bounds (and step) use the unit of the state pattern if set. However, this would probably have no effect, if the pattern is set to %unit%. Thus, binding developers would have to predefine to be able to set correct bounds, which does not allow the default state description to follow system default units (I assume bounds always use unit of pattern set by the binding).
Wouldn't it be clearer, if bounds (and step) could be set with appropriate unit (which could even differ from the one set in the pattern)? And potentially use the approach proposed here as a fallback if no dedicated unit is set?

ccutrer · 2023-07-17T21:26:58Z

I've not had time to look at this again, and likely won't in the near future. I do agree with @boehan that it would make more sense if the StateDescription carried the unit with it as a QuantityType, rather than assuming the consumer knows how to parse the unit out of the format pattern (if provided). But I suspect that would be a significant breaking change.

ccutrer requested a review from a team as a code owner October 24, 2022 22:52

ccutrer force-pushed the convert-bounds-in-state-descriptions branch 2 times, most recently from 8ac94ab to 815b0d6 Compare October 25, 2022 22:27

ccutrer mentioned this pull request Nov 26, 2022

Add QuantityType.toUnitRelative #3177

Merged

convert units for bounds when compiling full state descriptions

86d0b43

Signed-off-by: Cody Cutrer <cody@cutrer.us>

ccutrer force-pushed the convert-bounds-in-state-descriptions branch from 4083b5f to 86d0b43 Compare November 29, 2022 16:19

wborn changed the title ~~convert units for bounds when compiling full state descriptions~~ Convert units for bounds when compiling full state descriptions Nov 30, 2022

ccutrer mentioned this pull request Dec 22, 2022

Add defaultUnit metadata for NumberItem #3248

Closed

J-N-K self-assigned this May 13, 2023

J-N-K added enhancement An enhancement or new feature of the Core awaiting feedback labels May 13, 2023

J-N-K requested changes May 23, 2023

View reviewed changes

ccutrer mentioned this pull request Jun 2, 2023

[hue] Implement CLIP 2 / API v2 openhab/openhab-addons#13570

Merged

boehan mentioned this pull request Jul 1, 2023

[comfoair] Extend UoM support, add semantic tags & update state descriptions openhab/openhab-addons#15167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert units for bounds when compiling full state descriptions #3132

Convert units for bounds when compiling full state descriptions #3132

ccutrer commented Oct 24, 2022

J-N-K commented Nov 13, 2022

ccutrer commented Nov 13, 2022

ccutrer commented Nov 29, 2022

openhab-bot commented Dec 22, 2022

openhab-bot commented May 13, 2023

J-N-K commented May 13, 2023

J-N-K left a comment •

edited

J-N-K May 23, 2023

boehan commented Jul 5, 2023

ccutrer commented Jul 17, 2023


		if (minimum == null && (newValue = fragment.getMaximum()) != null) {
		minimum = new QuantityType(newValue, newUnit).toInvertibleUnit(oldUnit).toBigDecimal();

-                    if (minimum == null && (newValue = fragment.getMaximum()) != null) {
-                        minimum = new QuantityType(newValue, newUnit).toInvertibleUnit(oldUnit).toBigDecimal();
+                    BigDecimal newMinimum = fragment.getMaximum();
+                    if (minimum == null && newMinimum != null) {
+                        minimum = new QuantityType(newMinimum, newUnit).toInvertibleUnit(oldUnit).toBigDecimal();

Convert units for bounds when compiling full state descriptions #3132

Are you sure you want to change the base?

Convert units for bounds when compiling full state descriptions #3132

Conversation

ccutrer commented Oct 24, 2022

J-N-K commented Nov 13, 2022

ccutrer commented Nov 13, 2022

ccutrer commented Nov 29, 2022

openhab-bot commented Dec 22, 2022

openhab-bot commented May 13, 2023

J-N-K commented May 13, 2023

J-N-K left a comment • edited

Choose a reason for hiding this comment

J-N-K May 23, 2023

Choose a reason for hiding this comment

boehan commented Jul 5, 2023

ccutrer commented Jul 17, 2023

J-N-K left a comment •

edited