forked from me-no-dev/ESPAsyncWebServer
-
Notifications
You must be signed in to change notification settings - Fork 17
expose WebSocket makeBuffer() method to be publically available #8
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
Open
vortigont
wants to merge
9
commits into
yubox-node-org:yuboxfixes-0xFEEDC0DE64-cleanup
Choose a base branch
from
vortigont:yubxmod
base: yuboxfixes-0xFEEDC0DE64-cleanup
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+98
−106
Open
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
2d7fdbb
expose WebSocket makeBuffer() method to be piblically available
vortigont 68b21d2
set real "Last-Modified" header based on file's LastWrite time
vortigont ebde6d7
set real "Last-Modified" header based on file's LastWrite time
vortigont 481e092
set etag header's value to lastmod timestamp if available, otherwise …
vortigont d814400
rewrite IMS timestap generation to use std::strftime
vortigont 645d109
Merge branch 'ims-tstamps' into yubxmod
vortigont 5d76a35
update library.json - set AsyncTCPSock as dependency
vortigont 26b5769
switch dep to AsyncTCP-esphome
vortigont 7ff17c0
set Etag header value to last modified timestamp
vortigont File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I'm curious to understand how this library compare to the ESPHome maintained fork (https://github.com/esphome/AsyncTCP) and what it brings compare to AsyncTCP ?
I am using a fork of
yubox-node-org/ESPAsyncWebServer
also, but with the ESPHome maintained AsyncTCP.Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who knows... Do not see much activity in this fork recently.
How is it works
yubox-node-org/ESPAsyncWebServer
withesphome/AsyncTCP
?Is it more stable? My impression was that
yubox-node-org/ESPAsyncWebServer
must be used with it's own version ofAsyncTCPSock
.Have you tried
esphome/ESPAsyncWebServer
? They have added some fixes there, but yubox fork is almost a full rework.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I am maintaining a more up to date version of the
yubox-node-org
fork, which is deployed in PlatformIO registry and Arduino registry.The ESPHome folks decided to fork the original repo, which is IMO wrong because the yubox-node-org introduces a LOT of fixes regarding concurrency issues when using tasks especially on dual core systems. So they miss all that, especially on the WebSocket part.
I know a lot of projects depending on the
yubox-node-org
fork instead of the ESPHome one or original one for this reason. It adds a lot of stability and prevents corrupted heap causes by concurrent access to queues in the lib.The version I maintain is using
esphome/AsyncTCP
because the ESPHome team does a great job IMO to maintain it. They fixed a couple of issues and introduced IPv6.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll take a look into your fork also! If you like I can rebase this PR onto your fork.
I also have some improvements for async webserver that I can share.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! An adapted version of this change would definitely be interesting because sadly the api change in the buffer is not compatible with the original API. But making is compatible is really harder I think.
For the moment I have to workaround that by using feature detection like here:
https://github.com/ayushsharma82/ESP-DASH/pull/195/files#diff-b22c24b3d761e54f8997b5313d563e3b2b58217a99f7e9e95beb68ad19ba6e13R343-R351
The problem is that the original API returns a pointer to a buffer, which requires it to be responsible of its destruction:
https://github.com/esphome/ESPAsyncWebServer/blob/master/src/AsyncWebSocket.h#L333-L334
The change using yubox fork with a shared ptr allows a buffer created by the caller to still be referenced until all the clients have used it. His change is well explained in his commit here:
me-no-dev@4963ce9
I don't know how to make the original API compatible with the use of shared ptr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@proddy : did you find out ? is it possible that this increase is caused by the difference values for the queue sizes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be honest I can't remember! I'm pretty sure it's in AsyncTCP. I'm still using the ESPHome fork with IPv6 support with CONFIG_ASYNC_TCP_STACK_SIZE set to 5120 which is enough for ESP32 (2.3k) and ESP32S3 (3.5k)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Because I ran some tests this morning:
If you look from 10h35 and after: this is typically what I like to see. There is no big allocations / deallocation on heap, which avoids fragmentation. This is quite stable.
Spikes are app reloads (ESP-DASH PRo + WebSerial Pro).
This app also has mqtt publications each 5 seconds, dashboard refresh each 500ms, and log streamed to web console (websocket) in debug mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am running with my forks:
My AsyncTCP fork is based on ESPHome, but supports Arduino 3, Ipv6 for Arduino 3 also, and fixes some issus in the ESPHome implementation of Ipv6 (I've PR-ed them if I remember). It also include a workaround of a bug introduced in Arduino 3 in the IpAddress implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. I'll do some benchmarking too. Having local copies of the libraries is a nightmare to maintain so it'll be good if I switch to public libraries.