-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work. Requirement library names must only be imported within functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does work. Is there a reason for importing only within functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what is established is:
- There's a rule.
- The rule doesn't take into account when a platform derives from a base that has a requirement.
- The Elk alarm_control_panel has no REQUIREMENTS because the REQUIREMENTS are in the base.
- 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank-you. That helps.
|
||
DEPENDENCIES = [DOMAIN] | ||
|
||
STATE_ALARM_ARMED_VACATION = 'armed_vacation' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used.
DEPENDENCIES = [DOMAIN] | ||
|
||
STATE_ALARM_ARMED_VACATION = 'armed_vacation' | ||
STATE_ALARM_ARMED_HOME_INSTANT = 'armed_home_instant' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let each platform entity class define this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could not find a related documentation PR. Added |
I appreciate the comments! I'm looking to be platinum :) so I'll be working to fix them all plus some :) |
Checking, what is the protocol for using "Resolve conversation"? Do you click that when the comment has been resolved? Thx |
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. |
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. |
@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_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace
}) | ||
}, extra=vol.ALLOW_EXTRA) | ||
|
||
SUPPORTED_DOMAINS = ['alarm_control_panel',] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing whitespace after ','
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
} | ||
|
||
if self._element.alarm_state is None: | ||
self._state = STATE_UNKNOWN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
ELK_DOMAIN, 'alarm_display_message', | ||
_display_message_service, DISPLAY_MESSAGE_SERVICE_SCHEMA) | ||
|
||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this check to the config validation schema. We can break it out into validator function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't used.
description: Word number to speak. | ||
example: 458 | ||
|
||
sensor_speak_phrase: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use vol.Range
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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': {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still using 'entities'
and 'keypads'
keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entities no.
keypads will be used as soon as the keypad sensor is submitted as a PR
|
||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at the persistence file validator in the mysensors component for example.
Still can't figure out the elks:// using voluptuous. I want the config to be either:
Or
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conf = hass_config[DOMAIN]
|
||
for item, max_ in configs.items(): | ||
config[item] = {'enabled': conf[item][CONF_ENABLED], | ||
'included': [len(conf[item]['include']) == 0] * max_} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
...,
'included': [not conf[item]['include']] * max_,
}
@@ -0,0 +1,53 @@ | |||
# Describes the format for available ElkM1 alarm control panel services | |||
|
|||
elkm1_alarm_arm_vacation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these to the alarm control panel services.yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thank you. |
You should probably remote the climate, light, switch, and sensor labels. Those PRs coming real soon now. |
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):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices: