-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
Support Huawei LTE SSDP discovery #28214
Conversation
# Try to distinguish from other non-LTE Huawei router devices | ||
if ( | ||
discovery_info.get(ATTR_UPNP_DEVICE_TYPE) | ||
!= "urn:schemas-upnp-org:device:InternetGatewayDevice:1" |
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.
You should be able to add device type to the manifest https://developers.home-assistant.io/docs/en/creating_integration_manifest.html#ssdp
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.
The docs say that discovery is invoked if any of the specified info is found. I read that to mean that if I added device type to the manifest, I'd need to check both manufacturer and device type matches in the code (because I would be handed all cases where either of them matches), whereas I now only have to check device type there and manufacturer is taken care of for me. So adding device type to the manifest would just add complexity for no gain that I can see.
Now, if the info match in manifest data would require all specified bits to match (instead of any), then I could move this to the manifest.
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.
Verified the "any" behavior, so I guess it's a matter of taste whether to check manufacturer or device type in manifest and the other in code. Leaving as is for now.
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.
oh, interesting. That feels weird 🤔 Wonder if we should update the way it works? I don't think that we have any integrations relying on the any
behavior
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 agree and
would be better. Added that and some other improvements in #28285
Co-Authored-By: Paulus Schoutsen <paulus@home-assistant.io>
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.
Looks good!
Test failure is unrelated. |
Do we need a new test for the discovery flow entry point to keep coverage? |
Description:
Per subject.
Related issue (if applicable): fixes #
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#10992
Example entry for
configuration.yaml
(if applicable):Checklist:
tox
. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
python3 -m script.hassfest
.requirements_all.txt
by runningpython3 -m script.gen_requirements_all
..coveragerc
.If the code does not interact with devices: