-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Working version of the websocket support #3514
base: dev-esp32
Are you sure you want to change the base?
Conversation
It still has debugging statement that I will remove once it works well. |
I'm having problems in deciding what the lifetime of the ws object should be. I suspect that it isn't right at the moment. It deserves more thought. |
Any further thoughts on the |
After more thought, I've fixed the lifetime problems. Also I got rid of a memory leak. This is good to go. |
docs/modules/httpd.md
Outdated
httpd.start({ | ||
webroot = "<static file prefix>", | ||
max_handlers = 20, | ||
auto_index = httpd.INDEX_NONE + httpd.INDEX_ROOT + httpd.INDEX_ALL, |
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 think you want ||
rather than +
here; they're meant to be a choice, not bits to be set together :)
This turned out to be more effort than I'd expected, so I'm glad you tackled it instead of me :D I haven't tried it out or done an in-depth code review, but if you reckon it's ready to go, I'm happy to go with that. I left one comment about the docs that you might want to check before merging, but other than that, it'd be good to get this functionality in! |
@@ -76,7 +76,7 @@ configured. | |||
httpd.start({ | |||
webroot = "<static file prefix>", | |||
max_handlers = 20, | |||
auto_index = httpd.INDEX_NONE || httpd.INDEX_ROOT || httpd.INDEX_ALL, | |||
auto_index = httpd.INDEX_ALL, |
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.
This removes the mention of the other index modes, I can't say I'm in favour of that 🙃
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.
It turns out that these are actually enums and shouldn't be or'ed together. Worse, the example uses || which is the logical or. Thus, this expression returns the first non-zero value!!
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, logical OR, because you need to choose one of them. What's the correct syntax for that in this case, if not that? (genuine question, not a snarky comment)
Fixes #<GITHUB_ISSUE_NUMBER>.
dev
branch rather than for therelease
branch.docs/*
.This is the websocket enhancement to the httpd implementation. Unfortunately, it doesn't actually work due to a bug in the IDF, but there is a fix, and I'm hopeful that it will get merged soon. espressif/esp-idf#8596