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

[hue] Added humidity and pressure sensor #7426

Closed
wants to merge 5 commits into from
Closed

[hue] Added humidity and pressure sensor #7426

wants to merge 5 commits into from

Conversation

DerOetzi
Copy link
Contributor

First draft to add humidity and pressure sensor to hue binding

see #7420

I'm not sure about Device IDs of this two devices

Signed-off-by: Johannes DerOetzi Ott <[email protected]>
@DerOetzi DerOetzi requested a review from cweitkamp as a code owner April 19, 2020 20:35
Signed-off-by: Johannes DerOetzi Ott <[email protected]>
Copy link
Contributor

@cweitkamp cweitkamp left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement. Some remarks inside.

<!-- Pressure Channel -->
<channel-type id="pressure">
<item-type>Number:Pressure</item-type>
<label>Relative Humidity</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<label>Relative Humidity</label>
<label>Relative Pressure</label>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<channel-type id="pressure">
<item-type>Number:Pressure</item-type>
<label>Relative Humidity</label>
<description>Current relative humidity.</description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<description>Current relative humidity.</description>
<description>Current relative pressure.</description>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


<!-- Humidity Channel -->
<channel-type id="humidity">
<item-type>Number</item-type>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use Number:Dimensionless for this channel type and update its state by SmartHomeUnits.PERCENT (see SmartHomeUnits.PERCENT).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed


<representation-property>uniqueId</representation-property>

<config-description>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is time to introduced a "config-description-uri" for these new thing types to avoid too much duplicate code.

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 have introduced the config-description, for all things except PresenceSensor (0107) and LightLevelSensor (0106) because they have special parameters. Please have a look.

@@ -1,34 +1,38 @@
# binding
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to this file? Did you eventually change the encoding? We require ISO-8859.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups I have seen this already yesterday and forgot to push

| dark | Switch | This channel indicates whether the light level is below the darkness threshold or not. | 0106 |
| daylight | Switch | This channel indicates whether the light level is below the daylight threshold or not. | 0106 |
| presence | Switch | This channel indicates whether a motion is detected by the sensor or not. | 0107 |
| temperature | Number:Temperature | This channel shows the current temperature measured by the sensor. | 0302 |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the new channel here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added Channels

Signed-off-by: Johannes DerOetzi Ott <[email protected]>
@TravisBuddy
Copy link

Travis tests have failed

Hey @DerOetzi,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

Johannes DerOetzi Ott added 2 commits April 20, 2020 10:59
Signed-off-by: Johannes DerOetzi Ott <[email protected]>
Signed-off-by: Johannes DerOetzi Ott <[email protected]>
@DerOetzi
Copy link
Contributor Author

@cweitkamp I would offer you to do some code clean ups to reduce checkstyle warnings. To be able to concentrate on the real errors. But I would do this in a seperate branch after this pull request is finished

@J-N-K
Copy link
Member

J-N-K commented Apr 20, 2020

One question: is it possible to pair these sensors With a hue bridge? Or are they paired to a deconz and only made available via the hue binding instead of the deconz?

@DerOetzi
Copy link
Contributor Author

One question: is it possible to pair these sensors With a hue bridge? Or are they paired to a deconz and only made available via the hue binding instead of the deconz?

They are connected to deconz at the moment, for not having a hue bridge at the moment, but I have read somewhere the xiaomi aqara sensors can be paired with hue bridge as well, but can't find the link now. :(

@J-N-K
Copy link
Member

J-N-K commented Apr 20, 2020

But then: why don‘t we use them on the deconz bridge? And if they are used on the hue bridge, are they doubled on the deconz?

@DerOetzi
Copy link
Contributor Author

But then: why don‘t we use them on the deconz bridge? And if they are used on the hue bridge, are they doubled on the deconz?

Yes they are already present at the deconz binding. Didn't thought about this, because I'm using my PresenceSensors although for some time with hue binding without having to configure an extra binding (which I was glad to get rid of for being instable). So I didn't think about the deconz binding anymore, when writing the feature request. I would understand the argument to have only supported things inside hue binding and could withdraw this PR if you wish.

@J-N-K
Copy link
Member

J-N-K commented Apr 21, 2020

Don't get me wrong, I'm not against this change, I only fear that the same physical entity on two bridges might be confusing. If pairing the sensors with the hue bridge actually works, we could drop them from the deconz.

@cpmeister cpmeister added the enhancement An enhancement or new feature for an existing add-on label Apr 22, 2020
@cweitkamp
Copy link
Contributor

I agree with @J-N-K. As long as the sensors themselves do not work with Hue Bridge I do not see any reason to add support in the Hue binding. Compared to deCONZ binding the Hue binding has a big disadvantage for sensor handling because it uses a polling based strategy whereas deCONZ binding uses a websocket and event based handling.

Thanks for your work and of course

I would offer you to do some code clean ups to reduce checkstyle warnings

will be very much appreciated.

@DerOetzi
Copy link
Contributor Author

I cannot find the page, where I have read about the possibility to connect aqara sensors to hue bridge. So I think I will redraw this PR.

As I offered before I will do a new PR in some days with some code improvments, should I take over the changes to light-config.xml and sensor-config.xml as well?

@regnets
Copy link

regnets commented Apr 22, 2020

The official Philips motion sensor has a temperature sensor built in. So it would make sense to implement this for the hue binding.

@cweitkamp
Copy link
Contributor

@DerOetzi

should I take over the changes to light-config.xml and sensor-config.xml as well?

Pick whatever you like. I am a fan of configuration descriptions.

@regnets The temperature sensor is already available in Hue binding.

I am aware of other sensors which could be paired with Hue bridge. For example innr SP120 smart plug with integrated power and consumption measurements. But to be honest I never put them together and explored what is happening. I am using deCONZ for sensors 😉.

@DerOetzi
Copy link
Contributor Author

DerOetzi commented Apr 25, 2020

@DerOetzi

should I take over the changes to light-config.xml and sensor-config.xml as well?

Pick whatever you like. I am a fan of configuration descriptions.

@cweitkamp
I will wait for the huge PR #7476 to be finished, because I don't want to produce lot of merge conflicts and afterwards I will have a look on the cleanup task.

@cpmeister cpmeister added the awaiting other PR Depends on another PR label Apr 25, 2020
@DerOetzi
Copy link
Contributor Author

DerOetzi commented May 2, 2020

Closing because this sensor types are not connectable to hue system at the moment.

@DerOetzi DerOetzi closed this May 2, 2020
@wborn wborn removed the awaiting other PR Depends on another PR label Dec 23, 2022
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.

7 participants