[basicprofiles] Fix StateFilterProfile to use linked Item system unit #18144

andrewfg · 2025-01-20T18:33:27Z

Resolves #18122

There is a bug whereby the calculations over a StateFilterProfile set of time series state values are based on the Unit of the first member in the set of values. There are two problems: a) the order of the state values is not relevant, and b) the Unit of the incoming state values is not definitive to the Unit of the expected result. Therefore these calculations do produce variable and unexpected results.

This PR fixes the bug by normalising the calculations so that they are all based on the Unit of the target Item to which the StateFilterProfile is linked.

Furthermore this PR allows conversion between non- inverted and inverted Units so invertible conversions (e.g. Kelvin <=> Mirek) are supported.

And finally it also provides better support for cases where the binding may provide state updates in different mixed formats e.g. a mixture of QuantityType with different base units, UndefType, and various States that can yield a DecimalType value.

Signed-off-by: Andrew Fiddian-Green [email protected]

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg · 2025-01-20T18:41:44Z

@jimtng @mherwege for info..

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg · 2025-02-09T10:51:06Z

@jimtng good news that all your PRs have all now been merged; which means that this one is also now ready to go: => could you please have a look at it?

jimtng

Very nice refactorings!

One thing I'm wondering about is how would it handle the scenario when linked to an item with unit, but the condition is OtherItem1 > OtherItem2 (i.e. nothing to do with the linked item)?

.../src/main/java/org/openhab/transform/basicprofiles/internal/profiles/StateFilterProfile.java

jimtng · 2025-02-09T11:33:25Z

.../src/main/java/org/openhab/transform/basicprofiles/internal/profiles/StateFilterProfile.java

@@ -603,7 +635,7 @@ public int getWindowSize() {
                // the previous state is kept in the acceptedState variable
                return 0;
            }
-            return windowSize.orElse(DEFAULT_WINDOW_SIZE);
+            return windowSize.isPresent() ? windowSize.get() : DEFAULT_WINDOW_SIZE;


What is the difference?

It eliminates the yellow squiggly underline compiler warning in Eclipse IDE :)

jimtng · 2025-02-09T13:01:49Z

.../src/main/java/org/openhab/transform/basicprofiles/internal/profiles/StateFilterProfile.java

@@ -342,6 +354,9 @@ public boolean check(State input) {
                    if (rhsFunction.alwaysAccept()) {
                        return true;
                    }
+                    if (rhsFunction.getType() == FunctionType.Function.DELTA) {
+                        isDeltaCheck = true;
+                    }


This does seems like a better place for it, so can we remove the same code further down, i.e. line 415-418?

can we remove the same code further down

Umm not sure. The first sets isDeltaCheck if LHS is a function, and the second does it if RHS is. ??

I'm not talking about the lhs (line 404-406). I'm talking about line 415-418 - that's also rhs.

jimtng · 2025-02-09T13:18:55Z

.../src/main/java/org/openhab/transform/basicprofiles/internal/profiles/StateFilterProfile.java

+        return hasSystemUnit() //
+                ? toSystemUnitQuantityType(state) != null


Can we avoid calling toSystemUnitQuantityType repeatedly? It was already done in isAllowed and also it's called a lot in other places. Do we need to cache the result?

This is a very valid question. The thing about toSystemUnitQuantityType is that it converts incoming QuantityType to the system unit, but returns null if the conversion fails or if the incoming State is not a QuantityType. So in input, newState, acceptedState and previousStates I think we cannot store the result after toSystemUnitQuantityType but must instead store them in original raw form. I suppose we could create parallel variables input, newState, acceptedState and previousStates to cache post conversion states. ?? Or we could convert input, newState, acceptedState and previousStates rather than holding a single State to hold an Entry<State, State> that contains both original and cached post conversion versions. But that would be quite hairy code, so I elected to convert everything on the fly.

andrewfg · 2025-02-09T14:20:50Z

how would it handle the scenario when linked to an item with unit, but the condition is OtherItem1 > OtherItem2

Good question. If I recall correctly I think the code converts 'OtherItem' to the string representation of its State, and then the string is handled in the same way as one hard coded in the condition LHS / RHS. So if 'OtherItem' was a QuantityType we would attempt to convert it to the systemUnit. And in the case 'OtherItem1 < OtherItem2' .. if either or both would not convert to systemUnit we would have an error -- which I guess means that the input State is rejected??

PS in any case I will add some JUnit tests.. => EDIT: I see that there are actually already many such tests .. and they all still pass.

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[basicprofiles] Improve StateFilterProfile unit based calculations

b905a03

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg added the enhancement An enhancement or new feature for an existing add-on label Jan 20, 2025

andrewfg self-assigned this Jan 20, 2025

andrewfg requested a review from J-N-K as a code owner January 20, 2025 18:33

andrewfg marked this pull request as draft January 20, 2025 18:33

andrewfg mentioned this pull request Jan 20, 2025

[basicprofiles] Calculations over series of QuantityType are based on the wrong Unit #18122

Open

formatting

b440dbb

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg changed the title ~~[basicprofiles] Improve StateFilterProfile unit based calculations~~ [basicprofiles] Normalise StateFilterProfile calculations to use the linked Item's Unit Jan 21, 2025

andrewfg changed the title ~~[basicprofiles] Normalise StateFilterProfile calculations to use the linked Item's Unit~~ [basicprofiles] Normalise StateFilterProfile calculations to use linked Item's Unit Jan 21, 2025

andrewfg added 3 commits January 22, 2025 22:57

work in progress; some test still fail

fca17b5

Signed-off-by: Andrew Fiddian-Green <[email protected]>

revert toList

0e44b67

Signed-off-by: Andrew Fiddian-Green <[email protected]>

fix unit tests

f156748

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg mentioned this pull request Jan 23, 2025

[basicprofiles] Convert to relative unit in State Filter's Delta check #18127

Merged

andrewfg added the awaiting other PR Depends on another PR label Jan 23, 2025

andrewfg requested a review from jimtng January 23, 2025 18:38

andrewfg marked this pull request as ready for review January 23, 2025 18:38

work in progress

a3051d1

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg changed the title ~~[basicprofiles] Normalise StateFilterProfile calculations to use linked Item's Unit~~ [basicprofiles] StateFilterProfile calculations use linked Item's Unit Jan 23, 2025

andrewfg added 2 commits January 23, 2025 23:15

Merge remote-tracking branch 'upstream/main' into statefilter-functions

a995800

add todo

baf9679

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg changed the title ~~[basicprofiles] StateFilterProfile calculations use linked Item's Unit~~ [basicprofiles] StateFilterProfile calculations use linked Item SystemUnit Jan 23, 2025

andrewfg added 5 commits January 24, 2025 00:08

[basicprofiles] add invertible unit tests

75b3040

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[basicprofiles] work in progress

563c972

Signed-off-by: Andrew Fiddian-Green <[email protected]>

Merge branch 'main' into statefilter-functions

93ba80c

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[basicprofiles] fix invertible unit calculations

90237fa

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[basicprofiles] fix comment

bf1b250

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg changed the title ~~[basicprofiles] StateFilterProfile calculations use linked Item SystemUnit~~ [basicprofiles] StateFilterProfile use linked Item SystemUnit Jan 24, 2025

andrewfg changed the title ~~[basicprofiles] StateFilterProfile use linked Item SystemUnit~~ [basicprofiles] StateFilterProfile uses linked Item SystemUnit Jan 24, 2025

[basicprofiles] add time tests

065a280

Signed-off-by: Andrew Fiddian-Green <[email protected]>

[basicprofiles] optimise calculate and method order

1cb58bd

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg changed the title ~~[basicprofiles] StateFilterProfile uses linked Item SystemUnit~~ [basicprofiles] StateFilterProfile uses linked Item system unit Jan 27, 2025

andrewfg added 2 commits January 27, 2025 16:30

Merge branch 'openhab:main' into statefilter-functions

e11c8ba

readme and pre- merge prior PRs

20b0a5e

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg added bug An unexpected problem or unintended behavior of an add-on and removed enhancement An enhancement or new feature for an existing add-on labels Jan 27, 2025

andrewfg added 2 commits January 27, 2025 17:26

revert log message

4a92b76

Signed-off-by: Andrew Fiddian-Green <[email protected]>

Merge branch 'openhab:main' into statefilter-functions

7a4a916

andrewfg mentioned this pull request Feb 3, 2025

[basicprofiles] Fix division-by-zero error in $DELTA_PERCENT state filter #18089

Merged

filter previousStates for DecimalType only

4f6b04c

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg changed the title ~~[basicprofiles] StateFilterProfile uses linked Item system unit~~ [basicprofiles] StateFilterProfile now uses linked Item system unit Feb 3, 2025

andrewfg added 4 commits February 4, 2025 11:37

Merge branch 'main' into statefilter-functions

3449378

Signed-off-by: Andrew Fiddian-Green <[email protected]>

align with prior PRs

f497591

Signed-off-by: Andrew Fiddian-Green <[email protected]>

work in progress

b36852c

Signed-off-by: Andrew Fiddian-Green <[email protected]>

fix junit test failures

274b681

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg changed the title ~~[basicprofiles] StateFilterProfile now uses linked Item system unit~~ [basicprofiles] Fix: StateFilterProfile to use linked Item system unit Feb 8, 2025

andrewfg changed the title ~~[basicprofiles] Fix: StateFilterProfile to use linked Item system unit~~ [basicprofiles] Fix StateFilterProfile to use linked Item system unit Feb 8, 2025

andrewfg added 3 commits February 8, 2025 22:48

eliminate dependency on OH core PR

3e708e2

Signed-off-by: Andrew Fiddian-Green <[email protected]>

Merge branch 'main' into statefilter-functions

519e884

Signed-off-by: Andrew Fiddian-Green <[email protected]>

fix merge conflict

8dae92e

Signed-off-by: Andrew Fiddian-Green <[email protected]>

andrewfg removed the awaiting other PR Depends on another PR label Feb 9, 2025

jimtng reviewed Feb 9, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

work in progress

bcb1445

Signed-off-by: Andrew Fiddian-Green <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[basicprofiles] Fix StateFilterProfile to use linked Item system unit #18144

[basicprofiles] Fix StateFilterProfile to use linked Item system unit #18144

andrewfg commented Jan 20, 2025 •

edited

Loading

andrewfg commented Jan 20, 2025

andrewfg commented Feb 9, 2025

jimtng left a comment •

edited

Loading

jimtng Feb 9, 2025

andrewfg Feb 9, 2025

jimtng Feb 9, 2025

andrewfg Feb 9, 2025 •

edited

Loading

jimtng Feb 9, 2025

andrewfg Feb 9, 2025

jimtng Feb 9, 2025

andrewfg Feb 9, 2025 •

edited

Loading

andrewfg commented Feb 9, 2025 •

edited

Loading

This comment was marked as resolved.

		return hasSystemUnit() //
		? toSystemUnitQuantityType(state) != null

[basicprofiles] Fix StateFilterProfile to use linked Item system unit #18144

Are you sure you want to change the base?

[basicprofiles] Fix StateFilterProfile to use linked Item system unit #18144

Conversation

andrewfg commented Jan 20, 2025 • edited Loading

andrewfg commented Jan 20, 2025

andrewfg commented Feb 9, 2025

jimtng left a comment • edited Loading

Choose a reason for hiding this comment

jimtng Feb 9, 2025

Choose a reason for hiding this comment

andrewfg Feb 9, 2025

Choose a reason for hiding this comment

jimtng Feb 9, 2025

Choose a reason for hiding this comment

andrewfg Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

jimtng Feb 9, 2025

Choose a reason for hiding this comment

andrewfg Feb 9, 2025

Choose a reason for hiding this comment

jimtng Feb 9, 2025

Choose a reason for hiding this comment

andrewfg Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

andrewfg commented Feb 9, 2025 • edited Loading

This comment was marked as resolved.

andrewfg commented Jan 20, 2025 •

edited

Loading

jimtng left a comment •

edited

Loading

andrewfg Feb 9, 2025 •

edited

Loading

andrewfg Feb 9, 2025 •

edited

Loading

andrewfg commented Feb 9, 2025 •

edited

Loading