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

Update WiFiGeneric.cpp #7044

Merged
merged 3 commits into from
Jul 28, 2022
Merged

Conversation

zhangyanjiaoesp
Copy link
Contributor

@zhangyanjiaoesp zhangyanjiaoesp commented Jul 27, 2022

Description of Change

Enlarge the cache_tx_buf num

Related links

Related PR in Arduino Lib: #5791

close espressif/esp-idf#8992

@CLAassistant
Copy link

CLAassistant commented Jul 27, 2022

CLA assistant check
All committers have signed the CLA.

@@ -664,15 +664,6 @@ bool wifiLowLevelInit(bool persistent){

wifi_init_config_t cfg = WIFI_INIT_CONFIG_DEFAULT();

if(!WiFiGenericClass::useStaticBuffers()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was added to reduce the memory footprint of the WiFi driver as it increased considerably from IDF v3 to IDF v4.

Refer to #5791 for this. @SuGlider fyi...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... Default now is WiFi Dynamic Allocation in order to reduce HEAP consumption.
If the user wants to go with Static Allocation, the application shall use WiFiGenericClass::useStaticBuffers(true); before setting the WiFi Mode with WiFiGenericClass::mode()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SuGlider Although the customers can call WiFiGenericClass::useStaticBuffers(true) to solve the problem, but I think we also need to change the value cfg.cache_tx_buf_num = 1 to at least 4, this will ensure the customers can send several packets at the same time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, Agree. Please commit this change. This will cost 5K RAM in the heap. It is worth it.

@SuGlider
Copy link
Collaborator

SuGlider commented Jul 27, 2022

@zhangyanjiaoesp -
Please try using the API that sets Static WiFi allocation:

void setup() {
  Serial.begin(115200);
  WiFi.useStaticBuffers(true);
  WiFi.mode(WIFI_STA);
  
  // rest of the necessary code ...
}

WiFi.useStaticBuffers(true); will force Arduino to use default sdkconfig WiFi setup:

CONFIG_ESP32_WIFI_STATIC_RX_BUFFER_NUM=8
CONFIG_ESP32_WIFI_DYNAMIC_RX_BUFFER_NUM=32
CONFIG_ESP32_WIFI_STATIC_TX_BUFFER=y
CONFIG_ESP32_WIFI_TX_BUFFER_TYPE=0
CONFIG_ESP32_WIFI_STATIC_TX_BUFFER_NUM=8
CONFIG_ESP32_WIFI_CACHE_TX_BUFFER_NUM=16

@SuGlider SuGlider self-requested a review July 27, 2022 22:40
cfg.static_rx_buf_num = 4;
cfg.dynamic_rx_buf_num = 32;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reject this PR and changes.... as explained in
espressif/esp-idf#8992 (comment)
espressif/esp-idf#8992 (comment)

@SuGlider
Copy link
Collaborator

@zhangyanjiaoesp - Some background about the PR #5791:

At the time of this PR #5791, the idea was to free some heap space when WiFi was activated.

One possible solution, for the lack of heap, was to disable malloc over psram, as done in IDF 3.3, and then enable dynamic buffers in sdkconfig.

The PR #5791 was intended to find a way to keep psram malloc working and, at the same time, enable dynamic WiFi buffer allocation to free some heap memory. This combination was impossible due to a limitation imposed by KConfig.

This work around used in the PR was a nice suggestion from @igrr to try to override WiFi driver and kconfig limitation.
We implemented it and it is working for a number of Arduino users as of today. I don't see a real benefit in disabling it.

@SuGlider SuGlider assigned SuGlider and unassigned P-R-O-C-H-Y Jul 28, 2022
@SuGlider SuGlider self-requested a review July 28, 2022 02:54
Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zhangyanjiaoesp !

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

Successfully merging this pull request may close these issues.

Esp32 failed to send one to many using espnow protocol (IDFGH-7414)
6 participants