cryptk / haomnilogic-local

A Home Assistant integration for Hayward OmniLogic/OmniHub pool controllers using the local UDP api
Apache License 2.0
18 stars 4 forks source link

Temperature Units: Option to specify manually #70

Closed daytonturner closed 11 months ago

daytonturner commented 1 year ago

So, being in canada, my default temperature units are set to Celsius (C). For reasons I cant fully explain, we often mix imperial and metric measurements, but only in specific cases. For instance, we often use feet instead of metres for short distances, and, in this instance, I'm used to referencing pool temperature in farenheit instead of C.

I was able to go modify the unit_of_measurement in the WaterSensor sensor right in the lovelace GUI, but the "Heater" Control is reporting in Celsius. Not sure if this is being handled by HA, or by my Omnilogic system itself, as looking in the states inspector, all the values show as C. (Except "solar_set_poinr" which i also presume is a typo of "solar_set_point")

Dunno where makes the most sense to offer a forced unit override?

cryptk commented 1 year ago

I have to report the values the way they are provided to me, so if your Omni is set to C then I report it in C. You should also have a number entity for Solar Set Point, which you should be able to adjust the unit of measurement on as normal. And yes, solar_set_poinr is a typo, someone else submitted a PR to fix it, I'll get that merged tomorrow :)

cryptk commented 1 year ago

Typo corrected, can you let me know if you have a number entity for Solar Set Point and can you adjust the unit of measurement on that?

daytonturner commented 1 year ago

So I still only see v0.5.3 in HACS even if i specify beta versions, not sure if you're wanting me to test your patch, or are asking about this version, but..

On 0.5.3, I see solar set point as an attribute of the pool heater - it doesnt show as its own entity.

Here's a list of all the attributes on that entity:

min_temp: 12.8 max_temp: 33.3 operation_list: on, off current_temperature: 29.4 temperature: 29.4 target_temp_high: null target_temp_low: null operation_mode: on omni_system_id: 6 omni_bow_id: 1 solar_set_poinr: 85 omni_heater_heat pump_enabled: yes omni_heater_heat pump_system_id: 7 omni_heater_heat pump_bow_id: 1 omni_heater_heat pump_state: Off omni_heater_heat pump_sensor_temp: 81 friendly_name: Omnilogic Pool Heater supported_features: 3

So you can see that the pump_sensor_temp and solar_set_point are both clearly in F, although min_temp, max_temp, current_temperature and temperature are all reporting in C.

Looking in my omnilogic app on iOS it shows everything in F.

I tried observing the datastream as it comes back from the omnilogic controller on my LAN, but it appears to be a binary data response, despite the request being XML formatted.

(Also, as a sort of unrelated side note, I do see a very high number of instances where the HA logs report a timeout back from the controller, and while watching the packets on the LAN, it appears that the controller frequently tries to send replies to a UDP port that's no longer being used. It seems to figure itself out and continue, but it happens enough that I see thousands of these timeouts over the span of a couple days. Perhaps a wonky connection handler in there somewhere?)

cryptk commented 1 year ago

Interesting, if you have a solar system, you should have an entity named something like number.omnilogic_pool_solar_set_point which is used to control the solar set point. Can you pull the diagnostic data from the integration and attach it?

The current_temperature and temperature attributes are "built in" for a water_heater entity, and as such, HA can handle the unit conversion. The other attributes are extra attributes that are coming directly out of the omni controller. The integration does not know about your preference in HA for showing the temperature in C or in F. The only thing the integration can do is tell Home Assistant what the current temperature are, and what it's target temperature is as well as what unit of measurement they are bring reported in. Once the integration passes that data up to Home Assistant, the HA core handles converting the values between units of measurement if needed (this is not handled by the integration).

Because the integration does not know what your preferred unit of measurement is, it's not actually doing any unit conversions itself, you can see the code for all of that here: https://github.com/cryptk/haomnilogic-local/blob/main/custom_components/omnilogic_local/water_heater.py#L81-L100

That said, if you have a solar system, you should have a separate number entity specifically to control the solar set point, and if that isn't there, I might need to adjust some code somewhere, the diagnostic data from the integration should help me to figure out what is going on there.

As far as the timeouts, is your Omni connected over a wireless connection? The wifi connections are pretty flaky on these controllers. Can you upload a log snippet showing the timeouts while the integration is running in debug mode?

daytonturner commented 1 year ago

Ah - I should have mentioned, i do NOT have a solar system, I just saw the attribute for it in the attributes list was in Farenheit, so I mentioned it.

And yes, the Omni is connected wirelessly, and its probably not the best connetion - its decently far from the AP its connected to (around the side of the house from an outdoor AP) so, thats probably it. I would run an ethernet cable to it, but it seems Hayward made a really odd decision and put the ethernet jack on the touchscreen panel, instead of the interior control module box, and my control touchscreen is mounted outside with the rest of the pool equipment. Running an ethernet cable all the way out there is possible, but ultimately kind of awkward, so i opted not to bother.

Fair enough that there's no unit conversion happening in HA - I just wonder the feasibility of enabling a conversion. I dont know how/when HA decides to show the 'unit of measurement' selector like it did for the WaterSensor entity, but not on the heater controls - I presumed it could be as simple as instructing HA that it was allowed, and it would show an option for it.

cryptk commented 1 year ago

not much I can do about the flaky wireless connection, there is already ample retry logic in the API library. There might be some room for enhancement there, but I'm not currently sure what those enhancements would be.

Seems like the real bug here is that the solar set point attribute should only be included if you actually have a solar heater. The Omni app shows it either way, but it doesn't do anything unless you actually have a solar heater, I could pretty easily only include it in the attributes if you actually have a heater though.

daytonturner commented 11 months ago

So with repsect to the actual temperature units, I went and checked and my omnilogic is set to Farenheit, not Celsius, so something in Home assistant must be deciding to change this to C - I thought about trying to change my global setting in HA from C to F, but it says if i do that it'll go and update all of my sensors across the board, which I dont want to do.

cryptk commented 11 months ago

The way that it works is that the integration tells HomeAssistant "The native temperature unit for this device is Fahrenheit and the native temperature is 85 degrees". Home Assistant then says "This users preference is to show temperatures in Celsius, so I will convert 85F to 29.5C and store this sensor reading as 29.5C".

If you want to change the output unit of measurement for just a few entities, you can go into the settings for those entities and specify your preference for that specific entity. The functionality you are requesting, to be able to specify what unit of measurement to use for the temperature sensors, is built into Home Assistant. You can specify a global preference, as well as a per-entity preference.

cryptk commented 11 months ago

This functionality is built in to HA Core already

daytonturner commented 10 months ago

Totally understand that - And I have done that for the Temperature entity already:

Screenshot 2023-08-21 at 11 20 32 AM Screenshot 2023-08-21 at 11 20 36 AM

But for the heater control, it has no "Unit of Measurement" override:

Screenshot 2023-08-21 at 11 20 57 AM Screenshot 2023-08-21 at 11 21 01 AM

I presume there's a good reason for this, and I'm not sure if it should be HA's job to decide whether it presents a unit of measurement conversion for a custom water_heater entity, as its actually doing control, not just metric tracking.

I guess my question should have been - is it expected that the integration would provide something like this, or would it be on HA itself to decide to provide (or not) a unit conversion option for water_heater types? I presume it's the latter, and if thats true, I will open it as a feature request there.

Appreciate all your help and explanation!