Dynalite Integration #27841

ziv1234 · 2019-10-18T07:18:46Z

Breaking Change:

Not a breaking change. Only adds a new integration

Description:

Opened a new pull request to clean up fork

I used Troy Kelly's library and component as a starting point, added several features to them, and am now able to control my full house which is based on Dynalite. I know that no other open-source system supports it, so will be glad if it can go into the mainstream, so others can use it as well. Will also be happy to make fixes to issues that come.
Since I am new to open-source development, wanted to understand what is needed to get this merged. I documented the basic features, that are in this pull request and will continue to add the features and documentation, but was asked to do it in parts so each PR is small.
I tried to follow the guidelines in the dev docs, but I am sure that there are some things that I missed. If this is interesting to add, please let me know if there is anything other than the documentation that needs fixing and i will take care of it quickly.

Best,
Ziv

Related issue (if applicable): fixes #

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

Example entry for `configuration.yaml` (if applicable):

dynalite:
  bridges:
    - host: DEVICE_IP_ADDRESS
      port: 12345
      autodiscover: true
      polltimer: 1
      areacreate: auto
      log_level: debug
      area:
        '2':
          name: Living Room
          channel:
            '2': 
              name: Entrance Spot
              fade: 10.0
            '3': 
              name: Dining Table

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist

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

Documentation added/updated in home-assistant.io

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

The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

ziv1234 · 2019-10-18T09:10:42Z

I had a pull request Dynalite Integration #27621 but because I pulled from the upstream, the pull request included many commits. recreated the fork and added my files cleanly. it passes all the local tests.

please let me know if this is in the right direction. If it is, i can start working on the documentation

ziv1234 · 2019-10-23T00:12:52Z

Hi, there was no activity on the new pull request. Do I need to do something to re add it to the queue?

fabaff · 2019-10-24T15:21:32Z

It's Hacktoberfest...we are dealing with approx. 300 open PRs across the organisation. Thus, it can take a while till the review starts.

kookman · 2019-12-30T09:04:43Z

Is this still in the queue for review? I’d be very interested in using this in mainline release, rather than persisting with my own fork. Thanks Ziv.

ziv1234 · 2019-12-30T11:37:50Z

I would love to have it reviewed if possible. Not sure why no one picked it up. Was just about the send another question...

Note that there is much more functionality that I added in my module, but was suggested to split it into small pull requests, so anxious to have it there and start adding to it.

Ziv

ziv1234 · 2020-01-03T15:12:05Z

pinging again. can someone look at it or did i forget to do something to add it to the queues?

balloob · 2020-02-09T00:38:06Z

homeassistant/components/dynalite/__init__.py

+    if not await bridge.async_setup():
+        LOGGER.error("bridge.async_setup failed")
+        return False
+    hass.data[DOMAIN][host] = bridge


We should store each bridge by entry.entry_id

Can you please explain? Not sure what you mean by this. Is there any documentation or example of how this is done correctly?

ok. used the pattern from component tradfri. hopefully it is ok now

homeassistant/components/dynalite/bridge.py

balloob · 2020-02-09T00:39:00Z

homeassistant/components/dynalite/bridge.py

+        # Configure the dynalite devices
+        self.dynalite_devices = DynaliteDevices(
+            config=self.config,
+            loop=hass.loop,


Python is moving away from passing in loops. Why do we need to pass in the loop here?

done. thanks

balloob · 2020-02-09T00:39:22Z

homeassistant/components/dynalite/bridge.py

+    async def async_setup(self, tries=0):
+        """Set up a Dynalite bridge."""
+        LOGGER.debug(
+            "component bridge async_setup - %s", pprint.pformat(self.config_entry.data)


This is expensive. You're always going to pretty format the config entry data, even if it's not going to be logged. Please remove this.

balloob · 2020-02-09T00:40:20Z

homeassistant/components/dynalite/config_flow.py

+
+    async def async_step_import(self, import_info):
+        """Import a new bridge as a config entry."""
+        LOGGER.debug("async_step_import - %s", pprint.pformat(import_info))


Please remove all usage of pprint.

balloob · 2020-02-09T00:40:51Z

homeassistant/components/dynalite/const.py

+CONF_AREA_CREATE_AUTO = "auto"
+CONF_BRIDGES = "bridges"
+
+ENTITY_CATEGORIES = ["light", "switch", "cover"]


Doesn't seem to be used?

Did you forget to push the change?

balloob · 2020-02-09T00:41:05Z

homeassistant/components/dynalite/light.py

+from .const import DOMAIN, LOGGER
+
+
+async def async_setup_platform(hass, config, async_add_entities, discovery_info=None):


You can remove this. It was never in Home Assistant under the old way.

balloob · 2020-02-09T00:42:06Z

homeassistant/components/dynalite/light.py

+        """Return true if this entity should be hidden from UI."""
+        return self._device.hidden
+
+    @callback


This is not a callback.

balloob · 2020-02-09T00:42:28Z

homeassistant/components/dynalite/manifest.json

@@ -0,0 +1,9 @@
+{
+    "domain": "dynalite",
+    "name": "Direct Integration for Philips Dynalite",


Suggested change

"name": "Direct Integration for Philips Dynalite",

"name": "Philips Dynalite",

balloob · 2020-02-09T00:42:38Z

homeassistant/components/dynalite/manifest.json

+    "domain": "dynalite",
+    "name": "Direct Integration for Philips Dynalite",
+    "config_flow": true,
+    "documentation": "",


Please add url to docs link on website.

how do i know the docs link? Wouldn't it be merged at the same time this pull request goes into the code?

this is pull request 10866 in the docs

The docs link is https://www.home-assistant.io/integrations/<domain>/

balloob · 2020-02-09T00:44:59Z

Hey Ziv, Thanks for your pull request. It took a while but we're slowly going through our backlog. I left some comments. Most of the things already look very good !

ziv1234 · 2020-02-09T11:20:19Z

thanks. Looking at the comments now and will implement them

ziv1234 · 2020-02-09T21:12:59Z

I also rebased it (per the docs instructions) so it is now working off the latest dev version

ziv1234 · 2020-02-10T10:29:29Z

ok. all the requested changes have been implemented

ziv1234 · 2020-02-10T21:24:07Z

Thanks! From: Paulus Schoutsen <notifications@github.com> Sent: Monday, February 10, 2020 11:16 PM To: home-assistant/home-assistant <home-assistant@noreply.github.com> Cc: Ziv <zivk@hotmail.com>; Author <author@noreply.github.com> Subject: Re: [home-assistant/home-assistant] Dynalite Integration (#27841) @balloob approved this pull request. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#27841>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD5UNSYFW6DA6R23CJ3A3PDRCG7Y5ANCNFSM4JCDQVQQ>.

MartinHjelmare · 2020-02-10T22:40:08Z

homeassistant/components/dynalite/bridge.py

+from homeassistant.core import callback
+
+from .const import DATA_CONFIGS, DOMAIN, LOGGER
+from .light import DynaliteLight


We're leaking entity implementation details from the light platform to the bridge module. That's not a good pattern. The platform details should stay within the platform.

It's ok to extract a common function that's used by multiple platforms but the platform should still be responsible for creating and adding entities etc.

fixed. will be pushed shortly

MartinHjelmare · 2020-02-10T22:45:13Z

homeassistant/components/dynalite/config_flow.py

+        if same_hub_entries:
+            await asyncio.wait(
+                [
+                    self.hass.config_entries.async_remove(entry_id)


This is a legacy pattern. We now have the unique_id api for config flow and config entries.

https://developers.home-assistant.io/docs/en/config_entries_config_flow_handler.html#unique-ids

It is quite difficult to understand the config_flow. Do you know if there is any example component that has the most simple config flow (only initiated from configuration.yaml)?

I don't think there's an integration with config flow and only import step that uses the unique_id api. That api is still quite new.

Often the import step just forwards the config to the user step, since the user step often can accept a subset of the items in the config import.

The rainmachine config flow is quite simple and has just import + user step. It doesn't use the unique_id api yet though.

I found a simple flow with just import + user step and unique_id api: gdacs.

MartinHjelmare · 2020-02-10T22:46:25Z

homeassistant/components/dynalite/bridge.py

+                LOGGER.debug("Illegal device category %s", device.category)
+                continue
+            added_entities.append(entity)
+            self.all_entities[entity.unique_id] = entity


We should not store the entity instances outside their platforms in a shared container. It's ok to store a reference to the entity, eg the entity_id, to keep track of added entities.

fixed. will be pushed shortly

MartinHjelmare · 2020-02-10T22:46:49Z

homeassistant/components/dynalite/bridge.py

+
+        for device in devices:
+            if device.category == "light":
+                entity = DynaliteLight(device, self)


The entity should be created and added within its platform.

fixed. will be pushed shortly

MartinHjelmare · 2020-02-10T22:48:45Z

homeassistant/components/dynalite/light.py

+    @callback
+    def try_schedule_ha(self):
+        """Schedule update HA state if configured."""
+        if self.hass:


It's a bad sign that we have to have this check. Entity state update methods should not be called from outside the entity. That's error prone, and that's why this check is needed.

Instead, if we need to tell an entity to update from outside its platform, we can use our dispatch helper to signal the entity to update.

fixed. will be pushed shortly

MartinHjelmare · 2020-02-10T22:52:12Z

homeassistant/components/dynalite/bridge.py

+            else:
+                LOGGER.info("Disconnected from dynalite host")
+            for uid in self.all_entities:
+                self.all_entities[uid].try_schedule_ha()


Use our dispatch helper to signal the entity method instead.

See eg the tuya integration for an example how to use the dispatch helper:

https://github.com/home-assistant/home-assistant/blob/454e63b69ee3d74d6d8d5324395794642e35c988/homeassistant/components/tuya/__init__.py#L97-L100

https://github.com/home-assistant/home-assistant/blob/454e63b69ee3d74d6d8d5324395794642e35c988/homeassistant/components/tuya/__init__.py#L122-L127

fixed. will be pushed shortly

MartinHjelmare · 2020-02-10T22:53:03Z

homeassistant/components/dynalite/bridge.py

+        self.host = config_entry.data[CONF_HOST]
+        if self.host not in hass.data[DOMAIN][DATA_CONFIGS]:
+            LOGGER.info("invalid host - %s", self.host)
+            raise BridgeError(f"invalid host - {self.host}")


We want to avoid side effects like logging or raising an error in init method. Please move that outside.

MartinHjelmare · 2020-02-10T22:54:18Z

homeassistant/components/dynalite/__init__.py

+    config = hass.data[DOMAIN][DATA_CONFIGS].get(host)
+
+    if config is None:
+        LOGGER.error("__init async_setup_entry empty config for host %s", host)


We should not allow this to happen. This should be caught earlier in the config flow.

ziv1234 · 2020-02-11T07:01:46Z

Can you please explain? Not sure what you mean by the leak. Also saw your other comments. Will work on all of them tonight, but this one is one that I didn’t understand. From: Martin Hjelmare <notifications@github.com> Sent: Tuesday, February 11, 2020 12:41 AM To: home-assistant/home-assistant <home-assistant@noreply.github.com> Cc: Ziv <zivk@hotmail.com>; Author <author@noreply.github.com> Subject: Re: [home-assistant/home-assistant] Dynalite Integration (#27841) @MartinHjelmare commented on this pull request.

________________________________ In homeassistant/components/dynalite/bridge.py<#27841 (comment)>:

@@ -0,0 +1,118 @@

+"""Code to handle a Dynalite bridge.""" + +from dynalite_devices_lib import DynaliteDevices +from dynalite_lib import CONF_ALL + +from homeassistant.const import CONF_HOST +from homeassistant.core import callback + +from .const import DATA_CONFIGS, DOMAIN, LOGGER +from .light import DynaliteLight We're leaking entity implementation details from the light platform to the bridge module. That's not a good pattern. The platform details should stay within the platform. It's ok to extract a common function that's used by multiple platforms but the platform should still be responsible for creating and adding entities etc. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#27841>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD5UNS7ZGG2UEIEHOHEMAY3RCHJZJANCNFSM4JCDQVQQ>.

ziv1234 · 2020-02-11T07:34:00Z

BTW, if there is a good reference platform to examine and learn from, please tell me and I will look into it. Tried modeling it after the hue platform which is similar in the sense of having a bridge that manages many devices but can look at other design patterns On 11 Feb 2020, at 9:01, Ziv Kedem <zivk@hotmail.com> wrote: Can you please explain? Not sure what you mean by the leak. Also saw your other comments. Will work on all of them tonight, but this one is one that I didn’t understand. From: Martin Hjelmare <notifications@github.com> Sent: Tuesday, February 11, 2020 12:41 AM To: home-assistant/home-assistant <home-assistant@noreply.github.com> Cc: Ziv <zivk@hotmail.com>; Author <author@noreply.github.com> Subject: Re: [home-assistant/home-assistant] Dynalite Integration (#27841) @MartinHjelmare commented on this pull request.

________________________________ In homeassistant/components/dynalite/bridge.py<#27841 (comment)>:

@@ -0,0 +1,118 @@

+"""Code to handle a Dynalite bridge.""" + +from dynalite_devices_lib import DynaliteDevices +from dynalite_lib import CONF_ALL + +from homeassistant.const import CONF_HOST +from homeassistant.core import callback + +from .const import DATA_CONFIGS, DOMAIN, LOGGER +from .light import DynaliteLight We're leaking entity implementation details from the light platform to the bridge module. That's not a good pattern. The platform details should stay within the platform. It's ok to extract a common function that's used by multiple platforms but the platform should still be responsible for creating and adding entities etc. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#27841>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD5UNS7ZGG2UEIEHOHEMAY3RCHJZJANCNFSM4JCDQVQQ>.

MartinHjelmare · 2020-02-11T07:37:13Z

The platform entity creation and addition should stay within the platform and entities should not be accessed directly outside their platform. We're currently breaking that design pattern here. That's what I meant with "leak".

MartinHjelmare · 2020-02-11T08:45:31Z

It looks like the dynalite is using a push based model for updating state. It's good to take the model for updates into account when searching for examples.

Smartthings uses push model and our dispatch helper to update entities. Entities don't seem to be discovered there though.

Zha has a discovery function, using the dispatch helper, in each platform that gets called to add new entities when a new device is discovered.

Mysensors is using a gateway and push model with dispatch helper but doesn't use config entry api, so is a bit dated.

Unfortunately, the integrations with highest code quality and patterns is often not the simplest and may be hard to read as a clean example.

MartinHjelmare · 2020-02-11T09:33:16Z

Deconz is good example.

ziv1234 · 2020-02-11T12:00:31Z

That’s ok. I like the challenge… I was looking for the interfaces such as the dispatch and signals. It seems that async_dispatcher_connect is used, but I cannot find any documentation about it online. Did I miss something? From: Martin Hjelmare <notifications@github.com> Sent: Tuesday, February 11, 2020 10:46 AM To: home-assistant/home-assistant <home-assistant@noreply.github.com> Cc: Ziv <zivk@hotmail.com>; Author <author@noreply.github.com> Subject: Re: [home-assistant/home-assistant] Dynalite Integration (#27841) It looks like the dynalite is using a push based model for updating state. It's good to take the model for updates into account when searching for examples. Smartthings uses push model and our dispatch helper to update entities. Entities don't seem to be discovered there though. Zha has a discovery function, using the dispatch helper, in each platform that gets called to add new entities when a new device is discovered. Mysensors is using a gateway and push model with dispatch helper but doesn't use config entry api, so is a bit dated. Unfortunately, the integrations with highest code quality and patterns is often not the simplest and may be hard to read as a clean example. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#27841>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD5UNSZBLTGFKTSLQLII3XTRCJQSZANCNFSM4JCDQVQQ>.

ziv1234 · 2020-02-12T16:26:41Z

figured out the dispatcher from the examples provided. Will implement all these comments and send a new pull request soon with them

ziv1234 · 2020-02-12T20:39:34Z

Thanks. Very helpful. My flow is even simpler as I don’t have the user option so was able to take out most of the code I pushed it with a PR #31760. It is still going through the checks, but all the fixes should be there From: Martin Hjelmare <notifications@github.com> Sent: Wednesday, February 12, 2020 10:34 PM To: home-assistant/home-assistant <home-assistant@noreply.github.com> Cc: Ziv <zivk@hotmail.com>; Author <author@noreply.github.com> Subject: Re: [home-assistant/home-assistant] Dynalite Integration (#27841) @MartinHjelmare commented on this pull request.

________________________________ In homeassistant/components/dynalite/config_flow.py<#27841 (comment)>:

+ """Return a config entry from an initialized bridge."""

+ LOGGER.debug("entry_from_bridge - %s", host) + # Remove all other entries of hubs with same ID or host + + same_hub_entries = [ + entry.entry_id + for entry in self.hass.config_entries.async_entries(DOMAIN) + if entry.data[CONF_HOST] == host + ] + + LOGGER.debug("entry_from_bridge same_hub - %s", same_hub_entries) + + if same_hub_entries: + await asyncio.wait( + [ + self.hass.config_entries.async_remove(entry_id) I found a simple flow with just import + user step and unique_id api: gdacs. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#27841>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD5UNSZN5EJYCSZP6FR7TKTRCRMLJANCNFSM4JCDQVQQ>.

ziv1234 · 2020-02-12T20:52:40Z

I believe I am good there. Followed the instructions from the tutorial. If you can check out the code in the new PR it would be great. Should address all your comments From: Martin Hjelmare <notifications@github.com> Sent: Wednesday, February 12, 2020 10:31 PM To: home-assistant/home-assistant <home-assistant@noreply.github.com> Cc: Ziv <zivk@hotmail.com>; Author <author@noreply.github.com> Subject: Re: [home-assistant/home-assistant] Dynalite Integration (#27841) @MartinHjelmare commented on this pull request.

________________________________ In homeassistant/components/dynalite/config_flow.py<#27841 (comment)>:

+ """Return a config entry from an initialized bridge."""

+ LOGGER.debug("entry_from_bridge - %s", host) + # Remove all other entries of hubs with same ID or host + + same_hub_entries = [ + entry.entry_id + for entry in self.hass.config_entries.async_entries(DOMAIN) + if entry.data[CONF_HOST] == host + ] + + LOGGER.debug("entry_from_bridge same_hub - %s", same_hub_entries) + + if same_hub_entries: + await asyncio.wait( + [ + self.hass.config_entries.async_remove(entry_id) I don't think there's an integration with config flow and only import step that uses the unique_id api. That api is still quite new. Often the import step just forwards the config to the user step, since the user step often can accept a subset of the items in the config import. The rainmachine config flow is quite simple and has just import + user step. It doesn't use the unique_id api yet though. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#27841>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD5UNS5UOZKHRWOY6T5I2ELRCRMBRANCNFSM4JCDQVQQ>.

homeassistant added the cla-signed label Oct 18, 2019

probot-home-assistant bot added the Hacktoberfest label Oct 18, 2019

probot-home-assistant bot added has-tests integration: dynalite new-integration labels Oct 18, 2019

ziv1234 mentioned this pull request Oct 18, 2019

Dynalite Integration Documentation home-assistant/home-assistant.io#10866

Merged

2 tasks

balloob reviewed Feb 9, 2020

View reviewed changes

homeassistant/components/dynalite/bridge.py Outdated Show resolved Hide resolved

balloob reviewed Feb 9, 2020

View reviewed changes

ziv1234 added 5 commits February 9, 2020 22:50

Initial commit

b01c8f5

ran hassfest and gen_requirements_all scripts

27bf2e5

fixes per request from Paulus Schoutsen

34ee2b0

ran gen_requirements_all

f5fdd23

updated library version - removed some debug leftover

89b6ddf

ziv1234 added 2 commits February 9, 2020 23:02

get_requirements again...

1a9e2ca

added documentation URL

6c056d6

ran isort

c4fd813

This comment has been minimized.

Sign in to view

ziv1234 added 2 commits February 10, 2020 09:39

changed storage in hass.data[DOMAIN] to use entry_id instead of host

b17093d

adopted unit tests to latest fix

add61f8

Update const.py

b63d541

balloob approved these changes Feb 10, 2020

View reviewed changes

balloob merged commit 4467409 into home-assistant:dev Feb 10, 2020

MartinHjelmare reviewed Feb 10, 2020

View reviewed changes

lock bot locked and limited conversation to collaborators Feb 13, 2020

		from .const import DOMAIN, LOGGER


		async def async_setup_platform(hass, config, async_add_entities, discovery_info=None):

	"name": "Direct Integration for Philips Dynalite",
	"name": "Philips Dynalite",

Dynalite Integration #27841

Dynalite Integration #27841

Conversation

ziv1234 commented Oct 18, 2019 • edited Loading

Breaking Change:

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

ziv1234 commented Oct 18, 2019

ziv1234 commented Oct 23, 2019

fabaff commented Oct 24, 2019

kookman commented Dec 30, 2019

ziv1234 commented Dec 30, 2019

ziv1234 commented Jan 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Feb 9, 2020

ziv1234 commented Feb 9, 2020

ziv1234 commented Feb 9, 2020

This comment has been minimized.

ziv1234 commented Feb 10, 2020

ziv1234 commented Feb 10, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziv1234 commented Feb 11, 2020 via email

ziv1234 commented Feb 11, 2020 via email

MartinHjelmare commented Feb 11, 2020 • edited Loading

MartinHjelmare commented Feb 11, 2020

MartinHjelmare commented Feb 11, 2020

ziv1234 commented Feb 11, 2020 via email

ziv1234 commented Feb 12, 2020

ziv1234 commented Feb 12, 2020 via email

ziv1234 commented Feb 12, 2020 via email

ziv1234 commented Oct 18, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

balloob Feb 10, 2020 •

edited

Loading

MartinHjelmare commented Feb 11, 2020 •

edited

Loading