MDNS : Also increase # of sends when a send fails by hreintke · Pull Request #8641 · esp8266/Arduino

hreintke · 2022-07-17T11:49:53Z

Related to #8308

This solves the root cause of the infinite sending of host probes when STA is not connected.

hreintke · 2022-07-17T14:02:03Z

@d-a-v : Cannot find the output on what is wrong with the style, only the fact that it is failing.

mcspr · 2022-07-17T14:36:33Z

Check the log of the style / formatting job; either by clicking the check line down below or Checks tab above.

hreintke · 2022-07-17T22:07:24Z

That is what I did but the info is :

+++ b/libraries/ESP[826](https://github.com/esp8266/Arduino/runs/7377371467?check_suite_focus=true#step:4:827)6mDNS/src/LEAmDNS_Control.cpp
@@ -1395,18 +1395,18 @@ namespace MDNSImplementation
             {
                 if ((bResult = _sendHostProbe()))
                 {
-                    DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                        PSTR("[MDNSResponder] _updateProbeStatus: Did sent host probe to all links \n\n")););
+                    DEBUG_EX_INFO(
+                        DEBUG_OUTPUT.printf_P(PSTR("[MDNSResponder] _updateProbeStatus: Did sent "
+                                                   "host probe to all links \n\n")););
                 }

```


                 else
                 {
-                    DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                        PSTR("[MDNSResponder] _updateProbeStatus: Did not sent host probe to all links\n\n")););
-
+                    DEBUG_EX_INFO(
+                        DEBUG_OUTPUT.printf_P(PSTR("[MDNSResponder] _updateProbeStatus: Did not "
+                                                   "sent host probe to all links\n\n")););
                 }
                 m_HostProbeInformation.m_Timeout.reset(MDNS_PROBE_DELAY);
                 ++m_HostProbeInformation.m_u8SentCount;
-
             }
             else  // Probing finished
             {
@@ -1430,7 +1430,7 @@ namespace MDNSImplementation
                  && (m_HostProbeInformation.m_Timeout.expired()))
         {
             _announce(true, false);  // Don't announce services here
-            
+
             ++m_HostProbeInformation.m_u8SentCount;
             if (MDNS_ANNOUNCE_COUNT > m_HostProbeInformation.m_u8SentCount)
@@ -1446,7 +1446,6 @@ namespace MDNSImplementation
                 DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
                     PSTR("[MDNSResponder] _updateProbeStatus: Done host announcing.\n\n")););
             }
-            
         }
         //
@@ -1471,16 +1470,17 @@ namespace MDNSImplementation
                     if ((bResult = _sendServiceProbe(*pService)))
                     {
                         DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                            PSTR("[MDNSResponder] _updateProbeStatus: Did sent service probe to all links "
+                            PSTR("[MDNSResponder] _updateProbeStatus: Did sent service probe to "
+                                 "all links "
                                  "(%u)\n\n"),
                             (pService->m_ProbeInformation.m_u8SentCount + 1)););
-
                     }
                     else
                     {
                         DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                            PSTR("[MDNSResponder] _updateProbeStatus: Did not sent service probe to all links"
-                                "(%u)\n\n"),
+                            PSTR("[MDNSResponder] _updateProbeStatus: Did not sent service probe "
+                                 "to all links"
+                                 "(%u)\n\n"),
                             (pService->m_ProbeInformation.m_u8SentCount + 1)););
                     }
                     pService->m_ProbeInformation.m_Timeout.reset(MDNS_PROBE_DELAY);
@@ -1511,7 +1511,7 @@ namespace MDNSImplementation
                      && (pService->m_ProbeInformation.m_Timeout.expired()))
             {
                 _announceService(*pService);  // Announce service
-                
+
                 ++pService->m_ProbeInformation.m_u8SentCount;
                 if (MDNS_ANNOUNCE_COUNT > pService->m_ProbeInformation.m_u8SentCount)
@@ -1519,7 +1519,7 @@ namespace MDNSImplementation
                     pService->m_ProbeInformation.m_Timeout.reset(MDNS_ANNOUNCE_DELAY);
                     DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
                         PSTR("[MDNSResponder] _updateProbeStatus: Announcing service %s.%s.%s "
-                                "(%d)\n\n"),
+                             "(%d)\n\n"),
                         (pService->m_pcName ?: m_pcHostname), pService->m_pcService,
                         pService->m_pcProtocol, pService->m_ProbeInformation.m_u8SentCount););
                 }
@@ -1528,11 +1528,10 @@ namespace MDNSImplementation
                     pService->m_ProbeInformation.m_Timeout.resetToNeverExpires();
                     DEBUG_EX_INFO(
                         DEBUG_OUTPUT.printf_P(PSTR("[MDNSResponder] _updateProbeStatus: Done "
-                                                    "service announcing for %s.%s.%s\n\n"),
-                                                (pService->m_pcName ?: m_pcHostname),
-                                                pService->m_pcService, pService->m_pcProtocol););
+                                                   "service announcing for %s.%s.%s\n\n"),
+                                              (pService->m_pcName ?: m_pcHostname),
+                                              pService->m_pcService, pService->m_pcProtocol););
                 }
-                
             }
         }
         DEBUG_EX_ERR(if (!bResult) {
Error: Process completed with exit code 1.+++ b/libraries/ESP[826](https://github.com/esp8266/Arduino/runs/7377371467?check_suite_focus=true#step:4:827)6mDNS/src/LEAmDNS_Control.cpp
@@ -1395,18 +1395,18 @@ namespace MDNSImplementation
             {
                 if ((bResult = _sendHostProbe()))
                 {
-                    DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                        PSTR("[MDNSResponder] _updateProbeStatus: Did sent host probe to all links \n\n")););
+                    DEBUG_EX_INFO(
+                        DEBUG_OUTPUT.printf_P(PSTR("[MDNSResponder] _updateProbeStatus: Did sent "
+                                                   "host probe to all links \n\n")););
                 }
                 else
                 {
-                    DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                        PSTR("[MDNSResponder] _updateProbeStatus: Did not sent host probe to all links\n\n")););
-
+                    DEBUG_EX_INFO(
+                        DEBUG_OUTPUT.printf_P(PSTR("[MDNSResponder] _updateProbeStatus: Did not "
+                                                   "sent host probe to all links\n\n")););
                 }
                 m_HostProbeInformation.m_Timeout.reset(MDNS_PROBE_DELAY);
                 ++m_HostProbeInformation.m_u8SentCount;
-
             }
             else  // Probing finished
             {
@@ -1430,7 +1430,7 @@ namespace MDNSImplementation
                  && (m_HostProbeInformation.m_Timeout.expired()))
         {
             _announce(true, false);  // Don't announce services here
-            
+
             ++m_HostProbeInformation.m_u8SentCount;
             if (MDNS_ANNOUNCE_COUNT > m_HostProbeInformation.m_u8SentCount)
@@ -1446,7 +1446,6 @@ namespace MDNSImplementation
                 DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
                     PSTR("[MDNSResponder] _updateProbeStatus: Done host announcing.\n\n")););
             }
-            
         }
         //
@@ -1471,16 +1470,17 @@ namespace MDNSImplementation
                     if ((bResult = _sendServiceProbe(*pService)))
                     {
                         DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                            PSTR("[MDNSResponder] _updateProbeStatus: Did sent service probe to all links "
+                            PSTR("[MDNSResponder] _updateProbeStatus: Did sent service probe to "
+                                 "all links "
                                  "(%u)\n\n"),
                             (pService->m_ProbeInformation.m_u8SentCount + 1)););
-
                     }
                     else
                     {
                         DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
-                            PSTR("[MDNSResponder] _updateProbeStatus: Did not sent service probe to all links"
-                                "(%u)\n\n"),
+                            PSTR("[MDNSResponder] _updateProbeStatus: Did not sent service probe "
+                                 "to all links"
+                                 "(%u)\n\n"),
                             (pService->m_ProbeInformation.m_u8SentCount + 1)););
                     }
                     pService->m_ProbeInformation.m_Timeout.reset(MDNS_PROBE_DELAY);
@@ -1511,7 +1511,7 @@ namespace MDNSImplementation
                      && (pService->m_ProbeInformation.m_Timeout.expired()))
             {
                 _announceService(*pService);  // Announce service
-                
+
                 ++pService->m_ProbeInformation.m_u8SentCount;
                 if (MDNS_ANNOUNCE_COUNT > pService->m_ProbeInformation.m_u8SentCount)
@@ -1519,7 +1519,7 @@ namespace MDNSImplementation
                     pService->m_ProbeInformation.m_Timeout.reset(MDNS_ANNOUNCE_DELAY);
                     DEBUG_EX_INFO(DEBUG_OUTPUT.printf_P(
                         PSTR("[MDNSResponder] _updateProbeStatus: Announcing service %s.%s.%s "
-                                "(%d)\n\n"),
+                             "(%d)\n\n"),
                         (pService->m_pcName ?: m_pcHostname), pService->m_pcService,
                         pService->m_pcProtocol, pService->m_ProbeInformation.m_u8SentCount););
                 }
@@ -1528,11 +1528,10 @@ namespace MDNSImplementation
                     pService->m_ProbeInformation.m_Timeout.resetToNeverExpires();
                     DEBUG_EX_INFO(
                         DEBUG_OUTPUT.printf_P(PSTR("[MDNSResponder] _updateProbeStatus: Done "
-                                                    "service announcing for %s.%s.%s\n\n"),
-                                                (pService->m_pcName ?: m_pcHostname),
-                                                pService->m_pcService, pService->m_pcProtocol););
+                                                   "service announcing for %s.%s.%s\n\n"),
+                                              (pService->m_pcName ?: m_pcHostname),
+                                              pService->m_pcService, pService->m_pcProtocol););
                 }
-                
             }
         }
         DEBUG_EX_ERR(if (!bResult) {
Error: Process completed with exit code 1.

I don't know how to proceed with that

mcspr · 2022-07-18T09:11:15Z

Style job expects that the code is clang-format'ed; either apply the diff with patch / git-patch, or bash tools/restyle.sh <path-to-the-lea-cpp>

On topic, these announces are simply not essential? Couldn't we introduce a longer wait time when it is failing? (like exponential back-off, or simply add a fixed number)

hreintke · 2022-07-18T10:18:21Z

The probes/announces are there to :
1/ Verify that the hostname/service is not in use on another system.
2/ Let the network know which hostname/service is available on this system.

On timing :
This should be a process which is quick as the result is whether a base config as hostname is correct to use.
That is why I am not in favor of extending probe timing.

On Probe result.
The current implementation verifies this on the links that are "UP". If on all of these the probing doesn't show a conflicting host/service it considers it OK and does a "Probe OK" callback. In principle, the STA link is not in "UP state" -> I do not know (yet ?) why the "not connected STA" is considered UP. To make it working as defined, I decided to ignore the error completely.

One might discuss whether a "not UP link" or a link with errors should callback a Probe failed.
But that changes current MDNS behavior and is a breaking change, which I think should not be done in a 3.0 -> 3.1 release.

mcspr · 2022-07-18T14:01:09Z

OK. Lea approach is still a bit convoluted, but at least we get rid of this edge case.

Regarding UP, I think we need to look at glue layer. I just noticed these bits

where we initially call set addr (with whatever values) https://github.com/d-a-v/esp82xx-nonos-linklayer/blob/06164fbd55770b0dbb4f52a092f4b924e84f9b8a/glue-esp/lwip-esp.c#L588
internals do a check for non-zero IP once https://github.com/d-a-v/esp82xx-nonos-linklayer/blob/06164fbd55770b0dbb4f52a092f4b924e84f9b8a/glue-lwip/lwip-git.c#L419
but forget it in the next piece when calling esp2glue_netif_set_up1down0 https://github.com/d-a-v/esp82xx-nonos-linklayer/blob/06164fbd55770b0dbb4f52a092f4b924e84f9b8a/glue-lwip/lwip-git.c#L508

@d-a-v might have some explanation as to why it is that way
(or SDK somehow needs this behaviour b/c it is expecting the wrong thing)

d-a-v · 2022-07-18T16:13:43Z

@hreintke

In principle, the STA link is not in "UP state"
-> I do not know (yet ?) why the "not connected STA" is considered UP. To make it working as defined, I decided to ignore the error completely.

I assume this is #8308 (comment)

@mcspr I can't seem to remember whether you've been working on LwipIntf::stateUpCB recently
(otherwise this is a bug that is external to this PR and should be fixed independently)

but forget it in the next piece when calling esp2glue_netif_set_up1down0

Counterpart is there => lwIP => lwIP

mcspr · 2022-07-18T17:55:18Z

@hreintke

In principle, the STA link is not in "UP state"
-> I do not know (yet ?) why the "not connected STA" is considered UP. To make it working as defined, I decided to ignore the error completely.

I assume this is #8308 (comment)

@mcspr I can't seem to remember whether you've been working on LwipIntf::stateUpCB recently (otherwise this is a bug that is external to this PR and should be fixed independently)

but forget it in the next piece when calling esp2glue_netif_set_up1down0

Counterpart is there => lwIP => lwIP

Right, but I meant this with debugging enabled

0:<buffered:
1:lwESP: lwip_init
2:GLUE: lwip_init
3:GLUE: netif_sta_init
4:GLUE: netif_ap_init
5::dereffub>
6:lwESP: netif_add
7:lwESP: netif_add(ip:0) -> lwESP: netif_set_addr(0->0)
8:lwESP: check netif @0x3ffefcbc (sta@0x3ffefcbc ap@0x00000000)
9:lwESP: NEW netif
10:lwESP: esp-@0x3ffefcbc idx=0 STA name=ew0 state=0x3ffefa0c 2c:f4:32:4a:29:14|BROADCAST|LINK_UP|ETHARP|IGMP ip=0.0.0.0 mask=0.0.0.0 gw=0.0.0.0
11:lwESP: netif_add
12:lwESP: netif_add(ip:0) -> lwESP: netif_set_addr(0->0)
13:lwESP: check netif @0x3ffefcbc (sta@0x3ffefcbc ap@0x00000000)
14:lwESP: netif (0): already added
15:lwESP: esp-@0x3ffefcbc idx=0 STA name=ew0 state=0x3ffefa0c 2c:f4:32:4a:29:14 ip=0.0.0.0 mask=0.0.0.0 gw=0.0.0.0
16:GLUE: netif updated:
17:GLUE: netif 0: set up
18:GLUE: lwip-@0x3ffeeb58 idx=0(station) mtu=1500 state=0x00000000 2c:f4:32:4a:29:14|UP|BROADCAST|LINK_UP|ETHARP|IGMP ip=0.0.0.0 mask=0.0.0.0 gw=0.0.0.0 <<< UP???

d-a-v · 2022-07-18T21:24:50Z

18:GLUE: lwip-@0x3ffeeb58 idx=0(station) mtu=1500 state=0x00000000 2c:f4:32:4a:29:14|UP|BROADCAST|LINK_UP|ETHARP|IGMP ip=0.0.0.0 mask=0.0.0.0 gw=0.0.0.0 <<< UP???

There is the flag UP and LINK_UP
UP is when an IP address is assigned (following dhcp answer, or static setup)
LINK_UP is when link layer is up (WiFi associated, Ethernet powered up ...)
For example, one can get LINK_UP without UP when a dhcp request is made and before an answer lands back

It is true that ip-not-set and UP state are not compatible, and you points me here and there.

Maybe the the second link

		//netif_set_up(netif); // unwanted call to netif_sta_status_callback()
		netif->flags |= NETIF_FLAG_UP;

should be something like

		if (ip_addr_isany(&netif->ip_addr)))
		    netif_set_down(netif); // or test first whether it is already down
 		else
		    netif_set_up(netif); // or test first whether it is already up

(tests in comments are there because one has to be careful with the number of times netif callbacks are used)

d-a-v

The mdns clock example runs fine with both AP and STA enabled
approving, noting that

the netif up state case in comments is an external issue
I may have not tested all cases (ap-sta -> ap -> off -> sta -> ap_sta ...)

hreintke · 2022-07-18T21:46:37Z

I have been looking at the same code this afternoon 😃 .
@mcspr @d-a-v : How can I enable the lwip debug output ?

d-a-v · 2022-07-18T21:48:53Z

How can I enable the lwip debug output ?

There for the glue (@mcspr's debug log) and 5 lines below for lwIP.

edit: lwip2 must be recompiled

hreintke · 2022-07-18T21:49:32Z

@d-a-v : The check should be whether the clock example is running OK when mode = AP_STA and STA is not connected.

d-a-v · 2022-07-18T21:54:10Z

The check should be whether the clock example is running OK when mode = AP_STA and STA is not connected.

Maybe you should update the example to enable it with both AP and STA.
I used this patch and checked with 'Service Browser' android app while on LAN (STA) or connected to the AP.

diff --git a/libraries/ESP8266mDNS/examples/LEAmDNS/mDNS_Clock/mDNS_Clock.ino b/libraries/ESP8266mDNS/examples/LEAmDNS/mDNS_Clock/mDNS_Clock.ino
index 6234e9f10..33c7a7f17 100644
--- a/libraries/ESP8266mDNS/examples/LEAmDNS/mDNS_Clock/mDNS_Clock.ino
+++ b/libraries/ESP8266mDNS/examples/LEAmDNS/mDNS_Clock/mDNS_Clock.ino
@@ -210,7 +210,8 @@ void setup(void) {
   Serial.begin(115200);
 
   // Connect to WiFi network
-  WiFi.mode(WIFI_STA);
+  WiFi.mode(WIFI_AP_STA);
+  WiFi.softAP("ESPap", "thereisnospoon");
   WiFi.begin(ssid, password);
   Serial.println("");

(weird AP credentials are borrowed from another example)

hreintke · 2022-07-18T22:27:33Z

@d-a-v

edit: lwip2 must be recompiled

That will be a challenge for me.
Any chance that you can create a liblwip2-1460.a for me with debugging enabled ?

This solves the root cause of the infinite sending of host probes when STA is not connected.

Also increase # of sends when a send fails

cbffd5e

style update

e7a7c0c

d-a-v added this to the 3.1 milestone Jul 18, 2022

d-a-v added the component: MDNS label Jul 18, 2022

d-a-v approved these changes Jul 18, 2022

View reviewed changes

Merge branch 'master' into MDNS_Issue

3591cb0

mcspr merged commit e2a36ed into esp8266:master Jul 27, 2022

d-a-v mentioned this pull request Sep 13, 2022

More for fixing MDNS #8675

Closed

hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024

MDNS : Also increase # of sends when a send fails (esp8266#8641)

b491e46

This solves the root cause of the infinite sending of host probes when STA is not connected.

Conversation

hreintke commented Jul 17, 2022

Uh oh!

hreintke commented Jul 17, 2022

Uh oh!

mcspr commented Jul 17, 2022 via email

Uh oh!

hreintke commented Jul 17, 2022

Uh oh!

mcspr commented Jul 18, 2022

Uh oh!

hreintke commented Jul 18, 2022

Uh oh!

mcspr commented Jul 18, 2022

Uh oh!

d-a-v commented Jul 18, 2022

Uh oh!

mcspr commented Jul 18, 2022

Uh oh!

d-a-v commented Jul 18, 2022

Uh oh!

d-a-v left a comment

Choose a reason for hiding this comment

Uh oh!

hreintke commented Jul 18, 2022

Uh oh!

d-a-v commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hreintke commented Jul 18, 2022

Uh oh!

d-a-v commented Jul 18, 2022

Uh oh!

hreintke commented Jul 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

d-a-v commented Jul 18, 2022 •

edited

Loading