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

Working version of the websocket support #3514

Open
wants to merge 9 commits into
base: dev-esp32
Choose a base branch
from

Conversation

pjsg
Copy link
Member

@pjsg pjsg commented Apr 30, 2022

Fixes #<GITHUB_ISSUE_NUMBER>.

  • This PR is for the dev branch rather than for the release branch.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at 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

@pjsg
Copy link
Member Author

pjsg commented Apr 30, 2022

It still has debugging statement that I will remove once it works well.

@pjsg
Copy link
Member Author

pjsg commented Apr 30, 2022

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.

@jmattsson
Copy link
Member

Any further thoughts on the ws object lifetime? Is the current state good enough?

@pjsg pjsg changed the title Mostly working version of the websocket support Working version of the websocket support Feb 2, 2024
@pjsg
Copy link
Member Author

pjsg commented Feb 2, 2024

After more thought, I've fixed the lifetime problems. Also I got rid of a memory leak. This is good to go.

httpd.start({
webroot = "<static file prefix>",
max_handlers = 20,
auto_index = httpd.INDEX_NONE + httpd.INDEX_ROOT + httpd.INDEX_ALL,
Copy link
Member

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 :)

@jmattsson
Copy link
Member

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,
Copy link
Member

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 🙃

Copy link
Member Author

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!!

Copy link
Member

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants