mccloudaero / evpr

Electric Variable Pitch Rotor
http://mccloudaero.com
GNU General Public License v3.0
2 stars 1 forks source link

Minimal WiFi/ESPNOW initialization #83

Closed GeorgesOatesLarsen closed 3 years ago

GeorgesOatesLarsen commented 3 years ago

ESPNOW Docs mention only that WiFi "Should be initialized." They do not specify how.

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/network/esp_now.html

We currently follow an outdated initialization method relying on the deprecated tcpip_adapter_init function.

https://github.com/mccloudaero/evpr/blob/update_esp_idf_v4/firmware/head_node/main/main.c

ESP-NOW example provided by ESPIDF uses more up-to-date functions, but also make a call to esp_event_loop_create_default which we do not. In fact, we do not seem to use esp_event_loop at all.

Event loop library docs Source code for esp_event_loop_create_default

So is the event loop even necessary here??

There is an interesting section in the WiFi driver documentation: "Preconditions of Using esp_wifi_80211_tx()"

The Wi-Fi mode is Station, or AP, or Station+AP. Either esp_wifi_set_promiscuous(true), or esp_wifi_start(), or both of these APIs return ESP_OK. This is because we need to make sure that Wi-Fi hardware is initialized before esp_wifi_80211_tx() is called. In ESP32, both esp_wifi_set_promiscuous(true) and esp_wifi_start() can trigger the initialization of Wi-Fi hardware. The parameters of esp_wifi_80211_tx() are hereby correctly provided.

ESPNOW, uses raw 80211 frames. It is likely that this is the exact function being used.

It may be worthwhile to look at source code for ESPNOW

GeorgesOatesLarsen commented 3 years ago

Yikes. WiFi stack is provided as binaries. RIP. So, it's not totally clear how ESPNOW operates under the hood.

esp_event_loop_create_default calls esp_event_loop_create with s_default_loop.

esp-event documentation is strangely silent about what in the heck an event loop actually is in this context, so we'll have to read the source code I guess.

esp_event_loop_create starts an event queue, mutex, OPTIONALLY creates a task using "esp_event_loop_run_task".

Ahhh, okay, interesting. The "passed in" s_default_loop is, in fact, a return argument. Which, yeah, I see now the document was saying exactly that.

So this function exists to create a task operating "esp_event_loop_run_task". The default variant just performs config on its own and takes care of keeping track of the event loop handle. OK.

Better take a look at esp_event_loop_run_task then. Same file, esp_event.c.

It literally exists to call esp_event_loop_run ad infinitum. Which also lives in esp_event.c.

We receive from a queue, same one as before I do believe.

Hmm! We take a mutex on the queue. I wonder why?

Basically we just parse incoming events, and dispatch the registered handlers. It's really nothing special.

So who registers events?

OOOHHH. I'll bet you that mutex is to prevent someone adding an event listener mid-way through firing events.

If that's true, we should NOT see mutex grab for triggering event. exp_event_post_to appears to do this.

This is sort of true? If the loop has a dedicated task, then this is how it behaves. If the loop does not have a dedicated task, we still take the mutex. It might just be a shared mutex. I'm not too interested in the strange designs that allow these loops to migrate between tasks, so I won't bother to investigate any further.

esp_event_handler_register_with_internal appears to be the function actually responsible for registerring event callbacks. It does, indeed, take the mutex. It is called by various functions, including:

esp_event_handler_register_with

Which is a top-level function:

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/system/esp_event.html

And we're back here. Okay, it's not that the documentation is "weirdly silent", it's just poorly written. Or maybe I am bad at reading.

But yeah, this is a run-of-the-mill event listener system. I'm not sure why this is even in the ESPIDF examples for ESPNOW. Event handlers aren't even registered. Unless the WiFi stack binaries are using the event loop internally, this line of code is not necessary at all. I'll start an issue asking about this.

GeorgesOatesLarsen commented 3 years ago

Never mind, the process for starting an issue with them is absolutely pathological, lol. Not worth the time. Gonna skip the inclusion of esp_event_loop_create_default since first-glance reverse engineering indicates it is useless, and we have a history of not including it with no detrimental effects.

GeorgesOatesLarsen commented 3 years ago

https://esp32.com/viewtopic.php?t=17536 indicates that we can skip esp_wifi_set_storage if we disable nvs in the wifi config. I am going to try this.

GeorgesOatesLarsen commented 3 years ago

Note the inclusion of some interesting black magic in the ESPIDF example:

#if CONFIG_ESPNOW_ENABLE_LONG_RANGE
    ESP_ERROR_CHECK( esp_wifi_set_protocol(ESPNOW_WIFI_IF, WIFI_PROTOCOL_11B|WIFI_PROTOCOL_11G|WIFI_PROTOCOL_11N|WIFI_PROTOCOL_LR) );
#endif

Do we want this?

I'm not even 100% certain it would have any effect on ESPNOW, to be frank. Yes, it's part of the official ESPNOW example, but as we've established, that alone is not indicative of whether it is useful for ESPNOW.