8000 Fixed Tradfri whitebulbs handling after #9703 by matemaciek · Pull Request #10040 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixed Tradfri whitebulbs handling after #9703 #10040

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Nov 2, 2017
Merged

Fixed Tradfri whitebulbs handling after #9703 #10040

merged 15 commits into from
Nov 2, 2017

Conversation

matemaciek
Copy link
Contributor

No description provided.

houndci-bot
houndci-bot reviewed Oct 22, 2017
View reviewed changes
homeassistant/components/light/tradfri.py Outdated
if self._light_control.can_set_kelvin:
kelvin_color = self._light_data.kelvin_color_inferred
if kelvin_color is not None:
return color_util.color_temperature_kelvin_to_mired(kelvin_color)
Copy link

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

@ggravlingen
Copy link
Contributor

Unit tested in my environment and it works. Before this fix, I'm unable to control my TRADFRI bulb E27 W opal 1000lm bulbs properly and also, the computer load was unusually high.

@lwis
Copy link
Member
lwis commented Oct 22, 2017

Why is this required if we're not setting SUPPORT_COLOR_TEMP?

@matemaciek
Copy link
Contributor Author

It seems that UI is deciding wheter to treat bulb as supporting temperature basing on color_temp instead of on SUPPORT_COLOR_TEMP. This PR reverts regression introduced accidentally in #9703, after / if UI behaviour is fixed, we can do more clean up here.

@balloob
Copy link
Member
balloob commented Oct 23, 2017

We should not work around bugs in our UI, instead we should open PRs against the UI and fix it 👍

@ggravlingen
Copy link
Contributor

@lwis @balloob #9703 breaks functionality for white-bulbs for me (and possibly also causes performance issues, but I haven't been able to isolate this), so you might want to consider rolling it back until UI is fixed.

@matemaciek
Copy link
Contributor Author

Can you point me to code responsible for when temperature slider is shown? Just a rough direction would be appreciated (-:

@matemaciek
Copy link
Contributor Author
matemaciek commented Oct 23, 2017 •
edited
Loading

@balloob I agree, but just after #9703 has been merged @ggravlingen noticed regression with white lights. We have now some options:

  • fix UI, while dev remains broken for white lights
  • introduce this workaround for white lights, fix UI, remove workaround

Isn't second option better for users?

@matemaciek
Copy link
Contributor Author

Am I right that I should look at https://github.com/home-assistant/home-assistant-polymer/blob/master/src/more-infos/more-info-light.html ?

@matemaciek
Copy link
Contributor Author
matemaciek commented Oct 23, 2017 •
edited
Loading

Idea: not hardcode values, but convert min and max mireds here:
--ha-slider-background: -webkit-linear-gradient(right, rgb(255, 160, 0) 0%, white 50%, rgb(166, 209, 255) 100%);
To many ideas given so little free time (-:

@ggravlingen
Copy link
Contributor

@matemaciek this might be a good place to start:
home-assistant/frontend#263

@ggravlingen
Copy link
Contributor

After the latest commit, this is what I'm seeing in the UI for white bulbs
image

{
  "power_source": "Internal Battery",
  "model_number": "TRADFRI bulb E27 W opal 1000lm",
  "brightness": 169,
  "battery_level": null,
  "serial": "",
  "supported_features": 33,
  "firmware_version": "1.2.214",
  "manufacturer": "IKEA of Sweden",
  "rgb_color": [
    248,
    200,
    114
  ],
  "friendly_name": "Hall 1"
}

@matemaciek has the same bulb, which also returns color temp as a state. Odd behaviour! Could it be different versions of the white bulb?

@ggravlingen ggravlingen mentioned this pull request Nov 2, 2017
Merged
2 tasks
@matemaciek
Copy link
Contributor Author

I've removed workaround for UI misbehaviour experienced in my installation, maybe it can be merged now?

@lwis lwis merged commit 1c2224c into home-assistant:dev Nov 2, 2017
@balloob balloob mentioned this pull request Nov 2, 2017
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

Assignees
No one assigned
Labels
cla-signed Hacktoberfest integration: tradfri
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@matemaciek @ggravlingen @lwis @balloob @houndci-bot @emlove @homeassistant
0