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

Fix MagicHome LEDs with flux_led component #20733

Open
wants to merge 4 commits into
base: dev
from

Conversation

Projects
None yet
5 participants
@autinerd
Copy link

autinerd commented Feb 4, 2019

Description:

Fixing bugs which compromised the communication with MagicHome LEDs.

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.
@amelchio
Copy link
Member

amelchio left a comment

Please describe the bugs you are fixing?

(Many light PRs end up breaking bulbs different to the ones the author has)

Show resolved Hide resolved homeassistant/components/light/flux_led.py Outdated

# handle RGB mode
else:
self._bulb.setRgb(*tuple(rgb), brightness=brightness)

from time import sleep
sleep(2)

This comment has been minimized.

@amelchio

amelchio Feb 5, 2019

Member

It is not allowed to use time.sleep() since this will block other integrations from using the thread.

This comment has been minimized.

@autinerd

autinerd Feb 6, 2019

Author

Oh, I just used it for testing and forgot to remove it, thank you!

This comment has been minimized.

@Riwaha

Riwaha Feb 6, 2019

Some of the models seem to have a transition period ≈1 second, there needs to be a delay or scheduled call for those models, otherwise issue #19795 happens.

This comment has been minimized.

@autinerd

autinerd Feb 7, 2019

Author

Yeah, the problem is, the lamp needs about 1 second to tell about the new color.

This comment has been minimized.

@amelchio

amelchio Feb 7, 2019

Member

If you need to sleep, the best way is probably to implement async_turn_on() and use asyncio.sleep()

Let me know if you need help with that.

This comment has been minimized.

@autinerd

autinerd Feb 9, 2019

Author

So, it's now with asyncio sleep.

@MartinHjelmare MartinHjelmare changed the title bug fixing for MagicHome LEDs with flux_led component. Fix MagicHome LEDs with flux_led component Feb 6, 2019

@autinerd

This comment has been minimized.

Copy link
Author

autinerd commented Feb 6, 2019

The main purpose of this is that the RGB color is stored. When it is not stored, changing from RGB to warmwhite changes RGB to 0,0,0. When afterwards the color is changed from warmwhite to RGB, the value is set to 0,0,0, which let the bulb stuck, even with the Magic Home App it's difficult to let the bulb shine again.

Another question: How can I tell hass that the next update should be in 2 seconds? After changing color, the bulb needs a second until the new values are returned from the bulb, so the UI stucks at the old value for a few seconds.

self._bulb.setRgbw(*tuple(rgb), w=white, brightness=brightness)

if white is None:
self._bulb.setRgbw(*tuple(self._color))

This comment has been minimized.

@Riwaha

Riwaha Feb 7, 2019

When setting brightness from an off state, hs_color will be None, and this will not affect the brightness, as that happens on lines

if hs_color and brightness:
self._color = color_util.color_hsv_to_RGB(hs_color[0], hs_color[1],
brightness / 255 * 100)
instead it will use self._color, which will make it full brightness

This comment has been minimized.

@autinerd

autinerd Feb 9, 2019

Author

I implemented it now, so when only brightness is given, it uses the saved color and calculates it with the given brightness.

@amelchio
Copy link
Member

amelchio left a comment

I expect self._bulb to be doing I/O and in that case it cannot be called on the event loop.

You can move the meat to a separate method that is scheduled with async_add_executor_job() to make it execute outside of async context.

Probably you will need a lock if you start testing for races.

So something like

async def async_turn_on(self, **kwargs):
    async with self.lock:
        await self.hass.async_add_executor_job(self.meat, **kwargs)
        await asyncio.sleep(2)
@@ -193,7 +195,7 @@ def brightness(self):
@property
def hs_color(self):
"""Return the color property."""
return color_util.color_RGB_to_hs(*self._bulb.getRgb())
return color_util.color_RGB_to_hsv(*self._color)[0:2]

This comment has been minimized.

@amelchio

amelchio Feb 9, 2019

Member

color_RGB_to_hs() does exactly that array slicing.

"""Turn the specified or all lights on."""
if not self.is_on:
self._bulb.turnOn()
self._bulb.turnOn()

This comment has been minimized.

@amelchio

amelchio Feb 9, 2019

Member

If this does I/O with the bulb, it cannot be in async context.


if white is None and self._mode == MODE_RGBW:
white = self.white_value
if hs_color and brightness:

This comment has been minimized.

@amelchio

amelchio Feb 9, 2019

Member

You set brightness above, so is this test needed?

if hs_color and brightness:
self._color = color_util.color_hsv_to_RGB(hs_color[0], hs_color[1],
brightness / 255 * 100)
elif brightness and (hs_color is None) and self._mode != MODE_WHITE:

This comment has been minimized.

@amelchio

amelchio Feb 9, 2019

Member

You set brightness above, so is this test needed?

elif brightness and (hs_color is None) and self._mode != MODE_WHITE:
hsv = color_util.color_RGB_to_hsv(*self._color)
self._color = color_util.color_hsv_to_RGB(hsv[0], hsv[1],
brightness / 255 * 100)

This comment has been minimized.

@amelchio

amelchio Feb 9, 2019

Member

This will be much simpler if you store HSV values in self._color.

@autinerd autinerd referenced this pull request Feb 13, 2019

Closed

add support for Zengge Wi-Fi bulbs #19568

5 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment