Heatmode should support solar pref and solar only, not just heat/off

gjbadros commented 4 years ago

The node module supports:

heatMode - integer indicating the desired heater mode. Valid values are: 0: "Off", 1: "Solar", 2 : "Solar Preferred", 3 : "Heat Pump", 4: "Don't Change"

But only #3 seems to be reflected in the MQTT messages. It should both support setting and reading of the mode. This is important to control whether gas or solar is used when controlling the pool's heating function.

bwoodworth commented 4 years ago

Thanks for the issue. Are you able to test if I add the additional heat modes?

gjbadros commented 4 years ago

Actually, I see now that the underlying JS library has been improved to support mutations, so I updated my code that was using the same library. I have an all-Javascript solution that I tested last night and works great for me. Good luck and thanks for making me aware of the improvement to the library.

bwoodworth commented 4 years ago

Unfortunately the climate entity doesn't allow for custom heat modes. It does have custom preset modes, but the MQTT climate entity doesn't have those. I will either have to add presets to the MQTT climate entity or convert this addon to an integration.

gjbadros commented 4 years ago

That's not true, you can just list them under "modes" in the yaml config.

"""

platform: mqtt name: "Pentair Pool" min_temp: 40 max_temp: 104 modes:
- "off"
- "solar"
- "solar_pref"
- "heat" current_temperature_topic: pentair/pooltemp/state action_topic: pentair/circuit/505/state mode_command_topic: pentair/heater/pool/mode/set mode_state_topic: pentair/heater/pool/mode/state temperature_command_topic: pentair/heater/pool/temperature/set temperature_state_topic: pentair/heater/pool/setpoint/state """

On Mon, Apr 27, 2020 at 8:22 AM Brian notifications@github.com wrote:

Unfortunately the climate entity doesn't allow for custom heat modes. It does have custom preset modes, but the MQTT climate entity doesn't have those. I will either have to add presets to the MQTT climate entity or convert this addon to an integration.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bwoodworth/hassio-addons/issues/2#issuecomment-620054090, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOHTKQBZESQ6PQSUPN6Z3ROWPKLANCNFSM4MHJ5SKA .

bwoodworth commented 4 years ago

I have tried that. You will get the following error in the logs:

2020-04-27 10:06:01 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection.139920643912976] value is not allowed for dictionary value @ data['hvac_mode']

The MQTT HVAC documentation actually states you can't have custom modes:

modes (list)(Optional)

A list of supported modes. Needs to be a subset of the default values.

Default value:
[“auto”, “off”, “cool”, “heat”, “dry”, “fan_only”]

preset modes would work, but those are not implemented for MQTT HVAC yet.

gjbadros commented 4 years ago

It works fine for me -- I think you're just formatting the YAML wrong or using the wrong key -- it's called "modes" not "hvac_mode".

On Mon, Apr 27, 2020 at 10:07 AM Brian notifications@github.com wrote:

I have tried that. You will get the following error in the logs:

2020-04-27 10:06:01 ERROR (MainThread) [homeassistant.components.websocket_api.http.connection.139920643912976] value is not allowed for dictionary value @ data['hvac_mode']

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bwoodworth/hassio-addons/issues/2#issuecomment-620114256, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOHTNOZOZUOG2T2DIUH2LROW3U5ANCNFSM4MHJ5SKA .

bwoodworth commented 4 years ago

What version are you on? Can you show a screenshot of the climate device?

bwoodworth commented 4 years ago

Here is my config:

climate:
  - platform: mqtt
    unique_id: pool
    name: "Pool"
    min_temp: 40
    max_temp: 104
    modes:
      - "off"
      - "heat"
      - "solar"
      - "solar_pref"
    current_temperature_topic: pentair/pooltemp/state
    action_topic: pentair/circuit/505/state
    mode_command_topic: pentair/heater/pool/mode/set
    mode_state_topic: pentair/heater/pool/mode/state
    temperature_command_topic: pentair/heater/pool/temperature/set
    temperature_state_topic: pentair/heater/pool/setpoint/state

gjbadros commented 4 years ago

0.108.2

I don't even see "hvac_mode" in that config so have no idea how you got that error message with that config.

Anyway, mine is working fine, again thanks for alerting me to the update to the library. I'll probably make my docker config and source available sometime -- I do everything in JS instead of having the shell scripting complication.

bwoodworth commented 4 years ago

I actually don't get that error when HA loads. It happens when I try to select one of those mode on the front end. Here is what mine looks like with that config:

What does yours look like?

gjbadros commented 4 years ago

I don't use the UI, sorry.

bwoodworth commented 4 years ago

How to do you set the mode then? Sending a service call? If so what happens when you call the service climate.set_hvac_mode with hvac_mode: solar?

gjbadros commented 4 years ago

I was just using MQTT messages (though I did the setup for home assistant, but was only testing at the MQTT level).

It does look like the MQTT Climate "modes" setting is supposed to be a subset of the available modes per https://www.home-assistant.io/integrations/climate.mqtt/, but I don't remember seeing a warning when that rule was broken in the YAML config -- that's a bug in the MQTT implementation. Ideally, it'd support a wider set of possible values, obviously.

bwoodworth commented 4 years ago

Yeah, I think there should be a config error when attempting to add a non-supported mode. I think the best solution would be to use preset_modes. Then you could set it to Normal, Solar, or Solar Pref. Then you can turn it on and off with the modes Off and Heat. Problem is that isn't supported in mqtt climate unfortunately.

gjbadros commented 4 years ago

I don't really understand how preset modes are envisioned to work. It seems like the more proper solution is to just allow modes to be more comprehensive values and let folks filter down to the subset their system supports. That'd be an easy change. I may propose it with a PR but it's not my top priority right yet. Good bug find!

On Mon, Apr 27, 2020 at 11:37 AM Brian notifications@github.com wrote:

Yeah, I think there should be a config error when attempting to add a non-supported mode. I think the best solution would be to use preset_modes. Then you could set it to Normal, Solar, or Solar Pref. Then you can turn it on and off with the modes Off and Heat. Problem is that isn't supported in mqtt climate unfortunately.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bwoodworth/hassio-addons/issues/2#issuecomment-620160354, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOHTNVZN2HJYKW2OOH6ILROXGIHANCNFSM4MHJ5SKA .

bwoodworth commented 4 years ago

My understanding of how preset are supposed to work is that they determine the behavior that the device is supposed to follow. Based on documentation some common presets would be Home, Away, Eco. So when the device is set to Eco preset mode it will set the tempo range appropriately. It doesn't affect the actual operating mode. In our case if we had presets of Gas(Normal), Solar and Solar Pref. Then when the mode gets changed to Heat it would check the preset mode and then tell the controller which type of heat to use.

It's not perfect, but I am trying to fit within the confines of not being able to use custom modes.

gjbadros commented 4 years ago

Right, that doesn't match what the notion of a "solar" vs "heat" mode is -- that's affecting real-time mode. The right fix is just an improvement to the climate.py code. It's a simple improvement if they'll take it.

On Mon, Apr 27, 2020 at 12:07 PM Brian notifications@github.com wrote:

My understanding of how preset are supposed to work is that they determine the behavior that the device is supposed to follow. Based on documentation some common presets would be Home, Away, Eco. So when the device is set to Eco preset mode it will set the tempo range appropriately. It doesn't affect he actual operating mode. In our case if we had presets of Gas(Normal), Solar and Solar Pref. Then when the mode gets changed to Heat it would check the preset mode and then tell the controller which type of heat to use.

It's not perfect, but I am trying to fit within the confines of not being able to use custom modes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bwoodworth/hassio-addons/issues/2#issuecomment-620175688, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALOHTPHE5P46LAHIWA364DROXJYLANCNFSM4MHJ5SKA .

bwoodworth commented 4 years ago

I created a feature request to add preset modes to the MQTT climate integration. Vote on it if you want to help get this feature working.

https://community.home-assistant.io/t/add-preset-modes-to-mqtt-climate-hvac-integration/200371

thunderdanp commented 4 years ago

Following this, I am interested in this feature and get the same error you did @bwoodworth when trying to use the yaml provided by @gjbadros. I voted on the feature request. Thanks for your work on this great add-on!

bwoodworth / hassio-addons

Heatmode should support solar pref and solar only, not just heat/off #2