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

Fahrenheit conversion is not correct, seems to make the conversion twice. #240

Closed darkwolfgar closed 5 months ago

darkwolfgar commented 11 months ago

When converting Fahrenheit temperatures to Celsius and putting the Celsius temperature in the configuration settings, this seems to be correct, See below. image However, as shown in the picture below, the current temperature is taking the Fahrenheit temperature of my sensor and using the Fahrenheit to Celsius conversion on it again. image

darkwolfgar commented 11 months ago

Also it appears that even though its showing Fahrenheit, when I set the temperature to 75, on my thermostat its setting it to 45 degrees Fahrenheit instead of 75.

jmcollin78 commented 11 months ago

It seems you are in over_climate. It that mode it should use the temperature unit of the underlying climate. Is the underlying climate correctly displayed ?

How is it displayed with the Versatile Thermostat UI card ?

darkwolfgar commented 11 months ago

I am indeed in over_climate mode. My thermostat card is showing the correct temperature. You can see this in the picture below. The left card is my Ecobee thermostat, the middle card is the Thermostat UI card for Versatile Thermostat, and the far right card is the Versatile Thermostat UI Card. Both the right cards are showing the incorrect current temperature. image

jmcollin78 commented 11 months ago

I never do any translation of °C to °F or reverse. I let the system do like it always does.

The unit of all VTherm is the unit configured in the HA configuration. self._unit = self._hass.config.units.temperature_unit

Is your global HA configuration set the Kelvin ? Is your underlying climate also configured in °K ? You should be able to see that in the attributes of the climate (Dev Tools / States).

jmcollin78 commented 11 months ago

In the Parameters/ System/General you have an option "update unit of all sensors" (don't know exactly the real meaning of this option). Maybe you can try to check or uncheck it

Capture d’écran 2023-12-03 à 19 36 05

jmcollin78 commented 11 months ago

When I switch my test env with the checkox checked, I have this :

Capture d’écran 2023-12-03 à 19 37 41

This seems coherent. I guess the problem is not related to VTHerm itself.

darkwolfgar commented 11 months ago

In the Parameters/ System/General you have an option "update unit of all sensors" (don't know exactly the real meaning of this option). Maybe you can try to check or uncheck it

Capture d’écran 2023-12-03 à 19 36 05

All my temperature sensors are set to Fahrenheit, I double checked this in the states.

In the Parameters/ System/General you have an option "update unit of all sensors" (don't know exactly the real meaning of this option). Maybe you can try to check or uncheck it

Capture d’écran 2023-12-03 à 19 36 05

That checkmark only shows up if you are switching for Fahrenheit to Celsius or Celsius to Fahrenheit, however since all my sensors are already Fahrenheit there is no reason for me to changes this. I did attempt to switch to Celsius and switch back without checking this, it made no change.

When I switch my test env with the checkox checked, I have this :

Capture d’écran 2023-12-03 à 19 37 41

This seems coherent. I guess the problem is not related to VTHerm itself.

So oddly enough, I switched my other temperature sensors over to Celsius while my main home assistant instance is set to Fahrenheit. After this change the Versatile Thermostat now shows the correct Fahrenheit temperatures. So that tells me for some reason its making the conversion without regards to check if the sensors are in Celsius or Fahrenheit. I would love to leave it like this, however it messes up my other addins that use the temperature sensors, as they all now show in Celsius instead of Fahrenheit.

jmcollin78 commented 11 months ago

How to you set the temperature for preset in the VTherm configuration ? Do you set the temperature in °C or °K ?

darkwolfgar commented 11 months ago

How to you set the temperature for preset in the VTherm configuration ? Do you set the temperature in °C or °K ?

In Vtherm its set to Celsius, if I set it to Fahrenheit it still does the Celsius to Fahrenheit conversion.
image Here I have the temperature sensors set to Celsius, so the current temperature shows correct when I have this set. However I changed the Min and Max temp to Fahrenheit in the settings and that now shows incorrect.

And I also just realized even if I change my temperature sensors to Celsius to show the correct Fahrenheit temperature on the integration, when I change the target temperature, it does not convert it when it changes my climate control. For example, I set the target temp to 75 degrees, it sets my thermostat to 23 degrees.

jmcollin78 commented 11 months ago

I can't understand why this behavior. Even if I configure my HA into °F I don't have this but my sensor are all in °C. Can you please post me the attributes of the VTherm (in Development Tools / State, find your VTherm and post the attributes in yaml formatted with the <> button). I want to check if the unit is correctly set.

benjinne commented 11 months ago

I have a similar issue where my system is set to °F, and my heatpump defaults to °C then VThem converts my heatpump °C to °F and it's a really large value. I've tried temperature_unit of VThem and my heatpump in the entities editor but I can't seem to get it working

Here you can see my heatpump displaying correctly, but the VThem integration displaying the current temp incorrectly image

I'm actually using my dehumidifier as the temperature and it's unit is correct image image image image

jmcollin78 commented 11 months ago

Hello @benjinne ,

I think, and maybe @darkwolfgar can confirm, that the workaround is to configure VTherm with °C temperature in all fields that need a temperature. For an unknown reason, if temperature are typed in the local system, HA don't take it into account.

If someone is familiar with HA and metrics system, may be he could have a look of is wrong.

jmcollin78 commented 10 months ago

There have been no news from this issue. I will close it, don't hesitate to reopen if you think I'm missing something.

MattDahEpic commented 8 months ago

I am also encountering this issue. I am not sure how to "configure VTherm with °C temperature in all fields that need a temperature" as detailed in the workaround. image

bdonohoe commented 7 months ago

Same. I just tried to set up VTherm and gave up after encountering this issue.

mbbush commented 6 months ago

I'm experiencing a variant of this same issue. I'll try to describe it in a reproducible way:

Running Versatile Thermostat v6.2.3, on home assistant container 2024.5.1

Given:

When:

Then:

When:

Then:

So it seems to me that the problem is somewhere in the configuration flow, perhaps with converting between the objects used for the config flow, and those used to persist the configuration in the database?

Thanks for the clarification that there's no explicit unit conversion in your codebase. That means I can stop hunting for it! The remaining explanation I can think of is that it's probably some sort of implicit behavior of home assistant. If I had to guess I'd say it's most likely happening during serialization or deserialization, and might be fixable if you specified the expected unit. I haven't found a specific place in the code where this may be happening, but I'll keep looking.

Given that there are multiple people experiencing this (judging by the comments since it was closed), and (I hope) I've now given clear steps to reproduce, @jmcollin78 could you please reopen the issue?

jmcollin78 commented 6 months ago

Thank you @mbbush ,

I already spend many time trying the reproduce the problem. I will try the repro case you gave me but all my temperature sensor are in °C, so it is difficult to be in your strict repro case.

What is sure is that I never convert any temperature and I never suppose that the input is in °C. So, logically, if all your input are in °K this should work.

I set the temperature metric as the metric of the HA settings. So the temperature should be stored with °K.

Using the default min/max temperatures of 7 and 35 (the UI gives no indication as to units)

That seems wrong. I would expect, you set it in °K.

That means I can stop hunting for it! The remaining explanation I can think of is that it's probably some sort of implicit behavior of home assistant

That is also my guess.

I'm looking for someone, you will be able to debug and to figure out what is missing. Have you a dev environment and all conditions to test in °K ? If yes, I was waiting for this. The best way to find what goes wrong, is to install a development environment (VScode + DevContainer make it easy), and creates some VTherm and temperature sensor with °K, and breakpoint (I can help to find the good breakpoint).

could you please reopen the issue?

Done

EDIT: I just have seen your PR. So maybe you find the bug. That is really fine ! I will integrate it now and do a beta release.

jmcollin78 commented 6 months ago

https://github.com/jmcollin78/versatile_thermostat/releases/tag/6.2.4.beta1

Please install it and give me some feedback. I only put this fix in the beta release to avoid conflict.

mag2352 commented 6 months ago

I would like to add some clarification here as well. Firstly, Home Assistant is largely responsible for these problems. As for the fix, I'm trying to figure out why certain things are happening. The above fix, and beta version made no change to my problems. In my case, I have:

Now, I'll try and explain what I've noticed. My HA is set to Fahrenheit by default. This does one main thing. The underlying climate entity (which is in Celcius) is converted to Fahrenheit (and the user has no way to change the way its represented except for changing their preferred unit in HA, which impacts every entity).

VTherm is declaring itself as a climate entity with a unit of Celcius (assuming because the underlying entity is in Celcius). This means that the temperature sensors that have been converted to Fahrenheit by HA already (input into the VTherm) are converted to Fahrenheit again. VTherm places the auto converted Fahrenheit value as the current temperature value of the climate entity. The climate entity, however, is Celsius, so this results in just the current temperature value being converted twice. I will note that this can be worked around by making a template sensor, but that is tedious. The configuration should have a setting that allows for the proper conversion (not important for usability).

Secondly, VTherm saves the values that the user sets as Celcius (it's unit), so for systems with Fahrenheit, it is converted twice again. To reword, the user sets the target temperature to 70F. VTherm saves this target temperature as 70. When reloading the integration, VTherm sets the target temperature to 70, but because the underlying entity is in Celsius (and therefore VTherm is too), HA will convert this number again, resulting in 158 F being visible.

Putting all of this aside, the major issue with using VTherm with the default temperature set to F will result in the target temperature not being set properly. If I set VTherm to 68 F, the underlying entity will be set to 78 F (and this is not uniformly a 10 degree difference for other temperatures). This is with regulation off, etc, so the underlying entity is just not being set properly. I confirmed that if I switch my HA unit to Celcius, this problem does not exist.

mbbush commented 6 months ago

@mag2352 That's useful feedback, thanks!

For me, all my underlying entities are in Fahrenheit by the time they're exposed to HA, and my change resolved all my issues. Previously, the setpoints for each preset were undergoing a C -> F conversion each time I restarted HA, so after a few restarts it was trying to set my heat pumps to about 400 degrees F. It's a heat pump, not an oven!

I suspect that the biggest problem is https://github.com/jmcollin78/versatile_thermostat/pull/460/files#diff-2b96a6d22fdb5295d57d099f348d51ba2d2c4fbcc92490f320fcf515741437acR663-R667 using a different temperature scale depending on whether initialization is complete or not.

@jmcollin78 Maybe a better solution to this problem would be to store the preferred unit in the VTherm config, based on either the unit of the underlying climate entity (if it's an over_climate type), or the default from the user's preferences, or perhaps allowing the user to override it. Then you could use that as the fallback for the cases where the underlying climate entity may not be available.

mag2352 commented 6 months ago

I have made some progress for my personal setup. I'm not going to bother making a PR, since many of these changes are hacky. If there is a better way to make these changes, I will try to do a PR then. I just want to share my results. Firstly, I changed the Unit of the VTherm itself:

https://github.com/jmcollin78/versatile_thermostat/blob/549423b313f7ca1966f55584bdd743813c2554ce/custom_components/versatile_thermostat/thermostat_climate.py#L901-L906

I simply changed the lines that return the underlying entity's unit, and swapped it with HA's preferred unit. This made it so that I could reload the integration without the issue @mbbush described (C -> F conversion each time I restarted HA). The target temperature would be fine upon reboots. In addition, I could use sensors that are converted to Fahrenheit by HA automatically.

Obviously, this came with issues, and hence why I'm not sure if this is the way to go about things. Firstly, I'd like to note that the above change does have some merit to it; HA does not give the user the ability to change the unit of a single climate entity without changing all entities along with it. However, I feel like this change would break more things than fix them. For example:

I had to change set_temperature() in underlying,py, since cap_sent_values() uses the underlying entity's unit, and disagrees with the changes I made before. So, I simply removed the call to cap_sent_values() in the set_temperature() function:

https://github.com/jmcollin78/versatile_thermostat/blob/549423b313f7ca1966f55584bdd743813c2554ce/custom_components/versatile_thermostat/underlyings.py#L588-L604

With these two changes, VTherm is usable for me if I don't use self-regulation. I'd like to note that HA's temperature converter could maybe be used here to resolve the issues in the cap_sent_values() function, but I didn't do this. The final change I made that allowed me to use self-regulation while using the device temperature offset is above this line, I added:

https://github.com/jmcollin78/versatile_thermostat/blob/549423b313f7ca1966f55584bdd743813c2554ce/custom_components/versatile_thermostat/thermostat_climate.py#L223

device_temp = TemperatureConverter.converter_factory(self.underlying_entity(0).temperature_unit, self.hass.config.units.temperature_unit)(device_temp)

This converts the temperature properly.

Edit: I have thought on this for a bit further. There is another way to resolve this problem, but I haven't messed with the code yet. Firstly, VTherm should check the units used when restoring values (on integration reload, etc.) and do necessary conversions there. For example, if HA's preferred unit is F, and the underlying entity uses C, then the saved value to be restored later needs to be converted to C. Then, have an option/flag the user can set to convert the temperatures of their input sensors (so they don't have to input Celsius only sensors). Most importantly, changes need to be made for the set_temperature function. It needs to check HA's preferred unit, and needs to convert the temperature being sent to the underlying entity. For example, if the underlying entity has a unit of C, and HA prefers F, the service call to set the target temperature on the underlying entity needs to use the temperature in F (This is important for my case, this is what results in the temperature being off by approximately 10 degrees for no reason). This should allow for a majority of the codebase to stay the same.

mag2352 commented 6 months ago

I made a PR that addresses the issues that I have encountered. At this point, I believe it is working properly for me. Just to summarize, my HA preferred unit is F. All of my sensors and climate entities have an underlying unit of C, and HA automatically converts them to F. With the changes in the PR, the VTherm climate entity uses the preferred unit of HA to operate, removing the issue of conversion errors. This is inspired by BetterThermostat, which does the same thing. Changes are detailed in the PR.

Edit: If I need to make any changes to get the PR approved, let me know.

MattDahEpic commented 6 months ago

I installed the version 6.2.4.beta1 and it did not fix the issue. A freshly added thermostat-over-climate still suffers from the double conversion issue

mag2352 commented 6 months ago

@MattDahEpic You likely have the same setup I do (or close to that of my setup). 6.2.4beta1 did not fix my issue either. I made a PR earlier that fixed my issue, but it hasn't been reviewed/looked at yet. Of note, I have personally been using the changes I described for 5 days now, and it has worked fine the whole time.

If you are technically inclined, you can try applying those changes described in #461 manually, as I'm not sure what I have to do to make my fork work on HASS (have not attempted to do anything on that front). It might resolve the issue you are having.

Edit: I created a release tag named 6.2.4 on my forked repository (https://github.com/mag2352/versatile_thermostat). You can try adding it as a custom repository and test if my fix changes anything. Would help me make sure my changes were effective.

MattDahEpic commented 6 months ago

@mag2352 Your fork seems to work better, the freshly created entity is set to the converted value but once you set it manually it works as expected. Still plenty of quality-of-life features could be improved like the default min & max values or the defaultly created presets, but at least it works now!

jmcollin78 commented 5 months ago

Hello, is there any PR I can merge to try to fix this issue ?

mag2352 commented 5 months ago

Hello, is there any PR I can merge to try to fix this issue ?

The PR linked above will resolve this issue. I'm still working on the other suggestions I made on the other posts, so I removed the subsequent commits in the above PR (so it only contains the fix for this double conversion issue).

jmcollin78 commented 5 months ago

I have merged the PR. Can someone give it a trhy please ?

https://github.com/jmcollin78/versatile_thermostat/releases/tag/6.2.4.beta2

AlbertShovelton commented 5 months ago

Had the same issue and installed beta2. It seems to be working as expected.

benjinne commented 5 months ago

beta2 works with me. I just had to adjust the min/max to make sense for °F

jmcollin78 commented 5 months ago

Fine. I will release this beta 2 so.

jmcollin78 commented 5 months ago

https://github.com/jmcollin78/versatile_thermostat/releases/tag/6.2.4

jmcollin78 commented 5 months ago

Thank you all guys for your help !