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

Added presets (hold_modes) to climate sensor #3821 #4070

Merged
merged 7 commits into from
Aug 14, 2020
Merged

Added presets (hold_modes) to climate sensor #3821 #4070

merged 7 commits into from
Aug 14, 2020

Conversation

mgrom
Copy link
Contributor

@mgrom mgrom commented Aug 10, 2020

Added presets (hold_modes) to climate sensor #3821
and removed thermostatHeatCool as it was partially redundant

@branch-switcher
Copy link

Hello @mgrom. The base branch of this pull request has been updated to the dev branch. Please revisit the changes and make sure that there are no conflicts with the new base branch. Thank you for your contributions.

@branch-switcher branch-switcher bot changed the base branch from master to dev August 10, 2020 08:02
@mgrom
Copy link
Contributor Author

mgrom commented Aug 10, 2020

@Koenkk how to manage changes in zigbee-herdsman-converters that are related to this PR? Should I make them later or now but with some tag or something?

@Koenkk
Copy link
Owner

Koenkk commented Aug 10, 2020

@mgrom just make a pr to zigbee-herdsman-converters and mention this pr there, I will merge it after that is merged.

@Koenkk
Copy link
Owner

Koenkk commented Aug 14, 2020

/rebase

@Koenkk
Copy link
Owner

Koenkk commented Aug 14, 2020

/autorebase

@Koenkk Koenkk removed the autosquash label Aug 14, 2020
@Koenkk
Copy link
Owner

Koenkk commented Aug 14, 2020

Thanks!

@Koenkk Koenkk merged commit d3a22d4 into Koenkk:dev Aug 14, 2020
@presslab-us
Copy link
Contributor

@mgrom with the removal of thermostatHeatCool there is now no longer a way to set separate heating and cooling temperatures on a thermostat that supports both.

Also the new climate does not use temperature_unit so those of us that use Fahrenheit do not get the right values.

@mgrom
Copy link
Contributor Author

mgrom commented Aug 19, 2020

@presslab-us this wasn't just removal. I've merge it to climate:

const climate = (minTemp=7, maxTemp=30, temperatureStateProperty='occupied_heating_setpoint',
   tempStep=1, systemModes=['off', 'auto', 'heat'], fanModes=[], holdModes=[],
   temperatureLowStateTopic=false, temperatureHighStateTopic=false ) => {
.....

  // use low target temperature
   if (temperatureLowStateTopic) {
       retVal.discovery_payload.temperature_low_state_topic = temperatureLowStateTopic;
       retVal.discovery_payload.temperature_low_state_template = `{{ value_json.occupied_heating_setpoint }}`;
       retVal.discovery_payload.temperature_low_command_topic = 'occupied_heating_setpoint';
   }
   // use high target temperature
   if (temperatureHighStateTopic) {
       retVal.discovery_payload.temperature_high_state_topic = temperatureHighStateTopic;
       retVal.discovery_payload.temperature_high_state_template = `{{ value_json.occupied_heating_setpoint }}`;
       retVal.discovery_payload.temperature_high_command_topic = 'occupied_heating_setpoint';
   }

Regarding temperature it was hardcoded 'C' before, so there was no way to use Fahrenheit either.

@presslab-us
Copy link
Contributor

I haven't looked deeply into it, but for some reason it's not working any more. In Home Assistant it only allows for setting one temperature.

On thermostatHeatCool it did use temperature_unit.

temperature_unit: 'C',

@mgrom
Copy link
Contributor Author

mgrom commented Aug 19, 2020

@presslab-us what device are you using? I'll check configuration. Maybe I made some mistake in other place.

@presslab-us
Copy link
Contributor

Thanks @mgrom. I use 3157100 and RC-2000WH. I think I see the issue, here is a snip of the discovery topic:

  "temperature_command_topic": "zigbee2mqtt/lab_thermostat/set/occupied_heating_setpoint",
  "temperature_high_command_topic": "zigbee2mqtt/lab_thermostat/set/occupied_heating_setpoint",
  "temperature_high_state_template": "{{ value_json.occupied_heating_setpoint }}",
  "temperature_high_state_topic": "zigbee2mqtt/lab_thermostat",
  "temperature_low_command_topic": "zigbee2mqtt/lab_thermostat/set/occupied_heating_setpoint",
  "temperature_low_state_template": "{{ value_json.occupied_heating_setpoint }}",
  "temperature_low_state_topic": "zigbee2mqtt/lab_thermostat",
  "temperature_state_template": "{{ value_json.occupied_heating_setpoint }}",
  "temperature_state_topic": "zigbee2mqtt/lab_thermostat",

I believe that for devices that support both high and low state/command, there should not be temperature_command_topic. temperature_state_topic, and temperature_state_template defined.

@mgrom
Copy link
Contributor Author

mgrom commented Aug 19, 2020

@presslab-us ok, I'll make PR to fix it.

@presslab-us
Copy link
Contributor

Great, thanks a lot. I agree that combining them is the right thing. Also if you can add the temperature_unit that would be good, and it will now work for all devices.

@mgrom mgrom mentioned this pull request Aug 19, 2020
Koenkk added a commit that referenced this pull request Aug 23, 2020
* fix

* eslint fix

* don't use temp topic when low and high temp used

* fix for temp high

* Add tests

Co-authored-by: Koen Kanters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants