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

Air Purifier Pro: support for sound volume level and fix for bright propery #157

Merged
merged 5 commits into from
Jan 16, 2018

Conversation

yawor
Copy link
Contributor

@yawor yawor commented Jan 7, 2018

The Air Purifier Pro has a property controlling the volume level of sound notifications, which are used when any command is sent to it. I've added support for the property and a method to set it's value. The range is from 0 (mute) to 100 (full volume). I don't know if Air Purifier 2 also has this property or not.

There's a photosensitive element on top of the Air Purifier Pro (right next to the power/mode selection button). It's value is held in the bright property, which has been improperly used as a LED brightness for this model. Air Purifier Pro display probably doesn't support changing brightness. The value for the bright property changes from 0 (no light detected) up to 200. I don't know what's the unit it is given in. I've got 200 when I've used my phone's LED flashlight directly on the sensor.

yawor added 3 commits January 7, 2018 16:47
…ness

Air Purifier Pro contains a brightness level sensor (on top, right next
to the mode selection button). The bright propery contains the value
based on this sensor. Value ranges from 0 to 200.
@coveralls
Copy link

coveralls commented Jan 7, 2018

Coverage Status

Coverage increased (+0.007%) to 63.088% when pulling 1637818 on yawor:air_purifier_pro_fixes into dd76fcd on rytilahti:master.

@yawor
Copy link
Contributor Author

yawor commented Jan 7, 2018

OK, I've tested the bright value against another illuminance sensor and the value seems to be in lux. The value is not equal to the other sensor, but there's a difference of about 3 lx - probably because of where the sensor is located and gets slightly less light than the other one.

The name illuminance is more appropriate to the function of the value than
brightness
@coveralls
Copy link

coveralls commented Jan 7, 2018

Coverage Status

Coverage increased (+0.007%) to 63.088% when pulling 0031522 on yawor:air_purifier_pro_fixes into dd76fcd on rytilahti:master.

@yawor
Copy link
Contributor Author

yawor commented Jan 7, 2018

I've changed the name of the brightness property to illuminance. It's more appropriate regarding it's function. For example in the Home Assistant, illuminance is a type of sensor providing information about the amount of light.

@rytilahti
Copy link
Owner

rytilahti commented Jan 15, 2018

This looks fine for me to be merged, however I cannot test it myself. @syssi what's your opinion on this?

edit: to add, @yawor, could you mention your findings wrt illuminance in code comments too?

@syssi
Copy link
Collaborator

syssi commented Jan 15, 2018

Looks good! 👍

@yawor
Copy link
Contributor Author

yawor commented Jan 15, 2018

@rytilahti can you point me to where's the best place to add that information in the code?

@rytilahti
Copy link
Owner

@yawor I'd say directly into the docstring of the method, simply by adding "reported in lux, values between [0-200]" or similar would do fine. The point is just to make it somewhat visible for anyone stumbling upon that code / trying to use it in their own code.

@coveralls
Copy link

coveralls commented Jan 16, 2018

Coverage Status

Coverage increased (+0.007%) to 63.088% when pulling 313cfad on yawor:air_purifier_pro_fixes into dd76fcd on rytilahti:master.

@rytilahti
Copy link
Owner

Looks great, thanks!

@rytilahti rytilahti merged commit 74155d7 into rytilahti:master Jan 16, 2018
@yawor yawor deleted the air_purifier_pro_fixes branch January 19, 2018 21:30
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.

4 participants