Webserver Buffered Response

[romanseidl]

I have written a very simple response buffer that automatically flushes the buffer to the server response when there is no more space in the buffer. So you can write / print to the buffer endlessly and it never gets overrun.

https://gist.github.com/romanseidl/4d6ac8c247a4768417037b1a00b3bb9f

Is this of any interest to the project?

[devyte]

Definitely useful. Tbh, I thought this is how the current code worked. Guess my responses so far have been small enough, so lucky until now :)

A comment: considering that the buffer size is fixed at construction time, why not use an int template parameter for the size rather than a constructor runtime argument? That way, the buffer will live on the stack for the duration of the object, and possibly reduce heap fragmentation.

Usage would be: BufferedResponse<1024> bresponse(blah...);

On Jun 21, 2017 6:58 PM, "Roman Seidl" notifications@github.com wrote:

I have written a very simple response buffer that automatically flushes the buffer to the server response when there is no more space in the buffer. So you can write / print to the buffer endlessly and it never gets overrun.

https://gist.github.com/romanseidl/4d6ac8c247a4768417037b1a00b3bb9f

Is this of any interest to the project or anyone else?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BvnU53or94QtfiRuCIq1BvULaZl9ks5sGaAigaJpZM4OBnkl .

[romanseidl]

Ok, thats probably a good Idea. I would like to see a default, and I could not use template default params with a platformio build.

Should this work?

[devyte]

Yes, should be something like (off the toprandom off my head):

template class BufferedResponse { ... };

Usage should be something like: BufferedResponse buff1024(blah...); //1024 default BufferedResponse<2048> buff2048(bleh...); //2048

On Jun 21, 2017 7:45 PM, "Roman Seidl" notifications@github.com wrote:

Ok, thats probably a good Idea. I would like to see a default, is there such a thing as a default template?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310235592, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BlZ-M6RmFbmz80pRII6HxhjMnzCPks5sGasWgaJpZM4OBnkl .

[devyte]

In method write() you are assuming that len <= buffer size. If that is not the case, you have to chunk the input in a loop and flush each iterarion Also, I suggest a template method to cover cover both String and std::string (both have ::c_str() and ::length() methods in common).

On Jun 21, 2017 8:36 PM, "Develo Deveyes" deveyes@gmail.com wrote:

Yes, should be something like (off the toprandom off my head):

template class BufferedResponse { ... };

Usage should be something like: BufferedResponse buff1024(blah...); //1024 default BufferedResponse<2048> buff2048(bleh...); //2048

On Jun 21, 2017 7:45 PM, "Roman Seidl" notifications@github.com wrote:

Ok, thats probably a good Idea. I would like to see a default, is there such a thing as a default template?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310235592, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BlZ-M6RmFbmz80pRII6HxhjMnzCPks5sGasWgaJpZM4OBnkl .

[devyte]

Cosmetic: the .h define is reversed (_RESPONSEBUFFERH -> _BUFFEREDRESPONSEH)

Sorry for typos and short responses, on phone while stuck in traffic :P

On Jun 21, 2017 8:43 PM, "Develo Deveyes" deveyes@gmail.com wrote:

In method write() you are assuming that len <= buffer size. If that is not the case, you have to chunk the input in a loop and flush each iterarion Also, I suggest a template method to cover cover both String and std::string (both have ::c_str() and ::length() methods in common).

On Jun 21, 2017 8:36 PM, "Develo Deveyes" deveyes@gmail.com wrote:

Yes, should be something like (off the toprandom off my head):

template class BufferedResponse { ... };

Usage should be something like: BufferedResponse buff1024(blah...); //1024 default BufferedResponse<2048> buff2048(bleh...); //2048

On Jun 21, 2017 7:45 PM, "Roman Seidl" notifications@github.com wrote:

Ok, thats probably a good Idea. I would like to see a default, is there such a thing as a default template?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310235592, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BlZ-M6RmFbmz80pRII6HxhjMnzCPks5sGasWgaJpZM4OBnkl .

[romanseidl]

template <int buffsize = 1024> the .h define is reversed (_RESPONSEBUFFER_H_ -> _BUFFEREDRESPONSE_H_) I changed those accordingly, thanks for the hints. Still the currrent code seems to crash.

In method write() you are assuming that len <= buffer size. If that is not the case, you have to chunk the input in a loop and flush each iterarion Yes, thats right. At the moment it just crashes if you send in a larger chunk.

Also, I suggest a template method to cover cover both String and std::string (both have ::c_str() and ::length() methods in common). What do you mean by that? Implement those methods? One could actually consider extending or implementing String right from the start as it is needed by the server?

[devyte]

class BufferedResponse {

... //the following can be called either with String or std::string as argument template void write(const stringType &str) { write(str.c_str(), str.length()); }

On Jun 21, 2017 9:14 PM, "Roman Seidl" notifications@github.com wrote:

template the .h define is reversed (_RESPONSEBUFFERH -> _BUFFEREDRESPONSEH) I changed those accordingly, thanks for the hints.

In method write() you are assuming that len <= buffer size. If that is not the case, you have to chunk the input in a loop and flush each iterarion Yes, thats right. At the moment it just crashes if you send in a larger chunk.

Also, I suggest a template method to cover cover both String and std::string (both have ::c_str() and ::length() methods in common). What do you mean by that? Implement those methods? One could actually consider extending or implementing String right from the start as it is needed by the server?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310247942, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BuJANvc3FPH-Ze-9QJ3SiyF2rC7Gks5sGb_XgaJpZM4OBnkl .

[romanseidl]

What do you need the write String method for?

Why would one want to template that? Wouldn't it be smarter to just include 2 functions covering the types? I don't know if it happens, but I would hope for gcc to not link unused methods.

What I am not too sure is if the call to server.sendContent() which calls for a String is so optimal.

[devyte]

Trust the gcc compiler/linker, I'd be very surprised if user dead code made it to the binary.

The template method is better than two different specific methods.

On Jun 22, 2017 3:16 AM, "Roman Seidl" notifications@github.com wrote:

What do you need the write String method for?

Why would one want to template that? Wouldn't it be smarter to just include 2 functions covering the types? I don't know if it happens, but I would hope for gcc to not link unused methods.

What I am not too sure is if the call to server.sendContent() which calls for a String is so optimal.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310296753, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6Botu7I-HtJxBLsQwecn7hkMve8F8ks5sGhTagaJpZM4OBnkl .

[romanseidl]

The template method is better than two different specific methods.

Why so? In case somebody uses both then there is two distinct classes that AFAIK eat up space. Also wouldn't one have to specify which string class to use? Or am I getting something wrong?

I am having another problem. With the now templated version and thus a buffer as an instance of: uint8_t buf[buffer_size]; the copy operation memcpy((void *) (&buf[pos]), (const void *) data, len); seems to crash the machine.

I don't know why, somehow I thought this should be the same as using a char* right from the start. It crashes after pos is at 70, so it's far away from the buffer end. Any ideas?

[romanseidl]

I found out why it may crash: If i define the buffer to be larger than 2048 it seems it is simply not allocated?

Is there such severe stack size limits on the esp? Then maybe using a heap pointer might again be useful?

[romanseidl]

I am having even larger problems with stack space in a more filled up live environment. I now went back to using a buffer pointer in the heap.

[romanseidl]

I now migrated back to the heap buffer version and it works quite solid.

it also considers when the written length is larger than the buffer and splits it into several chunks that are sent to the server.

s. https://gist.github.com/romanseidl/4d6ac8c247a4768417037b1a00b3bb9f

It is not very fast as the whole SyncServer is not too fast. But at least it works well. Would be interesting if somebody else would test it.

[devyte]

@romanseidl my understanding (I'm no expert on the ESP internals) was that the stack wasn't limited, and it would grow down, while the heap allocations would grow up. A google search didn't turn up much info on this. However, I did find a comment in an issue that mentioned in passing that the stack is 4KB in size, which surprised me. If true, then a 2KB object would, of course, cause an overflow and crash.

Given that, I guess it's safest for now to keep the heap-based version, despite the possible extra fragmentation.

[devyte]

@romanseidl said:

Why so? In case somebody uses both then there is two distinct classes that AFAIK eat up space. Also wouldn't one have to specify which string class to use? Or am I getting something wrong?

That's not how template methods work. When you call a template method with specifc types, then the compiler will create a specific method with the types filled in to cover that use case. The compiler will create as many versions as there are different use cases with different combinations of types. With the template method I propose, if one user calls with String, then only the String-version will be compiled and linked in for him, while for a different user who uses str::string will get the other version. If any user makes calls to both versions, then the compiler will build one for String and one for std::string.

[romanseidl]

Thanks for digging into the stack/heap issue.

However, I did find a comment in an issue that mentioned in passing that the stack is 4KB in size, which surprised me. If true, then a 2KB object would, of course, cause an overflow and crash.

This would perfectly explain why it crashed with 4kb and even with 2kb in an environment where the stack is already filled up to certain extent.

That's not how template methods work. When you call a template method with specifc types, then the compiler will create a specific method with the types filled in to cover that use case.

So the only difference in the result should be that you only have to write one method but in the end it all comes down to the same compiled result as i i had written two methods that are exactly the same but for the method signature?

[devyte]

Correct. That's why the template method is better, even in this tiny case.

On Jun 22, 2017 7:21 PM, "Roman Seidl" notifications@github.com wrote:

Thanks for digging into the stack/heap issue.

However, I did find a comment in an issue that mentioned in passing that the stack is 4KB in size, which surprised me. If true, then a 2KB object would, of course, cause an overflow and crash.

This would perfectly explain why it crashed with 4kb and even with 2kb in an environment where the stack is already filled up to certain extent.

That's not how template methods work. When you call a template method with specifc types, then the compiler will create a specific method with the types filled in to cover that use case.

So the only difference in the result should be that you only have to write one method but in the end it all comes down to the same compiled result as i i had written two methods that are exactly the same but for the method signature?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310528898, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BiRzb0ax6dW7X7zJehuGUNqPJjiiks5sGvb6gaJpZM4OBnkl .

[romanseidl]

I added the template method, I hope it is as you thought it should be.

Concerning your "yield" issue: I don't think this is an issue. I might run a test to verify, my testing did not produce any errors unless your program is not too blocking between buffer flushs. I tested sending up to 1MB and it worked fine. I also tested using a buffer of 10 byte and writing 25 byte chars into it and it worked fine too.

sendContent delegates up to the ClientContext and there the data gets sent to the client. In doing so there is a call to yield, at least to esp_yield() [1]. This does not seem to be a surprise as I think that all network activity on the esp will need some kind of yield().

Should I leave the code in the Gist, should I start a repository / module or should I somehow integrate it into the esp core and produce a pull request?

[1] https://github.com/esp8266/Arduino/blob/d6f1f0d5d77508ed2b3bb6051a6fba3d69ab98ca/libraries/ESP8266WiFi/src/include/ClientContext.h#L370

[devyte]

My only remaining question is whether the default buffer size should be a multiple of the MTU, or something along those lines, to reduce packet fragmentation, but that would just be an optimization.

@igrr I'll defer to you on how to proceed :)

On Jun 24, 2017 10:59 AM, "Roman Seidl" notifications@github.com wrote:

I added the template method, I hope it is as you thought it should be.

Concerning your "yield" issue: I don't think this is an issue. I might run a test to verify, my testing did not produce any errors unless your program is not too blocking between buffer flushs. I tested sending up to 1MB and it worked fine. I also tested using a buffer of 10 byte and writing 25 byte chars into it and it worked fine too.

sendContent delegates up to the ClientContext and there the data gets sent to the client. In doing so there is a call to yield, at least to esp_yield() [1]. This does not seem to be a surprise as I think that all network activity on the esp will need some kind of yield().

Should I leave the code in the Gist, should I start a repository / module or should I somehow integrate it into the esp core and produce a pull request?

[1] https://github.com/esp8266/Arduino/blob/d6f1f0d5d77508ed2b3bb6051a6fba 3d69ab98ca/libraries/ESP8266WiFi/src/include/ClientContext.h#L370

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310843403, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6Bgc-Qb2yG9Fij9TLyD02qsS5bYrIks5sHSRBgaJpZM4OBnkl .

[romanseidl]

Fragmentation should probably only occur if the web request produces artefacts on the heap that are persistent. Or does the server produce such when sending the content?

To avoid that one could use a static global buffer, something for those afraid of dynamic memory management. With the sync server this is possible.

[devyte]

No, I meant packet fragmentation this time. Say your buffer is 2k, and your MTU is the standard 1470 or whatever. You then flush when your buffer is full, you sendContent for 2K, which the IP stack fragments to one packet of 1470 and another of 578. This happens again and again every flush.

On Jun 24, 2017 1:13 PM, "Roman Seidl" notifications@github.com wrote:

Fragmentation should probably only occur if the web request produces artefacts on the heap that are persistent. Or does the server produce such when sending the content?

To avoid that one could use a static global buffer, something for those afraid of dynamic memory management. With the sync server this is possible.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310851466, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BgwBPoMJkxBmFQnJD_dHC47IazPgks5sHUOmgaJpZM4OBnkl .

[romanseidl]

No, I meant packet fragmentation this time. Say your buffer is 2k, and your MTU is the standard 1470 or whatever. You then flush when your buffer is full, you sendContent for 2K, which the IP stack fragments to one packet of 1470 and another of 578. This happens again and again every flush.

My mind was still set on heap fragmentation.

Is it 1500? https://github.com/esp8266/Arduino/blob/4897e0006b5b0123a2fa31f67b14a3fff65ce561/tools/sdk/lwip/include/lwip/opt.h#L569 https://github.com/esp8266/Arduino/blob/4897e0006b5b0123a2fa31f67b14a3fff65ce561/tools/sdk/lwip/include/lwip/netif.h#L186

But the packet is probably also including a header, so it should be a bit smaller than that I suppose.

[igrr]

Folks, the code looks good, but I can't understand why it is needed. In the latest version, sendContent passes all the data to WiFiClient::write, which passes it into ClientContext, where the data is fragmented, if necessary, and passed to LwIP. By default, Nagle algorithm is enabled on all PCBs, so if WiFiClient::write is called with less than TCP_MSS of data, the chunk will not be sent immediately, and will be buffered by LwIP. If more data comes, LwIP will combine it with the buffered data and then send it. If the amount of data passed to WiFiClient::write is greater than what LwIP can take (2*TCP_MSS usually), then data will be fragmented and passed to LwIP in chunks which it can accept [1]. Please help me understand what does BufferedResponse add to this story.

[1] ClientContext.h

[romanseidl]

So you would directly write to sendContent, even with small pieces of Data? Wouldn't that create quite some call overhead? At least it would be very memory efficient.

Still then one need two have something implementing the "Print" interface, as this is what I want to have.

I want to have something I can write on and not care about the way it is sent and chunked etc. This works very nicely with the BufferedResponse. One could e.g. make the response implement it, then one could do "response.printf()" etc, only that the header would have to be sent out once I call that for the first time.

[devyte]

Ok, so my original thought was correct after all about sends being already buffered. I took a quick look, but I didn't go into ClientContext, so... /shrug

On Jun 25, 2017 4:34 AM, "Ivan Grokhotkov" notifications@github.com wrote:

Folks, the code looks good, but I can't understand why it is needed. In the latest version, sendContent passes all the data to WiFiClient::write, which passes it into ClientContext, where the data is fragmented, if necessary, and passed to LwIP. By default, Nagle algorithm is enabled on all PCBs, so if WiFiClient::write is called with less than TCP_MSS of data, the chunk will not be sent immediately, and will be buffered by LwIP. If more data comes, LwIP will combine it the buffered data and then send it. If the amount of data passed to WiFiClient::write is greater than what LwIP can take (2*TCP_MSS usually), then data will be fragmented and passed to LwIP in chunk which it can accept. Please help me understand what does BufferedResponse add to this story.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/esp8266/Arduino/issues/3366#issuecomment-310890110, or mute the thread https://github.com/notifications/unsubscribe-auth/AQC6BmBDkpFVnEGQq-Nco0TL8X6dmklEks5sHht8gaJpZM4OBnkl .

[romanseidl]

I didn't really expect that and thus didn't look to deep either, sorry.

So would it be best to write a class that immediately prints to sendContent?