jmcollin78 / versatile_thermostat

A full featured Thermostat for Home Assistant: presets, window, motion, presence and overpowering management
MIT License
329 stars 34 forks source link

"Presence Management" do not make use of the Person entity #195

Closed albertofralbe closed 1 year ago

albertofralbe commented 1 year ago

Hi there, I'm new to this integration but I'm liking it a lot so far, using the "valve" proportional control is giving me a far better regulation paired with underfloor radiant heating compared to the stock one.

Is your feature request related to a problem? Please describe. I'm noticing that the presence management feature do not make use of the person entity.

Describe the solution you'd like It would be useful to have the option to use the person "home" and "away" state as a trigger for occupancy instead of exposing another boolean sensor. It would also be great if the occupancy could switch between eco, comfort and boost mode instead of applying a different set point. Thank you all, great work!

maia commented 1 year ago

As for the presets, I‘m personally also a bit overwhelmed with eco/comfort/boost all having an _away preset too (plus a motion/activity preset). If I‘d design something just for myself, I‘d have 4 presets:

…but from what I see having „only“ 4 presets isn‘t enough for others. :)

jmcollin78 commented 1 year ago

Hello @albertofralbe ,

I'm new to this integration but I'm liking it a lot so far, using the "valve" proportional control is giving me a far better regulation paired with underfloor radiant heating compared to the stock one.

Great news !

It would be useful to have the option to use the person "home" and "away" state as a trigger for occupancy instead of exposing another boolean sensor.

This should definitively work out the box. The only limitation is that groups of person don't work because a group of person is not a person in HA (bug ?). So if you give one person, this should work (this is my test case so I think it is working) and you have more than one person you should do a template binary_sensor with occupancy

jmcollin78 commented 1 year ago

As for the presets, I‘m personally also a bit overwhelmed with eco/comfort/boost all having an _away preset too (plus a motion/activity preset). If I‘d design something just for myself, I‘d have 4 presets:

Hello @maia, I didn't understand why this response to this question. Is this related ?

maia commented 1 year ago

As for the presets, I‘m personally also a bit overwhelmed with eco/comfort/boost all having an _away preset too (plus a motion/activity preset). If I‘d design something just for myself, I‘d have 4 presets:

Hello @maia, I didn't understand why this response to this question. Is this related ?

I was just a reply to „ It would also be great if the occupancy could switch between eco, comfort and boost mode instead of applying a different set point.“ from @albertofralbe above, thought he is referring to the _away presets, but maybe I misunderstood him?

jmcollin78 commented 1 year ago

... from @albertofralbe above, thought he is referring to the _away presets, but maybe ...

Ok. I don't understand he need another preset but more instead of using another set a temperature for the same preset, he would prefer to switch preset depending of occupancy (like it is done for Motion and Activity preset). That is my understanding.

ben-ty commented 1 year ago

Hello @albertofralbe ,

I'm new to this integration but I'm liking it a lot so far, using the "valve" proportional control is giving me a far better regulation paired with underfloor radiant heating compared to the stock one.

Great news !

It would be useful to have the option to use the person "home" and "away" state as a trigger for occupancy instead of exposing another boolean sensor.

This should definitively work out the box. The only limitation is that groups of person don't work because a group of person is not a person in HA (bug ?). So if you give one person, this should work (this is my test case so I think it is working) and you have more than one person you should do a template binary_sensor with occupancy

I encountered the same issue. I think in PresenceBinarySensor, you should not just check for STATE_ON / STATE_OFF but also for STATE_HOME / STATE_NOT_HOME.

from homeassistant.const import STATE_ON, STATE_OFF,STATE_HOME, STATE_NOT_HOME
...
    async def async_my_climate_changed(self, event: Event = None):
        """Called when my climate have change"""

        _LOGGER.debug("%s - climate state change", self._attr_unique_id)
        old_state = self._attr_is_on
        # Issue 120 - only take defined presence value
        if self.my_climate.presence_state in [STATE_ON, STATE_OFF, STATE_HOME, STATE_NOT_HOME]:
            self._attr_is_on = self.my_climate.presence_state == STATE_ON || STATE_HOME
            if old_state != self._attr_is_on:
                self.async_write_ha_state()
        return
jmcollin78 commented 1 year ago

Hello @ben-ty ,

Thanks for your suggestion but the presence sensor management is in base_thermostat.py here:

async def _async_update_presence(self, new_state):
        _LOGGER.info("%s - Updating presence. New state is %s", self, new_state)
        self._presence_state = new_state
        if self._attr_preset_mode in HIDDEN_PRESETS or self._presence_on is False:
            _LOGGER.info(
                "%s - Ignoring presence change cause in Power or Security preset or presence not configured",
                self,
            )
            return
        if new_state is None or new_state not in (
            STATE_OFF,
            STATE_ON,
            STATE_HOME,
            STATE_NOT_HOME,
        ):
            return
        if self._attr_preset_mode not in [PRESET_BOOST, PRESET_COMFORT, PRESET_ECO]:
            return

        new_temp = self.find_preset_temp(self.preset_mode)
        if new_temp is not None:
            _LOGGER.debug(
                "%s - presence change in temperature mode new_temp will be: %.2f",
                self,
                new_temp,
            )
            await self._async_internal_set_temperature(new_temp)
            self.recalculate()

And it takes STATE_OFF, STATE_ON, STATE_HOME, STATE_NOT_HOME as you can see. This is why it works for a single person in the presence sensor configuration.

The code you are showing in not the right one. This piece of code only listen to the presence_state of the VTherm itself which can only be 'on' or 'off'.

I hope it is clear.

You are not the first to ask this, but this doesn't work for groups of person, and I don't remember exactly why (i believe that a group of person is not an occupancy sensor but i'm not sure).

ben-ty commented 1 year ago

@jmcollin78 Sorry but presence_state of the VTherm itself is not on or off when a person (we don't speak about a group of persons) entity is selected for presence detection... it's home / not_home ;)

presence_state = self.hass.states.get(self._presence_sensor_entity_id)

--> Take a look at https://github.com/home-assistant/core/blob/dev/homeassistant/components/person/__init__.py

The code in base_thermostat.py is ok (the right preset is applied) BUT the code in binary_sensor.py need to be modified to handle home / not_home state so the sensor on the VTherm device display the right status.

jmcollin78 commented 1 year ago

Ah ok @ben-ty, I got your point. I though we were talking about group of persons. I will do a quick fix today.

jmcollin78 commented 1 year ago

In release 4.1.0. I cannot totaly test it in my environment. Please have a test in yours and keep me in touch. Don't hesitate to reopen if not fixed.

Thanks for your report.