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

Initial attempt to add disabling of presets to config_flow #610

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hilburn
Copy link
Contributor

@hilburn hilburn commented Nov 6, 2024

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

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 tests
@jmcollin78
Copy link
Owner

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

@hilburn
Copy link
Contributor Author

hilburn commented Nov 6, 2024

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

@jmcollin78
Copy link
Owner

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
Copy link
Contributor Author

hilburn commented Nov 6, 2024

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
Copy link

maia commented Nov 6, 2024

@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
Copy link
Contributor Author

hilburn commented Nov 6, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

hilburn commented Nov 7, 2024

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':

@jmcollin78
Copy link
Owner

Hello @hilburn ,

Do you think you wish to continue this PR ?

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