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 ElkM1 alarm/automation panel #16952

Merged
merged 17 commits into from Oct 7, 2018

Conversation

Projects
None yet
6 participants
@gwww
Contributor

gwww commented Sep 28, 2018

Description:

Support for the ElkM1 alarm/automation panel. This has been in development for around 6 months. A number of people have installed and have been testing the code as a custom_component.

Looking for feedback on how to improve.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

elkm1:
  host: elk://192.168.1.201:2101
  temperature_unit: celsius
  area:
    include: [1]
  counter:
    include: [1]
  keypad:
    include: [1-3]
  output:
    include: [9]
  task:
    include: [1]
  thermostat:
    enabled: false
  plc:
    include: [1-10, 13-14, 17-20, 22-24]
  setting:
    enabled: false
  zone:
    include: [1-5, 8, 10-31]

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.
Show resolved Hide resolved homeassistant/components/light/elkm1.py Outdated
Show resolved Hide resolved homeassistant/components/light/elkm1.py Outdated
@MartinHjelmare

Thanks for the PR. First strip this PR down to only add the elkm1 component and the alarm_control_panel platform. Further platforms should be added one platform and PR at a time. We want PRs to be as small as possible to make review and merge quicker.

I've commented on the component and the alarm_control_panel platform.

STATE_ALARM_DISARMED, STATE_ALARM_PENDING,
STATE_ALARM_TRIGGERED, STATE_UNKNOWN)
from homeassistant.components.elkm1 import (DOMAIN, create_elk_devices,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Import the elkm1 domain as another name. This platform belongs to the alarm_control_panel domain.

from homeassistant.components.elkm1 import (DOMAIN, create_elk_devices,
ElkDeviceBase,
register_elk_service)
from elkm1_lib.const import AlarmState, ArmedStatus, ArmLevel, ArmUpState

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

This won't work. Requirement library names must only be imported within functions.

This comment has been minimized.

@gwww

gwww Oct 2, 2018

Contributor

This does work. Is there a reason for importing only within functions?

This comment has been minimized.

@BioSehnsucht

BioSehnsucht Oct 3, 2018

Contributor

I understand the normal reasoning (that things would crash if trying to import a library not yet installed by HASS as a dependency). In this case it won't cause that problem (though it breaks with the established pattern) since trying to load the component by itself probably fails terribly.

The alarm_control_panel.elkm1 (and light and so on) never get loaded unless they were loaded by the top-level elkm1 component being configured, which means the library must be installed, which makes this safe (even if not "how it's done").

Even if we made things get imported only in functions, you couldn't load alarm_control_panel by itself without configuring the elkm1 platform.

If we have to move those imports into functions it means a lot of repetitive imports. If we have to, we have to, it's just annoying and ugly (and maybe impacts performance, but I imagine that after the first import the repeats are nearly zero cost).

This comment has been minimized.

@gwww

gwww Oct 3, 2018

Contributor

So what is established is:

  1. There's a rule.
  2. The rule doesn't take into account when a platform derives from a base that has a requirement.
  3. The Elk alarm_control_panel has no REQUIREMENTS because the REQUIREMENTS are in the base.
  4. The Elk base elkm1/__init__.py follows the rule because the code won't run if the rule isn't followed (and, yah, I stumbled on this and it took an hour to figure out).

What would have to happen to improve the rule?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

The rule is there for a reason. We can't guard users from configuring a platform directly instead of the component. If the users would do that, there would be an error, since the platform is imported before the dependencies are loaded and requirements installed.

This comment has been minimized.

@gwww

gwww Oct 3, 2018

Contributor

Thank-you. That helps.

DEPENDENCIES = [DOMAIN]
STATE_ALARM_ARMED_VACATION = 'armed_vacation'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

This isn't used.

DEPENDENCIES = [DOMAIN]
STATE_ALARM_ARMED_VACATION = 'armed_vacation'
STATE_ALARM_ARMED_HOME_INSTANT = 'armed_home_instant'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

This isn't used.

STATE_ALARM_ARMED_VACATION = 'armed_vacation'
STATE_ALARM_ARMED_HOME_INSTANT = 'armed_home_instant'
STATE_ALARM_ARMED_NIGHT_INSTANT = 'armed_night_instant'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

This isn't used.

This comment has been minimized.

@BioSehnsucht

BioSehnsucht Oct 3, 2018

Contributor

The extra states aren't used yet because we couldn't use them on the old regular HASS UI, without them being added to polymer, etc.

We've been trying to hammer out what additional states are needed so we can get them added to HASS properly.

See home-assistant/architecture#54 for more on that, if you're curious.

'config']['temperature_unit'] == 'celsius' else TEMP_FAHRENHEIT
self._unique_id = platform + '.elkm1_' + \
self._element.default_name('_').lower()
self.entity_id = self._unique_id

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Don't do this. The best thing is to let the core define the entity_id via the name property. If we need a specific entity_id, then define it properly using entity_id format imported from each base component, alarm_control_panel, light etc.

return self._unique_id
@property
def state(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Let each platform entity class define this.

This comment has been minimized.

@gwww

gwww Oct 2, 2018

Contributor

Can you explain why?

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 2, 2018

Member

Home assistant is built around base components that define base entity classes that define certain properties. Only some of those properties should be overwritten to complete the implementation. We want to keep the entity model strict for each component, so usually state is defined in the base component and then we let the subclass overwrite another property that is used in the state property. Keeping the model strict per entity gives a number of advantages. Eg being able to build a nice GUI.

This comment has been minimized.

@gwww

gwww Oct 3, 2018

Contributor

I'm confused (still).

What I understand from the explanation is that the state property is defined in the base entity classes. So for example in switch/__init__.py is how I interpret that. I don't see the state property in that file. So, that can't be the meaning.

Continuing to use switch as the example, there's ElkSwitch(ElkDeviceBase, ToggleEntity) and ToggleEntity(Entity). Four classes to choose from. Right now I define the state property is defined in ElkDeviceBase. If I move the property to ElkSwitch, how does that provide an advantage?

The disadvantage is I define the exact same property ~10 times, once for each Elk entity type.

If nothing else, perhaps one thing that can come from this discussion is that a FAQ could be started with patterns such as mine. I can't see any other way of knowing a rule such as this one.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 3, 2018

Member

For a switch entity you should not overwrite the state property at all. This is handled by the base switch class, which extends ToggleEntity. You should overwrite is_on to define state for a switch entity. Different components have different requirements on properties and methods that should be overwritten. So when defining a general class that you want to reuse for multiple platforms, you should only have the common properties and methods that are not specific to any component in that class.

Please read our dev docs:
https://developers.home-assistant.io/docs/en/architecture_entities.html
https://developers.home-assistant.io/docs/en/entity_index.html
https://developers.home-assistant.io/docs/en/entity_switch.html

If you are interested in exactly how it all works, please read the source.

return False
@property
def hidden(self):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Probably don't use this. This is going away.

"""The underlying element's attributes as a dict."""
attrs = {}
attrs['index'] = self._element.index + 1
attrs['state'] = self._state

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Don't duplicate state in state attributes.

"""Register callback for ElkM1 changes and update entity state."""
self._element.add_callback(self._element_callback)
self._element_callback(self._element, {})
self._hass.data[DOMAIN]['entities'][self.entity_id] = self

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

We are not allowed to store entities in a shared dictionary like this. Entities should only be accessed directly from within their respective platforms.

It's ok to store entities in each platform and also store the entity_ids separately in a shared dictionary in hass.data. Then use our dispatch helper to signal the entity methods.

See the tuya component and platforms for example.

This comment has been minimized.

@gwww

gwww Sep 30, 2018

Contributor

Can you point me at the dispatch helper. I didn't see an example of that in tuya. I think all that I care about is that I can call an entity's method on a service call. If the dispatch helper will do that then I'm all good!

@frenck frenck added the docs-missing label Sep 30, 2018

@frenck

This comment has been minimized.

Member

frenck commented Sep 30, 2018

Could not find a related documentation PR. Added docs-missing label.

@gwww

This comment has been minimized.

Contributor

gwww commented Sep 30, 2018

I appreciate the comments! I'm looking to be platinum :) so I'll be working to fix them all plus some :)

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 2, 2018

Checking, what is the protocol for using "Resolve conversation"? Do you click that when the comment has been resolved? Thx

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 2, 2018

I prefer to let github automatically hide conversations when new commits are pushed that change the corresponding lines. Then I'll go through any remaining conversations and resolve them manually if they have been resolved.

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Oct 3, 2018

At this point it feels like we're arguing more than trying to get this PR merged. I'm happy to answer questions but defending the home assistant architecture is not what this PR review should be about. Please continue that discussion in the dev discord chat or in our architecture repo.

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 3, 2018

@MartinHjelmare You're right. I apologize. I'll do my best to make be better.

for item, max_ in configs.items():
config[item] = {}
(config[item]['enabled'], config[item]['included']) = \
parse_config(item, max_)

This comment has been minimized.

@houndci-bot

houndci-bot Oct 6, 2018

continuation line over-indented for visual indent

host = config_raw[CONF_HOST]
username = config_raw.get(CONF_USERNAME)
password = config_raw.get(CONF_PASSWORD)
if host.startswith('elks:') and (username is None or password is None):

This comment has been minimized.

@houndci-bot

houndci-bot Oct 6, 2018

trailing whitespace

})
}, extra=vol.ALLOW_EXTRA)
SUPPORTED_DOMAINS = ['alarm_control_panel',]

This comment has been minimized.

@houndci-bot

houndci-bot Oct 6, 2018

missing whitespace after ','

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 6, 2018

I believe that I have address all comments except the voluptuous parsing. I don't know any way to do it differently given there is a dependance on elkm1_lib to properly parse. I did tune up code, changing error logs to vol.Invalid exceptions.

}
if self._element.alarm_state is None:
self._state = STATE_UNKNOWN

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Use None to represent unknown state. The base entity class will handle this and convert to the correct state name.

for service in _arm_services():
hass.services.async_register(
ELK_DOMAIN, service, _arm_service, alarm.ALARM_SERVICE_SCHEMA)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

We probably should register these services under the alarm_control_panel domain, and add the platform name as a prefix to the service name.

If a service is only relevant for a platform it probably should be registered under the domain of the platform. But let me know what you think. You know the integration best.

async_dispatcher_send(hass, SIGNAL_DISPLAY_MESSAGE, *args)
hass.services.async_register(
ELK_DOMAIN, 'alarm_display_message',

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

See above.

ELK_DOMAIN, 'alarm_display_message',
_display_message_service, DISPLAY_MESSAGE_SERVICE_SCHEMA)
return True

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Nothing is checking this return value, so we can remove the statement.

from elkm1_lib.const import ArmedStatus
ELK_STATE_TO_HASS_STATE = {
ArmedStatus.DISARMED.value: STATE_ALARM_DISARMED,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

We only use one whitespace after the colon.

host = config_raw[CONF_HOST]
username = config_raw.get(CONF_USERNAME)
password = config_raw.get(CONF_PASSWORD)
if host.startswith('elks:') and (username is None or password is None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Move this check to the config validation schema. We can break it out into validator function.

This comment has been minimized.

@gwww

gwww Oct 6, 2018

Contributor

Would love some help on this. I spent a couple of hours and could not figure out how to make voluptuous handle making something Required based on another value matching a pattern.

I looked as using Match to no avail.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Look at the persistence file validator in the mysensors component for example.

description: Up to 16 characters of text (truncated if too long). Default blank.
example: the universe, and everything.
sensor_speak_word:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

This isn't used.

description: Word number to speak.
example: 458
sensor_speak_phrase:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

This isn't used.

def parse_value(val, max_):
"""Parse a value as an int or housecode."""
i = int(val) if val.isdigit() else (housecode_to_index(val) + 1)
if i < 1 or i > max_:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Can we use vol.Range instead?

This comment has been minimized.

@gwww

gwww Oct 6, 2018

Contributor

No. The problem with using voluptuous for this bit of config is that I don't know max until my REQUIREMENT is loaded. Since REQUIREMENTS aren't loaded until after voluptuous is run I can't think of any way to use voluptuous.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

I mean just replace the code here with a schema and use vol.Range in the schema, and call the schema with the values. Since we will need to catch a voluptuous error later anyway, it could fit nicely.

elk.connect()
hass.data[DOMAIN] = {'elk': elk, 'config': config,
'entities': {}, 'keypads': {}}

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Are we still using 'entities' and 'keypads' keys?

This comment has been minimized.

@gwww

gwww Oct 6, 2018

Contributor

entities no.

keypads will be used as soon as the keypad sensor is submitted as a PR

gwww added some commits Oct 6, 2018

def parse_value(val, max_):
"""Parse a value as an int or housecode."""
i = int(val) if val.isdigit() else (housecode_to_index(val) + 1)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Break out housecode_to_index(val) + 1 into a validator function, is_housecode. Then do eg:

schema = vol.Schema(vol.Any(vol.Coerce(int), is_housecode))
val = schema(val)

Expand the schema as needed before using it.

entity_ids = service.data.get(ATTR_ENTITY_ID, [])
arm_level = _arm_services().get(service.service)
code = service.data.get('code')
if arm_level and code:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

This should be validated by the service schema. arm_level is always there since that's the name of the service. Make code required in the schema if that's needed.

code = service.data.get('code')
if arm_level and code:
args = (entity_ids, arm_level, code)
async_dispatcher_send(hass, SIGNAL_ARM_ENTITY, *args)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Instead of signaling all entities, connect each entity to a specific signal by adding the entity_id to the signal string.

for keypad in self._elk.keypads:
keypad.add_callback(self._watch_keypad)
async_dispatcher_connect(
self.hass, SIGNAL_ARM_ENTITY, self._arm_service)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

See above.

async def _arm_service(self, entity_ids, arm_level, code):
if self.entity_id in entity_ids:
self._element.arm(arm_level, int(code))

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Move integer copy to service schema.

host = config_raw[CONF_HOST]
username = config_raw.get(CONF_USERNAME)
password = config_raw.get(CONF_PASSWORD)
if host.startswith('elks:') and (username is None or password is None):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 6, 2018

Member

Look at the persistence file validator in the mysensors component for example.

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 6, 2018

Still can't figure out the elks:// using voluptuous.

I want the config to be either:

host: elks://foo
username: bar
password: baz

Or

host: elk://foo

I cannot find a way to get first one to work where username/password are required for elks://

raise vol.Invalid("Invalid range {}".format(rng))
values[rng[0]-1:rng[1]] = [set_to] * (rng[1] - rng[0] + 1)
conf = hass_config.get(DOMAIN)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 7, 2018

Member
conf = hass_config[DOMAIN]
for item, max_ in configs.items():
config[item] = {'enabled': conf[item][CONF_ENABLED],
'included': [len(conf[item]['include']) == 0] * max_}

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 7, 2018

Member
{
    ...,
    'included': [not conf[item]['include']] * max_,
}

gwww added some commits Oct 7, 2018

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

@@ -0,0 +1,53 @@
# Describes the format for available ElkM1 alarm control panel services
elkm1_alarm_arm_vacation:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 7, 2018

Member

Move these to the alarm control panel services.yaml.

@MartinHjelmare

Nice!

I think the config parsing is hard to follow, but I don't have a better suggestion at the moment.

Can be merged when docs PR is linked and build passes.

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 7, 2018

Thank you.

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 7, 2018

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 7, 2018

You should probably remote the climate, light, switch, and sensor labels. Those PRs coming real soon now.

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

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

@MartinHjelmare MartinHjelmare changed the title from Add support for ElkM1 alarm/automation panel. to Add support for ElkM1 alarm/automation panel Oct 7, 2018

@MartinHjelmare MartinHjelmare merged commit 06a64c0 into home-assistant:dev Oct 7, 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.05%) to 93.625%
Details

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

@gwww gwww deleted the gwww:elkm1 branch Oct 7, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

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