core: HomeKit Controller Climate Integration Cannot Set Temp
The problem
Added Honeywell T6 Pro to HA with HomeKit Controller integration, states coming through correctly but service call climate.set_temp appears to not work. I am able to succesfully climate.turn_on so I am not fully locked out. Seems to be an issue with the temperature value being sent, possibly too many digits. No official errors are raised, just fails silently, but can see a status code in response from unit in debug logs.
Environment
- Home Assistant Core release with the issue: 0.111.4
- Last working Home Assistant Core release (if known): N/A
- Operating environment (OS/Container/Supervised/Core): OS
- Integration causing this issue: HomeKit Controller or base Climate
- Link to integration documentation on our website: https://www.home-assistant.io/integrations/homekit_controller/
Problem-relevant configuration.yaml
Traceback/Error logs
2020-06-24 21:59:18 DEBUG (MainThread) [aiohomekit.controller.ip.connection] REDACTED: raw request: b'PUT /characteristics HTTP/1.1\r\nHost: REDACTED\r\nContent-Type: application/hap+json\r\nContent-Length: 66\r\n\r\n{"characteristics":[{"aid":1,"iid":13,"value":22.22222222222222}]}'
2020-06-24 21:59:18 DEBUG (MainThread) [aiohomekit.controller.ip.connection] REDACTED: raw response: bytearray(b'{"characteristics": [ {"aid":1,"iid":13,"status":-70410}]}')
Additional information
I believe the issue is around the precision of the value. I googled the status code returned and found this https://github.com/apple/HomeKitADK/blob/master/HAP/HAPIPAccessoryServer.c#L59 which pointed me in that direction of something in the payload being off. I picked a Fahrenheit value that converts to Celcius, 77->25.0, and that worked so we probably just need to round a bit of the value. My problem is I don’t know if this is something that should be rounded across the board, would love to help but have not worked in this code base before so would need a bit of help.
2020-06-24 21:59:47 DEBUG (MainThread) [aiohomekit.controller.ip.connection] REDACTED: raw request: b'PUT /characteristics HTTP/1.1\r\nHost: REDACTED\r\nContent-Type: application/hap+json\r\nContent-Length: 53\r\n\r\n{"characteristics":[{"aid":1,"iid":13,"value":25.0}]}'
2020-06-24 21:59:47 DEBUG (MainThread) [aiohomekit.controller.ip.connection] REDACTED: raw response: bytearray(b'')
Note that an empty response seems to be good, got empty response when turning on/off when that worked. Also wrote in the Lyric thread https://community.home-assistant.io/t/honeywell-lyric-thermostat/3520/857?u=stshontikidis
About this issue
- Original URL
- State: closed
- Created 4 years ago
- Comments: 26 (24 by maintainers)
Man you got that reviewed fast! I have been sitting around on another PR for like 5 days waiting for a review. Was told there was no real process except to wait and someone will tackle it when they have time.
I am currently working on a new integration but when I get to a good point I will pull in dev.
There we go - PR raised here.
cool, not like the release cycle is that slow so not too worried. Though we had a flurry but it calmed down, are they gearing up for the move to 112? I guess I should have noted my testing environment was based on 112. Thanks again for the responsiveness, glad I don’t have to use Honeywell dev API and can keep it local.
Brilliant! Thanks for your help testing this! It’s the end of the day here but I’ll get a new release of aiohomekit made tomorrow, and hopefully put in a PR to HA as well. Off the top of my head I think we might miss the time window for the next HA release but we’ll see…
Been unable to get around to testing just yet, hope to get time tonight or tomorrow night at the latest (-5 GMT).
Great! Thanks for testing.
I’ve added a bunch more testing on my side and, sigh, I found some issues. First of all integers have a minStep property too. It’s usually
1but applying the code we have so far turns the integers into floats. Given the problems we’ve seen with the manufacturers supporting JSON i’m pretty sure there will be a device on the market that can’t handle receiving a float value for an int field, so I had to fix that. Whilst writing tests for it I learned something new about python3 rounding. It changed from how it worked in py2! The default is now bankers rounding. This meant some values that i was expecting to round up rounded down. So i’ve had to stop usinground()and instead lean even more into the decimal module. The tests I have still pass, as do the new tests, but it would be great if you could test again. The changes are quite a bit larger than before as i’ve refactored the code socheck_convert_valuehas access to all the information on a characteristic. The changes are back on the main master branch now. If you want to see the diff its here.Looks like a solid solution and should cover most sane device styles. Everything on my end testing against the Lyric T6 Pro was good, went through a ton of F values from and each one took. Output logs