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.

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

Provide an individual color temperature range per Yeelight model #17305

Merged
merged 5 commits into from
Oct 12, 2018
Merged

Provide an individual color temperature range per Yeelight model #17305

merged 5 commits into from
Oct 12, 2018

Conversation

syssi
Copy link
Member

@syssi syssi commented Oct 10, 2018

Description:

Related issue (if applicable): fixes #13785

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

Example entry for configuration.yaml (if applicable):

light:
  - platform: yeelight
    devices:
      192.168.1.25:
        name: Living Room
        transition: 1000
        use_music_mode: True #(defaults to False)
        save_on_change: False #(defaults to True)
        model: color1
      192.168.1.13:
        name: Front Door
        model: ceiling1

@syssi syssi requested a review from rytilahti as a code owner October 10, 2018 06:48
@ghost ghost assigned syssi Oct 10, 2018
@ghost ghost added the in progress label Oct 10, 2018
houndci-bot
houndci-bot reviewed Oct 10, 2018
View reviewed changes
homeassistant/components/light/yeelight.py Outdated
if self._min_mireds is None:
model_specs = self._bulb.get_model_specs()
self._min_mireds = kelvin_to_mired(model_specs['color_temp']['max'])
self._max_mireds = kelvin_to_mired(model_specs['color_temp']['min'])
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 (84 > 79 characters)

homeassistant/components/light/yeelight.py Outdated
@@ -306,6 +309,11 @@ def update(self) -> None:
if self._bulb_device.bulb_type == yeelight.BulbType.Color:
self._supported_features = SUPPORT_YEELIGHT_RGB

if self._min_mireds is None:
model_specs = self._bulb.get_model_specs()
self._min_mireds = kelvin_to_mired(model_specs['color_temp']['max'])
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 (84 > 79 characters)

homeassistant/components/light/yeelight.py Outdated
@@ -279,7 +282,7 @@ def _bulb(self) -> 'yeelight.Bulb':
import yeelight
if self._bulb_device is None:
try:
self._bulb_device = yeelight.Bulb(self._ipaddr)
self._bulb_device = yeelight.Bulb(self._ipaddr, model=self._model)
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 (82 > 79 characters)

@syssi
Copy link
Member Author

syssi commented Oct 10, 2018

I missed to update the requirements_all.txt. Will be fixed asap.

balloob
balloob reviewed Oct 10, 2018
View reviewed changes
homeassistant/components/light/yeelight.py
@@ -46,6 +47,7 @@
vol.Optional(CONF_TRANSITION, default=DEFAULT_TRANSITION): cv.positive_int,
vol.Optional(CONF_MODE_MUSIC, default=False): cv.boolean,
vol.Optional(CONF_SAVE_ON_CHANGE, default=False): cv.boolean,
vol.Optional(CONF_MODEL): cv.string,
Copy link
Member

Choose a reason for hiding this comment

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

Why would we add this? Also doesn't seem related to the PR description

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not possible to determine the yeelight model via the api. If you provide the model additional features can be enabled by the yeelight library: https://github.com/home-assistant/home-assistant/pull/17305/files#diff-1395165791bf7671502a6e2e7bd7d4cbR286

Copy link
Member

Choose a reason for hiding this comment

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

Can't we get the library to just try calling different APIs to see what is available and determine model capabilities like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case we would discover the device twice.

Copy link
Member

Choose a reason for hiding this comment

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

I asked about a possibility for getting an extension for the api and the yeelight people were not at least completely against it: https://forum.yeelight.com/t/topic/7075/5 . At the moment that model information is only available either via mDNS or their custom SSDP (related home-assistant-libs/netdisco#193).

Copy link
Member

Choose a reason for hiding this comment

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

Here is an idea: add config entries to Yeelight, add device info so we can store the device model in the registry and read that during setup.

@quthla
Copy link
Contributor

quthla commented Oct 10, 2018

You should adapt this to the newly introduced bulb type WhiteColor

@syssi
Copy link
Member Author

syssi commented Oct 11, 2018

@quthla Does tie BulbType WhiteTemp support effects like the RGB one?

@quthla
Copy link
Contributor

quthla commented Oct 11, 2018 •
edited
Loading

There's candle flicker and flash notify in the app but I'm not sure whether these are effects

@balloob balloob merged commit 5a00cc5 into home-assistant:dev Oct 12, 2018
@ghost ghost removed the in progress label Oct 12, 2018
@balloob
Copy link
Member

balloob commented Oct 12, 2018

Please add config entries and device registry support next, so we don't have users needing to add their own models in their config

MartinHjelmare
MartinHjelmare reviewed Oct 12, 2018
View reviewed changes
homeassistant/components/light/yeelight.py

# Not using hostname, as it seems to vary.
name = "yeelight_%s_%s" % (device_type,
name = "yeelight_%s_%s" % (legacy_device_type,
Copy link
Member

Choose a reason for hiding this comment

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

Use new style string formatting.

@balloob balloob mentioned this pull request Oct 26, 2018
Merged
@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019
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

@MartinHjelmare MartinHjelmare MartinHjelmare left review comments

@balloob balloob balloob left review comments

@rytilahti rytilahti Awaiting requested review from rytilahti

Assignees

@syssi syssi

Labels
cla-signed Hacktoberfest integration: yeelight small-pr PRs with less than 30 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Yeelight Wifi Lamp can't change Color Temperature
8 participants
@syssi @quthla @balloob @MartinHjelmare @rytilahti @houndci-bot @bachya @homeassistant