Support multiple Elk instances #22573
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
houndci-bot
reviewed
Mar 30, 2019
| elk_datas = hass.data[ELK_DOMAIN] | ||
| for prefix, elk_data in elk_datas.items(): | ||
| elk = elk_data['elk'] | ||
| entities = create_elk_entities(elk_data, elk.outputs, 'output', ElkOutput, []) |
There was a problem hiding this comment.
line too long (86 > 79 characters)
| @@ -1,26 +1,29 @@ | |||
| """Support for control of ElkM1 sensors.""" | |||
| import logging | |||
| elk_datas = hass.data[ELK_DOMAIN] | ||
| for prefix, elk_data in elk_datas.items(): | ||
| elk = elk_data['elk'] | ||
| entities = create_elk_entities(elk_data, elk.tasks, 'task', ElkTask, []) |
There was a problem hiding this comment.
line too long (80 > 79 characters)
| for prefix, elk_data in elk_datas.items(): | ||
| elk = elk_data['elk'] | ||
| async_add_entities( | ||
| create_elk_entities(elk_data, elk.lights, 'plc', ElkLight, []), True) |
There was a problem hiding this comment.
line too long (81 > 79 characters)
| elk_datas = hass.data[ELK_DOMAIN] | ||
| for prefix, elk_data in elk_datas.items(): | ||
| elk = elk_data['elk'] | ||
| entities = create_elk_entities(elk_data, elk.areas, 'area', ElkArea, []) |
There was a problem hiding this comment.
line too long (80 > 79 characters)
| device = devices.get(prefix, None) | ||
| if device != None: | ||
| if prefix != "": | ||
| _LOGGER.error("Need to use a prefix configuration command on second and later elk m1s") |
There was a problem hiding this comment.
line too long (103 > 79 characters)
|
|
||
| prefix = conf.get(CONF_PREFIX,"") | ||
| device = devices.get(prefix, None) | ||
| if device != None: |
There was a problem hiding this comment.
comparison to None should be 'if cond is not None:'
| _LOGGER.error("Config item: %s; %s", item, err) | ||
| return False | ||
|
|
||
| prefix = conf.get(CONF_PREFIX,"") |
| 'included': [not conf[item]['include']] * max_} | ||
| try: | ||
| _included(conf[item]['include'], True, config[item]['included']) | ||
| _included(conf[item]['exclude'], False, config[item]['included']) |
There was a problem hiding this comment.
line too long (81 > 79 characters)
| config[item] = {'enabled': conf[item][CONF_ENABLED], | ||
| 'included': [not conf[item]['include']] * max_} | ||
| try: | ||
| _included(conf[item]['include'], True, config[item]['included']) |
There was a problem hiding this comment.
line too long (80 > 79 characters)
2 tasks
houndci-bot
reviewed
Apr 1, 2019
| _LOGGER.error("Need to use a prefix configuration command " + | ||
| "on second and later elk m1s") | ||
| else: | ||
| _LOGGER.error("Duplicate prefix on elk prefix: %s", m1s) |
rohankapoorcom
requested changes
Apr 1, 2019
|
I like that idea -- as I wrote before, I modeled this change after
Doorbird's multi-device support (
https://www.home-assistant.io/components/doorbird/). hass arguably should
be consistent on whether the intervening tag is required, but I prefer to
avoid impacting all the singleton elkm1 users so I will switch to zm's
approach.
…On Sun, Mar 31, 2019 at 11:44 PM Rohan Kapoor ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In homeassistant/components/elkm1/__init__.py
<#22573 (comment)>
:
> - vol.Optional(CONF_TEMPERATURE_UNIT, default='F'):
- cv.temperature_unit,
- vol.Optional(CONF_AREA, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_COUNTER, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_KEYPAD, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_OUTPUT, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_PLC, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_SETTING, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_TASK, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_THERMOSTAT, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- vol.Optional(CONF_ZONE, default={}): CONFIG_SCHEMA_SUBDOMAIN,
- },
- _host_validator,
- )
+ DOMAIN: vol.Schema({
+ vol.Required(CONF_DEVICES): vol.All(cv.ensure_list, [DEVICE_SCHEMA])
I would recommend removing the key CONF_DEVICES to remove the breaking
change (similar to how the ZoneMinder component is implemented
<https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/zoneminder/__init__.py>
).
This would then look like:
CONFIG_SCHEMA = vol.Schema({
DOMAIN: vol.All(cv.ensure_list, [DEVICE_SCHEMA])
}, extra=vol.ALLOW_EXTRA)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22573 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABbjzcn4GZCMctnP7QjIEyuvgdLGSoNFks5vcarQgaJpZM4cT-8->
.
|
|
Rohan, I don't see in the zoneminder documentation where it makes clear
that structurally the host configuration section can be repeated. Having
the devices section means that I can document:
devices:
description:
required: true
type: List
And then put the rest of the keys in the next section. What am I missing?
Thanks,
Greg
…On Mon, Apr 1, 2019 at 7:30 AM Greg Badros ***@***.***> wrote:
I like that idea -- as I wrote before, I modeled this change after
Doorbird's multi-device support (
https://www.home-assistant.io/components/doorbird/). hass arguably
should be consistent on whether the intervening tag is required, but I
prefer to avoid impacting all the singleton elkm1 users so I will switch to
zm's approach.
On Sun, Mar 31, 2019 at 11:44 PM Rohan Kapoor ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
> ------------------------------
>
> In homeassistant/components/elkm1/__init__.py
> <#22573 (comment)>
> :
>
> > - vol.Optional(CONF_TEMPERATURE_UNIT, default='F'):
> - cv.temperature_unit,
> - vol.Optional(CONF_AREA, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_COUNTER, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_KEYPAD, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_OUTPUT, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_PLC, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_SETTING, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_TASK, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_THERMOSTAT, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - vol.Optional(CONF_ZONE, default={}): CONFIG_SCHEMA_SUBDOMAIN,
> - },
> - _host_validator,
> - )
> + DOMAIN: vol.Schema({
> + vol.Required(CONF_DEVICES): vol.All(cv.ensure_list, [DEVICE_SCHEMA])
>
> I would recommend removing the key CONF_DEVICES to remove the breaking
> change (similar to how the ZoneMinder component is implemented
> <https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/zoneminder/__init__.py>
> ).
>
> This would then look like:
>
> CONFIG_SCHEMA = vol.Schema({
> DOMAIN: vol.All(cv.ensure_list, [DEVICE_SCHEMA])
> }, extra=vol.ALLOW_EXTRA)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#22573 (review)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABbjzcn4GZCMctnP7QjIEyuvgdLGSoNFks5vcarQgaJpZM4cT-8->
> .
>
|
|
I just read through the documentation again for ZM and it's definitely not obvious that multiple hosts can be supported. I originally got this approach from @MartinHjelmare (to avoid the breaking change when adding multiple host support). He may have some suggestions on how to best document this. |
|
Please ask the question about the docs in the docs PR. The docs team will probably have a suggestion. So far we've just relied on the example I think. |
MartinHjelmare
requested changes
Apr 2, 2019
|
Sure; I've made the changes but don't have time to go through a full test
iteration now and will get to this by Thursday. Thx!
…On Mon, Apr 1, 2019 at 6:14 PM Martin Hjelmare ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In homeassistant/components/elkm1/alarm_control_panel.py
<#22573 (comment)>
:
> @@ -38,9 +38,12 @@
if discovery_info is None:
return
- elk = hass.data[ELK_DOMAIN]['elk']
- entities = create_elk_entities(hass, elk.areas, 'area', ElkArea, [])
- async_add_entities(entities, True)
+ elk_datas = hass.data[ELK_DOMAIN]
+ for _, elk_data in elk_datas.items():
+ elk = elk_data['elk']
+ entities = create_elk_entities(elk_data, elk.areas,
+ 'area', ElkArea, [])
+ async_add_entities(entities, True)
Collect entities in a list first, then make a single call to
async_add_entities.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22573 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABbjzef380W-Nt0Edj5BspoFdC2LxsBnks5vcq8AgaJpZM4cT-8->
.
|
|
Just pushed these changes to my fork -- thanks for the suggestion. What
are next steps? I still need to resolve the documentation issue about
using devices: in the YAML file or not. Any other changes or improvements
before this can be accepted?
…On Tue, Apr 2, 2019 at 10:58 PM Greg Badros ***@***.***> wrote:
Sure; I've made the changes but don't have time to go through a full test
iteration now and will get to this by Thursday. Thx!
On Mon, Apr 1, 2019 at 6:14 PM Martin Hjelmare ***@***.***>
wrote:
> ***@***.**** requested changes on this pull request.
> ------------------------------
>
> In homeassistant/components/elkm1/alarm_control_panel.py
> <#22573 (comment)>
> :
>
> > @@ -38,9 +38,12 @@
> if discovery_info is None:
> return
>
> - elk = hass.data[ELK_DOMAIN]['elk']
> - entities = create_elk_entities(hass, elk.areas, 'area', ElkArea, [])
> - async_add_entities(entities, True)
> + elk_datas = hass.data[ELK_DOMAIN]
> + for _, elk_data in elk_datas.items():
> + elk = elk_data['elk']
> + entities = create_elk_entities(elk_data, elk.areas,
> + 'area', ElkArea, [])
> + async_add_entities(entities, True)
>
> Collect entities in a list first, then make a single call to
> async_add_entities.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#22573 (review)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABbjzef380W-Nt0Edj5BspoFdC2LxsBnks5vcq8AgaJpZM4cT-8->
> .
>
|
| elk = elkm1.Elk({'url': conf[CONF_HOST], 'userid': | ||
| conf[CONF_USERNAME], | ||
| 'password': conf[CONF_PASSWORD]}) | ||
| elk.connect() |
There was a problem hiding this comment.
Could the connect fail if credentials are incorrect? Do we need to catch that?
There was a problem hiding this comment.
Handled by the library.
There was a problem hiding this comment.
We should fail set up if credentials are incorrect. Or at least if all devices have incorrect credentials.
|
Please update the PR description after the change in direction regarding the config schema. |
|
Great, thanks for the careful read and I'll make these changes this
weekend.
…On Fri, Apr 5, 2019 at 3:53 PM Martin Hjelmare ***@***.***> wrote:
Please update the PR description after the change in direction regarding
the config schema.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22573 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABbjzYQNOvhqHipXnnCGQ40xDwmF9S8eks5vd9PPgaJpZM4cT-8->
.
|
…tant#23408) * Allow host/ipv6 address for broadlink service This matches switch config and is a regression fix * Restore padding of packets for broadlink * Drop unused import * Fix comment on test
* Make setup more robust * Fix typing
…ome-assistant#23419) * Correct media player feature gates * Fix failing test * Lint...
* Refactor NETATMO_AUTH to use hass.data * Minor cleanup * Rename conf to auth and other suggestions by Martin * Revert webhook name change * Rename constant * Move auth * Don't use hass.data.get() * Fix auth string
* Fix Flux component * Update manifest.json * Update manifest.json
gjbadros
requested review from
cdce8p,
cgtobi,
Danielhiversen,
danielperna84,
dgomes,
dmulcahey,
elupus,
fabaff,
frenck,
Kane610,
kellerza,
ludeeus,
OttoWinter,
OverloadUT,
pvizeli,
robbiet480,
rytilahti,
scop,
Swamp-Ig,
syssi and
a team
as code owners
|
This PR is borked. You can try to rebase it and remove commits that aren't applicable, or you may need to start a new one. |
…ead of .items() when ignoring keys.
|
Updating to the latest in the dev branch and rebasing this doesn't seem to have gotten me to a reasonable pull request, so I'll abandon this one and start a new PR after I've tested the changes against the latest (I was away from this for 5 weeks.) |
|
New pull request replacing this borked one is 23839.
#23839
…On Sun, May 12, 2019 at 6:36 PM Aaron Bach ***@***.***> wrote:
This PR is borked. You can try to rebase it and remove commits that aren't
applicable, or you may need to start a new one.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#22573 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALOHTL64HJCDEC2U6F2SSLPVDAYJANCNFSM4HCP547A>
.
|
9 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow more than one Elk M1 alarm system to be integrated into a single hass instance.
Introduces new "devices" schema at the top level, each of which has
the prior configuration schema.
Requires new version of elkm1, 0.7.14, that gwww and I just updated (thanks Glen!)
Changes to the docs are in a separate PR on that repo.
Breaking Change:
Description:
Related issue (if applicable): fixes #
Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#9078
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:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: