New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lagute LW-12 Wifi LED control #13307

Merged
merged 26 commits into from May 22, 2018

Conversation

@jaypikay
Contributor

jaypikay commented Mar 18, 2018

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:

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.

@jaypikay jaypikay requested a review from andrey-git as a code owner Mar 18, 2018

@homeassistant

This comment has been minimized.

homeassistant commented Mar 18, 2018

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!

homeassistant/components/light/lw12wifi.py Outdated
# Setup feature list
self._supported_features = SUPPORT_BRIGHTNESS
self._supported_features |= SUPPORT_EFFECT
self._supported_features |= SUPPORT_FLASH

This comment has been minimized.

@houndci-bot

houndci-bot Mar 18, 2018

undefined name 'SUPPORT_FLASH'

homeassistant/components/light/lw12wifi.py Outdated
DEFAULT_EFFECT = None
DEFAULT_TRANSITION = 128
DOMAIN = 'lw12wifi'

This comment has been minimized.

@houndci-bot

houndci-bot Mar 18, 2018

redefinition of unused 'DOMAIN' from line 36

This comment has been minimized.

@fabaff

fabaff Apr 10, 2018

Member

Platforms belong to a domain. lw12wifi belongs to light.

homeassistant/components/light/lw12wifi.py Outdated
import asyncio
from enum import Enum
import logging
import socket

This comment has been minimized.

@houndci-bot

houndci-bot Mar 18, 2018

'socket' imported but unused

homeassistant/components/light/lw12wifi.py Outdated
"""
import asyncio
from enum import Enum

This comment has been minimized.

@houndci-bot

houndci-bot Mar 18, 2018

'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.

homeassistant/components/light/lw12wifi.py Outdated
vol.Required(CONF_HOST): cv.string,
vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
vol.Optional(CONF_RGB, default=DEFAULT_RGB):
vol.All(list, vol.Length(min=3, max=3),

This comment has been minimized.

@OttoWinter

OttoWinter Mar 18, 2018

Contributor

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

homeassistant/components/light/lw12wifi.py Outdated
try:
return self._effect.replace('_', ' ').title()
except AttributeError:
return None

This comment has been minimized.

@OttoWinter

OttoWinter Mar 18, 2018

Contributor

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.

homeassistant/components/light/lw12wifi.py Outdated
@asyncio.coroutine
def async_turn_on(self, **kwargs):
"""Instruct the light to turn on."""
_LOGGER.debug('async_turn_on: {}'.format(kwargs))

This comment has been minimized.

@OttoWinter

OttoWinter Mar 18, 2018

Contributor

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

homeassistant/components/light/lw12wifi.py Outdated
_LOGGER.debug('async_turn_on: {}'.format(kwargs))
import lw12
self._light.light_on()
if self._effect:

This comment has been minimized.

@OttoWinter

OttoWinter Mar 18, 2018

Contributor

PEP8 recommends doing None checks with is not None:

if self._effect is not None:
homeassistant/components/light/lw12wifi.py Outdated
"""Instruct the light to turn on."""
_LOGGER.debug('async_turn_on: {}'.format(kwargs))
import lw12
self._light.light_on()

This comment has been minimized.

@OttoWinter

OttoWinter Mar 18, 2018

Contributor

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

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

@jaypikay

This comment has been minimized.

Contributor

jaypikay commented Mar 18, 2018

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.

homeassistant/components/light/lw12wifi.py Outdated
rgb: [255, 255, 255]
"""
import asyncio

This comment has been minimized.

@houndci-bot

houndci-bot Mar 18, 2018

'asyncio' imported but unused

@OttoWinter

This comment has been minimized.

Contributor

OttoWinter commented Mar 18, 2018

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.

homeassistant/components/light/lw12wifi.py Outdated
@property
def assumed_state(self) -> bool:
"""Return False if unable to access real state of the entity."""

This comment has been minimized.

@OttoWinter

OttoWinter Mar 18, 2018

Contributor

The docstring should be:

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

as from the entity definition.

homeassistant/components/light/lw12wifi.py Outdated
time.sleep(.25)
brightness = int(self._brightness / 255 * 100)
self._light.set_light_option(lw12.LW12_LIGHT.BRIGHTNESS,
brightness)

This comment has been minimized.

@OttoWinter

OttoWinter Mar 18, 2018

Contributor

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.

This comment has been minimized.

@jaypikay

jaypikay Mar 18, 2018

Contributor

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

This comment has been minimized.

Contributor

jaypikay commented Mar 18, 2018

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.

homeassistant/components/light/lw12wifi.py Outdated
# Unknown effect, changing to no selected effect.
self._effect = None
if ATTR_RGB_COLOR in kwargs:

This comment has been minimized.

@armills

armills Mar 19, 2018

Member

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:

if ATTR_HS_COLOR in kwargs:
rgb_color = color_util.color_hs_to_RGB(*kwargs[ATTR_HS_COLOR])

@property
def hs_color(self):
"""Return last color value set."""
return color_util.color_RGB_to_hs(*self._rgb_color)

This comment has been minimized.

@jaypikay

jaypikay Mar 19, 2018

Contributor

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

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

@jaypikay

This comment has been minimized.

Contributor

jaypikay commented Mar 31, 2018

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

homeassistant/components/light/lw12wifi.py Outdated
"""
import lw12
self._light = lw12.LW12Controller(host, port)

This comment has been minimized.

@fabaff

fabaff Apr 10, 2018

Member

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

homeassistant/components/light/lw12wifi.py Outdated
self._supported_features = SUPPORT_BRIGHTNESS
self._supported_features |= SUPPORT_EFFECT
self._supported_features |= SUPPORT_COLOR
self._supported_features |= SUPPORT_TRANSITION

This comment has been minimized.

@fabaff

fabaff Apr 10, 2018

Member

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

This comment has been minimized.

@jaypikay

jaypikay Apr 14, 2018

Contributor

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 comment has been minimized.

@OttoWinter

OttoWinter Apr 14, 2018

Contributor

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:

SUPPORT_CAST = SUPPORT_PAUSE | SUPPORT_VOLUME_SET | SUPPORT_VOLUME_MUTE | \
SUPPORT_TURN_ON | SUPPORT_TURN_OFF | SUPPORT_PREVIOUS_TRACK | \
SUPPORT_NEXT_TRACK | SUPPORT_PLAY_MEDIA | SUPPORT_STOP | SUPPORT_PLAY

homeassistant/components/light/lw12wifi.py Outdated
return self._brightness
@property
def rgb_color(self):

This comment has been minimized.

@OttoWinter

OttoWinter Apr 28, 2018

Contributor

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

homeassistant/components/light/lw12wifi.py Outdated
if ATTR_EFFECT in kwargs:
self._effect = kwargs.get(ATTR_EFFECT).replace(' ', '_').upper()
self._light.set_effect(lw12.LW12_EFFECT[self._effect])
# Sending UDP messages to quickly after the previous message

This comment has been minimized.

@OttoWinter

OttoWinter Apr 28, 2018

Contributor

Typo, to -> too

homeassistant/components/light/lw12wifi.py Outdated
vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
vol.Optional(CONF_RGB, default=DEFAULT_RGB):
vol.All(list, vol.Length(min=3, max=3),
[vol.All(vol.Coerce(int), vol.Range(min=0, max=255))]),

This comment has been minimized.

@OttoWinter

OttoWinter Apr 28, 2018

Contributor

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)),
homeassistant/components/light/lw12wifi.py Outdated
vol.Optional(CONF_RGB, default=DEFAULT_RGB):
vol.All(list, vol.Length(min=3, max=3),
[vol.All(vol.Coerce(int), vol.Range(min=0, max=255))]),
vol.Optional(CONF_TRANSITION, default=DEFAULT_TRANSITION):

This comment has been minimized.

@OttoWinter

OttoWinter Apr 28, 2018

Contributor

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

homeassistant/components/light/lw12wifi.py Outdated
if self._effect.replace(' ', '_').upper() in lw12.LW12_EFFECT:
kwargs['effect'] = self._effect
else:
# Unknown effect: changing to no selected effect.

This comment has been minimized.

@OttoWinter

OttoWinter Apr 28, 2018

Contributor

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

homeassistant/components/light/lw12wifi.py Outdated
self._light.set_light_option(
lw12.LW12_LIGHT.FLASH, self._transition_speed)
if ATTR_TRANSITION in kwargs:
self._transition_speed = int(kwargs[ATTR_TRANSITION])

This comment has been minimized.

@OttoWinter

OttoWinter Apr 28, 2018

Contributor

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.

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

@fabaff fabaff referenced this pull request Apr 28, 2018

Merged

Add LW-12 documentation #5264

2 of 2 tasks complete
homeassistant/components/light/lw12wifi.py Outdated
vol.All(vol.Coerce(int), vol.Range(min=0, max=255)),
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
vol.Optional(CONF_RGB, default=DEFAULT_RGB):

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

homeassistant/components/light/lw12wifi.py Outdated
PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_HOST): cv.string,
vol.Optional(CONF_BRIGHTNESS, default=DEFAULT_BRIGHTNESS):

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

Should be removed.

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

homeassistant/components/light/lw12wifi.py Outdated
port = config.get(CONF_PORT)
rgb = config.get(CONF_RGB)
brightness = config.get(CONF_BRIGHTNESS)
effect = config.get(CONF_EFFECT)

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

homeassistant/components/light/lw12wifi.py Outdated
effect = config.get(CONF_EFFECT)
transition = config.get(CONF_TRANSITION)
light = lw12.LW12Controller(host, port)

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

This comment has been minimized.

@jaypikay

jaypikay May 10, 2018

Contributor

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

This comment has been minimized.

@jaypikay

jaypikay May 10, 2018

Contributor

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

@property
def assumed_state(self) -> bool:
"""Return True if unable to access real state of the entity."""
return True

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

Please add the property should_poll and have it return False

This comment has been minimized.

@balloob

balloob May 1, 2018

Member

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

@syssi

syssi approved these changes May 22, 2018

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

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.979%
Details

@jaypikay jaypikay deleted the jaypikay:lw12wifi branch May 26, 2018

MizterB added a commit to MizterB/home-assistant that referenced this pull request May 27, 2018

Lagute LW-12 Wifi LED control (home-assistant#13307)
* Added platform lw12wifi for Lagute LW-12 Wifi Lights

Supported features:
* RGB colors
* Variable brightness
* 29 effects
* Changing transitions speed for animated effects

* Added lw12wifi to the list of omitted files to test

* Added lw12 module as new requirement for lw12wifi platform

* Added configuration example docstring for platform lw12wifi

* Updating code according to review in PR:

* Removed unused imports: enum, socket.
* Unused and not imported feature SUPPORT_FLASH was removed.
* Unused import lw12 in setup_platform method removed.
* Fixed indention for valuptuous.
* Changed check if effect is None.
* Removed personal debug output.
* Blocking function are not async anymore.

* Further improvements to satisfy PR.

* Unused import asyncio removed.
* Fixed: Return value and docstring no match up for `assumed_state`.

* Check if the set effect is supported, otherwise revert to normal light.

* Added describing missing docstrings to all functions.

* Adopted code to work with HS color setting.

* Syntactical change in comment.

* Removed redefinition of DOMAIN.

* Refactored lw12 controller setup: removed requirement for host and port in LW12Wifi class.

* Rewritten supported feature setup to a more static  expression.

* Removed unused rgb_color property

* Fixed typo in comment for set_light_option

* Changed RGB option validation schema

* Removed instance properties as config options

* Removed optional settings to be more inline with code style.

* Removed unused option from config example

* Removal of unused import

* Added property to disable state polling for this entity.

* Raise an exception if an unknown effect was selected.

* Fixed an issue with the check for known effects.

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

* Log if an unknown effect was selected.

* Added link to future documentation.

iMicknl added a commit to iMicknl/home-assistant that referenced this pull request May 31, 2018

Lagute LW-12 Wifi LED control (home-assistant#13307)
* Added platform lw12wifi for Lagute LW-12 Wifi Lights

Supported features:
* RGB colors
* Variable brightness
* 29 effects
* Changing transitions speed for animated effects

* Added lw12wifi to the list of omitted files to test

* Added lw12 module as new requirement for lw12wifi platform

* Added configuration example docstring for platform lw12wifi

* Updating code according to review in PR:

* Removed unused imports: enum, socket.
* Unused and not imported feature SUPPORT_FLASH was removed.
* Unused import lw12 in setup_platform method removed.
* Fixed indention for valuptuous.
* Changed check if effect is None.
* Removed personal debug output.
* Blocking function are not async anymore.

* Further improvements to satisfy PR.

* Unused import asyncio removed.
* Fixed: Return value and docstring no match up for `assumed_state`.

* Check if the set effect is supported, otherwise revert to normal light.

* Added describing missing docstrings to all functions.

* Adopted code to work with HS color setting.

* Syntactical change in comment.

* Removed redefinition of DOMAIN.

* Refactored lw12 controller setup: removed requirement for host and port in LW12Wifi class.

* Rewritten supported feature setup to a more static  expression.

* Removed unused rgb_color property

* Fixed typo in comment for set_light_option

* Changed RGB option validation schema

* Removed instance properties as config options

* Removed optional settings to be more inline with code style.

* Removed unused option from config example

* Removal of unused import

* Added property to disable state polling for this entity.

* Raise an exception if an unknown effect was selected.

* Fixed an issue with the check for known effects.

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

* Log if an unknown effect was selected.

* Added link to future documentation.

@balloob balloob referenced this pull request Jun 8, 2018

Merged

0.71.0 #14876

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Lagute LW-12 Wifi LED control (home-assistant#13307)
* Added platform lw12wifi for Lagute LW-12 Wifi Lights

Supported features:
* RGB colors
* Variable brightness
* 29 effects
* Changing transitions speed for animated effects

* Added lw12wifi to the list of omitted files to test

* Added lw12 module as new requirement for lw12wifi platform

* Added configuration example docstring for platform lw12wifi

* Updating code according to review in PR:

* Removed unused imports: enum, socket.
* Unused and not imported feature SUPPORT_FLASH was removed.
* Unused import lw12 in setup_platform method removed.
* Fixed indention for valuptuous.
* Changed check if effect is None.
* Removed personal debug output.
* Blocking function are not async anymore.

* Further improvements to satisfy PR.

* Unused import asyncio removed.
* Fixed: Return value and docstring no match up for `assumed_state`.

* Check if the set effect is supported, otherwise revert to normal light.

* Added describing missing docstrings to all functions.

* Adopted code to work with HS color setting.

* Syntactical change in comment.

* Removed redefinition of DOMAIN.

* Refactored lw12 controller setup: removed requirement for host and port in LW12Wifi class.

* Rewritten supported feature setup to a more static  expression.

* Removed unused rgb_color property

* Fixed typo in comment for set_light_option

* Changed RGB option validation schema

* Removed instance properties as config options

* Removed optional settings to be more inline with code style.

* Removed unused option from config example

* Removal of unused import

* Added property to disable state polling for this entity.

* Raise an exception if an unknown effect was selected.

* Fixed an issue with the check for known effects.

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

* Log if an unknown effect was selected.

* Added link to future documentation.

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.