Operational Discovery with Continuous Query Support for Linux #33402
+155
−50
Conversation
This file contains 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
|
PR #33402: Size comparison from 0d67568 to 948e46e Increases (54 builds for bl702, bl702l, cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, nxp, qpg, stm32, telink)
Decreases (2 builds for bl702l, efr32)
Full report (80 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
su-shanka
force-pushed
the
feature/mDNS-operational-browse
branch
from
948e46e
to
31f1219
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy and
joonhaengHeo
pullapprove
bot
requested review from
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
|
PR #33402: Size comparison from 3219a5f to 31f1219 Increases (54 builds for bl702, bl702l, cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, nxp, qpg, stm32, telink)
Decreases (2 builds for bl702l, efr32)
Full report (81 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
|
PR #33402: Size comparison from 3219a5f to e0a8475 Increases (27 builds for bl602, bl702, bl702l, cc13x4_26x4, linux, nrfconnect, nxp, qpg, stm32)
Decreases (3 builds for bl702l, linux)
Full report (30 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, linux, mbed, nrfconnect, nxp, qpg, stm32)
|
| @@ -607,7 +607,7 @@ bool ResolveContext::TryReportingResultsForInterfaceIndex(uint32_t interfaceInde | |||
| { | |||
| auto delegate = static_cast<DiscoverNodeDelegate *>(context); | |||
| DiscoveredNodeData nodeData; | |||
| service.ToDiscoveredNodeData(addresses, nodeData); | |||
| service.ToDiscoveredCommissionNodeData(addresses, nodeData); | |||
There was a problem hiding this comment.
So this does not make much sense to me. This could be an operational browse, no? DiscoverNodeDelegate claims to support operational, commissionable, etc.
I think we are fundamentally making an odd assumption that "operational browse means just finding the instance name" or something like that, but that's not what DiscoverNodeDelegate consumers want or get....
There was a problem hiding this comment.
I was not sure if Darwin supports operational browse yet. I might have incorrectly assumed that it doesn't.
We ideally need to call either ToDiscoveredCommissionNodeData() or ToDiscoveredOperationalNodeBrowseData() based on the browse type. This is what I have done in Discovery_ImplPlatform.cpp.
Does that make sense?
There was a problem hiding this comment.
I was not sure if Darwin supports operational browse yet.
I am telling you that it does.
And what it expects out of an operational browse in this case is complete information including the IPs and whatnot, no?
There was a problem hiding this comment.
In current interface, we decided not to have IPs and other resolution information for operational browse right?
OperationalNodeBrowseData only has NodeId derived from instance and zerottl flag.
There was a problem hiding this comment.
Well, my point is that there is existing operational browse code, and that code is expected to produce IPs and whatnot, and it's used by other code...
Maybe that means we need to figure out what our data structures and their naming look like better, but that ResolveContext code is absolutely reached for any kind of browse, and DiscoverNodeDelegate explicitly supports any kind of browse and produces full information for it.
If we have to do things this way for now just to unblock you we can, but the naming is pretty broken and we should at least have a followup to fix it...
There was a problem hiding this comment.
DiscoverNodeDelegate previously only delivered CommissionNodeData, even if the darwin implementation supported operational browse, the user of DiscoverNodeDelegate would probably not be expecting it. Also, during the refactor we have made sure all existing users of DiscoverNodeDelegate check that DiscoverNodeData has CommissionNodeData instead of OperationalNodeBrowseData.
Anyways, I will need this to get unblocked. So, please suggest how to proceed. Do I need to have check for serviceType and fill the right structure (i.e. either CommissionNodeData or OperationalNodeBrowseData) here? or does the current changes looks good for now?
There was a problem hiding this comment.
As discussed on slack, have added check for serviceType to fill the right structure before calling delegate.
Damian-Nordic
approved these changes
May 22, 2024
su-shanka
force-pushed
the
feature/mDNS-operational-browse
branch
from
cf2d1d8
to
f5d275b
Compare
|
PR #33402: Size comparison from c3ef110 to 2ac0919 Increases (52 builds for bl702, bl702l, cc13x4_26x4, cyw30739, efr32, esp32, linux, nrfconnect, nxp, qpg, stm32, telink)
Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink)
|
Comment on lines
+99
to
+100
| bool isOperationalBrowse = (strncmp(services[i].mType, kOperationalServiceName, sizeof(services[i].mType)) == 0 && | ||
| strlen(services[i].mType) == strlen(kOperationalServiceName)); |
There was a problem hiding this comment.
OK, so what I don't understand is why this isn't just:
Suggested change
| bool isOperationalBrowse = (strncmp(services[i].mType, kOperationalServiceName, sizeof(services[i].mType)) == 0 && | |
| strlen(services[i].mType) == strlen(kOperationalServiceName)); | |
| bool isOperationalBrowse = (strcmp(services[i].mType, kOperationalServiceName) == 0); |
| { | ||
| HandleNodeOperationalBrowse(context, &services[i], error); | ||
| } | ||
| // if SRV, TXT and AAAA records were received in DNS responses |
There was a problem hiding this comment.
Suggested change
| // if SRV, TXT and AAAA records were received in DNS responses | |
| // Check whether SRV, TXT and AAAA records were received in DNS responses |
(the actual case following this comment is the case when they were not received!)
Comment on lines
+611
to
+613
| // mType(service name) exactly matches with operational service name | ||
| if (strncmp(services[i].mType, kOperationalServiceName, sizeof(services[i].mType)) == 0 && | ||
| strlen(services[i].mType) == strlen(kOperationalServiceName)) |
There was a problem hiding this comment.
Suggested change
| // mType(service name) exactly matches with operational service name | |
| if (strncmp(services[i].mType, kOperationalServiceName, sizeof(services[i].mType)) == 0 && | |
| strlen(services[i].mType) == strlen(kOperationalServiceName)) | |
| // Check whether this is a resolve that is part of an operational browse. | |
| if (strcmp(services[i].mType, kOperationalServiceName) == 0) |
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.
Added operational node discovery/browse support to linux platform mdns implementation. Also, enhanced browse implementation in linux mdns to support continuous query browse instead single shot query.
This is a follow up from #32750 for linux platform.