search

cgarwood / homeassistant-zwave_mqtt

Limited Pre-Release of the new OZW1.6 Z-Wave component. Currently has limited platform support. Check the README for more details.
72 stars 8 forks source link

New climate component #101

Closed quinnhosler closed 4 years ago

quinnhosler commented 4 years ago

adds support for climate entity, logic based off original zwave component. Requires python-openzwave-mqtt #65 to work as the value is a combined list/dict, which was previously not supported.

Currently works well with my thermostat of a Honeywell T6 Pro. However, I did not use the previous zwave component with a climate device!

marcelveldt commented 4 years ago

Hi @quinnhosler thanks for helping us out with your PR! Please see some of the comments we gave, especially regarding hardcoded string values. Please prefer the ID's at all times because the labels are dynamic.

Use a lookup table to retrieve the value for the ID or to convert the OZW ID (not the string label) to the Home Assistant representation.

quinnhosler commented 4 years ago

Alright, I took a look at all the corrections and suggestions and refactored the code.

I removed anything that appeared to be a workaround, which included the aux_heat and swing_mode support. They appeared to only support a specific device, and as I do not have a device that supports these functions, I am unable to test them. They have been removed completely.

Additionally, I did my best to use the OZW 1.6 mode integer values for the backend the labels are only used on the frontend. There is one spot where this was not possible, and that is regarding the HVAC_CURRENT_MAPPINGS constant. It appears OZW or qt-ozw does not send integer values and only labels. We can update this later as support is added.

Finally, I have no idea if the ZWaveClimateMultipleSetpoint works. Again, my device does not support it and I have no way to test it. The code is mostly left untouched from the previous component. If anyone knows more about this, I'd be happy to know.

MartinHjelmare commented 4 years ago

Please rebase on latest master branch and solve the merge conflict.

MartinHjelmare commented 4 years ago

Two comments, otherwise looks good.

marcelveldt commented 4 years ago

Great, thanks!

marcelveldt commented 4 years ago

Merged! A new version will be released to HACS in a bit, allowing some users to give this a testdrive!

joshtbernstein commented 4 years ago

Please let me know if this comment should be somewhere else. Previously zwave climate devices would show up as multiple climate entities (for me both a heating and cooling). With a recent version of HA, this was changed to the more traditional single climate entity. I've just switched to this integration and noticed that I now have two climate entities for my zwave climate devices. Was this expected/intended behavior?

marcelveldt commented 4 years ago

@joshtbernstein please open up a new issue for this report. While replying: I'd like to investigate your report. Can you create an mqtt dump for me please and provide it with the issue report ? In Hass --> developer tools --> services --> mqtt.dump

kpine commented 4 years ago

I submitted #116 . Same problem here.