Add keep alive enable configuration to the EspHttpConnection. - #579
Add keep alive enable configuration to the EspHttpConnection.#579gustavowd wants to merge 2 commits into
Conversation
…n the EspHttpConnection function.
| pub use_global_ca_store: bool, | ||
| pub crt_bundle_attach: Option<unsafe extern "C" fn(conf: *mut core::ffi::c_void) -> esp_err_t>, | ||
| pub raw_request_body: bool, | ||
| pub keep_alive_enable: Option<bool>, |
There was a problem hiding this comment.
What does None mean for keep_alive_enabled? If it means same as Some(false), then why are we hiding this flag behind an Option?
There was a problem hiding this comment.
What does
Nonemean forkeep_alive_enabled? If it means same asSome(false), then why are we hiding this flag behind anOption?
In fact, None would cause the http client library to use the default option for keep_alive_enabled, which is currently false. However, if the library changes in the future and starts specifying this option as true by default, None would maintain this condition.
In any case, I have no objection to removing the Option and specifying just a boolean.
There was a problem hiding this comment.
I think it is simpler for the users to just use false.
Moreover, there is no such thing as a predefined default in esp-idf that might change. The C struct is initially just all zeroes, which for that particular parameter corresponds to false.
There was a problem hiding this comment.
Actually, there is a default configuration for keep alive if you set in the config keep_alive_enable to true.
static bool init_common_tcp_transport(esp_http_client_handle_t client, const esp_http_client_config_t *config, esp_transport_handle_t transport)
{
if (config->keep_alive_enable == true) {
client->keep_alive_cfg.keep_alive_enable = true;
client->keep_alive_cfg.keep_alive_idle = (config->keep_alive_idle == 0) ? DEFAULT_KEEP_ALIVE_IDLE : config->keep_alive_idle;
client->keep_alive_cfg.keep_alive_interval = (config->keep_alive_interval == 0) ? DEFAULT_KEEP_ALIVE_INTERVAL : config->keep_alive_interval;
client->keep_alive_cfg.keep_alive_count = (config->keep_alive_count == 0) ? DEFAULT_KEEP_ALIVE_COUNT : config->keep_alive_count;
esp_transport_tcp_set_keep_alive(transport, &client->keep_alive_cfg);
}
Maybe should be interesting to have the keep_alive_enabled just as a boolean and the other configurations as Option<>.
What do you think about this?
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo fmtcommand to ensure that all changed code is formatted correctly.cargo clippycommand to ensure that all changed code passes latest Clippy nightly lints.CHANGELOG.mdin the proper section.Pull Request Details 📖
Simple add an option the enable the keep alive configuration of the http client.
Description
Due to not enable the keep alive, sometimes the connection timed out, bringing several issues to the upper layer functions.
Testing
I tested the new client with REST API requests.