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

Change VTherm temperature unit to HA's preferred unit. #461

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

mag2352
Copy link
Contributor

@mag2352 mag2352 commented May 14, 2024

With this change, I no longer suffer from #240.

I made 2 basic changes to the code, simplifying the changes I mentioned in #240.

  • Firstly, I changed the unit used by VTherm to match HA's preferred unit.
  • Secondly, I changed the references to the underlying entity's temperatures to use HA's state references. This automatically includes HA's converted values, and should be robust against having mismatched temperature units. I propose that all references to the underlying entity use HA's state, rather than directly accessing the values from the entity.

Example:

Change:
max_val = self._underlying_climate.max_temp

To:
max_val = self._hass.states.get(self._entity_id).attributes.get("max_temp")

This results in the correct converted value being grabbed if the underlying entity uses a different unit of temperature than HA's preferred unit. This should make no difference for those who have entities that match their HA's preferred unit. I only made the minimum changes to allow for my use case to work, but there may be other attributes that should be changed to reflect this.

@mbbush
Copy link
Contributor

mbbush commented May 15, 2024

This approach makes sense to me, although I haven't tested it yet or looked for other places to change.

@mag2352
Copy link
Contributor Author

mag2352 commented May 15, 2024

This approach makes sense to me, although I haven't tested it yet or looked for other places to change.

I would imagine there are other places that would require changes. Notably, I didn't perform any changes on the temperature sensors. I used the (rather loose) assumption that the temperature sensor being used would match HA's preferred unit, but it is fairly trivial to check the unit of measurement on the sensor, and convert if needed.

@jmcollin78
Copy link
Owner

Hello @mag2352 ,
This PR is still tagged as draft. Do you think I will be able to merge it and do a draft release ?

@mag2352
Copy link
Contributor Author

mag2352 commented May 30, 2024

Hello @mag2352 , This PR is still tagged as draft. Do you think I will be able to merge it and do a draft release ?

I think the mag2352/versatile_thermostat@f8e57b3 can be merged without issue (I removed the subsequent commits for now and moved them to another branch as I'm still testing those changes). So at the very least, the double conversion issue is, from what I can tell, resolved.

@mag2352 mag2352 changed the title [Draft] Change VTherm temperature unit to HA's preferred unit. Change VTherm temperature unit to HA's preferred unit. May 30, 2024
jmcollin78
jmcollin78 previously approved these changes Jun 9, 2024
@jmcollin78
Copy link
Owner

Unit tests are failing. Can you please have a look of what happens ?
The error seems not clear for me.

jmcollin78
jmcollin78 previously approved these changes Jun 9, 2024
@jmcollin78
Copy link
Owner

unit tests still failed on dependencies

@mag2352
Copy link
Contributor Author

mag2352 commented Jun 9, 2024

Yeah, I'm probably going to test this locally for now. I'm assuming its a pytest error, as that's what seems to be the most likely problem based on the first error message.

@jmcollin78
Copy link
Owner

Why is there any dependencies changes for so minor changes ?

@mag2352
Copy link
Contributor Author

mag2352 commented Jun 9, 2024

I'm not sure what is causing it, this is definitely strange. Wondering if some dependency updated recently and is causing this issue.

@mag2352
Copy link
Contributor Author

mag2352 commented Jun 9, 2024

Passes Unit Tests on my repository. I had to change the homeassistant version in the requirements. I suspect some dependency was updated (pytest was updated 4 days ago, might be why). After doing this, I had some unit tests fail.

To resolve unit test errors, had to change the cap_sent_value function to explicitly convert temperature values if needed rather than accessing HA's state (since it would sometimes return none when getting the entity). I have never had an issue with this when running on HA (I have been running my branch for weeks now without issue), but this is functionally the same and passes your unit tests.

@jmcollin78 jmcollin78 merged commit 53dce22 into jmcollin78:main Jun 10, 2024
5 checks passed
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.

None yet

3 participants