WifiUDP::stopAll() doesn't not stop classes that implement udpcontext directly. E.g. MDNS causes httpupdater to fail

[sticilface]

Using latest git, and most others...

I get the error

[httpUpdate] Update.writeStream failed! (ERROR[6]: Stream Read Timeout)

When using the HTTP update lib.

In sort it seems that lots of UDP traffic causes it to timeout...

:urch 306, 289
:urch 289, 306
:urch 306, 246
:urch 246, 246
:urch 246, 246
:urch 246, 246
:urch 246, 246
:urch 246, 246
:urch 246, 286
:urch 286, 306
:urch 306, 306
:urch 306, 306
:urch 306, 286
:urch 286, 286
:urch 286, 286
:urch 286, 286
:urch 286, 306
:urch 306, 289
:urch 289, 246
:urch 246, 246
ERROR[6]: Stream Read Timeout

Further investigation shows that this only occurs when using the Arduino IDE and is down to the MDNS being active during the update. I added in the IP and port for each packet, showing they come from all the other devices on the network running mdns.

:urch 291, 231 ip[192.168.1.218:5353]
Reading answers RX: REQ, ID:0, Q:0, A:4, NS:0, ADD:0
Not expecting any answers right now, returning
:urch 231, 231 ip[192.168.1.218:5353]
Reading answers RX: REQ, ID:0, Q:0, A:4, NS:0, ADD:0
Not expecting any answers right now, returning
LmacRxBlk:1
:urch 231, 286 ip[192.168.1.241:5353]
Reading answers RX: REQ, ID:0, Q:0, A:4, NS:0, ADD:0
Not expecting any answers right now, returning
:urch 286, 231 ip[192.168.1.218:5353]
Reading answers RX: REQ, ID:0, Q:0, A:4, NS:0, ADD:0
Not expecting any answers right now, returning
:urch 231, 306 ip[192.168.1.219:5353]
Reading answers RX: REQ, ID:0, Q:0, A:4, NS:0, ADD:0
Not expecting any answers right now, returning
:urch 306, 306 ip[192.168.1.219:5353]
Reading answers RX: REQ, ID:0, Q:0, A:4, NS:0, ADD:0
Not expecting any answers right now, returning
:urch 306, 289 ip[192.168.1.186:5353]
Reading answers RX: REQ, ID:0, Q:0, A:1, NS:0, ADD:3
Not expecting any answers right now, returning
:urch 289, 289 ip[192.168.1.186:5353]
Reading answers RX: REQ, ID:0, Q:0, A:1, NS:0, ADD:3
Not expecting any answers right now, returning
:urch 289, 261 ip[192.168.1.208:5353]

So is this normal behaviour for MDNS? Should replies be broadcast to everyone? I do not really know anything about MDNS. However, this is currently breaking uploads at least via HTTP...

[sticilface]

So I think i've solved it. MDNS is active during uploads, and causing problems. Adding a stop function to MDNS, and calling it before updating has fixed it, and made downloads a lot quicker:)

void MDNSResponder::stop() {
    if (_conn) {
    _conn->unref();
    _conn = nullptr; 
  }
}

[httpUpdate] runUpdate flash...
:pd 4, 4380, 0
:rpi 1460, 4
sleep disable
[begin] roundedSize:       0x00069000 (430080)
[begin] updateEndAddress:  0x00100000 (1048576)
[begin] currentSketchSize: 0x0006C000 (442368)
[begin] _startAddress:     0x00097000 (618496)
[begin] _currentAddress:   0x00097000 (618496)
[begin] _size:             0x00068FC0 (430016)
:c 1, 1460, 4380
:c 1, 1460, 2920
:rch 1460, 1460
:rch 2920, 1460
:rch 4380, 1460
...
:rch 4380, 776
:rcl
:abort
:c 1, 1460, 5156
:c 1, 1460, 3696
:c 1, 1460, 2236
:c0 1, 776
Staged: address:0x00097000, size:0x00068FC0
[httpUpdate] Update ok

no UDP traffic...

@igg what do u think?

[sticilface]

It would appear MDNS in general is active, and with more advanced logging I see all requests across the network for MDNS clients appearing. Adding the stop() function, stops this, and allows updates to progress unhindered. Also exposing the restart function allows it to be restated:)

[sticilface]

Took a while to get to the bottom of this. But basically libs that wish to roll async need to implement udpcontext directly and not use wifiudp, as there is no rx callback.

1. I would like to propose adding an onrx callback to wifiudp to allow async usage.
2. (and probably harder) would Be to add pause() and resume() functions so you can pause all wifiudp and tcp traffic whilst doing some task and then resume them.
3. maybe implement this into udpcontext so instances that do not use wifiudp can be paused.

As it stands the MDNS lib does cause my updates to stutter and sometimes fail.

Any thoughts?

[me-no-dev]

I am not sure that pausing in the upper layers will help. connections need to be freed so the lower layer does not actually process most of the packets

[sticilface]

i guess by pause and resume i mean allow stop to be called which calls https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/include/UdpContext.h#L108, and then have a method that allows restarting of the service. the udpcontext has a disconnect and listen function that should do this.

I guess the issue is quite complex. I was thinking of interfacing at the lower layer anyway, at least in udpcontext, as many people seem to roll their own implementation using that instead of WiFiUDP. I am one of those people, preferring to use udpcontext with a callback rather than polling using isavailable() from wifiUDP. My thoughts are that we could allow a callback to be used in wifiUDP, then modify at least MDNS to use that, the WiFiUDP.stopAll() would work with that. plus implement some way of restarting the stopped services. If implementing this alone in WiFiUDP the stop could call unref() and delete the udpcontext, then restart just creates another.

alternatively hook in at a lower level and chain together the udpcontext instances rather than the wifiudp. This might get more tricky as higher level classes like WiFiUDP will expect them to still exist, so it will have to be done carefully.

e.g.

class UdpContext : public SList<WiFiUDP> {
...
};

I'm happy to implement these changes I'm looking for some opinions as to what would be the best. keeping it all in wifiUDP would be best, but there are several libraries that don't use it. e.g. OTAupdater, MDNS, (some of mine).

I was also wondering if it could all be put together as part of the WiFi class object.

so you could have which contains both wifiUDP and wifiClient instances.

WiFi.stopAllClients()  -->  calls unref() 
WiFi.resumeAllClients()  -->  called restart(); 

[sticilface]

Ok i've made a pull request with some ideas i've mentioned.

#3305

1. add rx callback functions to WiFiUDP
2. Switch MDNS to use WiFiUDP -> now it gets stopped on the start of an update... and MDNS traffic does not interfere with it.

[sticilface]

@igrr any thoughts on this pull request? I've been using it for a while with no issues, and uploads are certainly much much smoother (especially if you have a lot of ESPs generating MDNS traffic) on your network.

[d-a-v]

#3305 was closed, should we close this one too ?

[devyte]

Why was #3305 closed and the branch deleted?
Given the comment about "smoother", I'd say study the proposal there before fully dismissing this.

[sticilface]

Looks like it was me that closed and deleted it... this might have just been an accident as i cleaned up my wild github account. tbh I've not looked into this for a very long time, my job has been a lot of covid ICU this past year and i've had professional exams and a new baby, so there has been very little time to spend on projects.

My vague memories of this were that the jumpy updates were due to UDP requesting being fired off especially if the arduino IDE was open. I'm no longer using the arduinoIDE as i use platform for everything.

I can't however commit time atm to look into the merge conflicts with #3305, i still have #7696 to sort out!