Lagute LW-12 Wifi LED control by jaypikay · Pull Request #13307 · home-assistant/core

jaypikay · 2018-03-18T12:00:45Z

Description:

New platform added to support the Lagute LW-12 WiFi LED Controller. The controller is stateless and communicates via UDP. To handle the controller itself the python module python-lw12 is required and automatically installed by the REQUIREMENTS.

The platform supports 29 effects, brightness, rgb color and transition speed settings.

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

Example entry for `configuration.yaml`:

light:
  - platform: lw12wifi
    # Host name or IP of LW-12 LED stripe to control
    host: 192.168.0.1
    # (Optional) Some firmware versions of the LW-12 controller listen
    # on different ports.
    port: 5000
    # (Optional) Friendly name
    name: LW-12 FC

Checklist:

The code change is tested and works locally.
Local tests pass with tox.

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.
New dependencies are only imported inside functions that use them.
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

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

homeassistant · 2018-03-18T12:00:46Z

Hi @jaypikay,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

houndci-bot · 2018-03-18T12:00:54Z

undefined name 'SUPPORT_FLASH'

houndci-bot · 2018-03-18T12:00:54Z

redefinition of unused 'DOMAIN' from line 36

Platforms belong to a domain. lw12wifi belongs to light.

houndci-bot · 2018-03-18T12:00:54Z

'socket' imported but unused

houndci-bot · 2018-03-18T12:00:54Z

'enum.Enum' imported but unused

OttoWinter

I don't understand why we should have configuration options like effect, brightness or rgb. The user can manually specify those in service calls.

OttoWinter · 2018-03-18T12:44:10Z

The indentation makes this quite hard to read, could you maybe consider indenting the vol.All blocks?

OttoWinter · 2018-03-18T12:45:49Z

Please do the following instead for None checks (I'm assuming this is checking for None):

if self._effect is None: return None return self._effect.replace('_', ' ').title()

It's just so much easier to understand.

OttoWinter · 2018-03-18T12:46:57Z

This seems like a debugging statement for development and not for later use. The service calls will be logged anyway from the core.

OttoWinter · 2018-03-18T12:49:00Z

PEP8 recommends doing None checks with is not None:

if self._effect is not None:

OttoWinter · 2018-03-18T12:50:56Z

This is a blocking method and it does I/O. It should therefore not be in an async context. Consider renaming the method to (especially when using time.sleep(0.25); it would block the entire core)

def turn_on(self, **kwargs):

The same applies for turn_off

jaypikay · 2018-03-18T15:21:47Z

To answer the question why we should have configuration options like effect, brightness or rgb? It was my understanding to use these options, as they are provided by homeassistant.const. Should I rename rgb to default_color instead? It is also the only way to define a default behaviour for the lights, because the controller itself is very unreliable. I can remove it, but on the other hand I do not see a disadvantage in this.

houndci-bot · 2018-03-18T15:25:27Z

'asyncio' imported but unused

OttoWinter · 2018-03-18T15:27:34Z

What I was trying to say is that I don't understand why we should provide these options at all. I mean if we don't know the state at startup, why should the configuration know for us what these brightness, color, ... values will be at startup?

This platform can't ask the device for states, ok. But then I would just assume the platform would initialize the light as off.

OttoWinter · 2018-03-18T15:29:37Z

The docstring should be:

"""Return True if unable to access real state of the entity."""

as from the entity definition.

OttoWinter · 2018-03-18T15:31:11Z

I think you would need another time.sleep(0.25) after this (and a couple of other places). Also I think halding kwargs with both rgb color and brightness can be handled a bit smarter. currently you're first sending the old brightness and then the new brightness from kwargs.

Another time.sleep(.25) is not needed here, if we call normal turn_on without the brightness setting, the light will "forget" the brightness and set the new color. The platform would still list the brightness as defined by either the default setting defined in the configuration or by previous turn_on calls with brightness as argument.

jaypikay · 2018-03-18T16:09:14Z

All settings but host are optional. Having the other values set are for a smoother user experience with these lights. Instead of defining a switch or similar, the light will turn on using the favored settings, when I click the power button. The RGB selection is not available when home assistant assumes the device as powered off.

I developed the platform using my experience with the hardware in mind and what would be nice to have. But I am happy to help by fixing my code.

Using service calls is possible and is at the moment the only way to set the transistion speed when using effects. I had hoped for a slider like brightness.

emlove · 2018-03-19T15:47:09Z

As a heads-up, we've just merged a big refactoring to how light platforms implement colors. You'll need to rebase on the latest dev. Platforms are now going to receive turn_on reqeusts with a hue/sat color, and need to report their current color in hue/sat.

Some examples of how to convert to/from RGB if that's the preferred platform interface are here:

https://github.com/home-assistant/home-assistant/blob/947218d51c94566feeb32c6f549493fa07683252/homeassistant/components/light/hyperion.py#L142-L143

https://github.com/home-assistant/home-assistant/blob/947218d51c94566feeb32c6f549493fa07683252/homeassistant/components/light/hyperion.py#L110-L113

Thanks you this information. I will update the code to match the new requirements.

jaypikay · 2018-03-31T14:03:12Z

Sorry for bumping, but do I need to improve something to get it merged in a future release, or simply wait?

fabaff · 2018-04-10T13:41:03Z

Move that to setup_platform and pass it down to LW12WiFi(). This way you don't need host and port.

fabaff · 2018-04-10T13:41:38Z

Please use the same style for listing the features as other platform do.

I did as in components/light/{group.py,mqtt_json.py,zha.py,zwave.py,} and other, sorry but I don't understand why this is not good for my solution but for others?

This style with using mutable features with |= is usually only done when there are "dynamic" supported features. For example, light.mqtt_json only enables color when it's defined in the configuration. For these static integrations like lw12wifi, it's usually cleaner to define the supported features in a global constant like this:

https://github.com/home-assistant/home-assistant/blob/c3388d63a1929a6c197644d1ae1f622c4d58c793/homeassistant/components/media_player/cast.py#L40-L42

OttoWinter · 2018-04-28T16:54:49Z

rgb_color is not used by the light component base. This property can be removed.

OttoWinter · 2018-04-28T16:55:18Z

Typo, to -> too

OttoWinter · 2018-04-28T16:57:52Z

It probably doesn't matter too much, but you could implement RGB color here as it's done in the light base:

vol.All(vol.ExactSequence((cv.byte, cv.byte, cv.byte)), vol.Coerce(tuple)),

OttoWinter · 2018-04-28T16:59:45Z

Could this parameter be renamed to transition_length or transition_speed or default_transition_length? I think just transition is not too descriptive.

OttoWinter · 2018-04-28T17:00:46Z

If it's an unknown effect, shouldn't we just raise HomeAssistantError. I'm not a huge fan of failing silently.

OttoWinter · 2018-04-28T17:03:04Z

So when using this, every time there's a turn_on call the transition length will be set for all future turn_on calls?

I think the transition length from the service call should just refer to a single request.

Otto is correct. We should not store the transition speed as an instance variable.

balloob · 2018-05-01T19:45:30Z

Users should not pass in RGB, TRANSITION. Please remove these config values.

balloob · 2018-05-01T19:45:42Z

Should be removed.

If you want to restore the previous known state after a restart, use helpers/restore_state.py (but do that in a new PR)

balloob · 2018-05-01T19:47:00Z

This is not being referenced in the config (and neither should it). Please remove.

balloob · 2018-05-01T19:47:14Z

Possible to validate here that the connection has been established successfully?

Unfortunatly not, it is an UDP connection, and the controller will never answerer to sent messages.

Unfortunatly it is an UDP connection and the device does not send answers back.

balloob · 2018-05-01T19:48:51Z

Please add the property should_poll and have it return False

Might not be needed if #14229 gets merged before this PR.

…rt in LW12Wifi class.

jaypikay requested a review from andrey-git as a code owner March 18, 2018 12:00

homeassistant added cla-needed new-platform platform: light.lw12wifi labels Mar 18, 2018

houndci-bot reviewed Mar 18, 2018

View reviewed changes

OttoWinter reviewed Mar 18, 2018

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Mar 18, 2018

homeassistant added the cla-signed label Mar 18, 2018

houndci-bot reviewed Mar 18, 2018

View reviewed changes

Comment thread homeassistant/components/light/lw12wifi.py Outdated

Copy link
Copy Markdown

houndci-bot Mar 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'asyncio' imported but unused

OttoWinter reviewed Mar 18, 2018

View reviewed changes

emlove reviewed Mar 19, 2018

View reviewed changes

homeassistant added the cla-signed label Mar 21, 2018

jaypikay changed the title ~~Lw12wifi~~ Lagute LW-12 Wifi LED control Mar 21, 2018

fabaff reviewed Apr 10, 2018

View reviewed changes

homeassistant added cla-signed labels Apr 15, 2018

OttoWinter reviewed Apr 28, 2018

View reviewed changes

fabaff mentioned this pull request Apr 28, 2018

Add LW-12 documentation home-assistant/home-assistant.io#5264

Merged

2 tasks

balloob reviewed May 1, 2018

View reviewed changes

jaypikay added 19 commits May 15, 2018 17:22

Added describing missing docstrings to all functions.

ce75f21

Adopted code to work with HS color setting.

52ef268

Syntactical change in comment.

bc92495

Removed redefinition of DOMAIN.

a543629

Refactored lw12 controller setup: removed requirement for host and po…

f8a0f7b

…rt in LW12Wifi class.

Rewritten supported feature setup to a more static expression.

97fbf6f

Removed unused rgb_color property

0d8c2ac

Fixed typo in comment for set_light_option

b99c33a

Changed RGB option validation schema

2ca9372

Removed instance properties as config options

d747674

Removed optional settings to be more inline with code style.

9e8fe2b

Removed unused option from config example

1bd6bee

Removal of unused import

cf76c39

Added property to disable state polling for this entity.

57bd2fd

Raise an exception if an unknown effect was selected.

09b89f7

Fixed an issue with the check for known effects.

895b2ab

As we do not need to set a default, use simple accessing by key.

1227a6a

Log if an unknown effect was selected.

4e1b8fc

Added link to future documentation.

5da9fe0

homeassistant added the cla-signed label May 15, 2018

syssi approved these changes May 22, 2018

View reviewed changes

MartinHjelmare approved these changes May 22, 2018

View reviewed changes

MartinHjelmare merged commit 72a1b7a into home-assistant:dev May 22, 2018

jaypikay deleted the lw12wifi branch May 26, 2018 13:27

balloob mentioned this pull request Jun 8, 2018

0.71.0 #14876

Merged

home-assistant locked and limited conversation to collaborators Sep 5, 2018

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

Uh oh!

Conversation

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

Uh oh!

Description:

Example entry for configuration.yaml:

Checklist:

Uh oh!

homeassistant commented Mar 18, 2018

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!

OttoWinter 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!

jaypikay commented Mar 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OttoWinter commented Mar 18, 2018

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!

jaypikay commented Mar 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaypikay commented Mar 31, 2018

Uh oh!

fabaff Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

jaypikay commented Mar 18, 2018 •

edited

Loading

Example entry for `configuration.yaml`:

fabaff Apr 10, 2018 •

edited

Loading