tonyp7 / esp32-wifi-manager

Captive Portal for ESP32 that can connect to a saved wireless network or start an access point where you can connect to existing wifis.
MIT License
648 stars 214 forks source link

Suggestion: Clearify: callback functions run in wifi_manager task and stack #122

Open S3phe opened 3 years ago

S3phe commented 3 years ago

Prerequisites

Description

The example inside the readme is this: "wifi_manager_set_callback(WM_EVENT_STA_GOT_IP, &cb_connection_ok);"

So if the esp32 station got an ip, the callback function "cb_connection_ok" runs inside the task of wifi_manager and uses its stack.

-> Better use an EventGroupHandle like this:

EventGroupHandle_t wifi_event_group; static const int CONNECTED_BIT = BIT0;

void setConnectedState(){ xEventGroupSetBits(wifi_event_group, CONNECTED_BIT) }

void app_main(){ wifi_event_group = xEventGroupCreate(); wifi_manager_start(); wifi_manager_set_callback(WM_EVENT_STA_GOT_IP, &setConnectedState) // Calls minimal function to set CONNECTED_BIT

xEventGroupWaitBits(wifi_event_group, CONNECTED_BIT, false, true, portMAX_DELAY); // Waits until CONNECTED_BIT is set.

/* normal main-stuff here, only executed when connected - this stays on the main task stack */

}

Steps to Reproduce

Take the demo and do something consuming on the stack. If you look at esp32 Highwater-Mark, you can see that the free memory in stack is shrinking, until you will receive an stack overflow which triggers a reboot.

System Configuration

esp_idf master branch

tonyp7 commented 3 years ago

First of all thank for this report and you are absolutely right.

on the other hand, having to deal with a group outside of the wifi manager starts to make things a little bit too user unfriendly and I don’t think this is meant to change. The nice thing about the callback is that it’s neat and tidy and you can keep the wifi manager as a black box to some extent.

A typical usage of these callbacks would be to send a message to your main program loop outside of the wifi manager. In that case this is not an issue. The callback is absolutely not meant to cram a heavy code base in.

tonyp7 commented 3 years ago

Also see:

https://github.com/tonyp7/esp32-nixie-clock/blob/7e670292a4909e7651a57cdcddf3f031d3a4b3fa/nixie_clock_code/main/main.c#L93

and

https://github.com/tonyp7/esp32-nixie-clock/blob/7e670292a4909e7651a57cdcddf3f031d3a4b3fa/nixie_clock_code/main/clock.c#L235

S3phe commented 3 years ago

I agree with your opinion, that the callback-function is by far the easiest way, but not the best, end especially beginners will run into problems, when they put too much functionality in the callback-function.

For beginners its hard to understand why there is a stack overflow, and that their code in their main file suddenly uses the wifi_managers stack. -> Maybe it is possible to clearify this in Readme and to mention, that using a Queue or EventGroup is best practice.