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

large file upload for OTA #126

Closed
squonk11 opened this issue Feb 24, 2020 · 27 comments
Closed

large file upload for OTA #126

squonk11 opened this issue Feb 24, 2020 · 27 comments
Labels

Comments

@squonk11
Copy link
Contributor

Hi,
currently I am trying to do a large file upload like in the http_server_test example. With this file I would like to do a ESP32-firmware update using OTA functionality. So, the file to upload has a size of approx 2,3MB. Unfortunately, the file seems to be loaded as whole and not in chunks so that my callback function is called only once for the whole file. For small files this works - but for lager files (approx. >1MB) the ESP32 crashes - probably due to memory limit.
What I would need to have is a functionality which reads chunks from the data stream and passes these chunks to my callback function in order to write to the partition which has to be flashed.
Do you have a hint how I can accomplish that?
Kind regards

@PerMalmberg
Copy link
Owner

Configure the HTTP server chunk size to a suitable size, then write each chunk in the response handler. The linked example doesn't handle this situation, but all you do is check first_partand last_partto know if when to open/close the file.

Please note that #42 needs to be implemented if you want to use the MIMEParser. I'm not sure the suggested solution in that issue is the best one, I imagine it is possible to handle any sized data also when MIME-decoding.

@squonk11
Copy link
Contributor Author

squonk11 commented Feb 24, 2020

thank you for your fast answer.
I am already using a chunk size for the HTTPServerConfig of 4096.
I was using the example callback "upload" and not "blob" - is "blob" the better approach? I thought "blob" is used for data download - from webserver to browser.

@PerMalmberg
Copy link
Owner

Heh, that's what I get for answering too qickly....

Yes, blob sends data to the browser.

They're both instances of the same method though, just for different use-cases.

@squonk11
Copy link
Contributor Author

squonk11 commented Feb 25, 2020

After taking a closer look at the MIME parser I think it should be possible to implement the handling of samaller dada chunks. I could imagine that passing an additional parameter (e.g. max_chunk_size) to the parser could do the job.
Another possibility would be to directly use the data chunks coming from the TCP/IP stack and not defining a certain size. In this case the callback must be designed to be able to handle these chunks. I could try to implement this and provide a PR - before we should agree on how to implement it.
What do you think?

@PerMalmberg
Copy link
Owner

I'd prefer to not have a fixed limit if there is a way to do it. Having the callback handle chunks isn't different from what other parts do so I see no reason to avoid such a design.

@squonk11
Copy link
Contributor Author

in order to keep the existing API of Smooth my suggestion would be to define another overloaded version of MIMEParser::parse with an extra parameter called "chunksize". In this new version of MIMEParser::parse I would first read all data until the end of the headers. Then I can search for the starting boundary and read the Content-Disposition data. After that I could push data chunks to the callback function until the end boundary is coming in.
Do you think this is the right approach?
Do you think it is it necessary to keep the callback parameter const URLEncodedDataCallback& url_data for this version of MIMEParser::parse? What it is needed for in general when I am using enctype='multipart/form-data' in my html?

@PerMalmberg
Copy link
Owner

I'm not against breaking the API if it means an improvement to the ability to handle incoming data.

Your solution will probably work, but the data being passed in the callback should probably be decoded already, otherwise HTTP-stuff leaks into the application layer. I'm not sure this is doable without first reading the entire data. Perhaps caching data to disk is an option to reduce memory usage while receiving? There still has to be some max size anyhow, but it can be much larger than if it is kept in memory only.

Not sure what you're asking re multipart/form-data in your html?

@squonk11
Copy link
Contributor Author

squonk11 commented Feb 27, 2020

having another overloaded version of MIMEParser::parse imo is not a problem because also the callback function needs to be a different one - maybe with a different signature; also because it will be called multiple times for the data chunks.
Sure I need to take care that no HTTP-stuff goes into the file - that would be overkill for OTA... . I also don't know if it will be possible without reading the entire file. I will have a try...
With multipart/form-data I mean the html attribute enctype of the <input> field in the upload form of the website. The mime-parser currently receives two callback functions: according to what I understand one is for multipart/form-data and the other one is for URL encoded forms. What I was asking for is if these need to be handeled by the same parser function.

@PerMalmberg
Copy link
Owner

What I was asking for is if these need to be handeled by the same parser function.

Well...the mimeparser detects which mode it needs to operate in. It doesn't necessarily have to be handled by the same method.

Re. overload or not. Once the framework supports chunked data via the mime-parser, there's really no use for the current implementation. And, considering that the current implementation is vulnerable to large data sizes, it really should be replaced.

@squonk11
Copy link
Contributor Author

One more question: what exacly is the meaning of the variable first_part? Does it mean:

  1. the current content allways contains the full/all http header(s) and nothing more
  2. the current content contains just the first http data chunk and thus might contain heders fully or only partially or fully plus some bytes of data; depending on the size of the data chunk

@PerMalmberg
Copy link
Owner

The headers are always fully read before the callback is called; hence the fixed max header size during setup of the HTTP server.

If first_part is true, then this is the first part of the data.
If last_part is true this is the last part of the data.
If both are true, this is the only data, i.e. it is fits in a single chunk.
If none are true, you're in the middle of the data.

@squonk11
Copy link
Contributor Author

squonk11 commented Mar 1, 2020

yesterday I worked on the MIMEParser. Now I have a first draft working but currently there is still one problem which I have to fix: the resulting file has an additional crlf at the end - but I think it should not be too difficult to fix. The upload speed I achieve is 63kByte/s with a direct connection between ESP32 and my PC - that is not really fast but probably o.k. for my purpose.
The next steps are:

  1. Tests with different file sizes and several different browsers (Firefox, Chrome, Edge are the ones I have available)
  2. Check if I can make it also work with multiple file upload
  3. Do some code fine tuning (c++ifying; maybe here I could need your help...)
  4. Adapt the test programs to the new API
  5. Create PR

Do I miss anything? Do you want to check in advance the changes I made?

@PerMalmberg
Copy link
Owner

Binary and non-binary files.
Text files with both nix and windows line endings.
Either binary comparisons between original file and uploaded, or a hash of the uploaded files if it is running on the device.

64kBytes/s is really slow. Have you determined where the choke point is?

@squonk11
Copy link
Contributor Author

squonk11 commented Mar 3, 2020

I want to try to implement the support for multi file upload first. Then I will do the testing according to what you suggested. But it will take some time because I am working on this during my free time - which is quite rare nowadays. (And additionally I am quite slow in coding...)
Concerning upload-speed: I didn't do any investigation on this but I assume that it is mainly related to the fact that my IRAM is becoming very short - I had to reduce the limit for PSRAM malloc for data chunks down to 256 bytes in order to get my SW running. Thus I think the data is always being pushed through slow PSRAM. The reason for being short in IRAM is that I am using in the same SW bluetooth classic, BLE and wifi. My SW binary currently is 2,3MB.

@PerMalmberg
Copy link
Owner

Take your time, I'm happy to help if/when needed.

256 bytes? yeah, that might be causing slowdowns.

@squonk11
Copy link
Contributor Author

squonk11 commented Mar 4, 2020

Now I am able to upload multiple files in one step. Upload speed is optimized and now is approx. 150kBytes/s. Improvement in upload speed is due to an optimization in searching for boundaries in the incoming data; this shows me once more that very likely slow PSRAM probably is the major issue.
My next step will be some code optimizations - there is one ugly thing in my code: I have multiple return statements in the MIMEParser::parse function. Due to the nature of the functioning principle (parsing data which is continuously coming in) I have to leave the parser for getting more data at different locations of the code. I need to think about this...

@PerMalmberg
Copy link
Owner

PerMalmberg commented Mar 4, 2020

That's a good improvement in speed!

I have to leave the parser for getting more data at different locations of the code

Sounds like a job for a finite state machine.

@squonk11
Copy link
Contributor Author

squonk11 commented Mar 7, 2020

Now I implemented a state machine and there is only one (none) return statement in void MIMEParser::parse. Upload speed for a 2,1MB file is 160kByte/s.
I am still having one problem: when uploading larger files I get error messages like:

E (118489) task_wdt: Task watchdog got triggered. The following tasks did not reset the watchdog in time:
E (118489) task_wdt:  - IDLE0 (CPU 0)
E (118489) task_wdt: Tasks currently running:
E (118489) task_wdt: CPU 0: main
E (118489) task_wdt: CPU 1: SocketDispatche

probably due to the watchdog not being serviced. Do you have an idea how I can circumvent this?
And I have a 2nd question: when uploading a 2,1MB file the total upload time is approx. 13s. 3 seconds of this is consumed by the loop:

        for (std::size_t i = 0; i < length; ++i)
        {
            data.emplace_back(p[i]);
        }

using emplace_back() for each single byte seems to be a quite expensive approach. Isn't there a cheaper approach available for moving uint8_t[] data into a std::vector<uint8_t>; e.g. like memcpy()? I didn't find any. Do you have an idea?

@PerMalmberg
Copy link
Owner

Unless you have tasks that run on on the idle-tick, the watchdog issues are nothing to worry about. At least that is what I've been told and I've never seen any issues re. this. The only way to prevent them is for other tasks to either kick the WD, or to yield processing time.

Try std::copy(), it is properly optimized and I'd guess it'll do better than any implementation we can come up with. I expect it to be must better than the for loop.

FYI: A vector is guaranteed to be contiguous memory so it can be used as an array, but you'll have to call reserve() at the appropriate time.

@squonk11
Copy link
Contributor Author

Now I tested the code under Windows and Linux with text and binary files using Firefox, Chrome and Edge. In all cases the uploads were without error.
Next I tested uploading 1000 files in one shot (the icons of the http_server_test) - unfortunately this did not work. After approx. 100 files the ESP32 crashes. But the first 100 files are stored properly on the SD card. I guess this might be a memory allocation issue...? Is this test important?
Then I also tested the upload speed without writing the file to SD card. In this case the upload speed is approx 300kBytes/s. The trick with std::copy() did not work. I could not figure out why - the number of copied bytes is always zero.
Next I would do some code fine tuning and then generate the PR. Do you agree? Or do you have other suggestions for testing/improvements?

@PerMalmberg
Copy link
Owner

Do 1000 files work under Linux? 10000? A device crashing is never a good thing, what's does it print to the console?

0 bytes with std::copy? Sounds like you had the wrong arguments, I doubt it is broken... paste/link the code snippet?

Feel free to do the PR whenever and I'll have look and do some testing too.

@squonk11
Copy link
Contributor Author

Some update:

  1. If I use my changes on Smooth in combination with the original http_server_test, I can upload 917 files before the ESP32 crashes. So, I could imagine it is somehow related to the total available memory (because it is much less in my application).

  2. I think my problem is that I am leaking memory: for each upload I am losing PSRAM and IRAM.
    The amount of IRAM I am leaking seems to be independent from the number of files I upload or the size of the file(s). I am losing approx 3kByte.
    The amount of PSRAM I am leaking seems to depend upon the size and number of files. It is several 10kBytes.

I need to analyze this a bit deeper. I will also check if the memory leak was not present with the previous version of the MIMEParser. These analysis will take some time because I am quite busy these days (also due to Corona). I will also provide a PR as soon as possible so that you also can check.

@PerMalmberg
Copy link
Owner

A memory leak should be easy to find if you run it under Linux with Valgrind. It'll even tell you where the leak is.

If you do a PR I can have a look too when time permits.

@squonk11
Copy link
Contributor Author

I now finished the work of porting the modifications for the MIMEParser into the last version of Smooth and I also adapted the http_server_test. Basically it works, but unfortunately I encounter the problem, that after the upload the final response only appears in the browser when I upload very small files (<4kB). This is due the fact, that last_part is not being updated at the end of the upload in HTTPServer<ServerType>::handle via a call to handler->update_call_params.
If I put an additional call to handler->update_call_params before handler->request (in HTTPServer.h), the final response to the browser is being sent multiple times.
I need to dig a bit deeper into this issue. I hope I will be able to do this during the next days - hoping to not stress your patience too much...

@PerMalmberg
Copy link
Owner

Ok, let me know if you need something from me.

@squonk11
Copy link
Contributor Author

I think this can be closed now.

@microElabDevelop
Copy link

microElabDevelop commented Oct 17, 2022

hi . im facing the same issue . i try to perfome ota inn esp32 , my size is greater then 1 Mb . i successfully download file with http server but i write succesfully i got error. it ry to change partition size and also try dafult 16Mb partition , but not resovle my issue.
and i m using 16mb esp32 chip.
ota_error1
esp32_16mb_custom_partition

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