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

Group SUM aggregation does not inherit the member's UoM #2449

Closed
shutterfreak opened this issue Aug 4, 2021 · 15 comments · Fixed by #3144
Closed

Group SUM aggregation does not inherit the member's UoM #2449

shutterfreak opened this issue Aug 4, 2021 · 15 comments · Fixed by #3144

Comments

@shutterfreak
Copy link

A Group containing members with a UoM setting that differs from the 'default' will not use the members' UoM setting in the SUM aggregation. This probably also applies to the AVG aggregation since it also generates a new value (as opposed to returning a matching Item's state when aggregating with MIN or MAX).

An example is provided below:

The Buienradar binding exposes rain intensity forecasts as Number:Speed expressed in mm/h. This is also 'inherited' by the Items linked to these binding channels: the relating Items are of subtype Number:Speed and the state is expressed in mm/h.

When I create a Group and assign all rain intensity Items from the Buienradar binding, and when I set the SUM aggregation, the Group uses a different UoM than the individual members, even if I set the member's subtype to 'Number:Speed`. The Group reports a state in km/h (1 million bigger unit than the individual members' unit)

Where an Item can be configured to have an explicit UoM for the quantity represented in the state, this is apparently not possible with a Group.

For Main UI widgets you can of course use the displayState property which behaves similarly to what the label="..." construct allows in sitemaps.

See also the following discussion:
https://community.openhab.org/t/oh3-group-item-number-speed-defaults-to-km-h-how-to-set-item-uom-e-g-to-mm-h/125087

@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/oh3-group-item-number-speed-defaults-to-km-h-how-to-set-item-uom-e-g-to-mm-h/125087/24

@Rossko57
Copy link

Rossko57 commented Aug 4, 2021

I don't see any reason at all why a Group should inherit UoM from it's members (which may have differing UoM from each other).

SUM/AVG/MIN etc. aggregations should produce results in the Group state, with units of course. The chosen default unit should be user-definable in the Group's [pattern] metadata. (for some Quantity types e.g. temperature, there is a system default unit also.)

Have you an example of metadata not working for Group?

@shutterfreak
Copy link
Author

See this post for screenshots:

https://community.openhab.org/t/oh3-group-item-number-speed-defaults-to-km-h-how-to-set-item-uom-e-g-to-mm-h/125087/16?u=shutterfreak

It appears that an Item's state UoM unit can be set when defining the Item (in the example: "Number:Speed: mm/h"). However, this is not the case when defining a Group (in the example: "Number:Speed: km/h").

For an Item, I can get the state in the UoM unit specified in the metadata or provided by the binding's channel description in a YAML widget code by means of:

items.itemName.state

whereas for a Group consisting only of member Items of the same type, I have to set the pattern in the Group state metadata and access the state by means of:

items.itemName.displayState

This is inconsistent.

The only workaround I can imagine, is to never use items.itemName.state in YAML widgets when dealing with UoM, and to only resort on using items.itemName.displayState after specifying the desired unit in the state pattern metadata. Only then can I have consistent behaviour across Items and Groups of the same subtype (in the example: Number:Speed)

@shutterfreak
Copy link
Author

To add to this issue, in case I want to consume the state of an Item in different units in custom YAML widgets, I must create an additional Item each time I need the state in another unit. For instance, wind speed may be relevant in m/s, km/h and on the Beaufort scale, depending on the particular use.

@Rossko57
Copy link

Rossko57 commented Aug 7, 2021

However, this is not the case when defining a Group (in the example: "Number:Speed: km/h").

Have you an an example of failing to set Group default units by setting the Group Items metadata state description / pattern element?

This is how it works in OH2 for a comparison.

Group:Number:Length:SUM gBananas "aggregation [%d mm]"
Number:Length test1 "test [%d m]" (gBananas)
Number:Length test2 "test [%d in]" (gBananas)

There's no metadata defaulting in OH2, only the label [state description]

results

2021-08-08 15:24:24.929 [vent.ItemStateChangedEvent] - test1 changed from NULL to 20.0 m
2021-08-08 15:24:24.930 [GroupItemStateChangedEvent] - gBananas changed from NULL to 20.0 m through test1
2021-08-08 15:25:06.656 [vent.ItemStateChangedEvent] - test1 changed from 20.0 m to 0.02 m
2021-08-08 15:25:06.657 [GroupItemStateChangedEvent] - gBananas changed from 20.0 m to 0.02 m through test1
2021-08-08 15:25:37.458 [vent.ItemStateChangedEvent] - test2 changed from NULL to 1.0 in
2021-08-08 15:25:37.459 [GroupItemStateChangedEvent] - gBananas changed from 0.02 m to 0.0454 m through test2

The SUM maths works fine, with auto conversions.
But
The assigned Group default doesn't work; the Group state takes on system default units for Length.
It'll get auto-converted for display in a UI, and we can get what units we want in a rule.

But in summary, Group cannot be assigned default units in OH2.
Similar experiments should be conducted in OH3, using metadata.

Your widget issue is a UI issue. Any Item can only have state in one unit at a time; displaying different units on demand is about UI not Item/Group.

@shutterfreak
Copy link
Author

shutterfreak commented Aug 9, 2021

But in summary, Group cannot be assigned default units in OH2.
Similar experiments should be conducted in OH3, using metadata.

That's precisely my point. Unit conversion works, but Group SUM aggregation seems to default to 'the' system default unit for the given QuantityType, with no way to configure it (as with plain Items):

  • Number:Length defaults to m
  • Number:Speed defaults to km/h

Hence the usual behaviour of not having to provide the state representation pattern apparently does not apply to Group SUM aggregation.

@rkoshak
Copy link

rkoshak commented Aug 9, 2021

To add to this issue, in case I want to consume the state of an Item in different units in custom YAML widgets, I must create an additional Item each time I need the state in another unit. For instance, wind speed may be relevant in m/s, km/h and on the Beaufort scale, depending on the particular use.

This is a different problem that should probably have a separate issue opened to request that and as rossko57 indicates, it should be filed on the webuis repo since that is where that needs to be implemented, or at least implemented in part.

Hence the usual behaviour of not having to provide the state representation pattern apparently does not apply to Group SUM aggregation.

Maybe there is some confusion here on this point. Something must provide the units to the Item. The binding or the State Description, or on a label must provide the units. If none of those apply then the system default units are chosen. In the case of your specific Items the binding is supplying the mm/h units. There is no binding on the Group, the State Description seems to be ignored, and we are not working with sitemaps so the only thing left is the system default.

In short, except for the State Description part which rossko57 shows is not working in OH 2 and I expect is not working in OH 3 either (after looking at the code), the Group is not behaving inconsistently from the Items.

If the Group were to inherit the units from its members that would be something unique and inconsistent about Groups, which isn't really a problem so don't take this as an argument against that.

But if a Group were to inherit the units from its members, which member does it inherit from? A Group:Number:Speed can have members with mm/h, km/h, and mph and the SUM function can and will calculate a correct answer. In that case, which units does the Group choose? The suggestion to inherit seems to assume that all the members will always have the same units which is not the case.

The actual code is located at https://github.com/openhab/openhab-core/blob/91307993ad2fbe1b357f1dab8d694d679e67ca8c/bundles/org.openhab.core/src/main/java/org/openhab/core/library/types/QuantityTypeArithmeticGroupFunction.java lines 115-150. That block of code loops through the members and for all those that are of a compatible dimension to the Group it adds the state to the sum. Those that have incompatible dimensions or Item types are simply ignored. If there were a place to have the Group inherit the units of its members, this would be it. But the question remains which child member does it inherit from? The first one it sees? Make a separate loop through the members and pick the one that occurs most frequently?

I just don't know if it does make sense for the Group to inherit the units at all. Maybe only in the case where all the Items have the same units. But then what happens if one of the members if NULL or UNDEF?

Looking through the NumberItem and GroupItem code it does look like GroupItem is missing code that handles the State Description like Number Items have. The equivalent to https://github.com/openhab/openhab-core/blob/79edf2b9e643ad802c31b2e5f54edb93534277fd/bundles/org.openhab.core/src/main/java/org/openhab/core/library/items/NumberItem.java lines 84-94 needs to be added to GroupItem, with changes to deal with the uniqueness in how Groups are defined. That should at least make it so we can define the units we want in the Item metadata for Groups.

@Rossko57
Copy link

Rossko57 commented Aug 10, 2021

with no way to configure it

There has so far been absolutely no evidence of any attempt to actually configure default units for your Group. Not one confirmation of setting Group metadata, and not getting the expected result. That's all I've been asking for.

@shutterfreak
Copy link
Author

I don't know what other evidence I can provide.

Here are the relevant portions of the JSON DB:

OPENHAB/USERDATA/jsondb/org.openhab.core.items.Item.json:
Group definition:

  "gBuienRadar_Intensity_sum": {
    "class": "org.openhab.core.items.ManagedItemProvider$PersistedItem",
    "value": {
      "baseItemType": "Number:Speed",
      "groupNames": [
        "gBuienradar"
      ],
      "itemType": "Group",
      "tags": [
        "WeatherService"
      ],
      "label": "Rain intensity (total)",
      "category": "rain",
      "functionName": "SUM"
    }
  },

and one of the items linked to the Buienradar binding channels:

  "BuienRadar_intensity_000_min": {
    "class": "org.openhab.core.items.ManagedItemProvider$PersistedItem",
    "value": {
      "groupNames": [
        "gBuienRadar_Intensity",
        "gBuienRadar_Intensity_Hr1",
        "gBuienRadar_Intensity_Hr1Q1",
        "gBuienRadar_Intensity_sum"
      ],
      "itemType": "Number:Speed",
      "tags": [],
      "label": "Rain intensity in 0-5 minutes",
      "category": "rain"
    }
  },

OPENHAB/USERDATA/jsondb/org.openhab.core.items.Metadata.json:
Group Item metadata:

  "stateDescription:gBuienRadar_Intensity_sum": {
    "class": "org.openhab.core.items.Metadata",
    "value": {
      "key": {
        "segments": [
          "stateDescription",
          "gBuienRadar_Intensity_sum"
        ],
        "uid": "stateDescription:gBuienRadar_Intensity_sum"
      },
      "value": " ",
      "configuration": {
        "pattern": "%.2f mm/h",
        "readOnly": true
      }
    }
  },

and metadata relating to the Item(s) linked to the Buienradar channels:

  "widgetOrder:BuienRadar_intensity_000_min": {
    "class": "org.openhab.core.items.Metadata",
    "value": {
      "key": {
        "segments": [
          "widgetOrder",
          "BuienRadar_intensity_000_min"
        ],
        "uid": "widgetOrder:BuienRadar_intensity_000_min"
      },
      "value": "0",
      "configuration": {}
    }
  },
  "stateDescription:BuienRadar_intensity_000_min": {
    "class": "org.openhab.core.items.Metadata",
    "value": {
      "key": {
        "segments": [
          "stateDescription",
          "BuienRadar_intensity_000_min"
        ],
        "uid": "stateDescription:BuienRadar_intensity_000_min"
      },
      "value": " ",
      "configuration": {
        "pattern": "%.02f mm/h",
        "readOnly": true
      }
    }
  },
  "listWidget:BuienRadar_intensity_000_min": {
    "class": "org.openhab.core.items.Metadata",
    "value": {
      "key": {
        "segments": [
          "listWidget",
          "BuienRadar_intensity_000_min"
        ],
        "uid": "listWidget:BuienRadar_intensity_000_min"
      },
      "value": "widget:rain_intensity_list",
      "configuration": {}
    }
  },

@Rossko57
Copy link

Rossko57 commented Aug 29, 2021

        "pattern": "%.2f mm/h",

That's all I was looking for. I totally agree that should define a default unit for this Group Item, and further I think aggregation results should be expressed in this unit, regardless of member unit.

A thought, if you'd like to experiment ... if you define a Group from xxx.items file instead, and include the 'pattern' in the label the old-fashioned way
Group:Number:Speed:SUM blah "my label [%.2f mm/h]"
does the aggregation code find it then? May be an OH3 updating oversight rather than a complete omission in coding. This 'pattern' ends up in a different JSONDB field.

@splatch
Copy link
Contributor

splatch commented Sep 28, 2021

I recently made few third party addons for my own use and one of them is quantity profile which turns numbers coming from handlers into quantities before they are set on items.
https://github.com/ConnectorIO/connectorio-addons/tree/master/bundles/org.connectorio.addons.profile.quantity
This allows to align various readings before item states are even touched. Issue you spotted is essentially related to what I did - the state description pattern is used for far too many things causing situations where we are forced to use "displayState" far more often than we should (I believe its use is mainly for user interface).

Current limitation I did not implement (yet) is that profile I made works in partial way. It does not try to align quantities to base unit. This is what you really need and what I'll try to add. :-)

@rkoshak
Copy link

rkoshak commented Sep 28, 2021

All of that is excellent work and I am happy that you've made these profiles. However, one can't use a profile with a Group because the profile only applies to a channel item link and you can't link a channel to a Group Item. So any solution that involves profiles won't address this issue unless/until profiles are changed to work with Items directly, not just links.

@splatch
Copy link
Contributor

splatch commented Sep 28, 2021

All of that is excellent work and I am happy that you've made these profiles. However, one can't use a profile with a Group because the profile only applies to a channel item link and you can't link a channel to a Group Item.

That's exactly what we need. It is sufficient to align group members to base unit. State description set on these items will be used for UI while state transformed by profile will be used by group calculations.

With present infrastructure and lack of quantity profile you end up with following situation:

handler 1 -> item 1 [state: mm/h,    display: mm/h ] --->
handler 2 -> item 2 [state: cm/h,    display: cm/h ] --->  group [mm/h], SUM  --> member 1
handler 3 -> item 3 [state: decimal, display: mm/h ] --->

With quantity profile I made you can do better:

handler 1 -> profile [mm/h] -> item 1 [state: mm/h, display: mm/h ] --->
handler 2 -> profile [mm/h] -> item 2 [state: mm/h, display: cm/h ] --->  group [mm/h], SUM  --> member 1, member 2, member 3
handler 3 -> profile [mm/h] -> item 3 [state: mm/h, display: mm/h ] --->

@Rossko57
Copy link

But you can't apply a profile type of thing to an Item to modify the state.
That would boil down to an infinite loop
when
state updates
then
update state with new value

This seems to have nothing to do with internal group aggregation process.

@splatch
Copy link
Contributor

splatch commented Sep 29, 2021

But you can't apply a profile type of thing to an Item to modify the state.
That would boil down to an infinite loop

Re-reading issue and where it occurs I agree I am wrong in this regard. My understanding of issue was wrong. Suggestion I gave above would help if one item is Decimal and other is Quantity.

Profile can and will modify item state which is linked to channel. That's all they do. Given that groups with functions listen to state changes of their own members they do not involve profiles. Sorry for mess!

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

Successfully merging a pull request may close this issue.

5 participants