Skip to content
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.

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

Fix to work on new async_upnp_client versions #11

Merged
merged 5 commits into from
May 4, 2022

Conversation

FilHarr
Copy link
Contributor

@FilHarr FilHarr commented Apr 4, 2022

Have made some changes to update the integration in light of changes made to async_upnp_client. Have done some testing using the current dev release of HA and everything seems to be working as expected (with the host having both IPv4 and IPv6 addresses).

Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @FilHarr! I'm wondering, are you absolutely sure that search() doesn't accept regular ipaddress-created objects? @StevenLooman, do you think v4/v6 handling could be implemented inside async_upnp_client?

@FilHarr
Copy link
Contributor Author

FilHarr commented Apr 5, 2022

As best I can tell async_search() is expecting the IP address to be provided as a tuple (AddressTupleVXType). For IPv4, it's a 2-tuple with address and port, and for IPv6 it's a 4-tuple with address, port, flowinfo and scope_id.

My knowledge of python is rudimentary at best, so I'm unsure if ipaddress can create a suitable object (although my brief scan of documentation around didn't suggest anything obvious). There may be a suitable function already added somewhere within async_upnp_client, but my digging around was limited to trying to work out why the error(s) was being thrown and how to resolve that.

@StevenLooman
Copy link

StevenLooman commented Apr 5, 2022 via email

@FilHarr
Copy link
Contributor Author

FilHarr commented Apr 5, 2022

@StevenLooman does that function take an IPv4 or V6 address without a port, either as a string or ipaddress object, and produce an appropriately formatted tuple?

It looks (and this may just be my lack of python knowledge) like it's expecting either a tuple or None as an input.

@StevenLooman
Copy link

No, it still requires a tuple. See https://github.com/home-assistant/core/blob/fcad10dd0d58dfd2486862860cbc847d95696ce8/homeassistant/components/ssdp/__init__.py#L436 for an example.

Unfortunately, the IPv6Address class in Python <3.9 does not support scope_id and the only workable thing is those tuples. Perhaps some time in the future, when support for Python 3.7/3.8 is dropped, this will be improved in async-upnp-client. For now, the tuples are used.

@FilHarr FilHarr requested a review from rytilahti April 11, 2022 15:34
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting this error (during the initial search) which also blocks the startup:

ERROR (MainThread) [homeassistant] Error doing job: Fatal write error on datagram transport
Traceback (most recent call last):
  File "/usr/lib/python3.10/asyncio/selector_events.py", line 1053, in sendto
    self._sock.sendto(data, addr)
TypeError: 'str' object cannot be interpreted as an integer

...
WARNING (MainThread) [homeassistant.components.binary_sensor] Setup of binary_sensor platform upnp_availability is taking over 10 seconds

Some of those parameters that are being passed to async_upnp_client are likely the culprit, but I'm done debugging for tonight.

@nigel-wells
Copy link

FYI I just tried out these changes in my brand new setup and everything has worked fine so far. I did find (even before this change) that the integration option didn't appear straight away which made me think it didn't work even though the logs didn't show any errors. I'm not sure what triggered it to appear but it might have taken a refresh of my browser?

@rytilahti
Copy link
Owner

My last commit fixes the initial discovery (sendto() excepts scope_id as an integer for v6), thanks for the PR @FilHarr! And thank you @StevenLooman for your insights & async_upnp_client! :-)

@rytilahti rytilahti changed the title Fixes for changes to async_upnp_client Fix to work on new async_upnp_client versions May 4, 2022
@rytilahti rytilahti added the bug Something isn't working label May 4, 2022
@rytilahti rytilahti merged commit e68da5f into rytilahti:master May 4, 2022
@StevenLooman
Copy link

You're welcome. It's always surprising and interesting to see where async_upnp_client is used. :)

@FilHarr FilHarr deleted the dev branch May 6, 2022 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants