8000 homekit_controller can't parse zeroconf records from Lennox iComfort S30 · Issue #51391 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

homekit_controller can't parse zeroconf records from Lennox iComfort S30 #51391

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.

Sign up for GitHub

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

Jump to bottom
Closed
bazilrat opened this issue Jun 2, 2021 · 27 comments · Fixed by #51418
Closed

homekit_controller can't parse zeroconf records from Lennox iComfort S30 #51391

bazilrat opened this issue Jun 2, 2021 · 27 comments · Fixed by #51418
Assignees
@Jc2k @bdraco
Labels
integration: homekit_controller

Comments

@bazilrat
Copy link
bazilrat commented Jun 2, 2021 •
edited
Loading

The problem

Recent change to Homekit Fahrenheit precision affecting Lennox iComfort S30 integration. #50415

Any chance this could be changed to a toggle-able option?

What is version of Home Assistant Core has the issue?

core-2021.6.0

What was the last working version of Home Assistant Core?

core-2021.5.5

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Homekit

Link to integration documentation on our website

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response
@probot-home-assistant
Copy link

homekit_controller documentation
homekit_controller source
(message by IssueLinks)

@Jc2k
Copy link
Member
Jc2k commented Jun 2, 2021

What makes you think that change is the cause?

@bazilrat
Copy link
Author
bazilrat commented Jun 2, 2021

Updated to core-2021.6.0 integration was unresponsive. Reset hub and thermostat and HA numerous times, restored core-2021.5.5 integration became responsive again.

Anything I can do to confirm further, I'm game.

@probot-home-assistant
Copy link

Hey there @Jc2k, @bdraco, mind taking a look at this issue as its been labeled with an integration (homekit_controller) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@Jc2k
Copy link
Member
Jc2k commented Jun 2, 2021

I'm not doubting you have a problem just trying to figure out if the symptoms match that PR, and so far I'm not sure they do. There is another Icomfort user in #51388 having connection problems.

They unpaired and now can't repair, so don't try that.

Do you have any errors in your logs?

@Jc2k
Copy link
Member
Jc2k commented Jun 2, 2021

Try turning on debug logs for homesssistant.components.homekit_controller and aiohomekit.

@tteck
Copy link
tteck commented Jun 2, 2021

I'm thinking #51388 is the same problem as mentioned here. I also can restore 2021.6.0.dev0 and everything is working.

@Jc2k
Copy link
Member
Jc2k commented Jun 2, 2021

I'm thinking #51388 is the same problem as mentioned here. I also can restore 2021.6.0.dev0 and everything is working.

See I was thinking it was the other way round and this was a dupe of your ticket. At the moment though we need logs of the connection failing. The temperature precision shouldn't cause the connection to fail, as if you read that PR you see it doesn't actually change the request to the homekit device.

@bdraco
Copy link
Member
bdraco commented Jun 3, 2021

I agree, this feels like a problem I recently fixed in zeroconf 0.32.0 dev that hasn't been released yet.

@tteck
Copy link
tteck commented Jun 3, 2021

Whatever changed, it happened between May 21st (2021.6.0.dev0) and Beta, I'm thinking

@bazilrat
Copy link
Author
bazilrat commented Jun 3, 2021

Apologies if this isn't exactly related to the Fahrenheit precision change, was the only thing I saw related to homekit in the change log.

So I turned on logging and did some quick testing, before and after the update.

Seems after core-2021.6.0 update homekit attempts to connect to an IP address that is in my home networks subnet but there is no host on that ip and timeouts repeatedly.

2021-06-03 01:00:14 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Attempting connection to 192.168.50.131:7373
2021-06-03 01:00:17 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Connecting to accessory failed. Retrying in 43 seconds
2021-06-03 01:01:17 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Attempting connection to 192.168.50.131:7373
2021-06-03 01:01:20 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Connecting to accessory failed. Retrying in 60 seconds
2021-06-03 01:02:20 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Attempting connection to 192.168.50.131:7373
2021-06-03 01:02:23 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Connecting to accessory failed. Retrying in 60 seconds

Not sure where it is getting this IP from. I've bounced HA core, HA host, hub, and thermostat to no avail.

@Jc2k
Copy link
Member
Jc2k commented Jun 3, 2021

Thanks for the logging data.

Can you check your .storage/core.config_entries file? It will store the host and port in there, and I think if it can't find a new one via zeroconf it will fallback to that one. That might explain where that ip is coming from.

And then if you are able to provide a packet dump or run python3 -m netdisco dump then we might be able to confirm what is wrong with the icomfort mdns broadcast.

@bazilrat
Copy link
Author
bazilrat commented Jun 3, 2021 •
edited by Jc2k
Loading

That is indeed the IP in the core.config_entires file.

Here is the netdisco info:

homekit:
[{'host': '192.168.50.2',
  'hostname': 'Home Assistant Bridge EF07E5._hap._tcp.local.',
  'name': 'Home Assistant Bridge EF07E5',
  'port': 21063,
  'properties': {'c#': '71',
                 'ci': '2',
                 'ff': '0',
                 'id': '29:A5:B3:EF:07:E5',
                 'md': 'Home Assistant Bridge',
                 'pv': '1.0',
                 's#': '1',
                 'sf': '0',
                 'sh': 'M8S4PA=='}},
 {'host': '192.168.50.124',
  'hostname': 'lcc-WL17J01935.local.',
  'name': 'iComfort S30 3eab79',
  'port': 7373,
  'properties': {'C#': '2',
                 'ci': '9',
                 'ff': '1',
                 'id': '34:52:7F:C4:F5:47',
                 'md': 'S30 2B',
                 'pv': '1.1',
                 's#': '1',
                 'sf': '0',
                 'sh': '/Txw7A=='}}]

@Jc2k
Copy link
Member
Jc2k commented Jun 3, 2021

One thing that stands out here is the the iComfort has a property called C# instead of c#. Looking at Jc2k/aiohomekit@0.2.60...0.2.66 aiohomekit recently got a _service_info_is_homekit_device that doesn't take this into account (unless ServiceInfo is normalized somewhere).

Looking at the diff for where this get called, the code used to call _build_data_from_service_info() and then check for c# in the output of that function. That function normalizes the payload from zeroconf. So C# was transformed to c#. Now it checks for c# before normalizing, so it no lonnger thinks your device is a homekit device. This definitely seems like it would be a problem.

This regression is likely limited to iComfort devices which aren't on a network where DHCP leases have been pinned to mac addresses. (If your IP had pinned to .124 you likely wouldn't have noticed this.

Jc2k added a commit to Jc2k/aiohomekit that referenced this issue Jun 3, 2021
@Jc2k
Some HomeKit devices use C# instead of c# in their mdns metadata.
732f634 
See home-assistant/core#51391.
@Jc2k Jc2k mentioned this issue Jun 3, 2021
Merged
@Jc2k
Copy link
Member
Jc2k commented Jun 3, 2021

This might sort it out. It's a one line code change if anyone is able to test it. What do you think @bdraco?

@bazilrat
Copy link
Author
bazilrat commented Jun 3, 2021

Currently restored core-2021.5.5 but I can upgrade again to test.

This one liner something I can edit in portainer bash shell?

@Jc2k
Copy link
Member
Jc2k commented Jun 3, 2021

i honestly have no idea about portainer, but i've tested changes like that on docker before. I just tried on my homeassistant container and it does have vi, and the file in question is at /usr/local/lib/python3.8/site-packages/aiohomekit/zeroconf/__init__.py. And the function in question is on line 158. I'm sure you've clicked through and seen the diff, but replace:

def _service_info_is_homekit_device(service_info: ServiceInfo) -> bool:
    props = service_info.properties                                    
    return (                              
        service_info.addresses and b"c#" in props and b"md" in props and b"id" in props
    )

with

def _service_info_is_homekit_device(service_info: ServiceInfo) -> bool:
    props = {key.lower() for key in service_info.properties.keys()}
    return (
        service_info.addresses and b"c#" in props and b"md" in props and b"id" in props
    )

If portainer can restart the container without deleting and recreating things we should be golden.

@bazilrat
Copy link
Author
bazilrat commented Jun 3, 2021

This edit fixed the issue!

@Jc2k
Copy link
Member
Jc2k commented Jun 3, 2021

Wonderful. I'm going to get the ball rolling on releasing this so we can try and get it into a .1/.2 release.

@Jc2k
Copy link
Member
Jc2k commented Jun 3, 2021

Fix in https://github.com/Jc2k/aiohomekit/releases/tag/0.2.67

@Jc2k Jc2k changed the title Homekit Fahrenheit precision change broke Lennox iComfort S30 integration homekit_controller can't parse zeroconf records from Lennox iComfort S30 Jun 3, 2021
@bazilrat
Copy link
Author
bazilrat commented Jun 3, 2021

Thanks for the help and all that you do! Much appreciated.

Jc2k added a commit to Jc2k/home-assistant that referenced this issue Jun 3, 2021
@Jc2k
Bump aiohomekit to 0.2.67 (fixes home-assistant#51391)
c1d5ff5
@Jc2k Jc2k mentioned this issue Jun 3, 2021
Merged
21 tasks
@Jc2k
Copy link
Member
Jc2k commented Jun 3, 2021

PR opened: #51418 and tagged for .1. Fingers crossed it will make it 🤞

@Jc2k Jc2k mentioned this issue Jun 3, 2021
Closed
bdraco pushed a commit that referenced this issue Jun 3, 2021
@Jc2k
Bump aiohomekit to 0.2.67 (fixes #51391) (#51418)
836ce44
balloob pushed a commit that referenced this issue Jun 3, 2021
@Jc2k @balloob
Bump aiohomekit to 0.2.67 (fixes #51391) (#51418)
08e58e9
@balloob balloob mentioned this issue Jun 3, 2021
Merged
thomasgermain pushed a commit to thomasgermain/home-assistant that referenced this issue Jun 10, 2021
@Jc2k @thomasgermain
Bump aiohomekit to 0.2.67 (fixes home-assistant#51391) (home-assistan…
25762ea 
…t#51418)
@bazilrat
Copy link
Author

Showing same error again since upgrade to core-2021.6.6.

2021-06-21 10:52:52 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Attempting connection to 192.168.50.131:7373
2021-06-21 10:52:55 DEBUG (MainThread) [aiohomekit.controller.ip.connection] Connecting to accessory failed. Retrying in 67E6 28 seconds

@Jc2k
Copy link
Member
Jc2k commented Jun 21, 2021

Did it work at any point between (and including) .1 and .5?

@bazilrat
Copy link
Author

Yes worked on all after the fix including .5.

Restoring to .5 now to confirm.

@Jc2k
Copy link
Member
Jc2k commented Jun 21, 2021

I haven't changed anything recently, and certainly the only backport i've requested was the fix in .1. I've skimmed through the changelogs and can't see anything.

Will need to repeat the checks from last time. Check netdisco still see's it. Check the ip matches what is in core.config_entries. Can you also try and curl the ip and port in your config_entries file (just curl --verbose http://192.168.50.124:7373 is enough).

@bazilrat
Copy link
Author

Please disregard false positive on my end. Had to reset thermo and hub gateway but not it's solid.

Thanks for the quick look.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Jc2k Jc2k

@bdraco bdraco

Labels
integration: homekit_controller
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Bump aiohomekit to 0.2.67 (fixes #51391) Jc2k/home-assistant
5 participants
@Jc2k @bdraco @Adminiuga @tteck @bazilrat
0