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

Do not convert the target temperature to integer #81

Merged
merged 1 commit into from
Dec 15, 2021
Merged

Do not convert the target temperature to integer #81

merged 1 commit into from
Dec 15, 2021

Conversation

vickyg3
Copy link
Contributor

@vickyg3 vickyg3 commented Dec 13, 2021

In places where Fahrenheit is the default temperature scale (like
US), home assistant UI shows the climate cards in Fahrenheit scale
and converts the temperature to Celcius implicitly for integrations
that use Celcius.

So the set_temperature() calls in this case will get a floating
point temperature value for celcius and converting that to an integer
before converting that to Kelvin makes setting certain temperature
values in Fahrenheit impossible because of the rounding errors.

Consider the following case:
User sets 74F -> Home assistant converts it to 23.3C -> Converting
that to integer becomes 23C which is equivalent to 73F. So setting
74F is impossible because of this rounding.

Not converting the value to integer and converting directly to a
floating point Kelvin temperature solves this issue and users would
be able to set any Fahrenheit temperatures they want. Note that the
underlying libdyson library supports setting any floating point
temperature in Kelvin and the rounding to integer was not really
necessary in the first place.

In places where Fahrenheit is the default temperature scale (like
US), home assistant UI shows the climate cards in Fahrenheit scale
and converts the temperature to Celcius implicitly for integrations
that use Celcius.

So the set_temperature() calls in this case will get a floating
point temperature value for celcius and converting that to an integer
before converting that to Kelvin makes setting certain temperature
values in Fahrenheit impossible because of the rounding errors.

Consider the following case:
User sets 74F -> Home assistant converts it to 23.3C -> Converting
that to integer becomes 23C which is equivalent to 73F. So setting
74F is impossible because of this rounding.

Not converting the value to integer and converting directly to a
floating point Kelvin temperature solves this issue and users would
be able to set any Fahrenheit temperatures they want. Note that the
underlying libdyson library supports setting any floating point
temperature in Kelvin and the rounding to integer was not really
necessary in the first place.
@FutureGUIs
Copy link

I like this, it's been a long time coming to fix this issue. Are there any side effects to removing that line and getting this done?

@vickyg3
Copy link
Contributor Author

vickyg3 commented Dec 14, 2021

Are there any side effects to removing that line and getting this done?

I don't believe there are any side effects other than making Fahrenheit working as it should be. For users with locale set to Celcius, the existing behavior will continue to work (since the int() conversion has no effect in that case).

@Kakise
Copy link
Collaborator

Kakise commented Dec 15, 2021

I did some tests at home with this patch applied and I can confirm it did work with my phone set to celsius and my American friends phone that I added to my home.

@Kakise Kakise merged commit b89e157 into shenxn:main Dec 15, 2021
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