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

Fix fritzbox climate HVAC mode / temperature #25275

Merged
merged 4 commits into from Jul 19, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Set the target temperature

  • Loading branch information...
cgtobi committed Jul 18, 2019
commit eb26fa029acf28551335337001ae0f1841ffac2b
@@ -93,9 +93,10 @@ def current_temperature(self):
@property
def target_temperature(self):
"""Return the temperature we try to reach."""
if self._target_temperature in (ON_API_TEMPERATURE,
OFF_API_TEMPERATURE):
return None
if self._target_temperature == ON_API_TEMPERATURE:

This comment has been minimized.

Copy link
@balloob

balloob Jul 18, 2019

Member

I don't know about this. It was already like this before: https://github.com/home-assistant/home-assistant/blob/0.95.0/homeassistant/components/fritzbox/climate.py#L91-L97

Also, this would mean target temp is hardcoded to 30 C?

This comment has been minimized.

Copy link
@pvizeli

pvizeli Jul 18, 2019

Member

Yes, I did not change that code

This comment has been minimized.

Copy link
@cgtobi

cgtobi Jul 18, 2019

Author Collaborator

The way I see it is that 127.0 and 126.5 are magic numbers where 'on' would equal 'boost' and 'off'... well ... 'off'. ;-)
So either it's one of the magic numbers, a temp equal either eco or comfort (which would translate to 'auto') or something else which is 'manual'.

This is from the upstream module:

            return {
                126.5: 'off',
                127.0: 'on',
                self.eco_temperature: 'eco',
                self.comfort_temperature: 'comfort'
            }[self.target_temperature]
        except KeyError:
            return 'manual'

https://github.com/hthiery/python-fritzhome/blob/b9d44918f015b491ed631cbf6e63884f6a25cea2/pyfritzhome/fritzhome.py#L492-L503

This comment has been minimized.

Copy link
@balloob

balloob Jul 18, 2019

Member

But we should not report the magic numbers in our frontend. They are internal and it's not actually the temperature that we want to reach. Imagine looking at your thermostat and it said 125 ;)

This comment has been minimized.

Copy link
@cgtobi

cgtobi Jul 18, 2019

Author Collaborator

It does not, it translates to 30.0/0.0.

This comment has been minimized.

Copy link
@balloob

balloob Jul 18, 2019

Member

But does that make sense ?

This comment has been minimized.

Copy link
@cgtobi

cgtobi Jul 18, 2019

Author Collaborator

Why not? 0.0/30.0 are the values that the upstream module defines as valid range for setting temperature. Those correspond to off/boost in preset terms. To me it makes perfect sense to use that in HA to show what the thermostat is set to and further translate that to the HVAC_MODE, being either off or heat.

See: https://github.com/hthiery/python-fritzhome/blob/b9d44918f015b491ed631cbf6e63884f6a25cea2/pyfritzhome/fritzhome.py#L202-L210

return ON_REPORT_SET_TEMPERATURE
if self._target_temperature == OFF_API_TEMPERATURE:
return OFF_REPORT_SET_TEMPERATURE
return self._target_temperature

def set_temperature(self, **kwargs):
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.