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

Register Ethernet netif in WiFiGeneric to #7632

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

s-hadinger
Copy link
Contributor

By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes

Checklist

  1. Please provide specific title of the PR describing the change, including the component name (eg. „Update of Documentation link on Readme.md“)
  2. Please provide related links (eg. Issue which will be closed by this Pull Request)
  3. Please update relevant Documentation if applicable
  4. Please check Contributing guide

This entire section above can be deleted if all items are checked.


Description of Change

Register the ethernet netif object in WiFiGeneric esp_netifs. Without this registry, IP6 events are never sent back to event callback, because of the tests here:

} else if (event_base == IP_EVENT && event_id == IP_EVENT_GOT_IP6) {
ip_event_got_ip6_t * event = (ip_event_got_ip6_t*)event_data;
esp_interface_t iface = get_esp_netif_interface(event->esp_netif);
log_v("IF[%d] Got IPv6: IP Index: %d, Zone: %d, " IPV6STR, iface, event->ip_index, event->ip6_info.ip.zone, IPV62STR(event->ip6_info.ip));
memcpy(&arduino_event.event_info.got_ip6, event_data, sizeof(ip_event_got_ip6_t));
if(iface == ESP_IF_WIFI_STA){
arduino_event.event_id = ARDUINO_EVENT_WIFI_STA_GOT_IP6;
} else if(iface == ESP_IF_WIFI_AP){
arduino_event.event_id = ARDUINO_EVENT_WIFI_AP_GOT_IP6;
} else if(iface == ESP_IF_ETH){
arduino_event.event_id = ARDUINO_EVENT_ETH_GOT_IP6;
}

Tests scenarios

Tested on Olimex POE ESP32 board, now connecting Ethernet to a network supporting IPv6 SLAAC, ARDUINO_EVENT_ETH_GOT_IP6 is correctly sent to callbacks. Before this fix, no event was called.

Related links

@mrengineer7777
Copy link
Collaborator

mrengineer7777 commented Dec 27, 2022

Code analysis performed on arduino-esp32 v2.0.5. Disclaimer: I don't fully understand why WiFiGeneric tracks eth_netif_t entries, or how ETH module works.

Discussion:

add_esp_interface_netif() isn't used anywhere else in code. If used, it should probably be exported in WiFiGeneric.h.

WiFiGeneric.cpp keeps track of network interfaces in private array static esp_netif_t* esp_netifs[ESP_IF_MAX] = {NULL, NULL, NULL}. Entries for AP and STA interfaces appear to be directly created in wifiLowLevelInit(), but the ethernet entry is not setup(?).

Ethernet interface is created in ETH begin:

bool ETHClass::begin(uint8_t phy_addr, int power, int mdc, int mdio, eth_phy_type_t type, eth_clock_mode_t clock_mode, bool use_mac_from_efuse)
{
...
    esp_netif_config_t cfg = ESP_NETIF_DEFAULT_ETH();
    esp_netif_t *eth_netif = esp_netif_new(&cfg);
...
}

I agree eth_netif should be registered with WiFiGeneric. I also agree add_esp_interface_netif(ESP_IF_ETH, eth_netif); is the correct way to register it. Not sure on placement of add_esp_interface_netif(), but seems reasonable. Definitely needs to be before esp_eth_start(), and makes sense to not add it until after esp_netif_attach().

Note ETHClass::begin() creates a new eth_netif_t entry every time it is called. If ETHClass::end() [#5949] is added later, end() may need to unregister the ESP_IF_ETH entry.

Would it be better to create the ethernet entry in wifiLowLevelInit()? Side effects unknown.

@TD-er
Copy link
Contributor

TD-er commented Dec 27, 2022

Yep, WiFi events also need to be unregistered, or else you will get them multiple times.

N.B. For WiFi events you need to register their callback function(s) each time you enable some interface that wasn't enabled before. e.g. AP only will not register STA events.

So it makes sense to register these events when you call begin() and unregister them when you call end().
However, if my memory serves me well, there wasn't an end() function for ETH.

If there still isn't an end() function, then you can register those events in the lowlevel init function (given it will only called once)

@mrengineer7777
Copy link
Collaborator

mrengineer7777 commented Dec 27, 2022

@TD-er

Yep, WiFi events also need to be unregistered, or else you will get them multiple times.

wifiLowLevelInit() checks for event handle before registering again. Looks safe.

N.B. For WiFi events you need to register their callback function(s) each time you enable some interface that wasn't enabled before. e.g. AP only will not register STA events.

AP and STA registered once in wifiLowLevelInit().

So it makes sense to register these events when you call begin() and unregister them when you call end(). However, if my memory serves me well, there wasn't an end() function for ETH.

Correct. Currently no ETH.end().

@TD-er
Copy link
Contributor

TD-er commented Dec 27, 2022

AP and STA registered once in wifiLowLevelInit().

But when you add your own event callback functions, the ones meant for interfaces not being enabled, will not be registered.

@VojtechBartoska VojtechBartoska added the Area: BT&Wifi BT & Wifi related issues label Jan 4, 2023
@VojtechBartoska VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Jan 25, 2023
@me-no-dev me-no-dev merged commit 024ba74 into espressif:master Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BT&Wifi BT & Wifi related issues Status: Review needed Issue or PR is awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants