Support binary_sensor and device_tracker for HomeKit by Yonsm · Pull Request #13701 · home-assistant/core

Yonsm · 2018-04-05T15:17:16Z

Description:

Support binary_sensor and device_tracker for HomeKit.

Map binary_sensor and device_tracker to HomeKit BinarySensor, based on
‘device_class’ attributes:

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

If there is no any matched class (e.g. device_tracker), the default sensor type is OccupancySensor.

Checklist:

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

Map binary_sensor and device_tracker to HomeKit BinarySensor, based on ‘homekit_device_class’ or ‘device_class’ attributes: 'gas': 'CarbonMonoxideSensor' 'co2': 'CarbonDioxideSensor' 'occupancy': 'OccupancySensor' 'opening': 'ContactSensor' 'motion': 'MotionSensor' 'moisture': 'LeakSensor' 'smoke': 'SmokeSensor' Default device_class is 'occupancy'.

homeassistant · 2018-04-05T15:17:18Z

Hi @Yonsm,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2018-04-05T15:17:25Z

+        service_characteristic = service_map[device_class] if (device_class in service_map) else service_map['occupancy']
+
+        service = add_preload_service(self, service_characteristic[0])
+        self.char_detected = service.get_characteristic(service_characteristic[1])


line too long (82 > 79 characters)

houndci-bot · 2018-04-05T15:17:25Z

+
+        device_class_key = 'homekit_device_class' if ('homekit_device_class' in state.attributes) else 'device_class'
+        device_class = state.attributes.get(device_class_key)
+        service_characteristic = service_map[device_class] if (device_class in service_map) else service_map['occupancy']


line too long (121 > 79 characters)

houndci-bot · 2018-04-05T15:17:25Z

+            'moisture': ('LeakSensor', 'LeakDetected'),
+            'smoke': ('SmokeSensor', 'SmokeDetected')}
+
+        device_class_key = 'homekit_device_class' if ('homekit_device_class' in state.attributes) else 'device_class'


line too long (117 > 79 characters)

houndci-bot · 2018-04-05T15:17:25Z

+
+    def __init__(self, hass, state, **kwargs):
+        """Initialize a BinarySensor accessory object."""
+        super().__init__(state.name, state.entity_id, CATEGORY_SENSOR, **kwargs)


line too long (80 > 79 characters)

homeassistant · 2018-04-05T15:23:10Z

Hi @Yonsm,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Yonsm · 2018-04-05T15:35:35Z

This is my first pull request. And if success, I'll update more HomeKit support for:
CarbonDioxideSensor
LightSensor
AirQualitySensor
AirPurifier (fully support by HomeKit)

cdce8p

Great first Home Assistant PR! Looking forward for more to come.
I've left some comments.

cdce8p · 2018-04-05T17:23:21Z


+    elif state.domain == 'binary_sensor' or state.domain == 'device_tracker':
+        _LOGGER.debug('Add "%s" as "%s"', state.entity_id, 'BinarySensor')
+        return TYPES['BinarySensor'](hass, state, aid=aid)


You don't need to pass in the state, only the entity_id: state.entity_id.

Only the entity_id is not enough, we need state.attributes to check binary sensor type. I'll follow your style to pass name, entity_id and one more param state.attributes.

cdce8p · 2018-04-05T17:24:37Z

+class BinarySensor(HomeAccessory):
+    """Generate a BinarySensor accessory as binary sensor."""
+
+    def __init__(self, hass, state, **kwargs):


entity_id instead of state

cdce8p · 2018-04-05T17:25:24Z

+
+    def __init__(self, hass, state, **kwargs):
+        """Initialize a BinarySensor accessory object."""
+        entity_id = state.entity_id


Isn't necessary if entity_id is passed to constructor.

cdce8p · 2018-04-05T17:31:57Z

+            'opening': ('ContactSensor', 'ContactSensorState'),
+            'motion': ('MotionSensor', 'MotionDetected'),
+            'moisture': ('LeakSensor', 'LeakDetected'),
+            'smoke': ('SmokeSensor', 'SmokeDetected')}


Those should be constants defined in ./const.py. E.g.:

DEVICE_CLASS_GAS = 'gas'

SERV_GAS_SENSOR = CarbonMonoxideSensor

CHAR_GAS_DETECTED = CarbonMonoxideDetected

...

service_map a constant in type_sensors.py outside any class, like so:

SERVICE_MAP = { DEVICE_CLASS_GAS: {SERV_GAS_SENSOR, CHAR_GAS_DETECTED), # ... }

Good coding style. I'll follow it.

cdce8p · 2018-04-05T17:35:32Z

+
+        device_class_key = 'homekit_device_class' \
+            if ('homekit_device_class' in state.attributes) \
+            else 'device_class'


Currently there is no homekit_device_class attribute. For this PR we should concentrate just on the device_class.

homekit_device_class it just like homebridge_outlet_type, etc in HomeBridge/homegbridge+homeassistant. It's preserved for user to assign this param in customization.yaml manually. Because sometimes there is no device_class in binary_sensor's attributes, and user can assign it to gas co/co2, or any other binary_sensor.

There will be more homekit_xxxx key for CO2/Light/AirQuality sensor to get correct sensor type instead of guessing by unit or entity id. I'll document it in PR comment.

We shouldn't write those in the state machine. I would prefer to leave it at device_class only for the moment. If you really want to implement this, use the entity_config dictionary from the homekit component. A good example is the alarm_code for type_security_systems.py.

Remove it at this time.

cdce8p · 2018-04-05T17:48:29Z

+        device_class_key = 'homekit_device_class' \
+            if ('homekit_device_class' in state.attributes) \
+            else 'device_class'
+        device_class = state.attributes.get(device_class_key)


Use ATTR_DEVICE_CLASS imported from homeassistant.const as key

Just follow it

houndci-bot · 2018-04-06T02:18:50Z

+                (SERV_MOTION_SENSOR, CHAR_MOTION_DETECTED),
+            DEVICE_CLASS_MOISTURE: 
+                (SERV_LEAK_SENSOR, CHAR_LEAK_DETECTED),
+            DEVICE_CLASS_SMOKE: 


trailing whitespace

houndci-bot · 2018-04-06T02:18:51Z

+                (SERV_CONTACT_SENSOR, CHAR_CONTACT_SENSOR_STATE),
+            DEVICE_CLASS_MOTION: 
+                (SERV_MOTION_SENSOR, CHAR_MOTION_DETECTED),
+            DEVICE_CLASS_MOISTURE: 


trailing whitespace

houndci-bot · 2018-04-06T02:18:51Z

+                (SERV_OCCUPANCY_SENSOR, CHAR_OCCUPANCY_DETECTED),
+            DEVICE_CLASS_OPENING: 
+                (SERV_CONTACT_SENSOR, CHAR_CONTACT_SENSOR_STATE),
+            DEVICE_CLASS_MOTION: 


trailing whitespace

houndci-bot · 2018-04-06T02:18:51Z

+                (SERV_CARBON_DIOXIDE_SENSOR, CHAR_CARBON_DIOXIDE_DETECTED),
+            DEVICE_CLASS_OCCUPANCY: 
+                (SERV_OCCUPANCY_SENSOR, CHAR_OCCUPANCY_DETECTED),
+            DEVICE_CLASS_OPENING: 


trailing whitespace

houndci-bot · 2018-04-06T02:18:51Z

+                (SERV_CARBON_MONOXIDE_SENSOR, CHAR_CARBON_MONOXIDE_DETECTED),
+            DEVICE_CLASS_CO2: 
+                (SERV_CARBON_DIOXIDE_SENSOR, CHAR_CARBON_DIOXIDE_DETECTED),
+            DEVICE_CLASS_OCCUPANCY: 


trailing whitespace

houndci-bot · 2018-04-06T02:18:51Z

+        service_map = {
+            DEVICE_CLASS_GAS: 
+                (SERV_CARBON_MONOXIDE_SENSOR, CHAR_CARBON_MONOXIDE_DETECTED),
+            DEVICE_CLASS_CO2: 


trailing whitespace

houndci-bot · 2018-04-06T02:18:51Z

+        self.entity_id = entity_id
+
+        service_map = {
+            DEVICE_CLASS_GAS: 


trailing whitespace

houndci-bot · 2018-04-06T02:18:51Z

-    CHAR_CURRENT_HUMIDITY, CHAR_CURRENT_TEMPERATURE, PROP_CELSIUS)
+    CHAR_CURRENT_HUMIDITY, CHAR_CURRENT_TEMPERATURE, PROP_CELSIUS,
+    DEVICE_CLASS_GAS, SERV_CARBON_MONOXIDE_SENSOR,
+        CHAR_CARBON_MONOXIDE_DETECTED,


continuation line unaligned for hanging indent

houndci-bot · 2018-04-06T02:36:45Z

    CATEGORY_SENSOR, SERV_HUMIDITY_SENSOR, SERV_TEMPERATURE_SENSOR,
-    CHAR_CURRENT_HUMIDITY, CHAR_CURRENT_TEMPERATURE, PROP_CELSIUS)
+    CHAR_CURRENT_HUMIDITY, CHAR_CURRENT_TEMPERATURE, PROP_CELSIUS,
+    DEVICE_CLASS_GAS, SERV_CARBON_MONOXIDE_SENSOR, 


trailing whitespace

cdce8p

Please take a look at: #13701 (comment) as well if you haven't seen it already.

cdce8p · 2018-04-06T10:05:03Z

+class BinarySensor(HomeAccessory):
+    """Generate a BinarySensor accessory as binary sensor."""
+
+    def __init__(self, hass, entity_id, name, attributes, **kwargs):


Don't pass attributes. Use hass instead, like so:
self.hass.states.get(self.entity_id).attributes

cdce8p · 2018-04-06T10:07:25Z

+        self.hass = hass
+        self.entity_id = entity_id
+
+        service_map = {


Please move this between _LOGGER and @TYPES.register('TemperatureSensor') and rename it to something like BINARY_SENSOR_SERVICE_MAP

cdce8p · 2018-04-06T10:09:06Z

+DEVICE_CLASS_SMOKE = 'smoke'
+
+# #### Attributes ####
+ATTR_DEVICE_CLASS = 'device_class'


ATTR_DEVICE_CLASS is already defined in homeassistant.const

cdce8p · 2018-04-06T10:09:57Z

+SERV_CONTACT_SENSOR = 'ContactSensor'
+SERV_MOTION_SENSOR = 'MotionSensor'
+SERV_LEAK_SENSOR = 'LeakSensor'
+SERV_SMOKE_SENSOR = 'SmokeSensor'


Please sort them alphabetically. The chars and device_classes as well.

Follow it.
Furthermore, I also sort CHAR_CURRENT_HUMIDITY and CHAR_CURRENT_POSITION, and this cause a conflict. Should I revert it, or would you resolve it?

Try rebaseing it against dev: https://www.home-assistant.io/developers/development_catching_up

1. Remove state param; 2. Sort const; 3. Import ATTR_DEVICE_CLASS from homeassistant.const; 4. Remove device_class_key: device_class_key = ATTR_HOMEKIT_DEVICE_CLASS \ if (ATTR_HOMEKIT_DEVICE_CLASS in attributes) \ else ATTR_DEVICE_CLASS

houndci-bot · 2018-04-06T10:59:31Z

+    DEVICE_CLASS_SMOKE:
+        (SERV_SMOKE_SENSOR, CHAR_SMOKE_DETECTED)}
+
+@TYPES.register('BinarySensor')


expected 2 blank lines, found 1

houndci-bot · 2018-04-06T10:59:31Z

-    CHAR_CURRENT_HUMIDITY, CHAR_CURRENT_TEMPERATURE, PROP_CELSIUS)
+    CHAR_CURRENT_HUMIDITY, CHAR_CURRENT_TEMPERATURE, PROP_CELSIUS,
+    DEVICE_CLASS_CO2, SERV_CARBON_DIOXIDE_SENSOR, CHAR_CARBON_DIOXIDE_DETECTED,
+    DEVICE_CLASS_GAS, SERV_CARBON_MONOXIDE_SENSOR, CHAR_CARBON_MONOXIDE_DETECTED,


line too long (81 > 79 characters)

cdce8p · 2018-04-06T11:38:22Z

+    DEVICE_CLASS_OCCUPANCY, SERV_OCCUPANCY_SENSOR, CHAR_OCCUPANCY_DETECTED,
+    DEVICE_CLASS_OPENING, SERV_CONTACT_SENSOR, CHAR_CONTACT_SENSOR_STATE,
+    DEVICE_CLASS_SMOKE, SERV_SMOKE_SENSOR, CHAR_SMOKE_DETECTED,
+    ATTR_HOMEKIT_DEVICE_CLASS)


ATTR_HOMEKIT_DEVICE_CLASS is an unused import.

cdce8p · 2018-04-06T11:39:15Z

+    DEVICE_CLASS_OPENING:
+        (SERV_CONTACT_SENSOR, CHAR_CONTACT_SENSOR_STATE),
+    DEVICE_CLASS_SMOKE:
+        (SERV_SMOKE_SENSOR, CHAR_SMOKE_DETECTED)}


Is that an option?

BINARY_SENSOR_SERVICE_MAP = { DEVICE_CLASS_CO2: (SERV_CARBON_DIOXIDE_SENSOR, CHAR_CARBON_DIOXIDE_DETECTED), DEVICE_CLASS_GAS: (SERV_CARBON_MONOXIDE_SENSOR, CHAR_CARBON_MONOXIDE_DETECTED), DEVICE_CLASS_MOISTURE: (SERV_LEAK_SENSOR, CHAR_LEAK_DETECTED), DEVICE_CLASS_MOTION: (SERV_MOTION_SENSOR, CHAR_MOTION_DETECTED), DEVICE_CLASS_OCCUPANCY: (SERV_OCCUPANCY_SENSOR, CHAR_OCCUPANCY_DETECTED), DEVICE_CLASS_OPENING: (SERV_CONTACT_SENSOR, CHAR_CONTACT_SENSOR_STATE), DEVICE_CLASS_SMOKE: (SERV_SMOKE_SENSOR, CHAR_SMOKE_DETECTED)}

Can you move the BINARY_SENSOR_SERVICE_MAP above the TemperatureSensor class. All constants should be defined belove the imports.

cdce8p · 2018-04-06T11:40:07Z

                          self.entity_id, humidity)
+
+
+# Binary sensor service map


You don't need that.

cdce8p · 2018-04-06T11:42:02Z

+        self.entity_id = entity_id
+
+        attributes = hass.states.get(entity_id).attributes
+        device_class = attributes.get(ATTR_DEVICE_CLASS)


You can combine those two lines and please use self.hass and self.entity_id:

device_class = self.hass.states.get(self.entity_id).attributes.get(ATTR_DEVICE_CLASS)

Use attributes for homekit_device_class checking in future. But I'll follow your advice to remove it at this time.

I don't think self.hass is better then hass. Actually, local variable is better performance then member variable, and it's good style for reading (less text).

It might be a little. ~~However this will change with #13707. This PR will require to use self.hass and self.entity_id.~~

Update:
IMO it's a style question. At the moment all other types use self.hass.

cdce8p · 2018-04-06T11:50:07Z

+        attributes = hass.states.get(entity_id).attributes
+        device_class = attributes.get(ATTR_DEVICE_CLASS)
+        service_char = BINARY_SENSOR_SERVICE_MAP[device_class] \
+            if (device_class in BINARY_SENSOR_SERVICE_MAP) \


You don't need brackets here. if device_class in BINARY_SENSOR_SERVICE_MAP \ is enough.

# Conflicts: # homeassistant/components/homekit/const.py

* Version bump to HAP-python==1.1.9 * Updated types and tests

cdce8p

The BinarySensor class is looking good. Just on issue relate to an update of HAP-python. Can you add tests and the documentation? The tests should go in 'test_get_accessory' and 'test_type_sensor'.

cdce8p · 2018-04-06T16:49:36Z

+
+        state = new_state.state
+        detected = (state == STATE_ON) or (state == STATE_HOME)
+        self.char_detected.set_value(detected, should_callback=False)


We just updated HAP-python to 1.1.9. 'Should_callback=False' will no longer be necessary. You probably need to rebase your branch, take take advantage of the update.

* Allow use of date_string in service call * Add stricter validation, fix descriptions

* Add async timeout feature * Decorator for setter methods to limit service calls to HA * Changed to async * Use async_call_later * Use lastargs, async_add_job * Use dict for lastargs * Updated tests to stop patch

* added support for smappee water sensors * fixed lint error and wrong location_id * fixed lint error * Use string formatting

* Initialise filter with historical values Added get_last_state_changes() * fix test * Major changes to accommodate history + time_SMA # Conflicts: # homeassistant/components/sensor/filter.py * hail the hound! * lint fixed * less debug * ups * get state from the proper entity * sensible default * No defaults in get_last_state_changes * list_reverseiterator instead of list * prev_state to state * Initialise filter with historical values Added get_last_state_changes() * fix test * Major changes to accommodate history + time_SMA # Conflicts: # homeassistant/components/sensor/filter.py * hail the hound! * lint fixed * less debug * ups * get state from the proper entity * sensible default * No defaults in get_last_state_changes * list_reverseiterator instead of list * prev_state to state * update * added window_unit * replace isinstance with window_unit

* Fixed bug - unable to set base readaonly property * PR fixes * Added line

Map binary_sensor and device_tracker to HomeKit BinarySensor, based on ‘homekit_device_class’ or ‘device_class’ attributes: 'gas': 'CarbonMonoxideSensor' 'co2': 'CarbonDioxideSensor' 'occupancy': 'OccupancySensor' 'opening': 'ContactSensor' 'motion': 'MotionSensor' 'moisture': 'LeakSensor' 'smoke': 'SmokeSensor' Default device_class is 'occupancy'.

1. Remove state param; 2. Sort const; 3. Import ATTR_DEVICE_CLASS from homeassistant.const; 4. Remove device_class_key: device_class_key = ATTR_HOMEKIT_DEVICE_CLASS \ if (ATTR_HOMEKIT_DEVICE_CLASS in attributes) \ else ATTR_DEVICE_CLASS

And refine BINARY_SENSOR_SERVICE_MAP.

# Conflicts: # homeassistant/components/homekit/type_sensors.py

Yonsm · 2018-04-07T06:32:31Z

Close this PR since it has more details. I'll re-PR later in a new branch.

Yonsm added 2 commits April 5, 2018 21:47

Merge remote-tracking branch 'home-assistant/dev' into dev

05041c9

Yonsm requested a review from cdce8p as a code owner April 5, 2018 15:17

homeassistant added cla-needed integration: homekit labels Apr 5, 2018

houndci-bot reviewed Apr 5, 2018

View reviewed changes

Line wrap to 79 chars

1e4cd78

homeassistant added cla-needed cla-signed and removed cla-needed labels Apr 5, 2018

Check 'home' for device_tracker for HomeKit

af89ce6

cdce8p requested changes Apr 5, 2018

View reviewed changes

Refine params/const based on advice:

cacf0e5

houndci-bot reviewed Apr 6, 2018

View reviewed changes

Trim trailing whitespace and align hanging indent

c72faea

houndci-bot reviewed Apr 6, 2018

View reviewed changes

Trim trailing whitespace

469b7e9

cdce8p requested changes Apr 6, 2018

View reviewed changes

houndci-bot reviewed Apr 6, 2018

View reviewed changes

Wrap long line and add a blank line

4eb721e

cdce8p requested changes Apr 6, 2018

View reviewed changes

cdce8p self-assigned this Apr 6, 2018

Merge remote-tracking branch 'home-assistant/dev' into Yonsm_dev

d10c6cf

# Conflicts: # homeassistant/components/homekit/const.py

homeassistant added the cla-signed label Apr 6, 2018

Yonsm added 2 commits April 6, 2018 22:10

Merge remote-tracking branch 'home-assistant/dev' into Yonsm_dev

ef448c7

Revise const and coding style

3fe9284

cdce8p and others added 2 commits April 6, 2018 10:20

Update Homekit to 1.1.9 (#13716)

8548761

* Version bump to HAP-python==1.1.9 * Updated types and tests

Update docstrings (#13720)

3394916

cdce8p requested changes Apr 6, 2018

View reviewed changes

cgtobi and others added 17 commits April 6, 2018 21:48

Add option to ignore availability in google calendar events (#13714)

48fe2d1

Allow use of date_string in service call (#13256)

c77d013

* Allow use of date_string in service call * Add stricter validation, fix descriptions

Add timeout / debounce (for brightness and others) (#13534)

262ea14

* Add async timeout feature * Decorator for setter methods to limit service calls to HA * Changed to async * Use async_call_later * Use lastargs, async_add_job * Use dict for lastargs * Updated tests to stop patch

added support for smappee water sensors (#12831)

fdf93d1

* added support for smappee water sensors * fixed lint error and wrong location_id * fixed lint error * Use string formatting

Fix Gogogate2 'available' attribute (#13728)

58f3690

* Fixed bug - unable to set base readaonly property * PR fixes * Added line

Line wrap to 79 chars

7fa34fe

Check 'home' for device_tracker for HomeKit

050d0d9

Refine params/const based on advice:

44436ce

Trim trailing whitespace and align hanging indent

8af2099

Trim trailing whitespace

43c5ea7

Wrap long line and add a blank line

260a47d

Revise const and coding style

73cac24

Remove should_callback=False for HAP-python 1.1.9

51d8e22

And refine BINARY_SENSOR_SERVICE_MAP.

Merge remote-tracking branch 'origin/Yonsm_dev' into Yonsm_dev

4e08e1e

# Conflicts: # homeassistant/components/homekit/type_sensors.py

Yonsm requested a review from a team as a code owner April 7, 2018 06:29

homeassistant added the cla-signed label Apr 7, 2018

Yonsm closed this Apr 7, 2018

Yonsm deleted the Yonsm_dev branch April 7, 2018 06:38

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

Uh oh!

Conversation

Yonsm commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

homeassistant commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

homeassistant commented Apr 5, 2018

Uh oh!

Yonsm commented Apr 5, 2018

Uh oh!

cdce8p left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdce8p left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yonsm Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Yonsm commented Apr 5, 2018 •

edited

Loading

Yonsm Apr 6, 2018 •

edited

Loading

Yonsm Apr 6, 2018 •

edited

Loading