Add discovery notify support and mysensors notify #4014

MartinHjelmare · 2016-10-23T21:57:51Z

Description:

Add support for discovery setup for notify component platforms.
Make add_devices optional in mysensors platform callback function.
Use new argument structure for all existing mysensors platforms.
Add mysensors notify platform.

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>

Example entry for configuration.yaml (if applicable):

mysensors:
  gateways:
    - device: '/dev/ttyACM0'
  version: 2.0

Checklist:
If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass

mention-bot · 2016-10-23T21:57:52Z

@MartinHjelmare, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kaustubhphatak, @OttoWinter and @mxtra to be potential reviewers.

MartinHjelmare · 2016-10-23T22:04:37Z

Don't merge! Need to rebase this when #4013 is merged.

balloob · 2016-11-02T05:17:40Z

Code looks fine with me 🐬 . You can merge this when you are ready.

MartinHjelmare · 2016-11-04T20:54:22Z

I'll postpone merging this until after 0.32 release. If possible I'd like to setup the platform via discovery, but I haven't looked into what that would take yet.

balloob · 2016-11-11T06:58:41Z

Bump

MartinHjelmare · 2016-11-15T22:34:41Z

I've added possibility of using discovery for notify platforms. I'd like @balloob to look at this, if it's viable.

As discussed in gitter, I've been experiencing occasional, 1/3 - 1/5 of every start, race conditions with this branch where the notify component is setup more than once. We don't want this to happen!

https://gitter.im/home-assistant/home-assistant/devs?at=58289aecdf5ae966454fe218
https://gitter.im/home-assistant/home-assistant/devs?at=58289fe831c5cbef43eec10b

From how I'm reading the code and from my testing it looks like this is due to how the _async_setup_component coroutine in bootstrap.py is written. It doesn't enforce a lock from the beginning of the function and only if the lock is not already locked will a run be required to acquire the lock. Discovery setup is required to acquire the lock if the lock is locked, before entering the coroutine, but will be allowed to run the coroutine as soon as the lock is released, since the next config component setup does not acquire the lock again immediately. Also, the check for if a component is already added, is done in the beginning of the function, and not after the lock is acquired, so it's more or less a lame duck.

With that said, I'd love to be proven wrong, or that we find a better way to implement discovery for notify platforms.

balloob · 2016-11-19T06:26:29Z

Sorry for not getting back to you earlier.

You are right here about the locking not being correct. I think an easy fix for this is to acquire the lock inside async_from_config_file. That way, no discovery will ever run while we are setting up from a config file.

balloob · 2016-11-19T08:07:06Z

I have resolved the discovery issue in #4463

MartinHjelmare · 2016-11-19T19:24:26Z

tests/components/notify/test_command_line.py

-        }))
+        # Platform should not be set up, but component should be set up.
+        with assert_setup_component(0):
+            self.assertTrue(setup_component(self.hass, notify.DOMAIN, {


With the change to notify component this test doesn't add value anymore, I think. The notify component will be set up even with bad config, due to config validation removing the bad parts.

MartinHjelmare · 2016-11-19T19:24:38Z

tests/components/notify/test_file.py

        with assert_setup_component(0):
-            assert not setup_component(self.hass, notify.DOMAIN, {
+            assert setup_component(self.hass, notify.DOMAIN, {


With the change to notify component this test doesn't add value anymore, I think. The notify component will be set up even with bad config, due to config validation removing the bad parts.

Ok, strip it.

MartinHjelmare · 2016-11-19T19:26:15Z

homeassistant/components/notify/__init__.py

+        if p_config is None:
+            p_config = {}
+        if discovery_info is not None:
+            p_config = discovery_info


Is this a good idea, using the passed discovery info directly as config for the platform?

balloob · 2016-11-23T06:12:02Z

I have made another fix for the discovery today #4529

balloob · 2016-11-23T06:13:39Z

homeassistant/components/notify/__init__.py

+
+    for platform, p_config in config_per_platform(config, DOMAIN):
+        if not setup_notify_platform(platform, p_config):
+            return False


The logic in bootstrap will actually continue setting up platforms after it finds invalid platforms. So let's mimic that behavior here.

balloob · 2016-12-02T05:41:42Z

Bump

* Make add_devices optional in platform callback function. * Use new argument structure for all existing mysensors platforms. * Add notify platform.

* Enable discovery of notify platforms. * Use discovery to set up mysensors notify platform.

* Continue setup of notify platforms if a platform fails setup. * Remove notify tests that check platform config. These tests are not needed when config validation is used. * Add config validation to APNS notify platform.

houndci-bot · 2016-12-03T13:03:01Z

tests/components/notify/test_command_line.py

@@ -6,7 +6,7 @@

 from homeassistant.bootstrap import setup_component
 import homeassistant.components.notify as notify
-from tests.common import get_test_home_assistant
+from tests.common import assert_setup_component, get_test_home_assistant


'assert_setup_component' imported but unused

MartinHjelmare · 2016-12-03T13:06:20Z

One lint violation and some tests for notify component is still missing. I'll fix that asap.

balloob · 2016-12-27T20:53:13Z

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

robbiet480 added the Hacktoberfest label Oct 23, 2016

robbiet480 removed the Hacktoberfest label Nov 1, 2016

MartinHjelmare force-pushed the add-mysensors-notify branch from 62ba635 to 87e5048 Compare November 15, 2016 22:16

MartinHjelmare changed the title ~~Add mysensors notify platform~~ RFC: Add mysensors notify platform Nov 15, 2016

balloob mentioned this pull request Nov 19, 2016

Better locking while setting up components + discovery #4463

Merged

2 tasks

MartinHjelmare force-pushed the add-mysensors-notify branch from 87e5048 to eab3160 Compare November 19, 2016 19:12

MartinHjelmare changed the title ~~RFC: Add mysensors notify platform~~ Add discovery notify support and mysensors notify platform Nov 19, 2016

MartinHjelmare changed the title ~~Add discovery notify support and mysensors notify platform~~ Add discovery notify support and mysensors notify Nov 19, 2016

MartinHjelmare commented Nov 19, 2016

View reviewed changes

balloob reviewed Nov 23, 2016

View reviewed changes

balloob self-assigned this Nov 23, 2016

MartinHjelmare added 5 commits December 3, 2016 13:54

Add mysensors notify platform

39ec614

* Make add_devices optional in platform callback function. * Use new argument structure for all existing mysensors platforms. * Add notify platform.

Update mysensors gateway

fe31c50

Refactor notify setup

1553e8d

* Enable discovery of notify platforms. * Use discovery to set up mysensors notify platform.

Fix notify tests

bd73c91

Update notify tests

4b994c6

* Continue setup of notify platforms if a platform fails setup. * Remove notify tests that check platform config. These tests are not needed when config validation is used. * Add config validation to APNS notify platform.

MartinHjelmare force-pushed the add-mysensors-notify branch from eab3160 to 4b994c6 Compare December 3, 2016 13:02

houndci-bot reviewed Dec 3, 2016

View reviewed changes

balloob closed this Dec 27, 2016

MartinHjelmare mentioned this pull request Jan 8, 2017

Add discovery notify support and mysensors notify #5219

Merged

3 tasks

home-assistant locked and limited conversation to collaborators Mar 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add discovery notify support and mysensors notify #4014

Add discovery notify support and mysensors notify #4014

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add discovery notify support and mysensors notify #4014

Add discovery notify support and mysensors notify #4014

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!