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

Addition of PsychicStreamResponse & update to PsychicFileResponse. #45

Merged
merged 8 commits into from
Jan 7, 2024

Conversation

Chris--A
Copy link
Contributor

@Chris--A Chris--A commented Dec 31, 2023

First, this PR makes a few changes to PsychicFileResponse. There are a few instance variables that are not needed, and a few redundant calls. Resolves #44

The main feature is a new class PsychicStreamResponse to allow using the Print interface to stream raw data.

I stayed away from copying what ESPAsyncWebServer does, especially as their implementation is limited in size (me-no-dev/ESPAsyncWebServer#179 (comment)).

This implementation makes use of the existing ChunkPrinter using a small buffer and supports unlimited size (no content length header is sent).

I was going to create two classes:

  1. PsychicStreamResponse to keep with the typical fashion of passing a Stream to the constructor and using response.send() to initiate the copy.
  2. PsychicResponseStream to use a Print interface (like ESPAsyncWebServer).

However, by adding the copyFrom() function to this candidate, it covers both classes in one.

Basic rundown of features:

  • Write to the response like Serial.
  • Can pass it to functions that accept a Print object.
  • Can pass a Stream to the response to send the entire stream.

Example 1 - Using the Print interface:

server.on("/testPrint", HTTP_GET, [](PsychicRequest *request) {
  PsychicStreamResponse response(request, "text/plain");

  response.beginSend();

  response.print("Free: ");
  response.print((double)ESP.getFreeHeap() / 1024.0, 2);
  response.print("Kb, Min: ");
  response.print((double)ESP.getMinFreeHeap() / 1024.0, 2);
  response.print("Kb, Max: ");
  response.print((double)ESP.getMaxAllocHeap() / 1024.0, 2);
  response.print("Kb, Size: ");
  response.println((double)ESP.getHeapSize() / 1024.0, 2); 

  return response.endSend();
});     

Example 2 - Passing to other functions as a Print object:

Sending a file can be done using PsychicFileResponse, this example is just to highlight the Print base class.

void printFile(fs::FS &fs, const char * path, Print &out){
  File file = fs.open(path);
  if(!file){
      out.println("Failed to open file for reading");
      return;
  }

  while(file.available()){
      out.write(file.read());
  }
  file.close();
}

//...

server.on("/testFile1", HTTP_GET, [](PsychicRequest *request) {
  PsychicStreamResponse response(request, "text/html");

  response.beginSend();
  printFile(SD, "/www/index.html", response);
  return response.endSend();
}); 

Example 3 - Copying from a Stream:

Sending a file can be done using PsychicFileResponse, this example is just to highlight copying any Stream.

server.on("/testFile2", HTTP_GET, [](PsychicRequest *request) {
  PsychicStreamResponse response(request, "text/html");

  response.beginSend();

  File file = SD.open("/www/index.html");
  response.copyFrom(file);
  file.close();

  return response.endSend();
});  

Example 4 - Sending a download.

There are two constructors, the 2 param constructor in the above examples sends the response as inline. The 3 param constructor takes a name which is used as the download file name.

PsychicStreamResponse response(request, "text/plain", "thing.txt");

I have a number of ideas we can discuss if warranted, this was the version I ended up with for the easiest use and safety.

One idea is to add a send() function. It can internally call the beginSend() & endSend(). An extra check will be needed to ensure no printer is setup after endSend() is called.

  return response.send([](Print &print){
    print.println("hello");
  });

Another overload could handle a Stream:

  return response.send(myStream);

However, there is no accommodation for closing a FIle if it is passed in, so another specific overload would be needed for File which can call the Stream overload, then close the file.

Although these ideas might make for some nice looking syntax, and can prevent using beginSend() & endSend(), they do not really add much value. But I'm open to adding these in (already tested) as they allow the user to choose.

@hoeken
Copy link
Owner

hoeken commented Dec 31, 2023

Okay, this is awesome. I'm a bit busy with the holidays now, but I will check this out and merge it shortly.

@Chris--A
Copy link
Contributor Author

Okay, this is awesome. I'm a bit busy with the holidays now, but I will check this out and merge it shortly.

All good, check it over and see if the other additions I mentioned might be worth it. Also, I'm confident the PsychicFileResponse changes are good, but double check it for me.

Enjoy your new year.

@Chris--A Chris--A marked this pull request as draft January 2, 2024 05:03
@Chris--A Chris--A marked this pull request as ready for review January 2, 2024 05:39
@Chris--A
Copy link
Contributor Author

Chris--A commented Jan 4, 2024

I have added another commit which adds an override for the Print function size_t write(const uint8_t *buffer, size_t size).

This makes ChunkPrinter slightly more efficient as any bulk writes previously used the Print::write(const uint8_t *buffer, size_t size) which simply serializes the data to the overridden ChunkPrinter::write(uint8_t c) function. Seeing as we have an internal buffer, serialization can be minimized and copy the data directly. ArduinoJson for example calls this function when doing conversions (int,bool to string).

Also, moving the implementation to a cpp file prevents duplication as it is used between PsychicJsonResponse & PsychicStreamResponse.

@Chris--A
Copy link
Contributor Author

Chris--A commented Jan 4, 2024

I have also updated PsychicStreamResponse to use the changes to ChunkPrinter. (d35b2c3)

if(_buffer)
return ESP_OK;

//Buffer to hold ChunkPrinter and stream buffer. Using placement new will keep us at a single allocation.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's clever!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers, seeing as this response type is split (begin/end) placement new suits this situation well.
The response stream could be derived directly from the ChunkPrinter, however that would require the memory to be allocated during the constructor; and short lived allocations help prevent fragmentation on our constrained hardware target.

This also follows the trend of the other responses by allocating buffers once send() is called.

@Chris--A Chris--A mentioned this pull request Jan 6, 2024
@hoeken hoeken merged commit a9279c6 into hoeken:master Jan 7, 2024
2 checks passed
@hoeken
Copy link
Owner

hoeken commented Jan 7, 2024

Sorry about the delay in merging this. Its been a busy holiday season.

@Chris--A is that Content-Disposition header in the constructor really necessary? Just don't want to have a situation where someone wants to remove it for something we didnt expect and cause other breakages in code that relies on it.

@Chris--A
Copy link
Contributor Author

Chris--A commented Jan 8, 2024

@Chris--A is that Content-Disposition header in the constructor really necessary? Just don't want to have a situation where someone wants to remove it for something we didnt expect and cause other breakages in code that relies on it.

@hoeken, are you referring to the 2 param constructor (inline disposition), or both constructors?

@johnnytolengo
Copy link

Hi, I tried to use the PsychicStreamResponse to serialize a JsonDocument, but it's slow and the header "Content-Type:" reponse was almost always garbage, causing problems on the client side.
I assume that there is some bug.

  PsychicStreamResponse response(request, "application/json");
  response.beginSend();
  serializeJson(json_buf, response);
  return response.endSend();

Sending a text buffer is faster and without any header garbage.
The only problem is that memory has to be allocated manually and can't be free().

char* jsonBufferString = (char*)malloc(1024*sizeof(char));
serializeJson(json_buf, jsonBufferString, 1024);
return request->reply(200, "application/json", jsonBufferString);

J.

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

Successfully merging this pull request may close these issues.

[Question] understanding file response.
4 participants