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 support for Lupusec alarm control panel #17691

Merged
merged 11 commits into from Nov 7, 2018

Conversation

@majuss
Contributor

majuss commented Oct 22, 2018

Description:

Adds support for the alarm control panel of the german company Lupus electronics (second try, I reseted everything so here is my new PR).

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

Example entry for configuration.yaml (if applicable):

lupusec:
  username: user 
  password: 'greatpassword'
  ip_address: 192.168.178.35

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.

@majuss majuss referenced this pull request Oct 22, 2018

Merged

Adds documentation for Lupusec alarm control panel #6601

2 of 2 tasks complete
@homeassistant

This comment has been minimized.

homeassistant commented Oct 22, 2018

Hi @majuss,

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!

Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/switch/lupusec.py Outdated

@homeassistant homeassistant added cla-signed and removed cla-needed labels Oct 22, 2018

@majuss

This comment has been minimized.

Contributor

majuss commented Oct 22, 2018

@MartinHjelmare thank you for the feedback I will work on it :)!

@houndci-bot

Some files could not be reviewed due to errors:

Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/linters/.local/bin/flake8", line 7, in 
    from flake8.main.cli import main
ModuleNotFoundError: No module named 'flake8'
@majuss

This comment has been minimized.

Contributor

majuss commented Oct 29, 2018

@MartinHjelmare It would be nice if you could look over it again :) Sorry to take so much time from you, but lupupy and the Lupusec hass integration are my first python projects. Thank you so much for all the feedback.

Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
@property
def device_class(self):
"""Return the class of the binary sensor."""
return self._device.type

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 29, 2018

Member

Shouldn't we map the lupusec device type to the home assistant device class? There are only so many home assistant device classes, so returning something else than those could have unwanted effects. I'd at least check if the lupusec device type is in the list of home assistant device classes, otherwise return None.

This comment has been minimized.

@majuss

majuss Oct 31, 2018

Contributor

Now: return self._device.generic_type

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member
from homeassistant.components.binary_sensor import DEVICE_CLASSES

...

    @property
    def device_class(self):
        """Return the class of the binary sensor."""
        if self._device.generic_type in DEVICE_CLASSES:
            return self._device.generic_type
        return None
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py Outdated
try:
hass.data[DOMAIN] = LupusecSystem(username, password, ip_address, name)
except LupusecException as ex:
_LOGGER.warning(ex)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 29, 2018

Member

We should validate that credentials are correct, otherwise log error and return False from component setup.

This comment has been minimized.

@majuss

majuss Oct 31, 2018

Contributor

Added a notification and false returning.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

Log error instead of warning.

@majuss

This comment has been minimized.

Contributor

majuss commented Oct 31, 2018

@MartinHjelmare please take a look again :)

@home-assistant home-assistant deleted a comment from houndci-bot Oct 31, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Oct 31, 2018

class LupusecAlarm(LupusecDevice, AlarmControlPanel):
"""An alarm_control_panel implementation for Lupusec."""
def __init__(self, data, device, name):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

name isn't used.

This comment has been minimized.

@MartinHjelmare
Show resolved Hide resolved homeassistant/components/lupusec.py
}),
}, extra=vol.ALLOW_EXTRA)
SCAN_INTERVAL = timedelta(seconds=2)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

This doesn't have an effect when just defining it in a component. In platforms it would have an effect and set the default scan interval for polling entities. Here it doesn't do anything since we're not actively using it.

try:
hass.data[DOMAIN] = LupusecSystem(username, password, ip_address, name)
except LupusecException as ex:
_LOGGER.warning(ex)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

Log error instead of warning.

devices.append(LupusecSwitch(data, device))
data.devices.extend(devices)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

Remove this.

for device in data.lupusec.get_devices(generic_type=device_types):
devices.append(LupusecBinarySensor(data, device))
data.devices.extend(devices)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

Remove this.

import lupupy
self.lupusec = lupupy.Lupusec(username, password, ip_address)
self.name = name
self.devices = []

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 31, 2018

Member

Remove this. We're not using it.

Show resolved Hide resolved homeassistant/components/binary_sensor/lupusec.py

@MartinHjelmare MartinHjelmare changed the title from Adds support for Lupusec alarm control panel - new try to Add support for Lupusec alarm control panel Oct 31, 2018

https://home-assistant.io/components/lupusec
"""
from datetime import timedelta

This comment has been minimized.

@houndci-bot

houndci-bot Nov 1, 2018

'datetime.timedelta' imported but unused

from homeassistant.components.lupusec import (LupusecDevice,
DOMAIN as LUPUSEC_DOMAIN)
from homeassistant.components.binary_sensor import BinarySensorDevice, DEVICE_CLASSES

This comment has been minimized.

@houndci-bot

houndci-bot Nov 1, 2018

line too long (85 > 79 characters)

@majuss

This comment has been minimized.

Contributor

majuss commented Nov 1, 2018

@MartinHjelmare thanks again for all the feedback. Keep up the good work and keep the code base clean!

majuss added some commits Nov 1, 2018

if self._device.generic_type in DEVICE_CLASSES:
return self._device.generic_type
else:
_LOGGER.error('Binary sensor device class not known')

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 1, 2018

Member

We don't want side effects in entity state properties.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 1, 2018

Member

Explicitly return None in this case. We want to be consistent when returning values.

This comment has been minimized.

@majuss

majuss Nov 1, 2018

Contributor

So I add a line under the LOGGER with return None?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 1, 2018

Member

No the logging should be removed. We don't allow side effects in entity state properties. If you copy the example I posted, that should be ok.

This comment has been minimized.

@majuss

majuss Nov 3, 2018

Contributor

Ok thanks, I was not sure what a side effect is. When I'am writing it like that:

@property
    def device_class(self):
        """Return the class of the binary sensor."""
        if self._device.generic_type not in DEVICE_CLASSES:
            return False
        else:
            return self._device.generic_type

It wont build sccuessfully since the linter says "unnecessary else after return."

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 4, 2018

Member

Return None if device class not found. Remove else and outdent the last return.

https://en.m.wikipedia.org/wiki/Side_effect_(computer_science)

@home-assistant home-assistant deleted a comment from houndci-bot Nov 4, 2018

def device_class(self):
"""Return the class of the binary sensor."""
if self._device.generic_type not in DEVICE_CLASSES:
return False

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 4, 2018

Member

Return None.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Nov 4, 2018

Two comments left to fix.

@MartinHjelmare

Some more fixes needed.

class LupusecAlarm(LupusecDevice, AlarmControlPanel):
"""An alarm_control_panel implementation for Lupusec."""
def __init__(self, data, device):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

We can remove this. The parent class init will be called.

"""Set up an alarm control panel for a Lupusec device."""
data = hass.data[LUPUSEC_DOMAIN]
alarm_devices = [LupusecAlarm(data, data.lupusec.get_alarm(), data.name)]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Update the call according to the new class signature.

Show resolved Hide resolved homeassistant/components/alarm_control_panel/lupusec.py
Show resolved Hide resolved homeassistant/components/binary_sensor/lupusec.py
https://home-assistant.io/components/switch.lupusec/
"""
import logging

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Remove blank line.

from homeassistant.components.alarm_control_panel import AlarmControlPanel
from homeassistant.components.lupusec import DOMAIN as LUPUSEC_DOMAIN
from homeassistant.components.lupusec import LupusecDevice

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Remove blank line.

https://home-assistant.io/components/binary_sensor.lupusec/
"""
import logging

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 5, 2018

Member

Remove blank line.

Show resolved Hide resolved homeassistant/components/switch/lupusec.py
data = hass.data[LUPUSEC_DOMAIN]
alarm_devices = [LupusecDevice(data, data.lupusec.get_alarm())]

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 6, 2018

Member

This should use LupusecAlarm.

class LupusecAlarm(LupusecDevice, AlarmControlPanel):
"""An alarm_control_panel implementation for Lupusec."""
# def __init__(self, data, device):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Nov 6, 2018

Member

Remove commented code.

This comment has been minimized.

@majuss

majuss Nov 6, 2018

Contributor

sorry pushed the code before testing

@MartinHjelmare

Looks good!

@MartinHjelmare MartinHjelmare merged commit ec732c8 into home-assistant:dev Nov 7, 2018

4 of 5 checks passed

coverage/coveralls Coverage decreased (-0.5%) to 93.053%
Details
Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Nov 7, 2018

zxdavb added a commit to zxdavb/home-assistant that referenced this pull request Nov 13, 2018

Add support for Lupusec alarm control panel (home-assistant#17691)
* Adds support for Lupusec alarm control panel

* fixed various mostly cosmetic issues

* fixed generic type of binary sensors

* fixed some formatting; removed scan interval completely -> defaults now to 2 secs

* removed unused data caches; added check if binary sensor class exists

* cosmetics

* generic type fix

* small fix

* small fixes

* guard clause added

* small fixes

@balloob balloob referenced this pull request Nov 29, 2018

Merged

0.83 #18776

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