search

opensmarthouse / opensmarthouse-core

Eclipse Public License 2.0
7 stars 0 forks source link

Group math operations are not reliable over restarts #34

Open splatch opened 2 years ago

splatch commented 2 years ago

I've found that math function calls SUM, MIN, MAX, AVG might generate false results due to initialization order or missing state for one or more of grouped items.

Especially for energy or power demand calculation this is an unintended behavior which leads to false results. For example when states of item A, B, C are set the result is correct, but when one of item states is missing (its null) then result, depending on its use, might be invalid.

Proposal to consider - all of math functions to have an extra parameter to decide if groups which use them are fine with a partial calculation. If user decides to not accept partial results then function should return an UNDEF state until all child item states are known.

5iver commented 2 years ago

This sounds useful. At first I thought this may only be an issue when the user does not use persistence and restoreAtStartup. However, I can see this being an issue when Things go offline or are disabled. Another proposal to consider is to implement a System configuration option, where the aggregation functions check the value before calculations.

This seems incomplete to me though. Aggregated group functions are not the only things affected by offline/disabled Things. On a larger scale, perhaps the system could completely ignore offline/disabled Things, their Channels, and even any linked Items. Group aggregation would ignore the Item and its missing value, but the Item could also disappear from UIs. Rules would need to assume Items do not always exist and check for their existence. Just thinking out loud here...

splatch commented 2 years ago

Hey Scott, Currently group handling is kind of half baked. Last year when we began split of core groups was one of corner cases which required special care due to number of references to GroupFunctionHelper, not to mention issues with REST/DTO mapping in this area. For compatibility reasons this helper is still used, however it is managed by ItemBuilderFactoryImpl. We definitely can move this forward and use managed configuration. I started from looking at functions and they can have parameters which are states. So we can pass an extra ON or OFF option to any of them if we wish. We might also retain default behavior which will accept calculation result even if not all item states are available.

You are also right about things getting offline. I believe a long time ago Chris did suggest that items should get into UNDEF state as soon as thing gets offline. Not sure if we have that code in place, but it would really sum nicely with this issue.

cdjackson commented 2 years ago

I believe a long time ago Chris did suggest that items should get into UNDEF state as soon as thing gets offline.

I'm not 100% sure, but I think I did change that in OSH. My gripe is that it's not handled consistently by bindings in OH, which gives a horrible UX and everyone asks the binding maintainer to fix it in some binding specific way which just propagates the mess...