bblanchon / ArduinoStreamUtils

💪 Power-ups for Arduino streams
MIT License
257 stars 21 forks source link

WriteBufferingStream does not buffer in some instances #12

Closed alfredosola closed 3 years ago

alfredosola commented 3 years ago

Arduino Mega, using IDE 2.0 beta. Ethernet library.

I have a function that parses incoming commands and calls other functions to send responses, passing the EthernetClient as a parameter. For instance (I have edited the names for clarity):

            } else if (strcmp(url, "/Sensors") == 0) {
              answerStatus(client, Temp_Status | Press_Status);

Then answerStatus does this at the very beginning (c is the EthernetClient):

  sendHeaders(c, 200);
  WriteBufferingStream buffer(c, 256);
  StaticJsonDocument<256> doc;

and then a json document is created, buffered and sent out.

Before that, as it can be seen we call a function that sends out the appropriate response headers, which can't be simpler (names edited for clarity):

void sendHeaders(EthernetClient c, int code) {
  WriteBufferingStream buffer{c, 192};
  switch (code) {
    case 200:
      buffer.println("HTTP/1.1 200 OK");
      break;
    case 400:
      buffer.println("HTTP/1.1 400 Bad Request");
      break;
    default:
      buffer.println("HTTP/1.1 200 OK I guess");
      break;
  }
  buffer.println("Content-Type: application/json");
  buffer.println("cache-control: private, max-age=0, no-cache");
  buffer.println("Access-Control-Allow-Origin: *");
  buffer.println("Connection: close");
  buffer.println();
  buffer.flush();
  return;
}

The issue is that, while headers (the function above) are always buffered and I can see them getting out in a single nice packet, the rest of the answer is sometimes not buffered and is seen on the wire with just one byte per packet, instead of a single packet. The key here is sometimes: all the functions that send out answers are structured like above (copypasting straight from the documentation), but some of them get their answers in one nice packet and others get a stream of packets with just one byte.

I'd like to know what could possibly make WriteBufferingStream not buffer its output, or perhaps the W5100 in the Mega deciding to send one byte per packet despite getting a nice big buffer to send out... Can't think of a way to debug this, and I can't see any reason why some functions get their output buffered and tucket in one packet while others don't, using the same technique.

Longest answer is below 256 bytes.

Any clue, pointer, idea or direction appreciated.

bblanchon commented 3 years ago

Hi @alfredosola,

When WriteBufferingStream fails to allocate the buffer (i.e., when malloc() returns NULL), it forwards the calls directly to the upstream Stream without buffering.

It seems that, in your case, malloc(192) succeeds, but malloc(256) fails. The solution is to either reduce the overall memory consumption or reduce the size of the buffer.

By the way, you don't need to create two WriteBufferingStream, you should use the same for headers and payload. Here is a sketch:

void sendHeaders(Print& client, int code) {
  switch (code) {
    case 200:
      client.println(F("HTTP/1.1 200 OK"));
      break;
    case 400:
      client.println(F("HTTP/1.1 400 Bad Request"));
      break;
    default:
      client.println(F("HTTP/1.1 200 OK I guess"));
      break;
  }
  client.println(F("Content-Type: application/json"));
  client.println(F("Cache-control: private, max-age=0, no-cache"));
  client.println(F("Access-Control-Allow-Origin: *"));
  client.println(F("Connection: close"));
  client.println();
}

// ...
WriteBufferingStream buffer(ethernetClient, 128);
sendHeaders(buffer, 200);
serializeJson(doc, buffer);
buffer.flush();

Notice that I moved some strings to Flash memory, which should free 195 bytes of RAM.

Best Regards, Benoit

alfredosola commented 3 years ago

Understood, thanks a lot!

I would suggest that it would be good to mention this behaviour in the documentation. It implies that WriteBufferingStream fails very safely and could help others.