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

Unit hint in thing channels #4079

Merged
merged 5 commits into from
Apr 6, 2024
Merged

Unit hint in thing channels #4079

merged 5 commits into from
Apr 6, 2024

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Feb 4, 2024

Resolves #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 (#4077 and #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 #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 mherwege requested a review from a team as a code owner February 4, 2024 21:47
@andrewfg
Copy link
Contributor

andrewfg commented Feb 5, 2024

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.

@mherwege mherwege marked this pull request as draft February 5, 2024 14:32
@andrewfg
Copy link
Contributor

andrewfg commented Feb 5, 2024

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

Signed-off-by: Mark Herwege <[email protected]>
@mherwege mherwege marked this pull request as ready for review February 5, 2024 20:11
@mherwege
Copy link
Contributor Author

mherwege commented Feb 5, 2024

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.

@jlaur
Copy link
Contributor

jlaur commented Feb 5, 2024

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.

@andrewfg
Copy link
Contributor

andrewfg commented Feb 5, 2024

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.

@mherwege
Copy link
Contributor Author

mherwege commented Feb 5, 2024

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
Copy link
Contributor Author

mherwege commented Feb 5, 2024

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.

@andrewfg
Copy link
Contributor

andrewfg commented Feb 6, 2024

@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..

@mherwege
Copy link
Contributor Author

mherwege commented Feb 6, 2024

@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
Copy link
Contributor Author

mherwege commented Feb 6, 2024

  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.

@jlaur
Copy link
Contributor

jlaur commented Feb 6, 2024

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?

@mherwege
Copy link
Contributor Author

mherwege commented Feb 6, 2024

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.

@andrewfg
Copy link
Contributor

andrewfg commented Feb 6, 2024

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

@mherwege
Copy link
Contributor Author

mherwege commented Feb 6, 2024

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.

Copy link
Contributor

@andrewfg andrewfg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that openhab/openhab-webui#2312 can be approved without requiring further changes in this PR => LGTM

BUT -- just a reminder -- the following things will also need to be done..

  1. Upload the revised XSD schema file to the OH main server. Otherwise the mvn build process for addons will fail due to a schema violation..
  2. Update the respective addon developer readMe entry.

florian-h05 pushed a commit to openhab/openhab-webui that referenced this pull request Mar 1, 2024
Closes openhab/openhab-core#4082.

This PR adds:

1. A curated list of units to show as a drop-down list when creating a Number item with dimension.
2. The possibility to use a different default unit on item creation than the system default unit.
3. The ability to change unit and state description for items.
4. The ability to use the `unitHint` provided by channel types for "Link channel to Item" -> "Create a new Item".

By default, the system default unit (for the configured measurement system) will be shown when editing or creating an item.

`units.js` contains a number of frequently used units by dimension and measurement system.
These will be available in a autosuggest dropdown list.
It is still possible to not select from the list and use any other string as a unit.

All units for the dimension in `units.js` will be in the dropdown list, but they will be sorted by measurement system. If the measurement system is set to US, imperial units will appear higher in the list.
Units that have not explicitely been listed as SI or US will always appear higher.

When typing a unit that is not in the curated list, a longer list will be used for autocompletion that considers allowed prefixes to base units and constructs all combinations.

`units.js` also contains a field to set a different default unit on item creation than the system default unit.

With openhab/openhab-core#4079, the REST API of channel types will provide a unit hint if defined in the binding channel types.
If such information is available, this PR adds support for this to be the the suggested unit.
If that information is unavailable, the UI will fall back to behavious described above.

---------

Also-by: Florian Hotze <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
@andrewfg
Copy link
Contributor

andrewfg commented Mar 7, 2024

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

@mherwege
Copy link
Contributor Author

mherwege commented Mar 7, 2024

@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.

@openhab-bot
Copy link
Collaborator

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
Copy link
Collaborator

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

@jlaur
Copy link
Contributor

jlaur commented Apr 3, 2024

@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. 🙂

@mherwege
Copy link
Contributor Author

mherwege commented Apr 3, 2024

@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.

@@ -83,6 +86,7 @@ protected ChannelType(ChannelTypeUID uid, boolean advanced, @Nullable String ite
}

this.itemType = itemType;
this.unitHint = unitHint;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general LGTM. Would it make sense to throw an IAE here when the item-type is not Number:* (similar to l. 85 above)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, makes sense. Done.

@kaikreuzer kaikreuzer requested a review from J-N-K April 5, 2024 08:21
@J-N-K J-N-K added this to the 4.2 milestone Apr 5, 2024
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@andrewfg
Copy link
Contributor

andrewfg commented Apr 5, 2024

@mherwege / @J-N-K see this openhab/openhab-docs#2283

@J-N-K J-N-K merged commit c2cbefe into openhab:main Apr 6, 2024
3 checks passed
@jlaur
Copy link
Contributor

jlaur commented Apr 6, 2024

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

@jlaur
Copy link
Contributor

jlaur commented Apr 6, 2024

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?

kaikreuzer added a commit to openhab/website that referenced this pull request Apr 6, 2024
kaikreuzer added a commit to openhab/website that referenced this pull request Apr 6, 2024
@kaikreuzer
Copy link
Member

@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.

@jlaur
Copy link
Contributor

jlaur commented Apr 6, 2024

@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
Copy link
Contributor

jlaur commented Apr 6, 2024

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. 🙂

@mherwege mherwege deleted the unit-hint branch April 8, 2024 06:39
@openhab-bot
Copy link
Collaborator

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

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 this pull request may close these issues.

[RfC] Make it possible for bindings to provide unit hints
6 participants