-
Notifications
You must be signed in to change notification settings - Fork 8k
feat(esp_http_server): Make HTTP(S)_SERVER_EVENT events optional (IDFGH-16707) #17799
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
base: master
Are you sure you want to change the base?
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 20. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
👋 Hello jimmyw, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
|
|
||
| #define ESP_HTTPD_DEF_CTRL_PORT (32768) /*!< HTTP Server control socket port*/ | ||
|
|
||
| #ifdef CONFIG_HTTPD_ENABLE_EVENTS |
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.
| #ifdef CONFIG_HTTPD_ENABLE_EVENTS | |
| #if CONFIG_HTTPD_ENABLE_EVENTS || __DOXYGEN__ |
| int fd; /*!< Session socket file descriptor */ | ||
| int data_len; /*!< Data length */ | ||
| } esp_http_server_event_data; | ||
| #endif |
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.
| #endif | |
| #endif // CONFIG_HTTPD_ENABLE_EVENTS |
| extern "C" { | ||
| #endif | ||
|
|
||
| #ifdef CONFIG_ESP_HTTPS_SERVER_EVENTS |
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.
| #ifdef CONFIG_ESP_HTTPS_SERVER_EVENTS | |
| #if CONFIG_ESP_HTTPS_SERVER_EVENTS || __DOXYGEN__ |
| HTTPS_SERVER_EVENT_DISCONNECTED, /*!< The connection has been disconnected */ | ||
| HTTPS_SERVER_EVENT_STOP, /*!< This event occurs when HTTPS Server is stopped */ | ||
| } esp_https_server_event_id_t; | ||
| #endif |
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.
| #endif | |
| #endif // CONFIG_ESP_HTTPS_SERVER_EVENTS |
| } | ||
| } | ||
| #else | ||
| #define http_dispatch_event_to_event_loop(event_id, event_data, event_data_size) do {} while (0) |
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.
Could we use similar approach (stub out the implementation) in HTTP server component as well?
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.
Fixed, i had to put the stub in the internal header, but i dont see any issue with that.
fe2dccf to
c4ae00e
Compare
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 PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Make it possible to disable http(s) server events. This improves performance of the server, as http server creates events on every signle read or write to the socket.
c4ae00e to
bd85dad
Compare
|
sha=bd85dad2cf2e7436eca2d096b85bd5dcecc50875 |
Description
In my quest to improve heap fragmentation, i noticed that the webserver was a big issue, but not for the reasons you think. A single web page read/write causes several esp_events to be sent, each one causing a malloc on its own and a contex switch to handle tasks. And the worst part of it, is that we never listened on any of these events.
This PR adds a KConfig option to disable events on the webserver, this was a hug improvement for us in 5.4.1
Note
Adds Kconfig switches to enable/disable HTTP(S) server events and gates event declarations/dispatches, with no-ops when disabled; examples updated accordingly.
CONFIG_HTTPD_ENABLE_EVENTSandCONFIG_ESP_HTTPS_SERVER_EVENTSto toggleESP_HTTP_SERVER_EVENT/ESP_HTTPS_SERVER_EVENT.ESP_EVENT_DECLARE_BASE,esp_http_server_event_data, andESP_EVENT_DEFINE_BASEwithCONFIG_HTTPD_ENABLE_EVENTS.esp_http_server_dispatch_event()only when enabled; define as no-op otherwise.httpd_main.c/httpd_txrx.cand maintainhttp_server_stateupdates.CONFIG_ESP_HTTPS_SERVER_EVENTS; define no-op dispatcher when disabled.Written by Cursor Bugbot for commit bd85dad. This will update automatically on new commits. Configure here.