Poor behavior with homekit_controller climate interface and ecobee due to precision change

dpippenger commented 3 years ago

The problem

The PR here to change precision from whole digits to tenths has created unpleasant interactions with ecobee devices when controlled via homekit.

https://github.com/home-assistant/core/pull/50415

The specific undesirable behavior is that when setting the temperature to 74 degrees F the result is the temperature gets set to 73.9. Setting the temperature to 73 will set the temp to 73... it's unclear what the specific pattern is to why some set points are whole digits and others are not.

The reason this is a problem is because lovelace cards that attempt to set the temp to 74F will show that the temp is not being set because it never reaches 74F and stays at 73.9F.

In addition to this I actually want the temperature set to 74F, not 73.9F. When setting the temperature to 74F via the ecobee app or thermostat directly the set point becomes 74F over homekit as well... so it at least doesn't appear to be an issue with the thermostat not being able to set the temp to 74F precisely.

Reverting the changes to precision in climate.py with no other changes on 2021.6.2 restores the previous better behavior and allows setting whole digit temps again. I'm sure the issue is more complex than just reverting precision change and probably has something to do with the way C to F conversions are occurring between homekit and homeassistant's integration.... but at the moment the prior behavior is subjectively "less bad" in my household.

What is version of Home Assistant Core has the issue?

2021.6.2

What was the last working version of Home Assistant Core?

2021.5

What type of installation are you running?

Home Assistant Container

Integration causing the issue

homekit_controller

Link to integration documentation on our website

https://www.home-assistant.io/integrations/homekit_controller/

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

probot-home-assistant[bot] commented 3 years ago

homekit_controller documentation homekit_controller source _{^{(message by IssueLinks)}}

Jc2k commented 3 years ago

You are right, this is quite complicated. Well, its simple and complicated at the same time. So I think what homekit_controller is doing is technically correct in terms of conversion, based on the requirements it has to deal with:

HomeKit devices are always in C, so for the US market there is always conversion. If the device is natively in F, it is converting internally as well - a "conversion sandwich"
Floating point numbers on computers are inherently lossy.
The Ecobee takes numbers in increments of 0.1, so the conversion is even more lossy. And we might have multiple rounding conversions because of the sandwich.
Some devices ignore changes that aren't clamped to these steps. So it's safe to send 33.3 C but not 33.299999999 C, so we can't get rid of the clamping.

The actual C -> F and F -> C conversions are part of Home Assistant. homekit_controller just tells home assistant the device uses C, it doesn't do any of the conversion itself. So I don't think thats at fault.

Based on your description what I think happens is this:

You ask for 74 F. That needs to be in C for HomeKit, so (74.0 - 32) * (5.0/9) == 23.333333333333336. Rounding to the nearest 0.1, that is 23.3. So that is what we send. Note that even if we sent the unrounded version, the device would still only send 23.3 back. This isn't changed by #50415.
HomeAssistant reads the change back out. 23.3 C - but it needs to convert it to F for display. 23.3 * (9.0/5.0) + 32 == 73.94. Home Assistant rounds this to 73.9.

It's hard to imagine that this can be "fixed" whilst also having more precise temperatures, as 74 F doesn't "exist" for devices using C under the hood and 0.1 rounding. E.g. If you send 23.3 C to your Ecobee you are asking for 73.94. If you send 23.4 C to your Ecobee you are asking for 74.12 F. It totally skips 74 F.

Actually, my expectation is that this is at least a little bit broken for your home before #50415. Regardless of whether that PR is present or not homeassistant will be doing the conversion (74.0 - 32) / 1.8, and homekit will be sending 23.3 to the device. The difference is when reading back out. 23.33 * 1.8 + 32 rounds to 74 F before but 73.9 F now (i.e. its just more accurate).

I hope this at least clears up why some points are whole numbers and some are not.

As for why the thermostat itself can be set to 74. I suspect it's either natively using F (and presenting C via the HomeKit API) OR internally it's using an unrounded number to store the current temps. HomeKit mandates C and has rounding, so both of these situations could explain a rounding problem on the HA side, and both are kinda unfixable. I would be surprised if setting it to 74 on the thermostat made it show as 74 in homeassistant, but i have some hunches about spec violations that could cause that sort of thing to happen.

I don't see how the lovelace UI can be fixed here. We ask for 74F (and can't ask for e.g. 74.1F). But due to conversion and rounding we get 73.9F. What is a UI supposed to do? The numbers are correct. Maybe there is some way to keep the raw sensor values in the state machine for graphs and automations to use, but round in the lovelace panels to preserve the pre-#50415 behaviour? This is a bigger change than homekit_controller, though and not something I can do.

And if that is not an option, then I think #50415 should probably be reverted. HomeKit devices are always in C, so for regions using F, every manufacturer would have the same problem with temperatures like 74F, and it sounds like this change makes the UI glitchy. Whilst I could live with a user having to see 73.9 in the ui instead of 74.0, a mismatch between requested and actual is harder to stomach.

What do you think about this @jjlawren?

jjlawren commented 3 years ago

I was worried there could be some side effect like this. Yes, the Ecobee operates in F natively and internally converts to C for HomeKit.

To make things a bit more complicated, it seems that Ecobee will use/accept non-integer F setpoints in their API but will round those values when exposing via their native apps & web interfaces (and likely in the actual thermostat operation). That was a behavior we had to emulate on the HA side when relaxing the F precision for the Ecobee integration in #48697.

I'm wondering if there could be a similar "patch" added to homekit_controller when working in an HA system configured to use imperial units. Setpoints could be read back as F-friendly C values which are guaranteed to convert to full- or half-degree F values when displayed . This would implicitly solve the setting side, as those values are relative to the current known setpoint.

And in the case where a setpoint is somehow sent 0.1F away from the target setpoint, I can't imagine any consumer thermostat where that would make any actual difference in operation. That level of measurement precision is outside the range of capabilities for those devices.

Jc2k commented 3 years ago

It'd be harder to do in homekit_controller because we never see the F version of the values, only the C version. I'm not really familiar enough with this side of HA to comment on that direction.

jjlawren commented 3 years ago

I mean that if the current HA instance is configured to work in F, the C values received over HomeKit could be gently adjusted to ensure that they will be represented properly when converted back to F. For example, if the thermostat reports 23.3C, that could be stored as 23.33 in the state machine which would more cleanly convert to 74F.

Instead of trying to come up with a clever algorithm, perhaps we just create a lookup table for the sandwich values.

Here are the values that I think would be suspect for C->F conversions in the range of 10-40C (a reasonable range for target temps I also just made up):

Source C	Suspect F
10.5	50.9
10.6	51.1
11.7	53.1
13.3	55.9
14.4	57.9
15.5	59.9
15.6	60.1
16.7	62.1
18.3	64.9
19.4	66.9
19.5	67.1
20.5	68.9
20.6	69.1
21.7	71.1
23.3	73.9
24.4	75.9
24.5	76.1
25.5	77.9
25.6	78.1
26.7	80.1
28.3	82.9
29.4	84.9
29.5	85.1
30.5	86.9
30.6	87.1
31.7	89.1
33.3	91.9
34.4	93.9
34.5	94.1
35.5	95.9
35.6	96.1
36.7	98.1
38.3	100.9
39.4	102.9

If those values look reasonable I can try to clean up the algo enough to where I'm comfortable sharing it. 😉

Jc2k commented 3 years ago

That's a hack too far for me. I'd want a 2nd opinion before progressing down that route. @bdraco do you have any thoughts on this one?

jjlawren commented 3 years ago

I'm not a fan of the idea either. Just trying to do something around the limitations of the data presented.

dpippenger commented 3 years ago

You have convinced me my F life is a lie (at least with homekit) and I may just consider moving over to C for my household.

tylerpieper commented 3 years ago

This is also acting up with Honeywell Lyric thermostats using homekit_controller. It appears that the thermostats are reporting temperature in 0.5 C increments, leading to results like: 24 C -> 75.2 F 24.5 C -> 76.1 F 25 C -> 77 F

and so on. I think a reasonable solution would be to allow changing the precision on the homekit_controller integration on a per-device basis, so users could either opt into higher precision (low precision by default) or opt out (higher precision by default).

Jc2k commented 3 years ago

I'd prefer to not go down that route. Manual workarounds are ok for power users but they suck for discoverability for new users and they increase my support burden.

Also right now they wouldn't be opting in to more precise data they'd be opting in to broken-ness.

If that's controversial then I will open an architecture ticket on it.

jjlawren commented 3 years ago

Perhaps an additional optional rounding param could be added to ~~homeassistant.util.temperature.celsius_to_fahrenheit~~ for use in situations like this? That's the helper called by the base climate platform.

Edit: Sorry, looks like homeassistant.helpers.temperature.display_temp would be the one we care about here.

Edit 2: This could be a secondary precision that's only used for the setpoint attributes here: https://github.com/home-assistant/core/blob/79da2bca3f210571f17a5eed64de861447392bda/homeassistant/components/climate/__init__.py#L258-L278

and perhaps the min/max: https://github.com/home-assistant/core/blob/79da2bca3f210571f17a5eed64de861447392bda/homeassistant/components/climate/__init__.py#L218-L223

Jc2k commented 3 years ago

I think the big takeaway for me is that really these devices do not really support a precision of 0.1, they just support a precision greater than 1. (0.1 C == 0.18F or something like that?). So doing this in core makes sense indeed.

jjlawren commented 3 years ago

Is it worth trying to sniff HomeKit traffic between an Ecobee and an iOS device? It's working there somehow.

Edit: This probably doesn't matter, all HomeKit temps appear to display in full digits.

Jc2k commented 3 years ago

homekit_python has a feature where you can MITM your accessory.

https://github.com/jlusiardi/homekit_python/blob/master/homekit/debug_proxy.py

jjlawren commented 3 years ago

@Jc2k since we've gone very far down this rabbit hole I'm trying to think of simpler alternative suggestions. What about this?:

Revert changes to the thermostat precision.
Use the measured temperature characteristic provided by the thermostat to create an additional HomeKitTemperatureSensor entity, which doesn't care about the concept of precision.

Jc2k commented 3 years ago

I had been wondering similar. I think i'd probably want to OK this with the wider team still but I feel like it aligns with https://github.com/home-assistant/architecture/discussions/573#discussioncomment-837556, and as long as the user can turn it off if they don't want it, I don't see the harm and i'm on board with it.

Jc2k commented 3 years ago

In terms of adding the sensor, it would normally be a case of making a characteristic bound sensor (rather than a service bound sensor) for the temperature characteristic. For this case, it is as easy as updating the SIMPLE_SENSOR dict in homekit_controller/sensor.py.

But there is a catch. Both climate devices use the "current temperature" characteristic. This is also used by the "Temperature Sensor" service, which we already support using a service based sensor entity. If we add a characteristic binding as well there will be 2 entities created for any temperature sensors out there. So we either have to:

do a breaking change and ditch the service based sensor
change the code that creates the entities to handle this situation.

There's no particular reason to use a service sensor here, beyond not wanting to break hundreds of installs. Instead will probably extend the function that creates SimpleSensor's so that it can put a probe callback in the dict. E.g.

SIMPLE_SENSOR = {
    CharacteristicsTypes.TEMPERATURE_CURRENT: {
        "name": "Current Temperature",
        "device_class": DEVICE_CLASS_TEMPERATURE,
        "units": TEMP_CELSIUS,
        # Temperature sensor service is already handled. This sensor is only for
        # temperature characteristics that are not part of a temperature sensor
        # service.
        "probe": lambda char: char.service.type != ServicesTypes.TEMPERATURE_SENSOR,
    }
}

jjlawren commented 3 years ago

@Jc2k is this followup something you eventually intend to take on yourself?

Jc2k commented 3 years ago

Yep, I want to do the extra plumbing in sensor.py as I've got a pretty clear idea of how I want it to work, I'll ping you to help test when I have a pr.

jjlawren commented 3 years ago

Amazing, I'd be happy to help. 👍

home-assistant / core