openhab-core icon indicating copy to clipboard operation
openhab-core copied to clipboard

Unit hint in thing channels

Open mherwege opened this issue 1 year ago • 16 comments

Solves https://github.com/openhab/openhab-core/issues/3854

To help the user in configuring the right unit when creating items from thing channels, one of the suggestions was to use a unitHint that could be provided by the binding developer. This PR implements the core part of this:

  • Add a unit-hint field to the channel type.
  • Expose this unit-hint field in the REST API.

The second part of this will have to be extending mainUI to use the unit-hint as proposed unit in configuration when available.

This can be especially useful for channels that provide % values, where QuantityType:Dimensionless has a default of Units.ONE. Note that an alternative solution with changing the default unit for QuantityType:Dimensionless has not gained support (https://github.com/openhab/openhab-core/issues/4077 and https://github.com/openhab/openhab-core/pull/4078). This PR does help for users creating new items from channels in mainUI. It will not help for users using textual configuration.

For dimensions with different units in SI or US measurement systems, it is possible to provide 2 unit hints in one string separated by comma. The first one will be SI, the second US. The suggested one in the UI will depend on the measurement system setting.

This PR extends the thing-description-1.0.0.xsd and adds a new attribute to the input-type. Further investigation is required, as this may make it impossible to run bindings that have this field added on an older core (parsing the xml may fail). This is very similar to the situation described in https://github.com/openhab/openhab-core/issues/4062 for a change in addon.xml. To limit issues with newer bindings on older cores (going forward from 4.2), this may have to be solved in parallel.

mherwege avatar Feb 04 '24 21:02 mherwege

I was thinking if this is sufficient to cover the requirements. First I just want to make an observation that Number:Dimensionless is really a pretty vague class / concept which actually has several sub-classes in physical reality as follows.

  • Number:Dimensionless:Ratio for example mass ratio (e.g. g/kg), volume ratio (e.g. ppM)
  • Number:Dimensionless:ProportionOfMaximum for example heating power kW / max heating power kW, or litres in tank / tank size in litres, water content of air / maximum water carrying capacity or air [at that temperature], etc. (aka "percent")
  • Number:Dimensionless:RelativeToReference for example sound pressure level / reference sound pressure level (aka dB)
  • Possibly other cases? Please advise..

Anyway, to summarise, I think this would have the ability to cover all those needs.

When creating an item, the MainUI would have to offer a selection of a) all the most likely units suggested in the bullet points above, plus b) the unit hint proposed in this PR, plus c) a 'custom' option allowing the user to manually enter some other unit.

andrewfg avatar Feb 05 '24 14:02 andrewfg

Apropos MainUI when creating a new item (see below) I have three suggestions..

  1. When editing the 'Unit' field, the Dimension and State Description pattern should ideally synchronize with it.
  2. Notwithstanding the objections of others elsewhere about 'one' not being the default in OH Core, I wonder if perhaps "%" could still be offered as the default in Main UI .. just a thought..
  3. The 'Unit' field can be a drop down offering all the possible (currently defined) units supported by Number:Dimensionless

image

andrewfg avatar Feb 05 '24 17:02 andrewfg

3. The 'Unit' field can be a drop down offering all the possible (currently defined) units supported by Number:Dimensionless

I don't think this can easily be generalized. Think about all the scale modifiers (micro, milli, centi, deca, hecto, kilo...). I know this is not relevant for Dimensionless, but I want to start with a general solution first, before trying to become specific.

mherwege avatar Feb 05 '24 20:02 mherwege

This PR extends the thing-description-1.0.0.xsd and adds a new field. Further investigation is required, as this may make it impossible to run bindings that have this field added on an older core (parsing the xml may fail).

Do you know if this would also be the case if added as an attribute to the item-type element?

<item-type unitHint="%">Number:Dimensionless</item-type>

I don't know if it makes any difference, but it seems somehow like a smaller change.

jlaur avatar Feb 05 '24 22:02 jlaur

if this would also be the case if added as an attribute to the item-type element?

<item-type unitHint="%">Number:Dimensionless</item-type>

I agree. 'item-type" is the most obvious and logical holder for this attribute.

andrewfg avatar Feb 05 '24 22:02 andrewfg

I don't know if it makes any difference, but it seems somehow like a smaller change.

It doesn't make a difference (and is actually a bigger change, as item-type is now a String node and would have to be converted in an object). The challenge with the xml files is that they are very unforgiving for extra data not in the code. The code goes through the elements and will fail on any field it does not recognize. And that's true for all xml processing. This is OK as long as it is stable, but it is hindering progress in many ways. I don't see a general solution for this.

mherwege avatar Feb 05 '24 22:02 mherwege

I agree. 'item-type" is the most obvious and logical holder for this attribute.

OK, will look into modifying it to be an attribute to item-type.

mherwege avatar Feb 05 '24 23:02 mherwege

@mherwege do you need to modify Rest API to include the new field JSON? But to answer my own question, I suppose GSON does that automatically..

andrewfg avatar Feb 06 '24 13:02 andrewfg

@mherwege do you need to modify Rest API to include the new field JSON? But to answer my own question, I suppose GSON does that automatically..

GSON does, but it needed to be added to the ChannelTypeResource in the Rest API package.

I tested with a modified UI from the linked PR (one example), and it was available in the UI and REST API.

mherwege avatar Feb 06 '24 13:02 mherwege

  1. When editing the 'Unit' field, the Dimension and State Description pattern should ideally synchronize with it.

I replaced the default pattern with %.0f %unit% (in the UI PR). That should make sure it is always in sync with the item unit. Overriding it becomes an explicit choice. I don't see how to set Dimension form unit without having a full mapping (see next point).

  1. The 'Unit' field can be a drop down offering all the possible (currently defined) units supported by Number:Dimensionless

There is no predefined list of allowed units (with their modifiers) I can query from the Rest API. So this is not easy to solve immediately and is subject for another potential enhancement.

  1. Notwithstanding the objections of others elsewhere about 'one' not being the default in OH Core, I wonder if perhaps "%" could still be offered as the default in Main UI .. just a thought..

Could be done, however, without a list of potential units, nobody will know they have to set one if they want it. It is not a natural thing. % is more obvious to set explicitely.

mherwege avatar Feb 06 '24 13:02 mherwege

I tested with a modified UI from the linked PR (one example), and it was available in the UI and REST API.

Did you consider also this scenario:

<channel-type id="water-consumption">
	<item-type unitHint="l,gal">Number:Volume</item-type>
	<label>Water Consumption</label>
	<description>Water consumption by the currently running program on the appliance</description>
	<category>Water</category>
	<tags>
		<tag>Measurement</tag>
		<tag>Water</tag>
	</tags>
	<state readOnly="true" pattern="%.1f %unit%"/>
</channel-type>

where it should default either "l" or "gal" depending on configured measurement system?

jlaur avatar Feb 06 '24 13:02 jlaur

where it should default either "l" or "gal" depending on configured measurement system?

I considered it and created the code for it, but I must say I didn't explicitely test it.

mherwege avatar Feb 06 '24 13:02 mherwege

without a list of potential units, nobody will know they have to set one if they want it.

That is why I was thinking of a drop down plus 'custom' input. Both one and % could be in the standard dropdown (with unit-hint preselected or % if unit-hint == null). The other standard options would be ppM, ppB, and dB. Any other unit, including those with milli, kilo, prefixes etc. could still be entered via the 'custom' input..

EDIT: the standard units being those here...

https://github.com/openhab/openhab-core/blob/c929e7dfe2b22298e3f99a3d95920dd08617befd/bundles/org.openhab.core/src/main/java/org/openhab/core/library/unit/Units.java#L105..L112

andrewfg avatar Feb 06 '24 15:02 andrewfg

That is why I was thinking of a drop down plus 'custom' input. Both one and % could be in the standard dropdown (with unit-hint preselected or % if unit-hint == null). The other standard options would be ppM, ppB, and dB. Any other unit, including those with milli, kilo, prefixes etc. could still be entered via the 'custom' input..

This makes sense for Dimensionless. But if you check the list of 'standard' units, you will notice it actually only lists exceptions not covered out of the box by the libraries. For instance, it creates mm/h, but km/h comes from the libraries. So the code you link is by no way exhaustive. Also, it would have to be created manually in the UI code. I think what is need is a way to query a set of units for a given dimension through the REST API from core, and then show that list. That would make sure things are in sync with core and there are no two places to maintain it. Again, I think this goes beyond the scope of this (and the linked UI) PR. It is probably worth creating a specific enhancement request issue for it.

mherwege avatar Feb 06 '24 16:02 mherwege

@mherwege given that the UI change has now been merged, I think this one must be merged too. Or??

andrewfg avatar Mar 07 '24 04:03 andrewfg

@mherwege given that the UI change has now been merged, I think this one must be merged too. Or??

Would be nice, but the UI PR provides extra functionality without this already. Merging this PR will enable binding developers to include a unit hint in their channel definitions and the merged UI PR will consider it. There would be no change required to the UI PR.

mherwege avatar Mar 07 '24 07:03 mherwege

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

https://community.openhab.org/t/units-are-not-propagated-when-creating-items/154748/6

openhab-bot avatar Mar 21 '24 16:03 openhab-bot

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

https://community.openhab.org/t/inconsistent-percentage-behaviour/154595/40

openhab-bot avatar Apr 03 '24 14:04 openhab-bot

@mherwege - this PR fully resolves #3854, so if you change the first word in the PR description from "Solves" to "Resolves", the issue will be automatically closed when/if this PR is merged. 🙂

jlaur avatar Apr 03 '24 15:04 jlaur

@mherwege - this PR fully resolves #3854, so if you change the first word in the PR description from "Solves" to "Resolves", the issue will be automatically closed when/if this PR is merged. 🙂

Thanks and done.

mherwege avatar Apr 03 '24 16:04 mherwege

@mherwege / @J-N-K see this https://github.com/openhab/openhab-docs/pull/2283

andrewfg avatar Apr 05 '24 17:04 andrewfg

@J-N-K - can you upload the new schema or is it only @kaikreuzer who can do that?

jlaur avatar Apr 06 '24 12:04 jlaur

This PR extends the thing-description-1.0.0.xsd and adds a new attribute to the input-type. Further investigation is required, as this may make it impossible to run bindings that have this field added on an older core (parsing the xml may fail). This is very similar to the situation described in #4062 for a change in addon.xml. To limit issues with newer bindings on older cores (going forward from 4.2), this may have to be solved in parallel.

I may have missed this, but did you analyze and find any impact on backwards/forwards compatibility?

jlaur avatar Apr 06 '24 14:04 jlaur

@jlaur It's very simple - you only need to create a PR against https://github.com/openhab/website/tree/main/.vuepress/public/schemas. I just updated the schema with https://github.com/openhab/website/pull/460.

kaikreuzer avatar Apr 06 '24 19:04 kaikreuzer

@jlaur It's very simple - you only need to create a PR against https://github.com/openhab/website/tree/main/.vuepress/public/schemas. I just updated the schema with openhab/website#460.

Thanks, and good to know! I assume some website build job is needed before the update is available here? https://openhab.org/schemas/thing-description-1.0.0.xsd

jlaur avatar Apr 06 '24 19:04 jlaur

I assume some website build job is needed before the update is available here? https://openhab.org/schemas/thing-description-1.0.0.xsd

I see it's already available now. 🙂

jlaur avatar Apr 06 '24 19:04 jlaur

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

https://community.openhab.org/t/unithint-error-in-xml-schema/156558/3

openhab-bot avatar Jun 05 '24 06:06 openhab-bot