jmcollin78 / versatile_thermostat

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

Initial attempt to add disabling of presets to config_flow #610

Open hilburn opened 2 weeks ago

hilburn commented 2 weeks ago

I thought I'd open the PR just so it can be discussed Related to #522

TODO:

  1. Load selected presets into BaseThermostat._attr_preset_modes
  2. Calculate the presets/presets_away based on 1. rather than use static
  3. Ensure all the various number entities load properly
  4. Add translations
  5. Add tests
jmcollin78 commented 2 weeks ago

Thank you for this, I will have a check later (I will be off for few days)

hilburn commented 2 weeks ago

It's nowhere near done but hopefully I'll get a chance before next week to do some more

jmcollin78 commented 2 weeks ago

I try to understand what you intend to do:

  1. have a list of presets,
  2. make the user select the presets he wants.

What will be tricky is the BaseThermostat:init_presets(self, central_config) method. Presets can be set locally or in the central configuration. And this piece of code initialize all presets temp from local or central configuration.

So you will have to reproduce what you have done in central config also (maybe you have done it but I don't read carefully the PR). Nothing impossible, but tricky.

What is also tricky is that is the preset are configured from centra configuration, a change in the central configuration should impact many VTherm. The logic is there, but you will certainly have to impact this. This was difficult to make it works (and I'm not tottaly satisfied of the solution I implement): too much complex and not easy to maintain.

Initialy to disable the preset, you just have to put a 0 in the temperature. You will find some code around this. I don't remember why but this do not work anymore. Maybe it is the best way to achieve this goal.

Other point you should have in mind, all tests have not been adapted with the new preset pattern (some external entities).

So I wish you a good luck, if you think you want to continue in this way.

hilburn commented 2 weeks ago

Yeah basically - Frost Protection, Boost, Comfort and Eco can all be individually disabled in both central config and independent VTherms There's a new config error if you select a disabled preset for motion (I couldn't find a nice way to remove the preset from the selectors) I should be able to tack on to that logic of termperature > 0 to disable the presets in each, I've already changed the logic for generating the TemperatureNumber entities to not generate them for disabled presets

maia commented 2 weeks ago

@hilburn , thanks for all your work! Also, would this be the right moment to re-suggest the removal of the "Frost away" preset? I assume having a single "Frost" preset is enough.

hilburn commented 2 weeks ago

I'd have to look at it, but probably! I think the way the presence sensor works currently is that internally there needs to be an "away" variant, but it can be hidden from the user and just take the same value as "Frost"

jmcollin78 commented 1 week ago

I'd have to look at it, but probably! I think the way the presence sensor works currently is that internally there needs to be an "away" variant, but it can be hidden from the user and just take the same value as "Frost"

Exactly, the genericity needs to have a Frost_away. TO remove it we must add an exception and an ugly "if frost". But maybe it worth it.

hilburn commented 1 week ago

I'd have to look at it, but probably! I think the way the presence sensor works currently is that internally there needs to be an "away" variant, but it can be hidden from the user and just take the same value as "Frost"

Exactly, the genericity needs to have a Frost_away. TO remove it we must add an exception and an ugly "if frost". But maybe it worth it.

It should be possible to add the exception here: https://github.com/jmcollin78/versatile_thermostat/blob/main/custom_components/versatile_thermostat/base_thermostat.py#L1385 - just make that:

if not self._presence_on or self._presence_state in [
                None,
                STATE_ON,
                STATE_HOME,
            ] or preset_mode == 'frost':