Support multiple Elk instances #23839
Merged
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
* 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!) QUESTION: Should the "devices" section be optional to avoid breaking old configuration files? I chose not to do that for simplicity and because I was following the doorbird code which requires the "devices" section for all configurations even with only one device.
Fixed issues raised by hound -- there was clearly a tool I was supposed to run to get those warnings before submitting the PR. Sorry! Updated REQUIREMENTS.
Also fixed unused prefix local variable lint warning.
Not sure if I missed these on the first pass or if the linter stopped after a certain number of warnings or something else. Switched logging to use %d and %s instead of string concatenation (per lint request and because I imagine it migth be better performing in some (oldish, I presume) implementations of python.
This eliminates the breaking change for configurations wanting a singleton elk m1 instance (the majority of users, no doubt). I did not do it like this before because I was following the lead of the doorbird component which introduced a devices: section when moving to support multiple doorbells. But Rohan Kapoor kindly pointed me at the zoneminder component which sets the other (IMO) preferable precedent. Will update the docs change shortly.
Just move async_add_entities() outside of the loops across the elk m1 controllers, so it's called once for each platform.
Move it to after the loop, so it's called only once per platform even when there are multiple elk m1 controllers.
Thanks to Martin Hjelmare for the careful review and suggestions. (All mistaken improvements and new bugs are my own.)
Use dict.values() (instead of making it easier to add local looping variable on the keys by using _, bar = ...items()) Use [] when the key is known to exist.
* 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!) QUESTION: Should the "devices" section be optional to avoid breaking old configuration files? I chose not to do that for simplicity and because I was following the doorbird code which requires the "devices" section for all configurations even with only one device.
Fixed issues raised by hound -- there was clearly a tool I was supposed to run to get those warnings before submitting the PR. Sorry! Updated REQUIREMENTS.
Also fixed unused prefix local variable lint warning.
Not sure if I missed these on the first pass or if the linter stopped after a certain number of warnings or something else. Switched logging to use %d and %s instead of string concatenation (per lint request and because I imagine it migth be better performing in some (oldish, I presume) implementations of python.
This eliminates the breaking change for configurations wanting a singleton elk m1 instance (the majority of users, no doubt). I did not do it like this before because I was following the lead of the doorbird component which introduced a devices: section when moving to support multiple doorbells. But Rohan Kapoor kindly pointed me at the zoneminder component which sets the other (IMO) preferable precedent. Will update the docs change shortly.
Just move async_add_entities() outside of the loops across the elk m1 controllers, so it's called once for each platform.
Move it to after the loop, so it's called only once per platform even when there are multiple elk m1 controllers.
Thanks to Martin Hjelmare for the careful review and suggestions. (All mistaken improvements and new bugs are my own.)
Use dict.values() (instead of making it easier to add local looping variable on the keys by using _, bar = ...items()) Use [] when the key is known to exist.
…ead of .items() when ignoring keys.
|
Ping @balloob ... I'd love to get my first commit to HA done :-) |
There was a problem hiding this comment.
@balloob decided to approve the configurable prefix to be part of unique_id.
…onna use .lower() on creating the entity id that has to be unique; do the _has_all_unique_prefixes check last so we get errors from the device schema before complaining about the uniqueness problem, if any
| CONFIG_SCHEMA_SUBDOMAIN = vol.Schema({ | ||
| def _has_all_unique_prefixes(value): | ||
| """Validate that each m1 configured has a unique prefix.""" | ||
| prefixes = [device.get(CONF_PREFIX, "").lower() for device in value] |
There was a problem hiding this comment.
Use dict[key] for the prefix since there's a default in the previous config schema.
There was a problem hiding this comment.
Move the lower string modification to be part of the prefix schema below instead and use vol.Lower.
| vol.Optional(CONF_ENABLED, default=True): cv.boolean, | ||
| vol.Optional(CONF_INCLUDE, default=[]): [_elk_range_validator], | ||
| vol.Optional(CONF_EXCLUDE, default=[]): [_elk_range_validator], | ||
| }) | ||
|
|
||
| DEVICE_SCHEMA = vol.Schema({ | ||
| vol.Required(CONF_HOST): cv.string, | ||
| vol.Optional(CONF_PREFIX, default=''): cv.string, |
There was a problem hiding this comment.
vol.All(cv.string, vol.Lower)…a pair of bugs for the alarm control panel display message service -- since data templates always generate strings, the values subject to range/set restrictions need to be coerced to their proper type before the check
MartinHjelmare
approved these changes
Jul 26, 2019
|
We should probably mark this a breaking change, due to changing unique_id, and ask users to remove old entities from the entity registry. |
| self._temperature_unit = elk_data['config']['temperature_unit'] | ||
| self._unique_id = 'elkm1_{}'.format( | ||
| self._element.default_name('_').lower()) | ||
| self._unique_id = 'elkm1_{prefix}_{name}'.format( |
There was a problem hiding this comment.
Should we remove the extra underscore between prefix and name to make this a non breaking change?
|
To avoid a breaking change, we would need to use a different start to the
entity I'd to signify there is a prefix. Maybe
elkm1_ for no prefix and back compatibility
And
elkm1m_prefix_ for new controllers where a prefix is specified
Otherwise I think the name of a zone on a default no-prefix m1 will risk
conflict with a prefix'd m1's zone.
Ok to use elkm1m_ if and only if there is a prefix for that controller? I
imagine we don't want to pollute the global before-first-underscore space
of entity IDs, so I'm asking.
…On Fri, Jul 26, 2019, 2:01 AM Martin Hjelmare ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In homeassistant/components/elkm1/__init__.py
<#23839 (comment)>
:
> self._temperature_unit = elk_data['config']['temperature_unit']
- self._unique_id = 'elkm1_{}'.format(
- self._element.default_name('_').lower())
+ self._unique_id = 'elkm1_{prefix}_{name}'.format(
Should we remove the extra underscore between prefix and name to make this
a non breaking change?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23839>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALOHTLD7VHJMDEEUSFFBMLQBK4N3ANCNFSM4HMMFL6Q>
.
|
|
As long as unique_id won't change for existing entities, there won't be a breaking change. Existing registered entities won't change entity_id, since they are registered in the entity registry. |
|
Understood I'm just trying to avoid future conflicts and address the
concern with the breaking entities... I'll make the fix later this morning.
Thx
…On Fri, Jul 26, 2019, 7:48 AM Martin Hjelmare ***@***.***> wrote:
As long as unique_id won't change for existing entities, there won't be a
breaking change. Existing registered entities won't change entity_id, since
they are registered in the entity registry.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23839>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALOHTMPNXJOQEBXOXZOL5DQBMFETANCNFSM4HMMFL6Q>
.
|
…on-empty prefix given; this enables backward compatibility to avoid a breaking change by letting the elkm1_ start to unique_id keep working exactly as it used to.
|
These build check failures look like an automation problem and not a problem with my changes, jfyi. |
MartinHjelmare
approved these changes
Jul 26, 2019
|
Sorry for n00b question, but is there something else I do now or do you do
the merge? Thanks for all your help, Martin -- this was by far the most
thorough open source review process which gives me great optimism for the
project!
…On Fri, Jul 26, 2019 at 11:16 AM Martin Hjelmare ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23839>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALOHTN5JPDDTV2WZHUXD5TQBM5RNANCNFSM4HMMFL6Q>
.
|
|
The build failed due to an unrelated problem it seems. You could try rebasing the last commit and force pushing to force a new build. Thanks for the feedback! I appreciate it. 👍 |
… failed for unrelated reasons last time
… tidying only so we might as well depend on it
MartinHjelmare
approved these changes
Jul 27, 2019
|
I think it's all good again. Anything else I need to do for the merge?
Thanks!
…On Fri, Jul 26, 2019, 6:10 PM Martin Hjelmare ***@***.***> wrote:
***@***.**** approved this pull request.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23839>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALOHTPSX4KABGVEJVU2QZ3QBOOBLANCNFSM4HMMFL6Q>
.
|
|
Thanks!
…On Sat, Jul 27, 2019, 1:37 AM Martin Hjelmare ***@***.***> wrote:
Merged #23839
<#23839> into dev.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23839>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALOHTN2AYBM3ZP2E3QTLXLQBQCKZANCNFSM4HMMFL6Q>
.
|
Merged
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 (Bug: recorder component does not Vacuum SQLite DB on purge #9441) on that repo.
Related issue (if applicable): fixes #
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#9441
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:
python3 -m script.hassfest.requirements_all.txtby runningpython3 -m script.gen_requirements_all..coveragerc.If the code does not interact with devices: