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 binary_sensor and device_tracker in HomeKit. #13735

Merged
merged 7 commits into from Apr 9, 2018

Conversation

Projects
None yet
8 participants
@Yonsm
Contributor

Yonsm commented Apr 7, 2018

Description:

Map binary_sensor and device_tracker to HomeKit Binary Sensor, based on 'device_class' attributes:

'co2': 'CarbonDioxideSensor'
'gas': 'CarbonMonoxideSensor'
'moisture': 'LeakSensor'
'motion': 'MotionSensor'
'occupancy': 'OccupancySensor'
'opening': 'ContactSensor'
'smoke': 'SmokeSensor'

If there is no any matched device_class, e.g. device_tracker, then default sensor type is OccupancySensor.

Checklist:

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

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

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

tests/components/homekit/test_type_sensors.py Outdated
from homeassistant.const import (
ATTR_UNIT_OF_MEASUREMENT, STATE_UNKNOWN, TEMP_CELSIUS, TEMP_FAHRENHEIT)
ATTR_UNIT_OF_MEASUREMENT, ATTR_DEVICE_CLASS, STATE_UNKNOWN, STATE_ON, STATE_OFF, STATE_HOME, STATE_NOT_HOME, TEMP_CELSIUS, TEMP_FAHRENHEIT)

This comment has been minimized.

@houndci-bot

houndci-bot Apr 7, 2018

line too long (143 > 79 characters)

@cdce8p cdce8p self-assigned this Apr 7, 2018

@cdce8p

It's looking good so far. Unfortunately I can't test it until tomorrow evening (CET).
Since you dynamically assign the service and char, can you add tests to verify that the right ones where selected. You can get the service / char name with service.display_name or char.display_name. Those must be equal to the HomeKit char / service constants.

tests/components/homekit/test_type_sensors.py Outdated
self.hass.states.set(entity_id, STATE_UNKNOWN,
{ATTR_DEVICE_CLASS: "opening"})
self.hass.block_till_done() # Ensure state.attributes

This comment has been minimized.

@cdce8p

cdce8p Apr 7, 2018

Member

You don't need that comment.

This comment has been minimized.

@Yonsm

Yonsm Apr 7, 2018

Contributor

done

tests/components/homekit/test_type_sensors.py Outdated
@@ -1,11 +1,13 @@
"""Test different accessory types: Sensors."""
import unittest
from homeassistant.components.homekit.const import PROP_CELSIUS
from homeassistant.components.homekit.const import (PROP_CELSIUS,
SERV_CONTACT_SENSOR, CHAR_CONTACT_SENSOR_STATE)

This comment has been minimized.

@houndci-bot

houndci-bot Apr 7, 2018

continuation line under-indented for visual indent

self.hass.states.set(entity_id, STATE_NOT_HOME,
{ATTR_DEVICE_CLASS: "opening"})
self.hass.block_till_done()
self.assertEqual(acc.char_detected.value, 0)

This comment has been minimized.

@cdce8p

cdce8p Apr 8, 2018

Member

Please add this to the end. That way you improve the coverage.

        self.hass.states.remove(entity_id)
        self.hass.block_till_done()
tests/components/homekit/test_type_sensors.py Outdated
self.assertEqual(acc.get_service(SERV_CONTACT_SENSOR).display_name,
SERV_CONTACT_SENSOR)
self.assertEqual(acc.char_detected.display_name,
CHAR_CONTACT_SENSOR_STATE)

This comment has been minimized.

@cdce8p

cdce8p Apr 8, 2018

Member

Instead of L85 - L88, why not do something like that:

from homeassistant.components.homekit.type_sensors import BINARY_SENSOR_SERVICE_MAP

class TestHomekitSensors(unittest.TestCase):

#####
#####

    def test_binary(self):

#####
#####

    def test_binary_device_classes(self):

        entity_id = 'binary_sensor.demo'

        for device_class, (service, char) in BINARY_SENSOR_SERVICE_MAP.items():
            self.hass.states.set(entity_id, STATE_OFF,
                                 {ATTR_DEVICE_CLASS: device_class})
            self.hass.block_till_done()

            acc = BinarySensor(self.hass, entity_id, 'Binary Sensor', aid=2)
            self.assertEqual(acc.get_service(service).display_name, service)
            self.assertEqual(acc.char_detected.display_name, char)

That way you check if every service and char can be added successfully.

This comment has been minimized.

@Yonsm

Yonsm Apr 9, 2018

Contributor

done

Yonsm added some commits Apr 7, 2018

Yonsm Yonsm
Add test for get_accessory and binary sensor
Passed:
 tox -e py36 -- tests/components/homekit/test_type_sensors.py -x
tox -e py36 -- tests/components/homekit/test_get_accessories.py -x
Yonsm Yonsm
Test service.display_name and char_detected.display_name
1. Passed: tox -e py36 -- tests/components/homekit/test_type_sensors.py
-x
2. And remove ‘# Ensure state.attributes’ comment

Yonsm and others added some commits Apr 9, 2018

Yonsm Yonsm
Refine testcase for type_sensors.py with more coverage
PASSED: tox -e py36 -- tests/components/homekit/test_type_sensors.py
PASSED: script/lint (in my submit)

@cdce8p cdce8p referenced this pull request Apr 9, 2018

Merged

Added binary_sensor to supported types HomeKit #5143

2 of 2 tasks complete
@cdce8p

cdce8p approved these changes Apr 9, 2018

@cdce8p cdce8p merged commit cb51553 into home-assistant:dev Apr 9, 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.139%
Details
@cdce8p

This comment has been minimized.

Member

cdce8p commented Apr 9, 2018

Thanks 👍
Looking forward to more PR's

@cdce8p cdce8p added the new-platform label Apr 9, 2018

@Yonsm

This comment has been minimized.

Contributor

Yonsm commented Apr 9, 2018

Thank you for your effective advice and guidance.

@Yonsm Yonsm deleted the Yonsm:homekit_binary_sensor branch Apr 9, 2018

@shuaiger

This comment has been minimized.

Contributor

shuaiger commented Apr 9, 2018

cong~ merged!

@nicx

This comment has been minimized.

nicx commented Apr 10, 2018

@Yonsm great, thanks a lot! @cdce8p maybe its possible to add a default mapping to binary_sensors with device_class "opening" for all sensors which returns states like "open/tilted/closed"? something similar to the existing temperature sensors. with that change I don't need to create extra template binary_sensors anymore :)

@cdce8p

This comment has been minimized.

Member

cdce8p commented Apr 10, 2018

@nicx There is a plan to add device_classes to normal sensors which would allow better types for HomeKit. I just don't know if I get to it this week.

@nicx

This comment has been minimized.

nicx commented Apr 10, 2018

@cdce8p great to hear, I can wait for this 👍 keep up this great work! 🥇

@cdce8p cdce8p added new-feature and removed new-platform labels Apr 11, 2018

@spacesuitdiver

This comment has been minimized.

spacesuitdiver commented Apr 17, 2018

Great work @Yonsm and @cdce8p! One more thing (cough I feel I say this too often), it would be nice to support "door" as a device_class for ContactSensor as well?

@spacesuitdiver

This comment has been minimized.

spacesuitdiver commented Apr 17, 2018

Or maybe it's best to default to ContactSensor since this allows the most flexibility in Homekit and is probably the likely use of a binary_sensor?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Apr 17, 2018

Please don't discuss feature requests in closed PRs.

@spacesuitdiver

This comment has been minimized.

spacesuitdiver commented Apr 17, 2018

@MartinHjelmare seemed like an omission, what SHOULD I do instead?

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Apr 17, 2018

If you want to suggest an enhancement please open a feature request in the Feature Requests section of our community forum.

Merged PRs should not be used for enhancement discussion or bug reports. If you've found a bug it's ok to make a review with inline comments and link to an issue that reports the bug.

Thanks!

@balloob balloob referenced this pull request Apr 27, 2018

Merged

0.68 #14124

@gerard33 gerard33 referenced this pull request Apr 29, 2018

Merged

Add device_tracker component to Homekit documentation #5272

2 of 2 tasks complete

Adminiuga added a commit to Adminiuga/home-assistant that referenced this pull request Jun 25, 2018

Support binary_sensor and device_tracker in HomeKit (home-assistant#1…
…3735)

* Support binary_sensor and device_tracker for HomeKit
* Add test for get_accessory and binary sensor
* Test service.display_name and char_detected.display_name
* Split test to improve speed

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018

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