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

Rationalise temperature localisation #48

Open
artmg opened this issue Feb 24, 2023 · 4 comments
Open

Rationalise temperature localisation #48

artmg opened this issue Feb 24, 2023 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@artmg
Copy link
Collaborator

artmg commented Feb 24, 2023

This is a new feature request. Previously there was some temperature conversion, but only on min / max temperatures, and not through the rest of this custom component. The little there was had to be removed (PR #46 and issue #42) as it used the deprecated temperature utility, which was about to be killed off.

We should use the current strategy tool TemperatureConverter (home assistant unit_conversion.py in util folder) to implement any temperature conversion required.

@artmg
Copy link
Collaborator Author

artmg commented Feb 24, 2023

See also issue #12 that introduces some facts around units used for temperature.
We should also bear in mind the current (minimal) documentation in this project wiki page on Device-attributes

@rct
Copy link
Collaborator

rct commented Mar 14, 2023

See the comments in #38 and #46. I may be misunderstanding things, but the temperature conversion may not be necessary.

#46 fixed the problem where a double conversion from C to F was happening. Min/Max are now being correctly localized.

There still are a number of temperature attributes that aren't being localized correctly. If Min/Max are being localized correctly without the integration doing the conversion, than it might be that the other temperature attributes need to have their type/class/unit set correctly so that Home Assistant will handle the localization.

@artmg
Copy link
Collaborator Author

artmg commented Mar 15, 2023

Renamed issue from Reintroduce temperature conversion. Porting diagnositics from #38

Worked previously

Temperature values that were already correctly localised into Fahrenheit:

  • temperature (set point)
  • current_temperature

Working now

Following recent removal of duplicate conversion, temperatures now correctly localised into Fahrenheit (temperature names followed by sample values):

  • min_temp: 41
  • max_temp: 86

Needing attention to make them work:

Temperature related attributes not yet being correctly localised (sample values appear to be Celsius even though system localised to Fahrenheit):

floor_temperature: 18
air_temperature: 21.5
away_temperature: 16
comfort_temperature: 20
fixed_temperature: 15
override_temperature: 20
override_duration_mins: 0
sleep_temperature: 18

Suggestion

Perhaps the types/units must be correctly identified for these. e.g. make sure Home Assistant knows they are device class == temperature and unit of measurement is 'C'

@artmg artmg changed the title Reintroduce temperature conversion Rationalise temperature localisation Mar 15, 2023
@artmg
Copy link
Collaborator Author

artmg commented Mar 15, 2023

Whilst testing any changes to this, also test whether the warmup.set_override values need adjusting at all (see issue #39)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants