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

Add Logi Circle component, camera and sensor platform #16540

Merged
merged 7 commits into from Sep 21, 2018

Conversation

Projects
None yet
4 participants
@evanjd
Contributor

evanjd commented Sep 10, 2018

Description:

This adds the logi_circle component, camera and sensor platforms for Logi Circle 1st and 2nd gen cameras.

Screenshot of working configuration (2x Logi Cameras shown)
screen shot 2018-09-13 at 8 40 44 pm

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

Example entry for configuration.yaml (if applicable):

logi_circle:
  username: YOUR_USERNAME
  password: YOUR_PASSWORD

camera:
  - platform: logi_circle

sensor:
  - platform: logi_circle

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

evanjd added some commits Aug 22, 2018

Integrated with Logo Circle API’s feature detection to exclude sensor…
…s not supported by device. Added services for recording livestream and taking a snapshot from the livestream.
Migrated livestream snapshot and recording functionality out of home …
…assistant components and into the logi circle API wrapper. Added services.yaml entries for logi services.
Added new Logi sensor types, updated to latest version of `logi_circl…
…e` and tidy up in preparation for pull request.

- Renamed `logi_set_mode` to `logi_set_config`.
- Live stream recording and snapshot methods now respect whitelisted path configuration.
- Added `streaming_mode` and `speaker_volume` sensors.
- Moved model-specific turn on/off logic to `logi_circle` library.
@homeassistant

This comment has been minimized.

homeassistant commented Sep 10, 2018

Hi @evanjd,

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!

asyncio.shield(self._camera.record_livestream(
stream_file, timedelta(seconds=duration)), loop=self.hass.loop)

async def livestream_snapshot(self, filename):

This comment has been minimized.

@evanjd

evanjd Sep 10, 2018

Contributor

A quick explanation on why this was implemented when there's already a generic snapshot service for cameras:

When not live streaming, Logi cameras (particularly battery-powered devices) go into a low power state, only taking a snapshot every 1-5 minutes. This cached image is what the async_camera_image method uses.

This method exists if you really need to wake the camera from sleep immediately to take a snapshot. I didn't want this to be the default source for camera stills as it increases battery drain.

@balloob

This comment has been minimized.

Member

balloob commented Sep 13, 2018

Can you rename the component to logi_circle

@evanjd

This comment has been minimized.

Contributor

evanjd commented Sep 13, 2018

@balloob Done.


from homeassistant.helpers import config_validation as cv
from homeassistant.components.logi_circle import (
DOMAIN, CONF_ATTRIBUTION)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

This platform belongs to the camera domain. Import the logi_circle domain as another name.

from homeassistant.components.logi_circle import (
    DOMAIN as LOGI_CIRCLE_DOMAIN, ...
DOMAIN, CONF_ATTRIBUTION)
from homeassistant.components.camera import (
Camera, PLATFORM_SCHEMA, CAMERA_SERVICE_SCHEMA, SUPPORT_ON_OFF,
ATTR_ENTITY_ID, ATTR_FILENAME, DOMAIN as CAMERA_DOMAIN)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

This domain should be imported as domain. 😉

})


async def async_setup_platform(hass,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Please break the line after the first parenthesis and then fill out as far as possible.


def __init__(self, hass, camera, device_info):
"""Initialize Logi Circle camera."""
super(LogiCam, self).__init__()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member
super().__init__()
'firmware': self._camera.firmware,
'generation': self._camera.model_generation,
'ip_address': self._camera.ip_address,
'mac_address': self._camera.mac_address,

This comment has been minimized.

self._icon = 'mdi:{}'.format(SENSOR_TYPES.get(self._sensor_type)[2])
self._name = "{0} {1}".format(
self._camera.name, SENSOR_TYPES.get(self._sensor_type)[0])
self._state = STATE_UNKNOWN

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Use None to represent unknown state.

ATTR_ATTRIBUTION: CONF_ATTRIBUTION,
'battery_saving_mode': ('on' if self._camera.battery_saving
else 'off'),
'firmware': self._camera.firmware,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

See above about appropriate state attributes.

'firmware': self._camera.firmware,
'generation': self._camera.model_generation,
'ip_address': self._camera.ip_address,
'mac_address': self._camera.mac_address,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Define unique id property with this.

def icon(self):
"""Icon to use in the frontend, if any."""
if (self._sensor_type == 'battery_level' and
self._state != STATE_UNKNOWN):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Don't use STATE_UNKNOWN.

else:
state = getattr(self._camera, self._sensor_type, STATE_UNKNOWN)
if isinstance(state, bool):
self._state = 'on' if state is True else 'off'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 17, 2018

Member

Use STATE_ON and STATE_OFF from const.py. But probably we should instead make a binary sensor platform for that type.

This comment has been minimized.

@evanjd

evanjd Sep 18, 2018

Contributor

The two binary sensors the camera exposes through the sensor platform are:

  • streaming mode: determines whether the camera can return video/stills and whether it will record activities (motion, etc)
  • privacy mode: determines whether the camera will record activities if they're detected

Neither of these seem to fit neatly into any existing binary sensor class, which is why I opted to make them sensors instead.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 18, 2018

Member

We can define binary sensors without a specific device class. If a sensor only has a binary state, it belongs with the binary sensor platform.

This comment has been minimized.

@evanjd

evanjd Sep 18, 2018

Contributor

Ah, my bad. As you advised, I'll migrate these sensors to a binary sensor in my next PR.

Updates based on PR feedback
* Added timeout of 15s to logi_circle initial setup requests (login and grabbing cameras).
* Added unique ID (uses MAC address for camera platform, MAC address + sensor type for sensor platform).
* Added battery level and battery charging attributes to camera.
* Removed static attributes from device_state_attributes.
* Replaced STATE_UNKNOWN with None, replaced ‘on’ & ‘off’ with STATE_ON and STATE_OFF.
* Removed redundant SCAN_INTERVAL in sensor, removed redundant hass param from async_setup_platform in camera and sensor.
* Style tweaks.
@MartinHjelmare

Looks good!

I would suggest moving the sensors that have binary states to the binary_sensor platform. That can happen in another PR though.

@evanjd

This comment has been minimized.

Contributor

evanjd commented Sep 18, 2018

@MartinHjelmare Thanks for the review!

@balloob balloob merged commit aeaf694 into home-assistant:dev Sep 21, 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 decreased (-0.07%) to 93.696%
Details

@wafflebot wafflebot bot removed the in progress label Sep 21, 2018

@balloob balloob referenced this pull request Sep 28, 2018

Merged

0.79.0 #16940

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