10000 Mqtt light options to fix #9330 and #7810 by TD22057 · Pull Request #9829 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Mqtt light options to fix #9330 and #7810 #9829

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

Merged
merged 9 commits into from
Oct 31, 2017

Conversation

TD22057
Copy link
Contributor
@TD22057 TD22057 commented Oct 12, 2017

Description:

Added an option to MQTT light to control when and if an ON command topic is published. Currently, ON is always sent after changing the light style (brightness, color, etc). This option allows the user to specify if ON should be sent before style topics, after style topics (the default to maintain current behavior), or to use the brightness topic as the ON command.

Related issue (if applicable): fixes #9330 and #7810

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

Example entry for configuration.yaml (if applicable):

Example added to docs in io PR above

  - platform: mqtt
    name: "Brightness light"
    state_topic: "office/light/status"
    command_topic: "office/light/switch"
    payload_off: "OFF"
    brightness_state_topic: 'office/rgb1/light/brightness'
    brightness_command_topic: 'office/rgb1/light/brightness/set'
    on_command_type: 'BRIGHTNESS'

Checklist:

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

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • 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:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@TD22057
Copy link
Contributor Author
TD22057 commented Oct 13, 2017

Don't merge this one yet - I'm looking into a possible issue reported by a tester.

@TD22057
Copy link
Contributor Author
TD22057 commented Oct 13, 2017

This is OK to merge (issue in prev comment was a user misunderstanding).


DEFAULT_BRIGHTNESS_SCALE = 255
DEFAULT_NAME = 'MQTT Light'
DEFAULT_OPTIMISTIC = False
DEFAULT_PAYLOAD_OFF = 'OFF'
DEFAULT_PAYLOAD_ON = 'ON'
DEFAULT_WHITE_VALUE_SCALE = 255
DEFAULT_ON_COMMAND_TYPE = "LAST"

VALUES_ON_COMMAND_TYPE = ["FIRST", "LAST", "BRIGHTNESS"]
Copy link
Member

Choose a reason for hiding this comment

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

What the advantage to use capital letters here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None. ON and OFF were capitalized so that's what I followed. It doesn't make a big difference if they're upper, lower, or case insensitive.

Copy link
Member

Choose a reason for hiding this comment

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

Those are configuration setting and all those are lower case. ON and OFF are payloads.

@@ -149,7 +156,7 @@ class MqttLight(Light):

def __init__(self, name, effect_list, topic, templates, qos,
retain, payload, optimistic, brightness_scale,
white_value_scale):
white_value_scale, on_command_type='LAST'):
Copy link
Member
@fabaff fabaff Oct 31, 2017

Choose a reason for hiding this comment

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

='LAST' is not needed, as far as I can tell. The schema validation will take care that the default is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - but I don't know that some other piece of code isn't calling the constructor directly. Having a default here keeps backward compatibility with any code (or test code) that might not have that argument in the calling sequence. I'm find taking it out if that's what you want...

Copy link
Member
@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Thanks 🐦

@fabaff fabaff merged commit 253c8ae into home-assistant:dev Oct 31, 2017
@balloob balloob mentioned this pull request Nov 2, 2017
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MQTT light: Order of packets: command_topic and brightness_command_topic
4 participants
0