-
-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
IGD review fixes #17400
IGD review fixes #17400
Conversation
@@ -31,7 +30,7 @@ | |||
|
|||
|
|||
REQUIREMENTS = ['async-upnp-client==0.12.4'] | |||
DEPENDENCIES = ['http'] | |||
DEPENDENCIES = ['http', 'discovery'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually thinking, why is it that we need both http and discovery to begin with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discovery
is required as the device is detected via upnp/M-SEARCH.
http
is required to determine the port home assistant is listening on (really only used the port mapping is enabled.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that is not needed for running this component, the SSDP response is stored in the config entry.
I think that we should remove both http and discovery, because marking it as dependencies means that we're setting it up.
If port mapping is enabled but http is not enabled: don't setup port mapping.
Not requiring discovery: nothing breaks?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we get a SSDP response discovery
is enabled already. The original component already (only) worked with discovery, but used its own library for it. Now the component is using more existing libraries, such as netdisco
and async_upnp_client
(used also by the dlna_dmr
-component).
The 'externals' (config, workings from the perspective of the user, etc) are mostly kept the same compared to the original upnp
component. Thus, depending on the http
component for getting the hass
-port for port mapping. discovery
is required as the user has (as before) no way to manually enter a SSDP description URL. Learned from the dlna_dmr
component, finding this URL is hard for the average user and discovery is preferable.
I get your point and I'm fine with not requiring these, but then there should be:
- a message in the logs clearly stating that discovery is required otherwise it will not work (as before)
- a message in the docs clearly stating that discovery is required otherwise it will not work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Philips Hue will, during their config flow, do their own discovery to find the device. The upnp component should do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That what is the purpose of netdisco
/discovery
if every component has to do its own discovery? This feels like unnecessary duplication of functionality, and hence, more suspect to bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It serves as purpose to find config entries. However, manual config entries will have to rely on their own scanning logic.
btw I notice that you're calling |
Creating port mappings does I/O as well. Where do you suggest to do it? |
Outside the event loop. No I/O allowed inside it.
|
192cf0f
to
a7b1f2c
Compare
@@ -31,7 +31,7 @@ | |||
|
|||
|
|||
REQUIREMENTS = ['async-upnp-client==0.12.7'] | |||
DEPENDENCIES = ['http'] | |||
DEPENDENCIES = ['discovery'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not depend on discovery.
Work has been slow, but will pick this up soon. |
It's already ok, will merge it. I removed the discovery dependency. |
Description:
Changes after review by @balloob
Related issue (if applicable): fixes #16300
Checklist:
tox
. Your PR cannot be merged unless tests passIf the code does not interact with devices: