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

WiFiUDP sending large udp packet only works once after powerup #1009

Closed
Rodmg opened this issue Nov 12, 2015 · 28 comments
Closed

WiFiUDP sending large udp packet only works once after powerup #1009

Rodmg opened this issue Nov 12, 2015 · 28 comments

Comments

@Rodmg
Copy link
Contributor

Rodmg commented Nov 12, 2015

Hello, I'm trying to send large UDP packets of (for example) 1345 bytes over my local network, but for some reason it only works once after powering on the ESP8266, after that, I cannot send any other packet of that size until the module is restarted.

Functions udp.beginPacket(), udp.write() and udp.endPacket() return always success.

However smaller packets are still working reliably (tried with a 204 byte packet), even after failing to send those large packets.

I'm using "Packet Sender" (https://packetsender.com/) app for receiving those packets in my PC.

I know that there could be problems with the MTU size, however I find it strange that it actually works the first time.

I'm using the Staging version of the libraries in Arduino 1.6.5.

@chaeplin
Copy link
Contributor

Check this :

const size_t pbuf_unit_size = 512;

512 bytes is max.

@igrr
Copy link
Member

igrr commented Nov 13, 2015

512 bytes is the maximum size of single pbuf, but multiple pbufs will be allocated and chained if required.

@chaeplin
Copy link
Contributor

I have tested pbuf_unit_size, changed to 1024, 1500, and 512.
https://gist.github.com/chaeplin/1b346a6c0bf6e2b56b28

Using udp.write, can't send more than pbuf_unit_size ?

@me-no-dev
Copy link
Collaborator

I could not send anything over 512Bytes as well. changing the unit size to 1460 helped

@igrr igrr added this to the 2.1.0 milestone Nov 15, 2015
@igrr igrr self-assigned this Nov 15, 2015
@igrr igrr added the type: bug label Nov 15, 2015
@manitou48
Copy link

I hacked pbuf_unit_size to 1024 and that fixed my problem with sending 1000-byte packets, but the other UDP write problem, I'm having is that I send 10 back-to-back 512-byte packets, but only 5 show up at the linux box (verified on linux box with tcpdump as well as receiving application) ... any thoughts?

@me-no-dev
Copy link
Collaborator

do you yield() after each 512 packet?

@manitou48
Copy link

just added yield() after endPacket(), didn't help . also confirmed that the first 5 packets make it on to the wire. endPacket() returns 1 every time, saying it thinks all is well.

unsurprisingly, If i add delay(1) between each write() all 10 packets go

@me-no-dev
Copy link
Collaborator

@igrr I think the problem with pbuf_unit_size is that when you add a new pbuf to the chain through _reserve, the pbuf added has 0 len so tot_len of the parent pbuf does not change.
Also if there are more pbufs before that they also do not know the new size of the child ones.
A fix to that is to hold the current TX pbuf until we are done adding stuff to it or we have filled it up, after which the pbuf can be cat to the current stack of pbufs and a new one be reserved. That in theory should ensure that all pbufs in a packet chain know what's going on.

@Rodmg
Copy link
Contributor Author

Rodmg commented Dec 4, 2015

@me-no-dev I'm currently debugging the problem, the _reserve seems to be working ok, I'm printing tot_len before it returns and it is correct. (i'm using @chaeplin sketch https://gist.github.com/chaeplin/1b346a6c0bf6e2b56b28 for testing).

I will continue debugging and keep you updated.

@Rodmg
Copy link
Contributor Author

Rodmg commented Dec 4, 2015

send() seems to be OK too, checked sizes of all the pbuf chain and the adjustment.

I'm thinking the problem could be inside udp_sendto() function, but I cannot find the implementation of that function, do you know where it is?

@me-no-dev
Copy link
Collaborator

oh if you are correct the we are screwed with this approach.
udp_sendto() is part of lwip.a library and we do not have the source.
I guess the method can accept only a single pbuf as a packet and does not expect the pbuf to have any other pbufs chained to it? that would be horrible...

@me-no-dev
Copy link
Collaborator

@igrr
Copy link
Member

igrr commented Dec 4, 2015

We do have the source, it was released by espressif both for SDK 1.3 and
1.4. This liblwip.a was built from source, FWIW.

On Fri, Dec 4, 2015, 22:58 Me No Dev notifications@github.com wrote:

found this:

https://android.googlesource.com/kernel/lk/+/1d0df6996457273367e6d9d9d08bf6adb0fc9b65/lib/lwip/src/core/udp.c


Reply to this email directly or view it on GitHub
#1009 (comment).

@me-no-dev
Copy link
Collaborator

where? where? why do we not have that in the repo and build from there?

@igrr
Copy link
Member

igrr commented Dec 4, 2015

http://bbs.espressif.com/viewtopic.php?f=46&t=951

We don't have that inside the repository because building it from source
each time would increase compilation time significantly.

On Fri, Dec 4, 2015, 23:05 Me No Dev notifications@github.com wrote:

where? where? why do we not have that in the repo and build from there?


Reply to this email directly or view it on GitHub
#1009 (comment).

@Rodmg
Copy link
Contributor Author

Rodmg commented Dec 4, 2015

I'm diving into lwIP code, it's full of preprocesor conditional code so it's hard to follow, but from what I see, udp_send() calls ip_output_if() for sending the data, and from a quick read I cannot find any p->next used. could it be that it doesn't expect chained pbufs? do you know of any documentation about it (its not mentioned in the code comments).

A quick fix would be, instead of chaining pbufs in _reserve, making pbuf_unit_size flexible, making it equal to the data size and maybe defining a max size. Do you know about any problem with this approach? NVM, I wasn't considering that you can append more data. the only workaround would be to make pbuf_unit_size bigger.

@Rodmg
Copy link
Contributor Author

Rodmg commented Dec 4, 2015

Ok, more info, I'm using Wireshark and I'm seeing that the packages of more than 512 bytes are being sent, but they have an error:

For a 524 byte package:
[Expert Info (Error/Malformed): Bad length value 524 > IP payload length]
Length: 524 (bogus, payload length 520)

For a 1442 byte package:
Length: 1442 (bogus, payload length 520)

Seems that it is reporting the Length correctly, but is not sending the whole package

btw, now I'm using the Git version of the libraries

igrr added a commit that referenced this issue Dec 6, 2015
Packets up to 1492 bytes may be sent this way.
Also reduced pbuf_unit_size to 128 bytes.
@lovelesh
Copy link

I am facing similar issues.
When flash the code for the first time, the content of the UDP packet is complete. but when I restart the module the content gets trimmed down.
I get only half the packet in wireshark. But on serial terminal I get the complete string being pushed.

String brdcast_msg;
  brdcast_msg += MAC_char;
  brdcast_msg += "|";
  brdcast_msg += ipStr;
  brdcast_msg += "|";
  Udp.write(brdcast_msg.c_str());
  Udp.endPacket();
  Serial.println(brdcast_msg);

@everslick
Copy link
Contributor

For the record: I see high packet loss when sending rapid UDP packets without yield() or delay(). A yield() after each packet does only help slightly, but delay(1) after each Udp.write() works around that bug.

@everslick
Copy link
Contributor

igrr, could you pls add a comment due to why this issue was closed? Is there a upstream bug we could track instead? i couldn't find any. Maybe Rodmg could bring this issue upstream? thx.

@igrr
Copy link
Member

igrr commented Feb 28, 2016

The original bug with incorrect packet size has been fixed here: 3d1fbc6

Regarding your issue with packet loss, please open a separate ticket. My feeling is that MAC layer will drop packets if send queue is full, and since LwIP doesn't make any guarantees regarding UDP delivery, this condition is not reported back to the API level.

@igrr
Copy link
Member

igrr commented Feb 28, 2016

Scratch that, errors from netif->output will bubble up to udp_send, but we aren't forwarding them to WiFiUdp class.

@everslick
Copy link
Contributor

ah, ok. but then it will be an easy fix. should i open an issue?

@igrr
Copy link
Member

igrr commented Feb 28, 2016

Sure, please do. Comments on closed issues tend to be lost and forgotten.

@mkeyno
Copy link

mkeyno commented May 13, 2016

hi folks , sorry to add comments to the closed issue , however I was wondering if it possible to increase UDP package much more that 1500 bytes for fast and reliable delivery, Actually I've plan to send package file in size of 7kb form processing on my laptop to ESP module through out home wifi-router in the time less than 40ms , is the UDP fast and reliable enough to perform such task ? the ESP module should receive the package entirely (in big String length array) and then start to handle it

@drmpf
Copy link

drmpf commented May 13, 2016

You cannot use the words 'fast and reliable' together with UDP!
I have detailed experience with a large scale UDP network for share trading.
UDP Packets get dropped, packets are delayed, packets are re-ordered, packets are repeated.
We overcame these problems by adding our own sequence number to each packet and then built a mini TCP stack at the receiver which buffered, re-ordered and detected missed and duplicate packets. To get the dropped packets, the receiver opened a TCP connection to the server and requested just those packets it had missed.
The coding was complicated and we kept finding 'gotchas' years after it went into production.

If you want 'fast and reliable' use TCP

@mkeyno
Copy link

mkeyno commented May 13, 2016

@drmpf can you elaborate on the

a mini TCP stack at the receiver which buffered, re-ordered and detected missed and duplicate packets
I intend to make Live TCP plain and simple with no header parts ,

@drmpf
Copy link

drmpf commented May 13, 2016

Basically you need to reproduce the TCP recovery stack. Buffer the incoming packets, check the sequence number, re-order them, drop duplicates, detect gaps and request the missing ones. Receive the missing ones, fill-in the gaps, release the packets to your application.
Repeat..
At a minimum you need to add a sequence number header.

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

No branches or pull requests

9 participants