-
Notifications
You must be signed in to change notification settings - Fork 138
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
Add memory_type Support for Flexible Memory Allocation in WebSocket Client to permit other memory type caps to be used (IDFGH-13127) #586
Conversation
+1 |
@filzek Thank you for your contribution. |
changes made to the original file as requested! |
I will provide corrections for the SDK version to build the SDK. Some memory types are strictly dependent on the SDK version, such as MALLOC_CAP_TCM, which is only available after version 5.2. I will check all kinds of memory for each SDK version and provide #ifdef directives for those types. |
@filzek Thank you for the update. However, there are still breaking changes in the CI due to the modifications. https://github.com/espressif/esp-protocols/actions/runs/9798927333/job/27229816060?pr=586 |
We're currently testing the option to choose the core on which tasks will run, in addition to specifying the memory region. We believe this could enhance speed and consistency more effectively than just allocating the memory region, as the majority of memory consumption is in buffer processing rather than executable code. Therefore, our priority has been to select the core. However, we can provide an option that allows both core selection and memory region allocation if a specific flag is set. @gabsuren and @euripedesrocha, here are my thoughts:
However, there is an issue with SDK versions prior to 5.1, which do not support Any ideas or feedback on this? |
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.
Memory updated to SDK versions.
if (!client->core) { Now it is working correct using the memory area, but we need to evaluate to tell that some internal heap memory will be used as the task function couldnt be placed in the external memory. |
@@ -1,5 +1,21 @@ | |||
# Changelog | |||
|
|||
## [1.x.x](https://github.com/espressif/esp-protocols/commits/websocket-v1.x.x) |
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.
You don't need to modify, in fact you should not, the Changelog.
As stated in the contribution guide, you only need to follow the conventional commits, and we take care of the release notes when releasing a new version of the component.
@@ -137,6 +153,8 @@ struct esp_websocket_client { | |||
int payload_offset; | |||
esp_transport_keep_alive_t keep_alive_cfg; | |||
struct ifreq *if_name; | |||
int memory_type; |
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'm not against making the selection in runtime but, in other components, we make this choice in compile time through Kconfig.
Do you have a use case to have it set in runtime?
ESP_WS_CLIENT_MEM_CHECK(TAG, client, return NULL); | ||
|
||
if (config->core){ |
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 would prefer it to be introduced in a separated PR. Since we'll have to take into consideration portability among different IDF versions given the proposal of usage xTaskCreatePinnedToCoreWithCaps
. Could you please move this to a second PR?
@filzek thank you for your contribution and diligent answer to our questions. I left a few comments that we have to address prior to merging. |
@@ -127,6 +127,8 @@ typedef struct { | |||
int network_timeout_ms; /*!< Abort network operation if it is not completed after this value, in milliseconds (defaults to 10s) */ | |||
size_t ping_interval_sec; /*!< Websocket ping interval, defaults to 10 seconds if not set */ | |||
struct ifreq *if_name; /*!< The name of interface for data to go through. Use the default interface without setting */ | |||
int memory_type; /*!< The name of memory region type to be used in the heap_caps_malloc / heap_caps_calloc */ | |||
esp_transport_handle_t ext_transport; /*!< External WebSocket tcp_transport handle to the client; or if null, the client will create its own transport handle. */ |
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 seems to be some kind of leftover.
Enable Usage of External or Specific Memory Types in WebSocket
Feature: Added a memory_type directive to esp_websocket_client_config_t to allow usage of various memory types in heap_caps_malloc and heap_caps_calloc.
Default Handling: If memory_type is not defined or is invalid, MALLOC_CAP_DEFAULT is used.
HTTP Auth Update: Updated http_auth_basic function to use the specified memory region.
All relevant functions have been updated to utilize the new memory_type configuration.
Now can set any memory CAPS region such as MALLOC_CAP_SPIRAM and others.