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

Support binary_sensor and device_tracker for HomeKit #13701

Closed
wants to merge 31 commits into from
Closed

Support binary_sensor and device_tracker for HomeKit #13701

wants to merge 31 commits into from

Conversation

Yonsm
Copy link

@Yonsm Yonsm commented Apr 5, 2018

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.

Yonsm added 2 commits April 5, 2018 21:47
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'.
@Yonsm Yonsm requested a review from cdce8p as a code owner April 5, 2018 15:17
@homeassistant
Copy link
Contributor

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!

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])

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)


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']

Choose a reason for hiding this comment

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

line too long (121 > 79 characters)

'moisture': ('LeakSensor', 'LeakDetected'),
'smoke': ('SmokeSensor', 'SmokeDetected')}

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

Choose a reason for hiding this comment

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

line too long (117 > 79 characters)


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

Choose a reason for hiding this comment

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

line too long (80 > 79 characters)

@homeassistant
Copy link
Contributor

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
Copy link
Author

Yonsm commented Apr 5, 2018

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

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

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

@@ -92,6 +92,10 @@ def get_accessory(hass, state, aid, config):
return TYPES['HumiditySensor'](hass, state.entity_id, state.name,
aid=aid)

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

class BinarySensor(HomeAccessory):
"""Generate a BinarySensor accessory as binary sensor."""

def __init__(self, hass, state, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

entity_id instead of state

Copy link
Author

Choose a reason for hiding this comment

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

Fixed


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

Choose a reason for hiding this comment

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

Isn't necessary if entity_id is passed to constructor.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

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

Choose a reason for hiding this comment

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

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),
    # ...
}

Copy link
Author

Choose a reason for hiding this comment

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

Good coding style. I'll follow it.


device_class_key = 'homekit_device_class' \
if ('homekit_device_class' in state.attributes) \
else 'device_class'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Remove it at this time.

device_class_key = 'homekit_device_class' \
if ('homekit_device_class' in state.attributes) \
else 'device_class'
device_class = state.attributes.get(device_class_key)
Copy link
Member

Choose a reason for hiding this comment

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

Use ATTR_DEVICE_CLASS imported from homeassistant.const as key

Copy link
Author

Choose a reason for hiding this comment

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

Just follow it

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

Choose a reason for hiding this comment

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

trailing whitespace

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

Choose a reason for hiding this comment

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

trailing whitespace

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

Choose a reason for hiding this comment

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

trailing whitespace

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

Choose a reason for hiding this comment

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

trailing whitespace

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

Choose a reason for hiding this comment

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

trailing whitespace

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

Choose a reason for hiding this comment

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

trailing whitespace

self.entity_id = entity_id

service_map = {
DEVICE_CLASS_GAS:

Choose a reason for hiding this comment

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

trailing whitespace

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,

Choose a reason for hiding this comment

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

continuation line unaligned for hanging indent


from . import TYPES
from .accessories import HomeAccessory, add_preload_service
from .const import (
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,

Choose a reason for hiding this comment

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

trailing whitespace

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

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

class BinarySensor(HomeAccessory):
"""Generate a BinarySensor accessory as binary sensor."""

def __init__(self, hass, entity_id, name, attributes, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@Yonsm Yonsm Apr 6, 2018

Choose a reason for hiding this comment

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

Follow it

self.hass = hass
self.entity_id = entity_id

service_map = {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@Yonsm Yonsm Apr 6, 2018

Choose a reason for hiding this comment

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

Follow it

DEVICE_CLASS_SMOKE = 'smoke'

# #### Attributes ####
ATTR_DEVICE_CLASS = 'device_class'
Copy link
Member

Choose a reason for hiding this comment

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

ATTR_DEVICE_CLASS is already defined in homeassistant.const

Copy link
Author

Choose a reason for hiding this comment

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

Follow it

SERV_CONTACT_SENSOR = 'ContactSensor'
SERV_MOTION_SENSOR = 'MotionSensor'
SERV_LEAK_SENSOR = 'LeakSensor'
SERV_SMOKE_SENSOR = 'SmokeSensor'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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
DEVICE_CLASS_SMOKE:
(SERV_SMOKE_SENSOR, CHAR_SMOKE_DETECTED)}

@TYPES.register('BinarySensor')

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

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,

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

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)
Copy link
Member

Choose a reason for hiding this comment

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

ATTR_HOMEKIT_DEVICE_CLASS is an unused import.

Copy link
Author

Choose a reason for hiding this comment

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

done

DEVICE_CLASS_OPENING:
(SERV_CONTACT_SENSOR, CHAR_CONTACT_SENSOR_STATE),
DEVICE_CLASS_SMOKE:
(SERV_SMOKE_SENSOR, CHAR_SMOKE_DETECTED)}
Copy link
Member

Choose a reason for hiding this comment

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

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)}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -75,3 +85,53 @@ def update_state(self, entity_id=None, old_state=None, new_state=None):
self.char_humidity.set_value(humidity, should_callback=False)
_LOGGER.debug('%s: Percent set to %d%%',
self.entity_id, humidity)


# Binary sensor service map
Copy link
Member

Choose a reason for hiding this comment

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

You don't need that.

Copy link
Author

Choose a reason for hiding this comment

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

removed

self.entity_id = entity_id

attributes = hass.states.get(entity_id).attributes
device_class = attributes.get(ATTR_DEVICE_CLASS)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Author

@Yonsm Yonsm Apr 6, 2018

Choose a reason for hiding this comment

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

  1. Use attributes for homekit_device_class checking in future. But I'll follow your advice to remove it at this time.
  2. 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).

Copy link
Member

@cdce8p cdce8p Apr 6, 2018

Choose a reason for hiding this comment

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

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.

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) \
Copy link
Member

Choose a reason for hiding this comment

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

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

@cdce8p cdce8p self-assigned this Apr 6, 2018
# Conflicts:
#	homeassistant/components/homekit/const.py
cdce8p and others added 2 commits April 6, 2018 10:20
* Version bump to HAP-python==1.1.9

* Updated types and tests
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

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'.


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

Choose a reason for hiding this comment

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

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.

cgtobi and others added 17 commits April 6, 2018 21:48
* 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 Yonsm requested a review from a team as a code owner April 7, 2018 06:29
@Yonsm
Copy link
Author

Yonsm commented Apr 7, 2018

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

@Yonsm Yonsm closed this Apr 7, 2018
@Yonsm Yonsm deleted the Yonsm_dev branch April 7, 2018 06:38
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants