Skip to content
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 flux_led only-white controllers (and remove explicit declaration as RGBW in automatic add) #22210

Merged
merged 13 commits into from Apr 16, 2019

Conversation

Projects
None yet
4 participants
@autinerd
Copy link
Contributor

autinerd commented Mar 20, 2019

Description:

When using only-white Magic Home controllers, the update routine didn't work properly. Now only-white controllers are treated as before my update for RGBW controllers.

Old description:

When flux_led devices are found with automatic discovery, they are automatically seen as RGBW devices, even when they are actually RGB devices, This pull request fixes this issue.
Issue #22161 will be completely fixed in the next pull request, because it needs an update to the flux_led module as well

Related issue (if applicable): fixes #22405

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.

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 or updated 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.
@amelchio
Copy link
Contributor

amelchio left a comment

This is tricky, we need to find someone that can test it.

@@ -151,7 +151,6 @@ def setup_platform(hass, config, add_entities, discovery_info=None):
if ipaddr in light_ips:
continue
device['name'] = '{} {}'.format(device['id'], ipaddr)
device[ATTR_MODE] = MODE_RGBW

This comment has been minimized.

Copy link
@amelchio

amelchio Mar 25, 2019

Contributor

The device[ATTR_MODE] value is always read in __init__() so this will make it always raise an exception. I think it could work if you set it to None instead.

Just a note for others, the mode is set automatically if not explicitly defined:

# After bulb object is created the status is updated. We can
# now set the correct mode if it was not explicitly defined.
if not self._mode:
if self._bulb.rgbwcapable:
self._mode = MODE_RGBW
else:
self._mode = MODE_RGB

(whether that works for all devices, I don't know)

@autinerd autinerd changed the title Remove explicit declaration of automatic found devices as RGBW in flux_led Fix flux_led only-white controllers (and remove explicit declaration as RGBW in automatic add) Apr 4, 2019

@autinerd

This comment has been minimized.

Copy link
Contributor Author

autinerd commented Apr 5, 2019

@amelchio this pull request shall remove the bug from #22405 and my previous pull request

@amelchio

This comment has been minimized.

Copy link
Contributor

amelchio commented Apr 5, 2019

Great. I can review after the weekend. Meanwhile, is this tested with affected devices?

@autinerd

This comment has been minimized.

Copy link
Contributor Author

autinerd commented Apr 5, 2019

The problem is that I don't have a only-white controller, so I can't test it. The behaviour for other types has not changed.

if not self.is_on:
self._bulb.turnOn()
if kwargs.get(ATTR_BRIGHTNESS) is None:
return

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 9, 2019

Contributor

This is wrong. I think you are addressing a UI issue but we can turn on with any parameters using light.turn_on.

Also, preferably turn on unconditionally because our local state could be stale.

I am not quite sure what you are trying to do so I cannot advise on a better way :)

This comment has been minimized.

Copy link
@autinerd

autinerd Apr 9, 2019

Author Contributor

One (in my opinion) not so optimal thing is, that turn_on is for turning stuff on and changing the state of an entity. What I want to do is that when the lamp is off and the user want to turn it on, it just turns on and nothing else (except when brightness is set, because it's possible to turn the light on with the brightness slider as well). When the lamp is already on, then the state should be changed.

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 9, 2019

Contributor

Yes, light.turn_on is indeed overloaded but each light platform should not work around that. It is quite possible to turn on to an RGB color, for example with an automation.

Instead, only call self._bulb.setRgb() and friends if relevant kwargs are actually passed.

This comment has been minimized.

Copy link
@autinerd

autinerd Apr 10, 2019

Author Contributor

Oh yeah, I forgot the automation, thank you. I will correct this.

@autinerd

This comment has been minimized.

Copy link
Contributor Author

autinerd commented Apr 10, 2019

I will test the stuff in the evening at home.

@autinerd

This comment has been minimized.

Copy link
Contributor Author

autinerd commented Apr 10, 2019

So, I have now tested the changes, they are fully working for RGBW devices (now waiting time is reduced to 1 second as well, this is enough)

@amelchio
Copy link
Contributor

amelchio left a comment

This is getting really complex, so many cases to handle :-(. I have some proposals that I think will simplify the code a bit.


def _turn_on(self, **kwargs):
"""Turn the specified or all lights on."""
self._bulb.turnOn()
if not self.is_on:

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 10, 2019

Contributor

Is there a reason for this condition? As I said, the state can be stale so it is better to not trust it.

self._custom_effect[CONF_COLORS],
self._custom_effect[CONF_SPEED_PCT],
self._custom_effect[CONF_TRANSITION])
if all(None for item in [hs_color, brightness, effect, white]):

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 10, 2019

Contributor

That looks weird, it will never be true? You probably meant it like this:

        if all(item is None for item in [hs_color, brightness, effect, white]):
# handle RGB mode
if brightness is not None:
self._bulb.setWarmWhite255(brightness)
return

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 10, 2019

Contributor

Unindent this return and remove the else: below

self._bulb.setPresetPattern(EFFECT_MAP[effect], 50)
return
# Preserve current brightness on color/white level change
if brightness is None:

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 10, 2019

Contributor

Move this block into the if hs_color is not None: below.


if hs_color is not None:
color = (hs_color[0], hs_color[1], brightness / 255 * 100)
elif brightness is not None and hs_color is None:

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 10, 2019

Contributor

Remove and hs_color is None, that's the "else" part of the "elif".

autinerd added some commits Apr 11, 2019

@amelchio
Copy link
Contributor

amelchio left a comment

Thanks, I think that improved the readability. One issue left though.

return
# Preserve current brightness on color/white level change
if hs_color is not None:
if brightness is not None:

This comment has been minimized.

Copy link
@amelchio

amelchio Apr 11, 2019

Contributor

Should be if brightness is None.

This comment has been minimized.

Copy link
@autinerd

autinerd Apr 11, 2019

Author Contributor

Thank you very much for your patience 😅

autinerd and others added some commits Apr 11, 2019

@amelchio
Copy link
Contributor

amelchio left a comment

Looks good now, I just moved some code to fix a too long line.

It would still be good to get someone to test the fixes with the light types that you do not have around.

@amelchio

This comment has been minimized.

Copy link
Contributor

amelchio commented Apr 12, 2019

Actually, can we just remove the bit that I moved since the same code seems to be present a bit further down in update()?

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Apr 15, 2019

@autinerd I think that when @amelchio said "we", he meant that it's a change you should make to get this PR approved.

@balloob balloob merged commit 4813818 into home-assistant:dev Apr 16, 2019

12 checks passed

ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 4803f31...b476390
Details
codecov/project 93.85% (target 90%)
Details

@wafflebot wafflebot bot removed the in progress label Apr 16, 2019

@autinerd

This comment has been minimized.

Copy link
Contributor Author

autinerd commented Apr 16, 2019

Oh yeah, thank you, I was just a bit confused. Thank you very much and hopefully there are no more problems with the logic of this component!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.