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

M-SEARCH timeout #35

Closed
xan86 opened this issue May 21, 2019 · 10 comments
Closed

M-SEARCH timeout #35

xan86 opened this issue May 21, 2019 · 10 comments
Labels

Comments

@xan86
Copy link

xan86 commented May 21, 2019

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
Copy link
Owner

ofekp commented May 27, 2019

🏁
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
Copy link
Author

xan86 commented May 28, 2019

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
Copy link

xaos3 commented Jan 1, 2020

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
Copy link
Owner

ofekp commented Jan 1, 2020

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
Copy link

xaos3 commented Jan 1, 2020

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
Copy link
Owner

ofekp commented Jan 1, 2020

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
Copy link
Owner

ofekp commented Jan 19, 2020

FYI, relevant PR #49

@ofekp
Copy link
Owner

ofekp commented Jan 19, 2020

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

@xaos3
Copy link

xaos3 commented Jan 20, 2020

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

@ofekp
Copy link
Owner

ofekp commented Jan 21, 2020

Perfect! Thank you! Will merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants