Include all SSDP data in discovery info#28197
Conversation
|
#28196 merged, but I'll leave this open for a bit in case people find it desirable. |
|
|
||
| if device_info: | ||
| info[ATTR_SSDP_DATA] = device_info | ||
| # Deprecated below, use from device_info instead |
There was a problem hiding this comment.
I think we had the below info for backwards compatibility. But since we can check all the integrations which have an async_step_ssdp, we should be able to verify this. Couldn't we just send device_info? as the actual thing? :D
There was a problem hiding this comment.
Since we use ATTR_ constants, we should be able to update the constants and things keep working ?
There was a problem hiding this comment.
We can, but for compatibility we then need to inject some things from the SSDP entry to the now UPnP device description data. This feels a bit ugly, but I guess it's bearable. Implemented now.
It would be better in my opinion if those injected values would have clearer constant names and better values; that would not apparently require any in-tree changes at the moment as seemingly nothing uses them yet, like from:
ATTR_HOST = "host"
ATTR_PORT = "port"
ATTR_SSDP_DESCRIPTION = "ssdp_description"
ATTR_ST = "ssdp_st"...to:
ATTR_SSDP_HOST = "ssdp_host"
ATTR_SSDP_PORT = "ssdp_port"
ATTR_SSDP_LOCATION = "ssdp_location"
ATTR_SSDP_ST = "ssdp_st"Actually, I think I'd rather just remove the host and port altogether; things that need them can easily enough urlparse them from the location.
Thoughts?
BTW the hopefully shortly incoming Huawei LTE SSDP support (#28214) does use the SSDP attributes, but I can easily take care of those if the renaming/removal is the way we want to go.
There was a problem hiding this comment.
Anything I can do to get this moving?
There was a problem hiding this comment.
Rebased and implemented the above plan.
balloob
left a comment
There was a problem hiding this comment.
The approach is good, but please search the codebase for async_step_ssdp and make sure that all config flows are working with the new constants.
For example, noticed heos uses
from homeassistant.const import CONF_HOST, CONF_NAME|
Ouch, will do -- I think those were kind of bugs to begin with as the ssdp steps should have used the same named constants from ssdp, not core const. |
|
Ok, should be in order now. I went a bit wild and changed attribute names to |
|
Sorry, I made some changes to the Hue flow. This is ok after merge conflicts resolved. |
Breaking Change:
Structure and constant names for SSDP discovery data has changed. All UPnP device description data is now made available, but the previously available host and port no longer are -- they can be obtained by urlparsing the
ATTR_SSDP_LOCATIONvalue if needed. Consulthomeassistant.components.ssdpfor the new attribute name constants.Description:
Some data is missing from SSDP discovery info, not sure why all of it wouldn't be passed on. See #28196 for some more info and an alternative PR.
Related issue (if applicable): #28196
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>
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.txtby runningpython3 -m script.gen_requirements_all..coveragerc.If the code does not interact with devices: