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

[netatmo] Semantics added to channel types #10973

Closed
wants to merge 4 commits into from

Conversation

lolodomo
Copy link
Contributor

Signed-off-by: Laurent Garnier [email protected]

@lolodomo lolodomo force-pushed the netatmo_semantics branch from fc81ff7 to e4027c1 Compare July 11, 2021 17:53
@lolodomo lolodomo added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jul 11, 2021
@Skinah Skinah added the enhancement An enhancement or new feature for an existing add-on label Jul 12, 2021
Copy link
Contributor

@clinique clinique left a comment

Choose a reason for hiding this comment

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

I will take in account these evolutions in the ongoing PR also.

</channel-type>

<channel-type id="floodlight">
<item-type>Switch</item-type>
<label>Floodlight</label>
<description>State of the floodlight</description>
<tags>
<tag>Switch</tag>
Copy link
Member

Choose a reason for hiding this comment

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

Can you help me understand what the "Switch" tag was good for? This channel seems to be related to light, so I'd rather would have expected a tag for that. Just because something is "switchable" is imho not enough for such a tag, especially as this information is already contained in the item-type.

Copy link
Contributor

@clinique clinique Jul 12, 2021

Choose a reason for hiding this comment

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

To tell the truth @kaikreuzer , I did not spend much time reviewing this because existing version of the Netatmo binding is hardly functional due to some evolutions on Netatmo API side. I rewrote it completely (opened PR on this) still WIP but 95% done. Heavy work ongoing. I expect to be able to target OH 3.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree, "Light" should be added.

Copy link
Member

Choose a reason for hiding this comment

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

But then again, "Light" is usually used to light up some room. A floodlight of a camera has a fairly special semantic and shouldn't be used as a random light (you don't want it to go on when you enter the room. You probably do not want it to turn off, when you're leaving the house, etc.)
I'd in such cases hence rather prefer to not put any tag on it, but leave it to the user on how he actually uses the device. Only he knows his concrete use cases.

Copy link
Member

Choose a reason for hiding this comment

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

Note that there a few other "Switch" tags in that PR as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just look at Netatmo product information, the outside camera integrates a real light, it combines a light and a camera.
https://www.netatmo.com/fr-fr/security/cam-outdoor
Considering it as a light that can be switch on and off is IMHO fully correct as it is what it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked again all channels with only "Switch" tag and fixed them.

@lolodomo lolodomo force-pushed the netatmo_semantics branch from 029fcd2 to 303aae4 Compare July 12, 2021 10:15
@lolodomo lolodomo force-pushed the netatmo_semantics branch from 49a6e33 to 9e8d7a3 Compare July 12, 2021 10:25
@kaikreuzer
Copy link
Member

I'd suggest we park this PR until we finish the discussion on openhab/openhab-core#1791. Putting "Status" on everything where we don't have dedicated semantic tags for does not feel right to me and I wouldn't want to see this to be done for 300+ bindings now...

@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 20, 2021

@kaikreuzer : to make progress, I propose to only keep tags that are obvious (in particular measurements like temperature, humidity, pressure, ...) and suppress "Status" where we don't have dedicated semantic tags. Can we at least do that ?

@kaikreuzer
Copy link
Member

I propose to only keep tags that are obvious (in particular measurements like temperature, humidity, pressure, ...)

Yes, that makes sense, but I would really want to keep it to channels that reflect the "current" measurements. Channels about "min" and "max" and "in the last hour", etc. are imho not an obvious match for the semantics. The question you should always ask yourself: Do two channels of the same item-type with the same semantic tags transport the same information, i.e. are they interchangeable?

@lolodomo
Copy link
Contributor Author

We come back to the need to tag channels to display them in equipment and property UI tabs. And any normal user will certainly expect to have all his temperatures (current, min, max, ...) at the same place in "temperature" property. That is more than a reasonable default that can be proposed to the user. That is my point of view and how I use MainUI like very probably a majority of users.

@kaikreuzer
Copy link
Member

My point is not so much about saying that the channel is related to "Temperature" property, but that it is a type "Measurement". While I understand your point that the user might like to see "everything related to temperature" on a single screen (although that can get easily cluttered), adding those tags will complete destroy other use cases like "tell me the temperature in the kitchen" - that would be a clear regression and that's why I don't see these tags as something obvious to add. I really believe that we need to find a common understanding of how the Main UI pages are supposed to be used (as mentioned here, for which I really want @ghys to be involved as the master of the Main UI.

@Skinah Skinah added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Aug 12, 2021
@cweitkamp cweitkamp added the awaiting feedback Awaiting feedback from the pull request author label Oct 16, 2021
@lolodomo
Copy link
Contributor Author

I close this PR as a refactoring PR for this binding is in progress.

@lolodomo lolodomo closed this Nov 12, 2021
@lolodomo lolodomo deleted the netatmo_semantics branch May 18, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Awaiting feedback from the pull request author enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants