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

Support CO2/PM2.5/Light sensors in HomeKit #13804

Merged
merged 3 commits into from Apr 12, 2018

Conversation

Projects
None yet
3 participants
@Yonsm
Contributor

Yonsm commented Apr 10, 2018

Description:

Map sensors to HomeKit AirQuality/CarbonDioxideSensor/SensorLightSensor, based on the following rules :

AirQualitySensor: `device_class == 'co2'` or `entity_id contains 'co2'`
CarbonDioxideSensor: `device_class == 'pm25'` or `entity_id contains 'pm25'`
LightSensor: `device_class == 'lux'` or `unit == 'lm'` or `unit == 'lux'`

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5146

Example entry for configuration.yaml (if applicable):

customize:
  sensor.air_meter:
    friendly_name: 'My Favorite Air Meter'
    device_class: 'pm25'

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox.

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@Yonsm Yonsm requested a review from cdce8p as a code owner Apr 10, 2018

@Yonsm Yonsm referenced this pull request Apr 10, 2018

Merged

Add CO2/PM2.5/Light sensors in HomeKit #5146

2 of 2 tasks complete

@cdce8p cdce8p self-assigned this Apr 10, 2018

homeassistant/components/homekit/__init__.py Outdated
@@ -92,6 +93,22 @@ def get_accessory(hass, state, aid, config):
return TYPES['HumiditySensor'](hass, state.entity_id, state.name,
aid=aid)
entity_id = state.entity_id

This comment has been minimized.

@cdce8p

cdce8p Apr 10, 2018

Member

Just use state.entity_id instead for all calls.

This comment has been minimized.

@Yonsm

Yonsm Apr 11, 2018

Contributor

Too many state.entity_id needs more text and wrap many lines, will reduce readability. And, theoretically, state.entity_id is low performance then entity_id if we access it many times.

This comment has been minimized.

@cdce8p

cdce8p Apr 11, 2018

Member

At least please change the log statement and TYPES call to match all other calls.

Regarding the string checks: I'm not sure we should add them. My plan is to add device_class support for sensor entities later this week. That should cover it.

homeassistant/components/homekit/type_sensors.py Outdated
density = convert_to_float(new_state.state)
if density is not None:
self.char_density.set_value(density)
self.char_quality.set_value(AirQualitySensor.map_quality(density))

This comment has been minimized.

@cdce8p

cdce8p Apr 10, 2018

Member

Set char value only once. Maybe add this as well: density = AirQualitySensor.map_quality(density)

This comment has been minimized.

@Yonsm

Yonsm Apr 11, 2018

Contributor

It's two different chars: density(native state value in sensor) and quality (int 1-5). Or, sorry I don't understand what you mean.

This comment has been minimized.

@cdce8p

cdce8p Apr 11, 2018

Member

Didn't saw that. It might have been quite late yesterday. Disregard that comment.

homeassistant/components/homekit/type_sensors.py Outdated
_LOGGER.debug('%s: Set to %d', self.entity_id, density)
@staticmethod
def map_quality(density):

This comment has been minimized.

@cdce8p

cdce8p Apr 10, 2018

Member

This method should be a function (outside the class). It would fit in homekit/utils.py. Maybe density_to_air_quality is a better name.

This comment has been minimized.

@Yonsm

Yonsm Apr 11, 2018

Contributor

done

homeassistant/components/homekit/type_sensors.py Outdated
return 3
elif density >= 116 and density <= 150:
return 4
return 5

This comment has been minimized.

@cdce8p

cdce8p Apr 10, 2018

Member

Keep in mind that you should be able to handle float values from convert_to_float. What happens if density = 35.1?

You could do something like this (untested):

from collections import OrderedDict
map = OrderedDict(sorted({1: 0, 2: 35, 3: 76, 4: 116, 5: 151}.items(), reverse=True))

def density_to_air_quality(density):
    for key, value in map.items():
        if density > value:
            return key
    return 0

This comment has been minimized.

@Yonsm

Yonsm Apr 11, 2018

Contributor

density = 35.1, It's my carelessness. Thank you for correcting it.
About OrderedDict, I keep the original if statements, since it's simple and has more theoretic performance in this simple case.

This comment has been minimized.

@cdce8p

cdce8p Apr 11, 2018

Member

Works as well.

@@ -64,3 +64,16 @@ def temperature_to_homekit(temperature, unit):
def temperature_to_states(temperature, unit):
"""Convert temperature back from Celsius to Home Assistant unit."""
return round(temp_util.convert(temperature, TEMP_CELSIUS, unit), 1)
def density_to_air_quality(density):

This comment has been minimized.

@cdce8p

cdce8p Apr 11, 2018

Member

Can you add tests for this function test_util.py?

This comment has been minimized.

@Yonsm

Yonsm Apr 11, 2018

Contributor

done

@Yonsm

This comment has been minimized.

Contributor

Yonsm commented Apr 12, 2018

Rebase and reset commits

Yonsm and others added some commits Apr 12, 2018

Small changes
* Added tests
* changed device_class lux to light
@cdce8p

cdce8p approved these changes Apr 12, 2018

I have added additional tests and changed device_class: 'lux' to device_class: 'light'.

@cdce8p cdce8p merged commit c863b96 into home-assistant:dev Apr 12, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 94.147%
Details
@cdce8p

This comment has been minimized.

Member

cdce8p commented Apr 12, 2018

Thanks 🥇

@Yonsm

This comment has been minimized.

Contributor

Yonsm commented Apr 12, 2018

Good job, thanks. Actually, I have been used light before but the line is too long to fit 79-chars style, haha:). I think light is a good word for that.

@balloob balloob referenced this pull request Apr 27, 2018

Merged

0.68 #14124

@Yonsm Yonsm deleted the Yonsm:homekit_co2_light_airquality_sensors branch Apr 28, 2018

@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.