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

Crashes when pixels > 288 #5

Closed
hputzek opened this issue Sep 6, 2019 · 12 comments
Closed

Crashes when pixels > 288 #5

hputzek opened this issue Sep 6, 2019 · 12 comments
Assignees

Comments

@hputzek
Copy link
Collaborator

hputzek commented Sep 6, 2019

When I tested with @martinberlin testing tool, I had the following issue:

When I used 288 pixels to test, it sometimes crashed.
When using 432px, the esp crashed after the first push of the animation button.

To be more exact:
It did not crash, but "disconnected from mqtt" "reconnected to mqtt" for several times and then stopped logging anything. As soon as that happened the first time, the output stopped and did not work anymore.

This was tested with rgbw output, see the #4 issue

@martinberlin
Copy link
Owner

martinberlin commented Sep 7, 2019

Alright, will test this with RGBW but sending it to RGB since I don't have such a large stripe at the moment. But that should be enough to reproduce right?
I really don't know what could launch the disconnected callback from MQTT lib. @IoTPanic I test this but I assign this to you and we can debug together. What I would like to find out is what is the max. large that can be send with the combination of libs we are using to the Stripe.
We also need to take in account that having much more pixels, needs more CPU process time to send it, and may influence how the rest work.
https://github.com/Makuna/NeoPixelBus/wiki/NeoPixelBus-object-API#void-show
"300 pixel strip can take 9ms to send the data. The more pixels, the longer it takes, the less pixels, the quicker it can send them."
I don't know any feasible way to test this, than to have such a large stripe and send the binary push there, that would be the only way to assure that the output is correct. Please let's review the methods, since maybe sending large amounts, we need to use more than only show()

@martinberlin
Copy link
Owner

martinberlin commented Sep 10, 2019

Testing in feature/debug-400px
The hard limit I reach is 489 pixels.

Took 908 micro seconds to consume
Took 816 micro seconds to consume
Took 859 micro seconds to consume

UDP receiving 1472 bytes. One pixel more at 490 pixels it just does not do anything.
We are hitting the maximum transmission unit number in the ESP32. If I see that correctly MTU is max 1500 on all other wifi/ethernet devices.
When I asked Samuel about the max. UDP size, he answered "The max UDP size is 65,535 bytes" but what I wanted to know is the MTU. Now I think we all know it by heart.

What would be the way to receive passing this MTU limit @IoTPanic? Can you debug deeper and share your thoughts?

What I find cool about the ESPAsyncHttp is that the callback function is aware of this and splits the packages in 1,2 Kb approx. calling the function twice and sending len, index, total as a parameter.

Notes: After 490 pix PIXELS::unmarshal -> PIXELS::receive() is never called. It's just like if the UDP receive never happened hitting this limit.
A solution to overcome this limit is to enable Brotli compression again.

RESEARCH Links:
https://esp32.com/viewtopic.php?f=19&t=10656&p=43698&hilit=udp#p43698 -> Most important
espressif/arduino-esp32/issues/2899 -> Similar issue, has already a bug filed.
ESP-IDF Http server One good idea with this project is to recode it for the IDF instead of using Arduino framework. Don't know if it will be better though but I just think it's worth a try.
And I think will make sense also to have a Raspberry Zero client in case you need to control a very large stripe. There I don't think we will hit this limits since you count with much more powerful resources.

@IoTPanic
Copy link
Collaborator

Hm, okay, that makes sense, so a UDP packet size is 65535 bytes, it is the wi-fi/ethernet layers job to split it into MTU sized frames and reassemble on the ESP. There must be a good solution for this, I am on the case.

@IoTPanic
Copy link
Collaborator

IoTPanic commented Sep 10, 2019

Hendrik and I talked, going to work on multi frame s data to transport PIXELs data since the ESP cannot handle multi frame UDP packets.

@martinberlin
Copy link
Owner

martinberlin commented Sep 10, 2019

Would be nice to hear a more descriptive and understandable explanation from both of you. Unless there is something more to add here we can close this. In http side it’s working good and tested for 1000 pixels but of course this recursive function is not meant to handle animations.

@IoTPanic
Copy link
Collaborator

Keep it open until I solve the issue, since the ESP doesn't stitch together wi-fi frames like computers do, it needs a wrapper to break up a single pixels packet to multiple UDP packets. We already have a layer between pixels and UDP, which is s, so I need to both finish s and add multi frame capabilities.

@hputzek
Copy link
Collaborator Author

hputzek commented Sep 11, 2019

@martinberlin why would you close it if the issue is not solved?

The problem is #5 (comment)
Do you need more details? If so please say so if anything is unclear.

@martinberlin
Copy link
Owner

martinberlin commented Sep 11, 2019

@hputzek I though was going to be solved elsewhere since I did not understood the initial explanation. Now I will like to picture out in existant documentation is how the s library will act as a layer between pixels and UDP. But I guess we will have to wait until Samuel is done for that.
At the beginning I though s will act as a transport layer handling the compression.

Now what I want to understand, after this MTU revelation, is how multi-UDP packages of max. 1.4 Kb are going to be joined together and if that will have an effect over the animations.

@IoTPanic
Copy link
Collaborator

It will be interesting to see how it effects aninstions. Things will start slowing down at work in the next week or two, I hope to have s done way before that, but than I will be able to put more time into this project. Also, I was going to merge PIXELS+s to development after I was done, but you seem to be working on the development branch. Should I merge today?

@martinberlin
Copy link
Owner

martinberlin commented Sep 11, 2019

Do it! And let's keep a master branch with Tagged versions.
So when this is done we can test, merge in master, and tag it like 1.0.0
So we can have some kind of version control and know what each new version brings.

I already made a copy of master in https://github.com/martinberlin/udpx/tree/feature/json-v0.1 just to keep a historic version of the first one we made based on Hendrik's first example.

@martinberlin
Copy link
Owner

martinberlin commented Oct 11, 2019

@IoTPanic: The branch to test S is now then Pixels+s right. I was making a feature/s when it was already there!

When I test this one with @hputzek tester using Pixels+s option I get:
Took 63micro seconds to consume
Missing checkvalue

From S readme:

One every fragment zero of a transaction, the first two bytes of the message is the byte length as an unsigned 16-bit integer. After that, a 8-bit XOR checksum follows and the payload than follows.

One thing regarding the 16 bit integer on the first 5 bytes header, I think you are calculating it wrong Hendrik, please look at how I calculated it on the udpx tester. Let's say you send 10 pixels, it should be for Pixels only:

50 00 00 0A 00

On your tester is coming out for same amount:

50 00 00 1e 00 that is 30 on decimal, so if I'm thinking correct, is the wrong number for the amount of Pixels.

I'm calculating that like this:

  let MSB = parseInt(pixLength/256);
  let LSB = pixLength - (MSB*256);
  // header bytes Less significant byte first:
  hByte = [80,0,0,LSB,MSB];

Now if I see it correctly then on Pixels+s protocol you are not sending the header as documented in the library:
First two bytes: byte length as an unsigned 16-bit integer. So for a 10 pixels message we should have
1e 00
For a 900 we should have
E8 03

NOTE: In s docuementation this length should represent the lenght in bytes (That's already done correctly)

And then should come 8-bit XOR checksum. @IoTPanic please explain how to generate this number because it's not explained how to generate it in the Readme. Also is not clear if it should come from the data itself or from where. And before handling this to Hendrik again please check if my explanations are correct.

Please let's use the testing/s branch ( I deleted PIXELS+s to avoid confusion )

@martinberlin
Copy link
Owner

martinberlin commented Dec 9, 2019

Sorry but I'm closing this here since it's an MTU issue that has nothing to do with the Firmware itself and has to be done elsewhere, like with the S implementation of https://github.com/IoTPanic/s#fragmentation the UDP receive.
It does really not help at this point to have an issue here get stale forever and there is another issue for the implementation of S:
#9

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

No branches or pull requests

3 participants