Skip to content

Added parameters to component Climate.MQTT#12223

Closed
diogos88 wants to merge 2 commits intohome-assistant:devfrom
diogos88:dev
Closed

Added parameters to component Climate.MQTT#12223
diogos88 wants to merge 2 commits intohome-assistant:devfrom
diogos88:dev

Conversation

@diogos88
Copy link
Copy Markdown
Contributor

@diogos88 diogos88 commented Feb 7, 2018

Description:

Added parameters to component Climate.MQTT
initial_mode
initial_fan_mode
initial_swing_mode
min_temp
max_temp
target_temp_step

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

Example entry for configuration.yaml (if applicable):

climate:
  - platform: mqtt
    name: Dummy
    retain: true
    send_if_off: true
    initial: 20
    initial_mode: Heat
    initial_fan_mode: Medium
    initial_swing_mode: Manual
    payload_on: 1
    payload_off: 0
    min_temp: 18
    max_temp: 30
    target_temp_step: 1
    modes:
      - Auto
      - Heat
      - Cool
      - Dry
      - Fan Only
      - Idle
    fan_modes:
      - Auto
      - Quiet
      - Low
      - Medium
      - High
    swing_modes:
      - Manual
      - Swing
    current_temperature_topic: 'dummy/ac1/dht/Temperature'
    power_command_topic: "dummy/ac1/power/set"
    mode_command_topic: 'dummy/ac1/mode/set'
    temperature_command_topic: 'dummy/ac1/temperature/set'
    fan_mode_command_topic: 'dummy/ac1/fan/set'
    swing_mode_command_topic: 'dummy/ac1/swing/set'
    away_mode_command_topic: 'dummy/ac1/away/set'
    hold_command_topic: 'dummy/ac1/hold/set'
    aux_command_topic: 'dummy/ac1/aux/set'

…AL_SWING_MODE, ATTR_MIN_TEMP, ATTR_MAX_TEMP, ATTR_TARGET_TEMP_STEP
@todschmidt
Copy link
Copy Markdown
Contributor

Can you add what parameters you added to the description. Yes I can look at the code but you put more description in your git commit message than in the PR description.

@diogos88
Copy link
Copy Markdown
Contributor Author

Parameters added in PR description.

@tinloaf
Copy link
Copy Markdown
Contributor

tinloaf commented Feb 13, 2018

Your code adds the initial_* settings, but never does anything with them? Or am I missing anything?

@diogos88
Copy link
Copy Markdown
Contributor Author

The constructor parameters where already there but set to SPEED_LOW, STATE_OFF, STATE_OFF when instantiated.

With this modification, these parameters are by default set to SPEED_LOW, STATE_OFF, STATE_OFF and if the user set the field in the config file, they will be set to the users value.

f824a97#diff-ccca5b5c585c48089cb2a2815c3f1bf1L177

@tinloaf
Copy link
Copy Markdown
Contributor

tinloaf commented Feb 14, 2018

Ah, I see! But:

  • Initial values should never have a default. In HA, the semantics are: If there is an initial value, we always set the device to that initial value on HA restart. Thus, if your initial value has a default value, you can't start HA without setting the device to some fixed state.
  • Having that in mind, if an initial value is set, it must not just be set internally, but also be sent out via MQTT. Otherwise, you gain nothing by setting that internal value.

@diogos88
Copy link
Copy Markdown
Contributor Author

So basically, the component from day one did not respect this.

I always forced an upgrade via mqtt when hass start.

@balloob
Copy link
Copy Markdown
Member

balloob commented May 2, 2018

I don't think that we should add initial options. If people set the retain flag on their MQTT messages, Home Assistant will receive the messages during startup.

@balloob
Copy link
Copy Markdown
Member

balloob commented May 2, 2018

Better alternative is restore the state by querying the values from before HASS shutdown. See #14151 for an example how it was added to the MQTT switch. Will close this PR and you can open a new PR using restore_state if you want.

@balloob balloob closed this May 2, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Sep 5, 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.

5 participants