Updates MdnsDiscovery class and affected test scripts to address issues #39737
+2,790
−690
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request updates the mdns class and TC-SC-4.3 test case. The changes include adding new methods for retrieving SRV, TXT, and AAAA records, improving error handling, and refactoring some of the existing code. The code review identified opportunities to improve error handling, logging, and documentation.
|
PR #39737: Size comparison from 41c7192 to 796d6ec Full report (3 builds for cc32xx, stm32)
|
raul-marquez-csa
changed the title
|
PR #39737: Size comparison from 06523c2 to 1ec8aa9 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39737: Size comparison from e3d8447 to 613631c Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39737: Size comparison from 1c3303e to 4e11744 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39737: Size comparison from 1c3303e to 1fbfb65 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
…aming convention alignment
|
PR #39737: Size comparison from da00cfb to be87f70 Full report (4 builds for cc32xx, nrfconnect, stm32)
|
|
PR #39737: Size comparison from da00cfb to aa8ce10 Full report (57 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39737: Size comparison from da00cfb to f6f089c Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
cecille
reviewed
Aug 19, 2025
| logger.info(f"Service record information lookup {rec_types} for '{service_name}' in progress...") | ||
| await wait_for(service_listener.updated_event.wait(), SERVICE_LISTENER_TIMEOUT_SEC) | ||
| except TimeoutError: | ||
| logger.info( |
There was a problem hiding this comment.
This code eats the TimeoutError and turns it into a log. It seems like this should be raised since you won't have service info in the call below if this timed out.
There was a problem hiding this comment.
That would ultimately change the return type of for example get_srv_record which is Optional[MdnsServiceInfo], at the moment it returns None, by design.
Failing the test because of not finding a particular service (weather expected or unexpected) is handled by each test script as needed.
So it should stay as is IMO.
| A service listener required during mDNS service discovery | ||
| """ | ||
|
|
||
| def __init__(self): |
There was a problem hiding this comment.
can you put that as a method on this class so it's clear how callers are intended to use this? Right now it's totally not clear.
|
|
||
|
|
||
| @dataclass | ||
| class SpecialAddressTypes: |
There was a problem hiding this comment.
Added clarifying comments to the class
@dataclass
class SpecialAddressTypes:
"""
Represents special categories of IPv6 addresses that can be derived
from or mapped to IPv4. Used to identify and annotate IPv6 addresses
with special handling or semantics.
"""
# IPv6 address derived from a Teredo tunnel (2001::/32 range).
# Used for NAT traversal — wraps IPv4 inside IPv6.
teredo: Optional[str]
# IPv6 address derived from 6to4 tunneling (2002::/16 range).
# Automatically maps an IPv4 into an IPv6 address.
sixtofour: Optional[str]
# Whether the address falls into a reserved IPv6 block
# (e.g., ::/128, ::1/128, ::ffff:0:0/96, etc.).
is_reserved: bool
# An IPv6 address that is explicitly mapped to an IPv4 address
# (::ffff:0:0/96 prefix). Example: ::ffff:192.0.2.128
ipv4_mapped: Optional[str]
To answer the question directly, at the moment those properties aren't being used. But they are part of what is returned by the original ZeroconfIPv6Address object, the quada class groups some of those togetger for easier handling.
This is an example logging of a quada query response:
{
"00155D5436BF.local.": [
{
"address": "fe80::215:5dff:fe54:36bf",
"version": 6,
"interface": "eth0",
"type_info": {
"is_global": false,
"is_link_local": true,
"is_loopback": false,
"is_multicast": false
},
"special_types": {
"teredo": null,
"sixtofour": null,
"is_reserved": false,
"ipv4_mapped": null
},
"meta": {
"max_prefixlen": 128,
"packed": "fe8000000000000002155dfffe5436bf"
}
}
]
}
| Spec: | ||
| https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/secure_channel/Discovery.adoc#21-operational-instance-name | ||
| """ | ||
| consts = [ |
There was a problem hiding this comment.
It took me a while to figure out why you were doing it like this, and I THINK the intent here is to be able to list multiple failure modes in one assert so testers can see all the things that are wrong. But because failed is getting overwritten every time, you don't actually get this behaviour. To make this work, you'd need to have failed start as an empty list and append to it.
There was a problem hiding this comment.
Also, these functions are good candidates to have their own tests to show this.
There was a problem hiding this comment.
This will be addressed in a separate PR
| Spec: | ||
| https://github.com/CHIP-Specifications/connectedhomeip-spec/blob/master/src/secure_channel/Discovery.adoc#16-txt-key-for-vendor-id-and-product-id-vp | ||
| """ | ||
| constraints = [ |
There was a problem hiding this comment.
is this the same as the above function with just a different name?
There was a problem hiding this comment.
Yes in that they both have the same constraints (Vendor ID and Product ID)
I created both so it's a clearer read when using them in test scripts
Do you suggest merging them?
Co-authored-by: C Freeman <cecille@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing
Main Fixes
PTR record info retrieval failure, #37154 and #524
AAAA record info retrieval failure and query request spamming
Ensures mDNS info isn't retrieved from cache
Ensures queries are sent over IPv6 only #536
Main updates
Individual Methods: Implemented individual methods per record type:
get_srv_record,get_txt_record,get_quada_records,get_ptr_records.Improved AAAA record handling: Switched previous AAAA record query code for Zeroconf out-of-box address resolver. Encapsulates AAAA record data into it's own
QuadaRecordclass.Improved PTR record handling: Removed service info querying for discovered instances since only the PTR info is required. Encapsulates the PTR record data into it's own
PtrRecordclass.Improved logging: Improved logging for
get_all_service_typesmethod #524Added new method:
get_commissionable_subtypesAdded asserts file: Contains assertions for TXT keys, instance/hostnames, service types, as per the spec. These can be reused wherever needed in the test scripts.
MdnsDiscovery class cross-updates: Updates
TC-SC-4.3,TC-CADMIN-1.3.4,TC-CADMIN-1.5,TC-CADMIN-1.15,TC-ICDM-5.1, andcadmin_support.pyto matchMdnsDiscoveryclass updates.Documentation
Added documentation for the MdnsDiscovery class to assist with PR review and help understand how the different components fit together, given the size and scope of the codebase.
This is the official documentation.
📃 MdnsDiscovery Class Documentation