Skip to content

Add new iGlo component#11171

Merged
fabaff merged 4 commits into
home-assistant:devfrom
jesserockz:iglo-lights
Jan 6, 2018
Merged

Add new iGlo component#11171
fabaff merged 4 commits into
home-assistant:devfrom
jesserockz:iglo-lights

Conversation

@jesserockz
Copy link
Copy Markdown
Member

@jesserockz jesserockz commented Dec 16, 2017

Description:

Adding support for new iGlo component

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

Example entry for configuration.yaml (if applicable):

light:
  - platform: iglo
    host: "192.168.1.10"
    name: Kids Light
    port: 8080

Checklist:

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

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

return


def turn_off(self, **kwargs):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

too many blank lines (2)

port = config.get(CONF_PORT)
add_devices([IGloLamp(name, host, port)])

class IGloLamp(Light):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.string,
})

def setup_platform(hass, config, add_devices, discovery_info=None):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

expected 2 blank lines, found 1

from homeassistant.components.light import (
ATTR_BRIGHTNESS, ATTR_RGB_COLOR, ATTR_COLOR_TEMP,
SUPPORT_BRIGHTNESS, SUPPORT_COLOR_TEMP, SUPPORT_RGB_COLOR
Light, PLATFORM_SCHEMA
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

SyntaxError: invalid syntax

Add extra blank lines
Comment thread homeassistant/components/light/iglo.py Outdated
"""Initialize the light."""
from iglo import Lamp
self._name = name
self._host = host
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not used.

Comment thread homeassistant/components/light/iglo.py Outdated
from iglo import Lamp
self._name = name
self._host = host
self._port = port
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dito.

Comment thread homeassistant/components/light/iglo.py Outdated
self._host = host
self._port = port
self._lamp = Lamp(0, host, port)
self.update()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

Comment thread homeassistant/components/light/iglo.py Outdated
host = config.get(CONF_HOST)
name = config.get(CONF_NAME)
port = config.get(CONF_PORT)
add_devices([IGloLamp(name, host, port)])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use True as second parameter.

I assume that there will be a exception of the device is not present.

Comment thread homeassistant/components/light/iglo.py Outdated
self._host = host
self._port = port
self._lamp = Lamp(0, host, port)
self.update()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

Comment thread homeassistant/components/light/iglo.py Outdated
"""Turn the light on."""
if not self._on:
self._lamp.switch(True)
self._on = True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You shouldn't change any instance attributes that store state in the turn on/off methods. The update method will be called after turn_on or turn_off is called, since this is a polling entity, so state will be updated that way.

Comment thread homeassistant/components/light/iglo.py Outdated
self._lamp.switch(True)
self._on = True
if ATTR_BRIGHTNESS in kwargs:
self._brightness = int((kwargs[ATTR_BRIGHTNESS] / 255.0) * 200.0)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

Comment thread homeassistant/components/light/iglo.py Outdated
return

if ATTR_RGB_COLOR in kwargs:
self._rgb = kwargs[ATTR_RGB_COLOR]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

Comment thread homeassistant/components/light/iglo.py Outdated
return

if ATTR_COLOR_TEMP in kwargs:
self._color_temp = 255 - kwargs[ATTR_COLOR_TEMP]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

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

def turn_off(self, **kwargs):
"""Turn the light off."""
self._on = False
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See above.

Remove unused variables
Update before add
@jesserockz
Copy link
Copy Markdown
Member Author

Thanks for the tips @fabaff & @MartinHjelmare

Copy link
Copy Markdown
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

🐦

@fabaff fabaff dismissed MartinHjelmare’s stale review January 6, 2018 20:52

Comments addressed

@fabaff fabaff merged commit 3fbf09e into home-assistant:dev Jan 6, 2018
@property
def min_mireds(self):
"""Return the coldest color_temp that this light supports."""
return 1
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just saw this merged so I know I am too late to the party.

Anyway, this is supposed to be in mired, the unknown "reciprocal megakelvin" unit.

You find the mired value with this formula: 1000000/kelvin. So if the light does 7000 Kelvin as its coldest white, the min_mired value should be 143.

Same goes for max_mireds and color_temp.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't really understand the mirad thing, but the lights accept a colour temp value between 1 and 255 to change between warm and cool white. Maybe I will look into it more once I move back into the part of my house with my new lights and start using this on my main HA.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doing it this way will mean that examples for other lights do not work properly. It also breaks the kelvin alias for white temperature.

But now you know where the problem is, once you notice :)

@balloob balloob mentioned this pull request Jan 11, 2018
@jesserockz jesserockz deleted the iglo-lights branch February 8, 2018 04:16
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants