Move chip tool resolve command to address_resolver #15544
Conversation
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel and
lazarkov
pullapprove
bot
requested review from
LuDuda,
mlepage-google,
msandstedt,
mspang,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
|
PR #15544: Size comparison from a4d427d to f0188e8 Increases (2 builds for linux)
Full report (45 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
fast track after 24h |
|
|
||
| /////////// DiscoverCommand Interface ///////// | ||
| CHIP_ERROR RunCommand(NodeId remoteId, uint64_t fabricId) override | ||
| { | ||
| ReturnErrorOnFailure(mDNSResolver.Init(chip::DeviceLayer::UDPEndPointManager())); | ||
| mDNSResolver.SetOperationalDelegate(this); | ||
| ChipLogProgress(chipTool, "Dnssd: Searching for NodeId: %" PRIx64 " FabricId: %" PRIx64 " ...", remoteId, fabricId); |
There was a problem hiding this comment.
Why did this logging get removed?
There was a problem hiding this comment.
I replaced the entire content of the command , so I ended up removing all existing content. Did not seem like a very important log as it basically repeats command-line arguments so it will likely not provide extra information for the user (although it could be used to validate that parsing is ok).
I can add it back if it seems important.
|
|
||
| if (retryInterval.HasValue()) | ||
| ChipLogProgress(chipTool, " MRP retry interval (active): %" PRIu32 "ms", retryInterval.Value().count()); | ||
| result.address.ToString(addrBuffer); |
There was a problem hiding this comment.
But ... this command was in fact meant to be used to log all the addresses being advertised, so it could be used for testing that. This breaks the command, no?
There was a problem hiding this comment.
I could see no obvious docks that would say "it lists all addresses". I assumed any mention of "all addresses" is not correct because our mdns implementations cannot seem to guarantee this - at least avahi can only show one it seems (at least when I poked at it I could not get it to give me multiple addresses).
There was a problem hiding this comment.
This command is meant for testing purposes. Yes, the docs are not great. But this was one of the few things we had that we could use to actually see what ips are ending up resolving in practice.... What is the replacement?
And yes, with Avahi the results were wrong. That should be fixed in the Avahi intergration bits, no?
There was a problem hiding this comment.
My understanding of the use was do resolve like chip library does and give me the result hence my implementation. Is there a need for full results? I was assuming no since we are aware not all libs coud do it.
I see no very direct replacements so I would be willing to re-work or undo the change (prefer rework really since I would change the name to "resolve-all-addresses").
For debug the resolver logs its own lookups so there is data there beyond just one address and we can enhance that (this command was using LogProgress as well, so maybe no loss in data, but some loss in formatting and we may need to force resolver to log all addresses instead of just new best picks).
I would like to understand the use case though - who uses this command and where. My change was assuming "this does a resolve of a node and wants the same result as the regular chip library". If instead we want "Log content of DNSSD resolve packets" that works too. In that case I think we should not be using Proxy but rather use the DNSSD interfaces only (as there is no parallelism).
There was a problem hiding this comment.
tagging @vivien-apple to see how to follow up on this. I am unsure who uses this and where
There was a problem hiding this comment.
The original goal of this command was to get the ip/port used by the controller to talk to a commissioned device. It was mostly a tool to debug the darwin backend without having to resort to ChipTool iOS.
Now I believe it has evolved over time to be used as a debug tool when dealing with mdns advertisement related issues, so a replacement would be useful. About using Proxy versus dnssd interfaces, this is mostly a story of what is the easiest thing to use since the focus for this utility is really on the dnssd data, not on the API.
There was a problem hiding this comment.
Do I need to take any specific action here to have all addresses reported or is this usable as-is?
I would prefer as little delta as possible - either keep as is or if we need to see all addresses to add logging into the address resolver for each address since that seems to serve the "use for debug" purpose and the final result is useful to "get the ip/port used by the controller".
There was a problem hiding this comment.
Adding logging into the resolver would be a good idea, I think, and sufficient ofor debugging purposes.
There was a problem hiding this comment.
created #15653 for this.
Problem
Looking for a path to start using address resolution logic instead of ResolveProxy, since address resolution handles prioritization and timeouts as well as parallel search potential.
Change overview
Changed
chip-tool discovery resolveto use addresss_resolve. This PR is a delta from #15536 (since it requires MRP and tcp data to display).Since nodedata MRP parameters seem to NOT be optional, I updated the tool to always print out MRP settings that it would use. This means that it would not print out DNSSD data but rather the configuration that other CHIP operations would/should use.
Testing
Manually ran
./out/linux-x64-chip-tool/chip-tool discover resolve 0x000000000001E1B9 0x116362B9D227A22Aobserved values were printed out.