ofekp/TinyUPnP

M-SEARCH timeout

xan86 opened this issue · 10 comments

xan86 commented

Sometimes timeout can occur before InternetGatewayDevice detection, especially in networks with many UPnP devices

INTERNET_GATEWAY_DEVICE was not found
Timeout expired while waiting for the gateway router to respond to M-SEARCH message
ERROR: Invalid router info, cannot continue

To solve the issue and make communication with the right device more efficient I propose to change the M-SEARCH Search Target (ST) by replacing

strcat_P(body_tmp, PSTR("ST: ssdp:all\r\n\r\n"));

with

strcat_P(body_tmp, PSTR("ST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n"));

as specified in http://upnp.org/specs/arch/UPnP-arch-DeviceArchitecture-v1.1.pdf (PDF pag38 ~ printed pag31)

ofekp commented

🏁
keep them coming please.

I have pushed it to master.

One thing I am not certain about, though, the version I put in the end of the string is 1 and according to the pdf this means the max version supported. So I tried placing 2 there and it did not work.
What happens if we have a version 2 device? Will it not answer back to the broadcast?

What do you suggest here?

xan86 commented

This is an interesting question that I had not considered.
To have the code tolerant to different IGD versions, it is certainly necessary to change the search string defined in the variable INTERNET_GATEWAY_DEVICE
then change:
#define INTERNET_GATEWAY_DEVICE "urn:schemas-upnp-org:device:InternetGatewayDevice:1"
to:
#define INTERNET_GATEWAY_DEVICE "urn:schemas-upnp-org:device:InternetGatewayDevice"
so the library can read every version in the router response.

As for the question (O.T. it would be very complicated if the answer was only "42" - The Hitchhiker's Guide to the Galaxy) I read and searched in the forums and for what I could find the solutions do too many steps. In my opinion the goal is to keep TinyUPnP as light as possible, the esp8266 has a very limited memory compared to a linux server.

So I propose two alternatives, both work for me.

  • First, omit the version in the Search Target:
    strcat_P(body_tmp, PSTR("ST: urn:schemas-upnp-org:device:InternetGatewayDevice:\r\n"));
    it works, I'm afraid it does not comply with upnp protocol so it's worth testing if mine is just a case.

  • The second, also not very elegant but working, is to add both versions to the body of the M-SEARCH:

void TinyUPnP::broadcastMSearch() {
	debugPrint(F("Sending M-SEARCH to ["));
	debugPrint(ipMulti.toString());
	debugPrint(F("] Port ["));
	debugPrint(String(UPNP_SSDP_PORT));
	debugPrintln(F("]"));

	_udpClient.beginPacketMulticast(ipMulti, UPNP_SSDP_PORT, WiFi.localIP());

	strcpy_P(body_tmp, PSTR("M-SEARCH * HTTP/1.1\r\n"));
	strcat_P(body_tmp, PSTR("HOST: 239.255.255.250:1900\r\n"));
	strcat_P(body_tmp, PSTR("MAN: \"ssdp:discover\"\r\n"));
	strcat_P(body_tmp, PSTR("MX: 5\r\n"));
	strcat_P(body_tmp, PSTR("ST: urn:schemas-upnp-org:device:InternetGatewayDevice:1\r\n")); 
	strcpy_P(body_tmp, PSTR("M-SEARCH * HTTP/1.1\r\n"));
	strcat_P(body_tmp, PSTR("HOST: 239.255.255.250:1900\r\n"));
	strcat_P(body_tmp, PSTR("MAN: \"ssdp:discover\"\r\n"));
	strcat_P(body_tmp, PSTR("MX: 5\r\n"));
	strcat_P(body_tmp, PSTR("ST: urn:schemas-upnp-org:device:InternetGatewayDevice:2\r\n")); 

	_udpClient.write(body_tmp);
	_udpClient.endPacket();

	debugPrintln(F("M-SEARCH sent"));
}
xaos3 commented

Hi , i want to make a note about the M-SEARCH timeout,

In my router (ZXHN H268N) the M-SEARCH timeout problem was the normal and very few times the library actually succeded to receive a response from the router. After a lot of testing i found the cause.

I dont know if this is specific to my router or is a general problem , but the library is sending the UDP packet with a call to _udpClient.beginMulticast(WiFi.localIP(), ipMulti, UPNP_SSDP_PORT) ] and the UPNP_SSDP_PORT is defined as 1900.

This is the same port as the destination port. When the ports are the same e.g. source port 1900 and destination port 1900, my router does not send a response ( or the NodeMcu does not proccess the packet ) and the success rate (to succesfully recieve the notify message) is 1 to 20 or 30.

To solve the problem i changed the code to [_udpClient.beginMulticast(WiFi.localIP(), ipMulti, 0)].
In that way the source port is randomly selected and the reponse is always ( in my test at least ) received.

ofekp commented

Hey @xaos3, I think you're right, the method you changed is simply changing the source port, and even though I could not find a justification to the randomization in the documentation, I think this is true and it actually randomizes a port larger than 1024.

Took me a while to find the place where I enforce the destination port since I did not use the constant for the port. Here it is https://github.com/ofekp/TinyUPnP/blob/master/src/TinyUPnP.cpp#L523

This is a great find. Thank you for that.

Would you be willing to create the PR? If you prefer, I can take this.

xaos3 commented

I'm very glad that i was able to help even a little!
I created the account in github only to report this so if you are so kind please create the PR ,
because i am not familiar with the procedure! :D

ofekp commented

I am so glad you did that. I think helping others is what we're here for :)
I bet it was very hard to find and I appreciate the effort you took to pinpoint the issue. This will hopefully continue to help others in the future.

NP, I will make the PR.

Keep it up!

ofekp commented

FYI, relevant PR #49

ofekp commented

I tested on my device and it looks good, @xaos3 can you try my PR too?

xaos3 commented

Yes, i test it on my network , the responce was fast and consistent! 😃

ofekp commented

Perfect! Thank you! Will merge the PR.