Add support for Nanoleaf Aurora Light Panels by Oro · Pull Request #13456 · home-assistant/core

Oro · 2018-03-25T14:41:07Z

Description:

Added support for Nanoleaf Aurora lights

Related issue (if applicable): None, discussion at https://community.home-assistant.io/t/nanoleaf-aurora-component/21054/

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#5018

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

light:
  - platform: aurora
    host: 192.168.1.10
    token: xxxxxxxxxxxxxxxxxxxxx

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

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:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

Oro · 2018-03-26T07:18:02Z

seems like there was a recent change to RGB_COLOR and xy colors instead of rgb; I'll need to dig in to that to check (see #11288 )
Also it seems as if I am not importing modules within functions, which I'll also dig in soon (this weekend hopefully)
I'd appreciate any and all pointers

emlove

Really great work! I've just added a few comments here for best practices.

emlove · 2018-04-03T22:28:42Z

homeassistant/components/light/aurora.py

+    host = config.get(CONF_HOST)
+    name = config.get(CONF_NAME)
+    token = config.get(CONF_TOKEN)
+    aurora_light = aurora().Aurora(host, token)


You can just import nanoleaf in setup_platform. The wrapper isn't necessary.

emlove · 2018-04-03T22:36:29Z

homeassistant/components/light/aurora.py

+
+    def __init__(self, light):
+        """Initialize an Aurora."""
+        self._brightness = light.brightness


Instead of duplicating the update logic here, just assign self._light ans self._name, and then pass True as the second argument (update_before_add) of add_devices. Hass will then call update before adding the entity to the state machine.

Example:
https://github.com/home-assistant/home-assistant/blob/a502c4db3485cc2ec9f9755e6ea28d6713cf6c66/homeassistant/components/light/iglo.py#L39

emlove · 2018-04-03T22:40:56Z

homeassistant/components/light/aurora.py

+    @property
+    def icon(self):
+        """Return the icon to use in the frontend, if any."""
+        return "mdi:triangle-outline"


Not sure about overriding the icon for a light, but no strong feelings myself if it's aligned with the actual device. Could you add some screenshots of what this looks like in a card?

I kinda like it since the device itself is a triangle, however this is more of a special snowflake kind of situation, so if you'd like to keep this as a regular light bulb I'll happily oblige.

It looks like this (I hope it's okay to attach it here, I don't think it fits into the documentation itself):

👍 Looks nice to me, and these aren't normal lights, so a different icon seems reasonable to me.

Oro · 2018-04-04T14:27:17Z

@armills Thanks for the review and the comments!
pylint now results in

W:146, 8: Attribute '_hs_color' defined outside init (attribute-defined-outside-init)

I'd simply __init__ them with None instead of leaving them out, would that be OK as well?

emlove · 2018-04-04T14:28:43Z

Ah yeah, that's my mistake. They should be initialized to None in the constructor.

Norien · 2018-04-04T23:45:01Z

This is working great for me on .66 thanks for all you do. I was just wondering if it's possible to get the motion sensor exposed to use with automations!

Oro · 2018-04-06T08:46:24Z

@Norien glad it works!
if possible, please use the community thread for requests (I'm also monitoring it) https://community.home-assistant.io/t/nanoleaf-aurora-component/21054/

emlove · 2018-04-06T13:35:08Z

Thanks for the contribution! 🌟

MartinHjelmare

Commenting for possible improvement in a future PR.

MartinHjelmare · 2018-04-06T14:25:10Z

homeassistant/components/light/aurora.py

+https://github.com/software-2/nanoleaf
+
+For more details about this platform, please refer to the documentation at
+https://home-assistant.io/components/light.nanoleaf_aurora/


The url is wrong. It should end with the platform name.

I don't think I get it. There is no platform for the nanoleaf aurora lights, there is however a aurora sensor which has nothing to do with those lights (which is why I named the documentation light.nanoleaf_aurora) - did I misunderstand something/do something incorrectly?

The name of the platform module is aurora.py. Then you have to name the docs page https://home-assistant.io/components/light.aurora/ eg.

I get it now, thanks - If I rename it to aurora the component page will display a link to the binary sensor, thinking they are related, is there a way to overwrite that behaviour or do you recon that this is OK? - if not, I will probably have to rename the component itself

Yes, we shouldn't have two platforms with the same name that are not connected. Please rename this platform. nanoleaf_aurora.py is ok.

Thanks, I've renamed it in #13831 , for the others I'll create separate PRs as well, if possible it would be great if this could get in 0.67 so that nobody has to update their config in the version following afterwards

MartinHjelmare · 2018-04-06T14:27:26Z

homeassistant/components/light/aurora.py

+    if aurora_light.on is None:
+        _LOGGER.error("Could not connect to \
+        Nanoleaf Aurora: %s on %s", name, host)
+    add_devices([AuroraLight(aurora_light)], True)


Do we still want to add devices if the above error happens?

Probably not, my mistake - I'll send a PR hopefully this weekend

MartinHjelmare · 2018-04-06T14:28:35Z

homeassistant/components/light/aurora.py

+    name = config.get(CONF_NAME)
+    token = config.get(CONF_TOKEN)
+    aurora_light = nanoleaf.Aurora(host, token)
+    aurora_light.hass_name = name


Why put the name on the aurora light instance? You can pass the name separately when instantiating the entity.

That is a very good question, probably not needed. I'll have a look at it this weekend, thank you for the comments!

Oro requested a review from andrey-git as a code owner March 25, 2018 14:41

homeassistant added new-platform platform: light.aurora cla-signed labels Mar 25, 2018

Oro added 3 commits March 27, 2018 12:38

Added support for Nanoleaf Aurora Light Panels

13a65db

aurora light module - fixed lint errors

1e9b441

aurora light module - use SUPPORT_COLOR instead of SUPPORT_RGB_COLOR

a7f5a5e

Oro changed the title ~~Added support for Nanoleaf Aurora Light Panels~~ WIP - Add support for Nanoleaf Aurora Light Panels Mar 27, 2018

Oro force-pushed the light-aurora branch from bff47b0 to a7f5a5e Compare March 27, 2018 10:46

homeassistant added the cla-signed label Mar 27, 2018

nanoleaf aurora light - support_hs_color instead of rgb

6f30ea2

Oro changed the title ~~WIP - Add support for Nanoleaf Aurora Light Panels~~ Add support for Nanoleaf Aurora Light Panels Mar 30, 2018

Oro mentioned this pull request Apr 2, 2018

Added Nanoleaf Aurora Light documentation home-assistant/home-assistant.io#5018

Merged

2 tasks

emlove reviewed Apr 4, 2018

View reviewed changes

review comments from armills implemented

0de9391

homeassistant added the cla-signed label Apr 4, 2018

emlove approved these changes Apr 4, 2018

View reviewed changes

nanoleaf aurora lights - put attributes into constructor (pylint)

9be42be

homeassistant added the cla-signed label Apr 4, 2018

emlove merged commit 0a25d30 into home-assistant:dev Apr 6, 2018

MartinHjelmare reviewed Apr 6, 2018

View reviewed changes

balloob mentioned this pull request Apr 13, 2018

0.67.0 #13856

Merged

home-assistant locked and limited conversation to collaborators Jul 26, 2018

ghost added integration: aurora and removed platform: light.aurora labels Mar 21, 2019

Uh oh!

Conversation

Oro commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Oro commented Mar 26, 2018

Uh oh!

emlove left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Oro commented Apr 4, 2018

Uh oh!

emlove commented Apr 4, 2018

Uh oh!

Norien commented Apr 4, 2018

Uh oh!

Oro commented Apr 6, 2018

Uh oh!

emlove commented Apr 6, 2018

Uh oh!

MartinHjelmare left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Oro commented Mar 25, 2018 •

edited

Loading

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