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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use dataclass properties in axis discovery #60558
Conversation
Hey there @Kane610, mind taking a look at this pull request as it has been labeled with an integration ( |
"""Prepare configuration for a SSDP discovered Axis device.""" | ||
url = urlsplit(discovery_info["presentationURL"]) | ||
url = urlsplit(discovery_info.upnp["presentationURL"]) |
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.
Why is it upnp and not 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.
@chism please correct me if I am explain it wrongly, as I may have to explain it in multiple integrations.
The ssdp
discovery works by combining the result of two broadcasts :
- one broadcast/response contains only SSDP data
- another broadcast/response contains only UPnP data
The full data is now available in two separate fields: discovery_info.ssdp_headers
and discovery_info.upnp
, with some of the SSDP data extracted into the various discovery_info.ssdp_XXX
fields.
In the case of axis
, I believe presentationURL
, serialNumber
and friendlyName
are part of the UPnP data not part of the SSDP
data but I could be wrong and it would be great if you had the opportunity to check it.
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.
There is probably a better source than Microsoft for this, but this gives an indication of the fields that are provided by UPnP: https://docs.microsoft.com/en-us/windows/win32/api/upnp/nn-upnp-iupnpdevice
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 have gone even further on the PR, and use constants for the UPNP information. This highlights the fact that it is indeed UPnP data and not part of the SSDP headers.
In my opinion this is ready to merge.
Note: this PR is required for #60561 |
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.
Still a bit confusing at times, but ok
"""Prepare configuration for a SSDP discovered Axis device.""" | ||
url = urlsplit(discovery_info["presentationURL"]) | ||
url = urlsplit(discovery_info.upnp[ssdp.ATTR_UPNP_PRESENTATION_URL]) |
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.
This is some real meta stuff right here 馃し馃徎
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.
Ignore this comment
Proposed change
Use dataclass properties in
axis
discovery.Linked to #60540
Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: