jmcollin78 / versatile_thermostat

A full featured Thermostat for Home Assistant: presets, window, motion, presence and overpowering management
MIT License
332 stars 36 forks source link

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

Closed mag2352 closed 5 months ago

mag2352 commented 6 months ago

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

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

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 commented 6 months ago

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

mag2352 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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@f8e57b3eb7beda6cc481211fd4bbb854e8172d87 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.

jmcollin78 commented 5 months ago

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

jmcollin78 commented 5 months ago

unit tests still failed on dependencies

mag2352 commented 5 months ago

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 commented 5 months ago

Why is there any dependencies changes for so minor changes ?

mag2352 commented 5 months ago

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

mag2352 commented 5 months ago

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.