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

Color temperature manual range setting, HS(Hue, Saturation) Topic Support #14308

Closed
wants to merge 2 commits into
base: dev
from

Conversation

Projects
None yet
9 participants
@stkang
Copy link

stkang commented May 6, 2018

Light / MQTT

Description:

Add Color temperature manual range setting, HS(Hue, Saturation) Topic Support

Related issue (if applicable): fixes #

  1. The color temperature was expanded to 153-500.
  2. HS(Hue, Saturation) Topic

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

Example entry for configuration.yaml (if applicable):

light:
  - platform: mqtt
    name: bed_led_strip
    command_topic: "smartthings/Bed Led Strip/switch"
    state_topic: "smartthings/Bed Led Strip/switch"
    brightness_command_topic: "smartthings/Bed Led Strip/level"
    brightness_state_topic: "smartthings/Bed Led Strip/level"
    brightness_scale: 100
    color_temp_command_topic: "smartthings/Bed Led Strip/colorTemperature"
    color_temp_state_topic: "smartthings/Bed Led Strip/colorTemperature"
    color_temp_min_mireds: 2700
    color_temp_max_mireds: 6500
    hue_command_topic: "smartthings/Bed Led Strip/hue"
    hue_state_topic: "smartthings/Bed Led Strip/hue"
    hue_value_template: '{{ (value | float / 100 * 360) | int }}'
    hue_command_template: '{{ (value | float / 360 * 100) | int }}'
    saturation_command_topic: "smartthings/Bed Led Strip/saturation"
    saturation_state_topic: "smartthings/Bed Led Strip/saturation"
    on_command_type: "last"
    payload_on: "on"
    payload_off: "off"
    retain: true

[added option]
color_temp_min_mireds: 2700
color_temp_max_mireds: 6500
hue_command_topic: "smartthings/Bed Led Strip/hue"
hue_state_topic: "smartthings/Bed Led Strip/hue"
hue_value_template: '{{ (value | float / 100 * 360) | int }}'
hue_command_template: '{{ (value | float / 360 * 100) | int }}'
saturation_command_topic: "smartthings/Bed Led Strip/saturation"
saturation_state_topic: "smartthings/Bed Led Strip/saturation"

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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 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.
@homeassistant

This comment has been minimized.

Copy link

homeassistant commented May 6, 2018

Hi @stkang,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@@ -212,6 +254,9 @@ def __init__(self, name, effect_list, topic, templates, qos,
SUPPORT_WHITE_VALUE)
self._supported_features |= (
topic[CONF_XY_COMMAND_TOPIC] is not None and SUPPORT_COLOR)
self._supported_features |= (
topic[CONF_HUE_COMMAND_TOPIC] is not None and \

This comment has been minimized.

@houndci-bot

houndci-bot May 6, 2018

the backslash is redundant between brackets

@@ -176,6 +212,9 @@ def __init__(self, name, effect_list, topic, templates, qos,
self._optimistic = optimistic or topic[CONF_STATE_TOPIC] is None
self._optimistic_rgb = \
optimistic or topic[CONF_RGB_STATE_TOPIC] is None
self._optimistic_hs_color = \
optimistic or (topic[CONF_HUE_STATE_TOPIC] is None and \
topic[CONF_SATURATION_STATE_TOPIC])

This comment has been minimized.

@houndci-bot

houndci-bot May 6, 2018

continuation line under-indented for visual indent

@@ -176,6 +212,9 @@ def __init__(self, name, effect_list, topic, templates, qos,
self._optimistic = optimistic or topic[CONF_STATE_TOPIC] is None
self._optimistic_rgb = \
optimistic or topic[CONF_RGB_STATE_TOPIC] is None
self._optimistic_hs_color = \
optimistic or (topic[CONF_HUE_STATE_TOPIC] is None and \

This comment has been minimized.

@houndci-bot

houndci-bot May 6, 2018

the backslash is redundant between brackets

@@ -74,6 +88,10 @@
vol.Optional(CONF_COLOR_TEMP_COMMAND_TOPIC): mqtt.valid_publish_topic,
vol.Optional(CONF_COLOR_TEMP_STATE_TOPIC): mqtt.valid_subscribe_topic,
vol.Optional(CONF_COLOR_TEMP_VALUE_TEMPLATE): cv.template,
vol.Optional(CONF_COLOR_TEMP_MIREDS_MIN, default=DEFAULT_COLOR_TEMP_MIREDS_MIN):
vol.All(vol.Coerce(int), vol.Range(min=1)),
vol.Optional(CONF_COLOR_TEMP_MIREDS_MAX, default=DEFAULT_COLOR_TEMP_MIREDS_MAX):

This comment has been minimized.

@houndci-bot

houndci-bot May 6, 2018

line too long (84 > 79 characters)

@@ -74,6 +88,10 @@
vol.Optional(CONF_COLOR_TEMP_COMMAND_TOPIC): mqtt.valid_publish_topic,
vol.Optional(CONF_COLOR_TEMP_STATE_TOPIC): mqtt.valid_subscribe_topic,
vol.Optional(CONF_COLOR_TEMP_VALUE_TEMPLATE): cv.template,
vol.Optional(CONF_COLOR_TEMP_MIREDS_MIN, default=DEFAULT_COLOR_TEMP_MIREDS_MIN):

This comment has been minimized.

@houndci-bot

houndci-bot May 6, 2018

line too long (84 > 79 characters)

@homeassistant homeassistant added cla-signed and removed cla-needed labels May 6, 2018

@@ -37,6 +37,8 @@
CONF_COLOR_TEMP_COMMAND_TOPIC = 'color_temp_command_topic'
CONF_COLOR_TEMP_STATE_TOPIC = 'color_temp_state_topic'
CONF_COLOR_TEMP_VALUE_TEMPLATE = 'color_temp_value_template'
CONF_COLOR_TEMP_MIREDS_MIN = 'color_temp_mireds_min'
CONF_COLOR_TEMP_MIREDS_MAX = 'color_temp_mireds_max'

This comment has been minimized.

@OttoWinter

OttoWinter May 6, 2018

Member

I think we should call these options color_temp_min_mireds and color_temp_max_mireds as those are the names used throughout Home Assistant AFAIK.

This comment has been minimized.

@stkang

stkang May 6, 2018

Author

I'll fix it. Thanks.

vol.Optional(CONF_HUE_SCALE, default=DEFAULT_HUE_SCALE):
vol.All(vol.Coerce(int), vol.Range(min=1)),
vol.Optional(CONF_SATURATION_COMMAND_TOPIC): mqtt.valid_publish_topic,
vol.Optional(CONF_SATURATION_STATE_TOPIC): mqtt.valid_subscribe_topic,

This comment has been minimized.

@OttoWinter

OttoWinter May 6, 2018

Member

I don't think hue and saturation should be two separate topics as XY and RGB both also have all components in a single topic/template.

Also, hue without saturation doesn't make much sense. And saturation without hue doesn't do so either.

This comment has been minimized.

@stkang

stkang May 6, 2018

Author

SmartThings Mqtt detects and transmits changes in a single theme of hue and saturation. And so is the command.

vol.Optional(CONF_HUE_STATE_TOPIC): mqtt.valid_subscribe_topic,
vol.Optional(CONF_HUE_VALUE_TEMPLATE): cv.template,
vol.Optional(CONF_HUE_SCALE, default=DEFAULT_HUE_SCALE):
vol.All(vol.Coerce(int), vol.Range(min=1)),

This comment has been minimized.

@OttoWinter

OttoWinter May 6, 2018

Member

Do we really need a hue scale? I mean users can easily convert the hue from their scale of choice to the internal 0-360 range using templates.

Also, having the hue scale be an integer doesn't make much sense to me because maybe some people want the range 0-2π

This comment has been minimized.

@stkang

stkang May 6, 2018

Author

I will remove the scale and add a command template.

@callback
def color_temp_received(topic, payload, qos):
"""Handle new MQTT messages for color temperature."""
self._color_temp = int(templates[CONF_COLOR_TEMP](payload))
self._color_temp = int(float(templates[CONF_COLOR_TEMP](payload)))

This comment has been minimized.

@OttoWinter

OttoWinter May 6, 2018

Member

? If we don't use floating point numbers internally, why accept floating point numbers? It would IMO just cause confusion because some users would expect us to handle color temp output in floating point to numbers too.

This comment has been minimized.

@stkang

stkang May 6, 2018

Author

This can be resolved by template, so I will delete it.

Thank you for all your review

@@ -87,6 +105,14 @@
vol.Optional(CONF_RGB_STATE_TOPIC): mqtt.valid_subscribe_topic,
vol.Optional(CONF_RGB_VALUE_TEMPLATE): cv.template,
vol.Optional(CONF_STATE_VALUE_TEMPLATE): cv.template,
vol.Optional(CONF_HUE_COMMAND_TOPIC): mqtt.valid_publish_topic,
vol.Optional(CONF_HUE_STATE_TOPIC): mqtt.valid_subscribe_topic,
vol.Optional(CONF_HUE_VALUE_TEMPLATE): cv.template,

This comment has been minimized.

@OttoWinter

OttoWinter May 6, 2018

Member

Please also add a CONF_HS_COMMAND_TEMPLATE.

This comment has been minimized.

@stkang

stkang May 6, 2018

Author

Why do I need CONF_HS_COMMAND_TEMPLATE?

Added CONF_HUE_COMMAND_TEMPLATE.

@@ -176,6 +211,9 @@ def __init__(self, name, effect_list, topic, templates, qos,
self._optimistic = optimistic or topic[CONF_STATE_TOPIC] is None
self._optimistic_rgb = \
optimistic or topic[CONF_RGB_STATE_TOPIC] is None
self._optimistic_hs_color = \
optimistic or (topic[CONF_HUE_STATE_TOPIC] is None and
topic[CONF_SATURATION_STATE_TOPIC] is None)

This comment has been minimized.

@houndci-bot

houndci-bot May 6, 2018

continuation line under-indented for visual indent

This comment has been minimized.

@stkang

stkang May 6, 2018

Author

I do not know how to fix it.
If anyone knows, please let me know the example.

This comment has been minimized.

@amelchio

amelchio May 6, 2018

Member

It wants you to indent line 216 so it aligns with the bracket on line 215.

(Click the Details link on the Travis status below to get to the full build output which contains a few more issues.)

@andriej

This comment was marked as off-topic.

Copy link
Contributor

andriej commented May 6, 2018

Is there possibility to add also RGBW support?
Currently there's no way to use i.e. H801 with Tasmota and RGBW (or RGBWW) led strip.

Connected with: #12533

@fabaff fabaff changed the title light-mqtt - Color temperature manual range setting, HS(Hue, Saturation) Topic Support Color temperature manual range setting, HS(Hue, Saturation) Topic Support May 10, 2018

@rafabene

This comment has been minimized.

Copy link

rafabene commented Jul 18, 2018

What does this PR need to have it merged and part of release? cc @tchellomello

@frenck frenck added the docs-missing label Jul 21, 2018

@frenck

This comment has been minimized.

Copy link
Member

frenck commented Jul 21, 2018

The PR listed as the documentation PR does not exist (the PR number is way to high).
I've added the docs-missing label.

tpl = self._templates[CONF_HUE_COMMAND_TEMPLATE]
if tpl:
hue_value = int(tpl.async_render_with_possible_json_value(
str(hs_color[0])))

This comment has been minimized.

@balloob

balloob Sep 21, 2018

Member

this is completely wrong. you can't convert the passed in color to a string and say it's possible JSON value?

else:
hue_value = int(hs_color[0])

saturation_value = int(hs_color[1])

This comment has been minimized.

@balloob

balloob Sep 21, 2018

Member

No need to convert to int?

@balloob
Copy link
Member

balloob left a comment

This requires tests to be added. Which is good because it will weed out some of the weirdness from the code.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Nov 6, 2018

This PR seems to have gone stale. Closing it.

@balloob balloob closed this Nov 6, 2018

@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.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.