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

[OmniLink] Add semantic tags to channels #11100

Merged
merged 9 commits into from
Feb 8, 2022
Merged

[OmniLink] Add semantic tags to channels #11100

merged 9 commits into from
Feb 8, 2022

Conversation

ecdye
Copy link
Contributor

@ecdye ecdye commented Aug 12, 2021

Signed-off-by: Ethan Dye [email protected]

@ecdye ecdye added this to the 3.2 milestone Aug 12, 2021
@digitaldan
Copy link
Contributor

digitaldan commented Aug 12, 2021

Hi @ecdye , have not yet played with adding semantic tags to the thing definition xml yet, definitely peaked my interest! After looking up the documentation, it seems to be limited right now to a small number of tags, not sure if the documentation is up to date, but curious where the tags you are using come from? The docs at https://next.openhab.org/docs/developer/bindings/thing-xml.html#default-tags states:

"Please note that only tags from a pre-defined tag library should be used. This library is still in development., and only a very small set of tags are defined so far:"

Tag Item Types Description
Lighting Switch, Dimmer, Color A light source, either switchable, dimmable or color
Switchable Switch, Dimmer, Color An accessory that can be turned off and on.
CurrentTemperature Number, Number:Temperature An accessory that provides a single read-only temperature value.
TargetTemperature Number, Number:Temperature A target temperature that should engage a thermostats heating and cooling actions.
CurrentHumidity Number An accessory that provides a single read-only value indicating the relative humidity.

@ecdye
Copy link
Contributor Author

ecdye commented Aug 12, 2021

Hmm, interestingly a number of other PRs with similar tags have been accepted, including ones not listed in that list. See #10969
#11098
#10977

@digitaldan
Copy link
Contributor

Yeah, i figured the docs might be out of date and some precedence was being set elsewhere, thanks for pointing out those PRs.

@ecdye
Copy link
Contributor Author

ecdye commented Aug 12, 2021

Yeah, i figured the docs might be out of date and some precedence was being set elsewhere, thanks for pointing out those PRs.

No problem, a user pointed it out to me on the forums and asked if I was willing to implement this because many of us have large numbers of lights and other devices that can be a pain to manually tag one at a time.

@lolodomo
Copy link
Contributor

lolodomo commented Aug 13, 2021

This Is controversial. Please read the PRs I submitted, @kaikreuzer does not like the current approach consisting in adding tags on many channels.
I don't know if he agrees to add at least "obvious" tags on a limited number of channels?

@Skinah
Copy link
Contributor

Skinah commented Aug 15, 2021

@digitaldan I don't know if they are documented, but here is how I find them...
When you edit an item you can get a drop down list of possible options. Semantic Class and Semantic Property.

http://www.pcmus.com/openhab/semantic.png

Why add them? See this picture of my locations and note the Exclaimation mark, this is what appears when an item is marked as an Alarm and it's state turns ON. The temperature shows up when 'Measurement' and 'Temperature' is tagged and these are POINTS directly in the location, must not be under an equipment. 'Control' and 'Light' gives the locations a number of how many lights are turned ON in that location.. etc.

http://www.pcmus.com/openhab/locations.png

Note that this only takes effect if you create the items AFTER the binding is already updated and the user is offered the ability to change the tags when the items are created.

Personally I believe the framework could be made to not need the tags added for some use cases, unless it increases the complexity of the core too much then it is probably easier to leave it to tags as a binding maintainer will have better grasp on what is a good set of tags. One example is a channel that uses Number:Temperature and also marked as read only, if there are no binding tags should the core treat it as 'Measurement' and 'Temperature' ? My opinion in this case is yes.

Another example is a color channel in the WLED binding. These led light strips can have a primary color and second color selected and then the light will alternate between these two colors. The fact that the color item has an ON state is wrong as the light can be turned OFF, so you really don't want the sematic labels showing that a room has a light left on then the light is OFF. Marking every color item as a 'Control' for a 'Light' in this case is wrong.

I have had to work this out myself, I have not seen any guidance on the topic anywhere.

@hmerk hmerk added the enhancement An enhancement or new feature for an existing add-on label Aug 18, 2021
@ecdye
Copy link
Contributor Author

ecdye commented Sep 4, 2021

Any updates on this PR? What needs to be done before it can be merged?

@wborn wborn removed this from the 3.2 milestone Nov 7, 2021
@ecdye
Copy link
Contributor Author

ecdye commented Nov 8, 2021

Bump

@ecdye
Copy link
Contributor Author

ecdye commented Dec 7, 2021

@lolodomo Has there been any update on if this is going to be the accepted approach or not? If it is not, then can you point me to what is so I can update the PR.

@ecdye ecdye added the rebuild Triggers Jenkins PR build label Dec 7, 2021
@lolodomo
Copy link
Contributor

lolodomo commented Dec 7, 2021

Unfortunately, this approach was more or less rejected without any concrete alternative.
Adding just a tag "Control" makes no sense, I agree with that.
Discussion here: openhab/openhab-core#1791

@ecdye ecdye removed the rebuild Triggers Jenkins PR build label Dec 8, 2021
@ecdye
Copy link
Contributor Author

ecdye commented Dec 8, 2021

@lolodomo A follow up question, if by all technicality this works as of right now's there any problem with implementing it as is until a better solution has been created/decided.

@kaikreuzer Your input would be welcome as well.

Personally I believe that for the moment we should at the very least allow this sort of workaround to make the user experience better until a better solution is made. Especially because even advanced users like myself don't take the time to create very custom pages in the webUI. Just imagine the beginner user who is trying to add these items and then never finds them because the were not tagged properly in the model.

I personally see the overview tabs / semantics tabs as very powerful if used correctly for all users which could make openHAB easier to get into for the average user as they have understandable and readily available access to their devices directly from the home page without having to go manually create a sitemap which IMHO is actually a very daunting task for a brand new home automation user.

@jlaur
Copy link
Contributor

jlaur commented Jan 19, 2022

One example is a channel that uses Number:Temperature and also marked as read only, if there are no binding tags should the core treat it as 'Measurement' and 'Temperature' ? My opinion in this case is yes.

Let me add an example that might make this choice less obvious, or at least add something more to think about. In the Miele@home binding there are temperature channels for an oven - a target temperature and a measured temperature. Both are read-only since the API doesn't allow setting the target temperature, unfortunately. So in this case it would be wrong to treat it as a measurement.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

A few generic comments added as I don't know this binding.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 20, 2022

@jlaur : for your information,, @kaikreuzer is generally against this approach consisting in adding semantics on a lot of channels. So before merging, maybe you should have his agreement. I imagine he agrees only when the semantic is obvious for the channel.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Sorry that I never commented here.

I just had to recap myself on the semantic tags. I think my main concerns where tags, which weren't 100% obvious that they are correct like in this or that case.
Seeing the issues that users have with manually tagging items through the Main UI right now, I would agree that it is absolutely fine and welcome to add tags in the obvious situations - like I consider those in that PR.
So from my side it can be merged, once @jlaur's comments are addressed.

@lolodomo
Copy link
Contributor

lolodomo commented Jan 23, 2022

This is a breaking change for users using UI to setup things/channels. Replacing the channel type of a channel is a breaking change. They will have to delete and create again the concerned things. It just means that a new entry is expected in the file update,ps1 in the distro repo after the merge.

@jlaur
Copy link
Contributor

jlaur commented Jan 23, 2022

Replacing the channel type of a channel is a breaking change.

Please see #11668 (comment)

@lolodomo
Copy link
Contributor

Replacing the channel type of a channel is a breaking change.

Please see #11668 (comment)

My feeling here is that we are EXACTLY in the case of the hue binding where existing channel types were replaced by system channel types. This lead to many complaints in the community forum (do a search) because users did not see anymore their channels (in the thing UI) until they remove and create again the thing (meaning all channel links have to be redone).

I am not discouraging this change, I am even in favour of this change. I just want that we alert the users.

One time again, this change is fully transparent for any user like me who relies on config files.

@jlaur
Copy link
Contributor

jlaur commented Jan 23, 2022

My feeling here is that we are EXACTLY in the case of the hue binding where existing channel types were replaced by system channel types.

I don't know the details of that case. But I tested (while working in 3.2) that changing channel type id while preserving channel id and item type does not cause any problems after openHAB has been restarted. If this scenario is the same as you have bad feelings about, I can only encourage you to try it out also for certainty.

@lolodomo
Copy link
Contributor

I don't know the details of that case. But I tested (while working in 3.2) that changing channel type id while preserving channel id and item type does not cause any problems after openHAB has been restarted. If this scenario is the same as you have bad feelings about, I can only encourage you to try it out also for certainty.

Yes, I should probably just to convince myself. Did your test match exactly this case ?
I can't imagine so many users could have mentioned an issue which is not one. They discovered it after migrating to a new OH version (from 3.0 to 3.1). You can even find at least two issues created in GitHub.

@ecdye
Copy link
Contributor Author

ecdye commented Jan 24, 2022

Force pushed to bring branch up to date with head.

@jlaur
Copy link
Contributor

jlaur commented Jan 25, 2022

@ecdye - please update resources/OH-INF/i18n/omnilink.properties according to new keys in XML files.

@jlaur
Copy link
Contributor

jlaur commented Jan 25, 2022

Yes, I should probably just to convince myself. Did your test match exactly this case ?

My test was exactly the case of changing channel type id while preserving channel id and item type. This worked in 3.2 (didn't test earlier versions) after restarting openHAB. I still don't know the exact Hue case, so can't compare. But the concern raised for this PR seems to match what I previously verified not being a problem.

Signed-off-by: Ethan Dye <[email protected]>
@ecdye
Copy link
Contributor Author

ecdye commented Jan 30, 2022

@jlaur Fixed, I think we are ready to merge.

@lolodomo
Copy link
Contributor

lolodomo commented Feb 8, 2022

@jlaur : what is your status ?
Regarding properties and semantic tags, for me, it is OK.
I have not checked the other changes about channel types and i18n.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@jlaur
Copy link
Contributor

jlaur commented Feb 8, 2022

what is your status ?
Regarding properties and semantic tags, for me, it is OK.

Looks good to me also.

I have not checked the other changes about channel types and i18n.

I didn't verify everything, but it seems correct. Since migrated to using system channel types, the labels/descriptions had to be moved from custom channel types to channels directly, which meant also changed i18n keys.

@lolodomo
Copy link
Contributor

lolodomo commented Feb 8, 2022

I let you the lead to merge this PR.

@jlaur jlaur merged commit 20da017 into openhab:main Feb 8, 2022
@jlaur jlaur added this to the 3.3 milestone Feb 8, 2022
@lolodomo
Copy link
Contributor

lolodomo commented Feb 8, 2022

@ecdye : please keep an eye on the community forum, in case users complain about channels having disappeared.

NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Apr 27, 2022
* Add semantic tags to channels
* Use system channels where possible

Signed-off-by: Ethan Dye <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jun 29, 2022
* Add semantic tags to channels
* Use system channels where possible

Signed-off-by: Ethan Dye <[email protected]>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* Add semantic tags to channels
* Use system channels where possible

Signed-off-by: Ethan Dye <[email protected]>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* Add semantic tags to channels
* Use system channels where possible

Signed-off-by: Ethan Dye <[email protected]>
Signed-off-by: Andras Uhrin <[email protected]>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* Add semantic tags to channels
* Use system channels where possible

Signed-off-by: Ethan Dye <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants