Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mdns add browse support (IDFGH-12512) #522

Merged

Conversation

zwx1995esp
Copy link
Contributor

@zwx1995esp zwx1995esp commented Mar 8, 2024

This MR introduces a new query mode: browse(like the avahi-browse -r _test._tcp).

mdns_browse_new is used for starting a browse, it will add a browse in the link list of _mdns_server, if there is some service start or leave, it will be notified by the callback passed in the API. When the service is leaved, the ttl will be 0, the related result will be freed after syncing to the notifier callback.

mdns_browse_delete is used for stop the browse query and free the related memory.

All apis are followed the mdns action mechanism.

@zwx1995esp zwx1995esp force-pushed the feature/mdns_add_browse_suport branch 2 times, most recently from 43500eb to 155f270 Compare March 11, 2024 08:01
@zwx1995esp zwx1995esp changed the title Draft: mdns add browse support mdns add browse support Mar 11, 2024
@gabsuren gabsuren added the mdns label Mar 11, 2024
@zwx1995esp zwx1995esp force-pushed the feature/mdns_add_browse_suport branch 5 times, most recently from 57a2c35 to 23446f1 Compare March 13, 2024 02:45
components/mdns/include/mdns.h Show resolved Hide resolved
components/mdns/mdns.c Show resolved Hide resolved
components/mdns/mdns.c Show resolved Hide resolved
components/mdns/mdns.c Show resolved Hide resolved
@jonsmirl
Copy link

jonsmirl commented Apr 1, 2024

The algorithm for doing this is described in the MDNS specification. Is it being followed?
https://datatracker.ietf.org/doc/html/rfc6762#section-5.2

I just encountered this inability to monitor when trying to build a matter controller. esp-matter needs this feature.

@github-actions github-actions bot changed the title mdns add browse support mdns add browse support (IDFGH-12512) Apr 1, 2024
@zwx1995esp
Copy link
Contributor Author

The algorithm for doing this is described in the MDNS specification. Is it being followed? https://datatracker.ietf.org/doc/html/rfc6762#section-5.2

I just encountered this inability to monitor when trying to build a matter controller. esp-matter needs this feature.

No, I just send the query once and then listen for the service which has been queried.

@jonsmirl
Copy link

jonsmirl commented Apr 1, 2024

For esp-matter with just need to do a query on something like _matterc._tcp and then monitor add/deletes. This is the event interface. https://github.com/project-chip/connectedhomeip/blob/master/src/lib/dnssd/platform/DnssdBrowseDelegate.h

There is no requirement to implement everything in the MDNS spec, all that is needed is enough to implement this interface.

@chshu
Copy link

chshu commented Apr 2, 2024

@jonsmirl This MR introduces a continuous browse feature which is similar to avahi-browse, it will send the query once, then monitor the incoming responses, and trigger the callbacks immediately once the service changes.

It was required by Thread BR project, while it should also works for your Matter controller use case. Let us know If there are any functionalities missing.

components/mdns/mdns.c Outdated Show resolved Hide resolved
components/mdns/mdns.c Outdated Show resolved Hide resolved
components/mdns/mdns.c Outdated Show resolved Hide resolved
components/mdns/mdns.c Outdated Show resolved Hide resolved
components/mdns/mdns.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for posting these changes and implementing browse support. This LGTM! I like the way you treat browsing separately from the searching and other features. It may be helpful to reuse some code from the searching routines, but this change is less invasive and more compact 👍

just left few nitpicks and of course we need to test the changes properly.

@zwx1995esp zwx1995esp force-pushed the feature/mdns_add_browse_suport branch 2 times, most recently from 85ad383 to 67f9cb7 Compare April 7, 2024 08:17
@zwx1995esp
Copy link
Contributor Author

zwx1995esp commented Apr 7, 2024

just left few nitpicks and of course we need to test the changes properly.

Hi, @david-cermak @gabsuren . I do these tests manually in my locally.

And in my local test, I test for these scenarios manually:

  1. Start browsing with the service _test._udp on S3, and start/stop publishing these services via linux host using avahi
(S-A) avahi-publish-service test _test._udp 12347 test=4444 dn="for_ci_br_test1010"
(S-B) avahi-publish-service aaa _test._udp 20617 zwx=esp dn="for_ci_br_test"

case-1: Start browsing with the service _test._udp, start (S-A), stop (S-A), Stop browsing with the service _test._udp (check the heap usage during the test).
case-2: Start browsing with the service _test._udp, start (S-A), start (S-B), stop (S-A), stop (S-B), Stop browsing with the service _test._udp (check the heap usage during the test).
case-3: Start browsing with the service _test._udp, start (S-A), start (S-B), stop (S-B), stop (S-A), Stop browsing with the service _test._udp (check the heap usage during the test).
case-4: start (S-A), start (S-B), start browsing with the service _test._udp, stop (S-B), stop (S-A), Stop browsing with the service _test._udp (check the heap usage during the test).
2. Start browsing with the service _test._udp and _testtt._udp, and start/stop publishing these services via linux host using avahi

(S-A) avahi-publish-service test _test._udp 12347 test=4444 dn="for_ci_br_test1010"
(S-B) avahi-publish-service aaa _test._udp 20617 zwx=esp dn="for_ci_br_test"

(S-C) avahi-publish-service testxxx _testtt._udp 1234 test=1235 dn="for_ci_br_test"

case-1: Start browsing with the service _test._udp, Start browsing with the service _testtt._udp, start (S-A), stop (S-A), , start (S-C), stop (S-C), Stop browsing with the service _test._udp, Stop browsing with the service _testtt._udp (check the heap usage during the test).
case-2: Start browsing with the service _test._udp, Start browsing with the service _testtt._udp, start (S-A), start (S-B), start (S-C), stop (S-A), stop (S-B), stop (S-C), Stop browsing with the service _test._udp , Stop browsing with the service _testtt._udp (check the heap usage during the test).
3. The API mdns_browse_new and mdns_browse_delete
case-1: Start browsing with the service _test._udp for several times, check the heap usage during the test.
case-2: Stop browsing with the service _test._udp for several times, check the heap usage during the test.
case-3: Start browsing with the service _test._udp, then sop browsing with the service _test._udp. Rerun for several times, check the heap usage during the test.

All the test case seems work fine in my local test env.

Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@zwx1995esp zwx1995esp force-pushed the feature/mdns_add_browse_suport branch from 67f9cb7 to af330b6 Compare April 10, 2024 03:23
@gabsuren gabsuren merged commit fbdb248 into espressif:master Apr 10, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants