Migrate callbacks of new mdns responder to functional

[devyte]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: [All
• Core Version: latest git today
• Development Env: All
• Operating System: All

Settings in IDE

• Module: All

Problem Description

The new MDNSResponder implementation written by @LaborEtArs allows for setting up some user callbacks. This is currently implemented in the classic C way with specific function signatures. It would be more flexible to allow std::function, lambdas, and general functors.

MCVE Sketch

See new MDNS examples.

[devyte]

@hreintke given your great past work in similar areas, could I ask for your help with this? There is no hurry.

[hreintke]

@devyte OK, I will look into this

[hreintke]

@devyte @LaborEtArs
Started working on this by first getting the mDNS_ServiceMonitor.ino example to work.

I think there is an error in the MDNSProbeResultCallback function.

The

      else {
        // Change hostname, use '-' as divider between base name and index
        if (MDNSResponder::indexDomain(pcHostDomain, "-", 0)) {
        	Serial.printf("insert - %s",pcHostDomain);
          p_pMDNSResponder->setHostname(pcHostDomain);
        } else {
          Serial.println("MDNSProbeResultCallback: FAILED to update hostname!");
        }

is now on :

if (!bHostDomainConfirmed) {

But it should be on

if (true == p_bProbeResult) {

[LaborEtArs]

Unfortunately you’re absolutely right. In fact, the mDNS_Clock example has got the same error...

[devyte]

So both the clock and servicemonitor examples have the same mistake?
Are we talking about this condition?

[hreintke]

Yes, That is the one.
If you want, I can make a PR for it.

The update for the functional option is very simple. Only the typedefs neeed change.
Need to make decent examples for using it. When finished that I will PR that also.
Will come on short notice.

[devyte]

@hreintke yes to the PR please!

[hreintke]

@devyte See #5563

[hreintke]

@devyte @LaborEtArs
I am not experienced in the mDNS area, so the issue below can be because of that but I have the following behavior.

I have trouble getting the LEAnMDS working steady, so I decided to compare legacy and lea using the mDNS_Web_Server.ino example.
Used the using MDNSResponder options from ESP8266nDNS.h to switch between both.
Confirmed by serial print that i am using them.

I test by submitting esp8266.local in chrome browser.
When using legacy, it works ok, also for longer periods.
When using LEA it works at the start, but after a short while I see the message "resolving host" and then getting the "This site cannot be reached" error.

Is there a chance that this is caused by the LEA implementation itself ?
Would it be possible that you do the same test ?

[devyte]

@hreintke that sounds like the missing update() call. Are you using the .ino from latest git or beta2? Please make sure the following line is in the loop in your example .ino:

MDNS.update();

[hreintke]

I am using latest git where MDNS.update() is in the loop.
I do see the cache flush responses coming from the esp every 100 secs.

Checking more, learning of mDNS and debugging it.

[hreintke]

It is indeed the missing MDNS.update();

Although it is in de loop(), the sketch has :

  while (client.connected() && !client.available()) {
    delay(1);
  }

which prevents the update to be executed.
adding it into this also makes it work.

Still to check why using a browser it 'hangs' there and using curl it waits in server.available()

[LaborEtArs]

These lines of code are also included in both new examples (1:1 taken from the original mDNS example). Have you tried these examples also?
Your mentioning a 'short while' before the system fails... might this be around 100 seconds (s. below), or is it an completely different time span?

The mentioned code should only be executed, if a client connects via port 80. Why does the browser already opens a connection to his port if it is asking in parallel for the general 'coordinates' of the target via the MDNS call? Might it be a 'Chrome' speciality?

The 'cache flush' should be the mDNS response to some call to the mDNS responder of the ESP. The request should be triggered by the caller at about 80% of the TTL of the ESPs IP address record, which is 120s. So your observed 100s seems to be fine, but: The ESP shouldn't advertise these records 'just fur fun'. So who is the caller? Are there other devices around, that might be interested in the ESP? Have you tried to move the HTTP service to another port?

Unfortunately, I haven't got a running ESP environment currently, as my system (using Eclipse as well as Arduino) currently doesn't like to successfully build or upload even a simple 'Hello world' sketch.... working on this....

[hreintke]

Yes, I've seen the same way of working in the other examples. Those are also failing at my site.
So is most certainly an issue here. Never noticed it because I always use webserver (async) and never wificlient (my loop() is always as empty as possible).
There must be something in my network. Needs investigation.
BTW, also sync webserver runs without problems.

The mDNS responses are indeed triggered by a caller. Missed that due to a wrong filtering in Wireshark.
First time mDNS gives some startup challenges. Sorry for the confusion from this.

Workarounds ready for this, I will make an example for the functional callbacks.
If you have specific ideas for that, just let me know.

[hreintke]

@devyte @LaborEtArs
Took some detailed debugging/investigation but I think I found the cause.

Running Windows & Chrome browser.
Chrome has an option to "use a prediction service to load pages more quickly", default on.
This results in a "preconnect", the setting up of a new TCP connection before it is actually needed for the next HTTP request.
In the examples that results in the server.available() returning true but no data will be sent (yet).
And the application being stuck in :

  while (client.connected() && !client.available()) {
    delay(1);
  }

In most examples you won't notice as the loop will end and request will be handled when the new HTTP request comes in. But it also results in the loop() only executing once at each HTTP request.

I noticed that you also added mDNS to other examples, those can also be affected by this.

[devyte]

@hreintke then a timeout in those wait loops is a must, and not just in the examples.
Thank you for figuring it out!

[hreintke]

I think best way forward for the mDNS examples is not to use WifiClient but ESP8266Webserver.
Otherwise you get the timeout (or other) mitigation in the examples which detract from the mDNS functionality.
ESP8266Webserver is not affected by the preconnect as it is non blocking until the full HTTP request is available.

If you agree I can do the work on these examples,first updating one in a PR and from there discuss this solution.

[devyte]

If you mean migrate examples that do http manually to use the relevant objects, then please go ahead!
However, we still need small examples that use wificlient/server. But I guess we can figure those out later.

[hreintke]

@LaborEtArs :
Want to do some more testing.
Would be nice to have the TTL lowered to speed that up.
Can you point me to the codelocation to do that hardcoded ?
This because this is in LEADNS.h uint32_t p_u32TTL = 120) { // ignored

[LaborEtArs]

@hreintke: Have a look into ‚LEAmDNS_Priv.h‘, the timeouts for host and service records are defined there. As the timouts are recommended values from the mDNS spec, I have hardcoded them...

[hreintke]

@devyte @LaborEtArs
See #5589 I updated the ServiceMonitor example to use HTTP Server and added some HTML markup to give a nicer web page.
Please let me know if this (type of) update is OK. If so, I will also update the other examples.

@devyte : Beware, as the Legacy mDNS is using async UDP, it does not require a MDNS.update() in the loop(); LEAmDNS, which is now the default, however does. Meaning : The Legacy ->Lea patch is a breaking change !
Checked also on ESP32, there the mDNS implementation uses the IDF, which also does not require an update().

[LaborEtArs]

@hreintke: The mDNSResponder has two response modes. All services, the were supported by the legacy mDNS version (mainly the service response when asked for) should also be supported in passiv/async mode (without the update call in the loop)...
However, when using new functions, like hostname probing, the update call is needed to avoid running user mode code in the ESP wifi ‚thread‘.
So existing code, that uses only the ‚old‘ functionality, should be unaffected by the switch

[hreintke]

@LaborEtArs
When using the LEAmDNS without the update()

• There is no response from the esp on mDNS requests
• Heap memory usage is increasing as UDP request are coming in which are not processed

[LaborEtArs]

If so, something has got the flu...
The separation of user stack code from the ESP wifi stack code has happened later in the development phase; I guess, there have not been much tests without the 'update' then...

To be a bit faster, I would propose to continue this discussion inside the gitter 'mDNS lobby' @devyte created: https://gitter.im/esp8266-mdns/Lobby?utm_source=ios&utm_medium=link&utm_campaign=ios-share-link

[devyte]

Closing via #5653