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

ESPAsyncWebServer 2.2.0 + features #3828

Merged

Conversation

willmmiles
Copy link
Collaborator

@willmmiles willmmiles commented Mar 16, 2024

Update to ESPAsyncWebServer 2.2.0 and leverage the new features.

  • (ESP8266) Use the ContentType symbols from the AsyncWebServer instance, reducing PROGMEM duplication
  • (ESP8266) Pass PROGMEM types to server.on(), so the String class will use the correct constructors instead of relying on the exception handler
  • Use explicit allocation for AsyncWebSocket handling, eliminating unneeded object allocations and frees; also fixes deprecation warnings
  • serveLiveLeds: avoid unneeded stack->buffer copy, and fix buffer overflow

These were mostly PROGMEM already, but every little bit helps.
Rather than relying on the exception handler, indicate the
__FlashStringHelper type so the correct String constructor is
used.
Eliminate the extra indirection and allocate shared buffers directly.
No need to filter or look up content type, just pitch it over the wall.
Also fixes .gz'd content processing.
There were three problems here:
- AsyncWebServer is going to copy to a heap buffer anyways, so we might
   as well just pass it one it can use
- The buffer size estimate was wrong -- we need 9 bytes per pixel
   ("RRGGBB",), so the buffer could overflow, and it was not
   considering the extra 2D requirements
- On ESP8266, the stack allocation was overflowing the stack, causing
  corruption and crashes.
@blazoncek
Copy link
Collaborator

@willmmiles is this ready to be merged? I will blindly believe you. 😄

@willmmiles
Copy link
Collaborator Author

I think so, but the whole point of a PR is to get a second pair of eyes in case I made some easy to spot mistake! The changes are pretty shallow, with the intent to make it easier to review - it's probably easier to go through them one commit at a time.

There are a couple places where the code could maybe be cleaner at the expense of the patches being harder to follow. One example is factoring out the now-duplicate names in wled_server. I had planned to send a second PR with cleanup later after you'd checked that the logic was sound.

The last commit (5f2480c) is probably the most critical, as it both fixes an actual buffer overflow when (used/n)>226 and improves performance by avoiding the stack-to-heap copy. Looking at it now, there's still room for improvement there too - it works, but we could shrink the dynamic allocation to fit the actually needed size instead of using a fixed value.

Use the CONTENT_TYPEs exported by AsyncWebServer directly.
Allocate the serialization buffer size at the required length, rather
than always allocating the maximum size.
@blazoncek
Copy link
Collaborator

Code inspection reveals no discrepancies but I have not tested it yet.
In any case everything is logical so if AsyncWebSocketBuffer or DynamicBuffer code have flaws it would need to be addressed there.

@willmmiles
Copy link
Collaborator Author

I went ahead and did those two code quality improvements. Might as well save on the testing effort!

@blazoncek blazoncek merged commit f1987b9 into Aircoookie:0_15 Mar 20, 2024
3 of 17 checks passed
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.

2 participants