Feature/unified ethernet#195
Conversation
Move internal-only functions out of public header: - network_get_event_group - network_request_reconnect - network_is_playback_active - network_is_our_netif - network_events_deinit - network_if_get_ip Use extern declarations in eth_interface.c and wifi_interface.c for functions they need from network_interface.c. Addresses review feedback on PR CarlosDerSeher#195.
Replace extern declarations in eth_interface.c and wifi_interface.c with a proper private header using ESP-IDF's PRIV_INCLUDE_DIRS CMake feature. This follows the convention used by improv_wifi component and addresses PR CarlosDerSeher#195 review feedback. Changes: - Create priv_include/network_interface_priv.h with declarations for network_get_event_group, network_request_reconnect, network_is_playback_active, and network_is_our_netif - Add PRIV_INCLUDE_DIRS to CMakeLists.txt - Replace extern declarations with #include in eth_interface.c - Replace extern declaration with #include in wifi_interface.c
|
So I found some time to test this just now and found a Problem.
Another thing, is there a reason why you disabled ethernet by default? I'd prefer to have it enabled on DHCP per default if ethernet is configured through menuconfig. The above error doesn't seem to happen again now... |
I thought i had it set to DHCP by default - unsure why its default is disabled now.. Is it now not crashing? I havent seen this on my boards which is interesting, I have had occasions where i was testing with rapid dicsonnections from ethernet to wifi and back where the server didnt close the connections and continued to use to old route once the device became availble again. That looked like a server issue rather than client as the client cannot inform the server that the connection is on a new route. As its failover i didnt see it as too much of an issue as majority of users will just want to have both enabled with DHCP and be able to move them around the house which will give enough time for timeouts to occur on the server and clear the routes etc. |
|
No the reboot just happened once, from the back trace it even seems unrelated maybe? |
is there any possibility to avoid this error? |
I have added a disconnect first before stopping which should stop this error |
It was always a bug in the code but never impacted until these changes, have correctly NULLed now. |
|
DHCP is now default for Ethernet as well |
There was a problem hiding this comment.
Pull request overview
This PR refactors networking into a unified Ethernet/WiFi management approach, adding coordinated handover (Ethernet priority with deferred takeover during playback) plus user-configurable Ethernet static IP settings exposed via the web UI.
Changes:
- Added FreeRTOS EventGroup-based signaling for reconnect requests and playback start/stop coordination.
- Implemented Ethernet priority/failover logic (including DHCP restart on reconnect) and added static-IP configuration persisted in NVS.
- Extended the HTTP UI/backend to configure/clear/read Ethernet mode + static IP/gateway/netmask/DNS and added a global “Restart Device” button.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| main/main.c | Binds snapclient connection to the preferred/default netif, adds reconnect handling, and initializes network events before network init. |
| components/ui_http_server/ui_http_server.c | Adds REST handlers for Ethernet settings (GET/POST/DELETE) and keeps /restart endpoint. |
| components/ui_http_server/html/index.html | Adds a global “Restart Device” button in the nav bar. |
| components/ui_http_server/html/general-settings.html | Adds Ethernet configuration UI (mode + static IP fields) and client-side validation/CRUD. |
| components/settings_manager/settings_manager.c | Persists Ethernet mode and static IP parameters in NVS and exposes them via JSON. |
| components/settings_manager/include/settings_manager.h | Declares Ethernet settings APIs. |
| components/network_interface/network_interface.c | Adds EventGroup-based reconnect/playback signaling and a network_has_ip() helper. |
| components/network_interface/include/network_interface.h | Exposes new network event APIs and network_has_ip(). |
| components/network_interface/priv_include/network_interface_priv.h | Adds internal-only network_interface APIs for other files within the component. |
| components/network_interface/eth_interface.c | Major rewrite: takeover logic, static IP apply + gateway reachability ping, DHCP restart, playback-deferral logic. |
| components/network_interface/wifi_interface.c | Pulls in private header for internal helpers. |
| components/network_interface/CMakeLists.txt | Makes Ethernet source conditional and adds private include dir + dependencies. |
| components/lightsnapcast/player.c | Signals playback start/stop to the network layer; adds guards to avoid restart races. |
| components/lightsnapcast/CMakeLists.txt | Adds dependency on network_interface. |
Comments suppressed due to low confidence (1)
components/lightsnapcast/player.c:503
network_playback_started()is called beforeplayer_setup_i2s()succeeds. Ifplayer_setup_i2s()fails, the function returns without sendingnetwork_playback_stopped(), leaving the network layer thinking playback is active (which can indefinitely defer Ethernet takeover/static-IP work). Move the playback-start signal until after all failure points, or ensure all error returns also signal playback stopped.
// Clear shutdown flag - we're starting a new session
player_shutdown_in_progress = false;
playerstarted = true;
if (network_playback_started() != ESP_OK) {
ESP_LOGW(TAG, "Failed to signal playback started to network layer");
}
int ret = 0;
ret = player_setup_i2s(setting);
if (ret < 0) {
ESP_LOGE(TAG, "player_setup_i2s failed: %d", ret);
playerstarted = false;
return -1;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I still see Another thing I think that could be useful, when music is playing and I disconnect the cable it takes quite long until playback resumes from wifi. Do you think it would be possible to speed up this process if music was active while ethernet disconnection? And also the other way around maybe? Also there is Other than that, it seems to work as advertised :) |
ah ok, so NAN is a bug in 5.1.5 espressif/esp-idf#12473 - harmless but incorrect log level. The E (168310) esp_netif_handlers: invalid static ip error - happens because we stop DHCP on disconnect, and it doesn't automatically restart on reconnect - causing "invalid static ip" errors until the IP is applied. DHCP is stopped for a reason (avoid confusion during static IP transitions). Another thing I think that could be useful, when music is playing and I disconnect the cable it takes quite long until playback resumes from wifi. Do you think it would be possible to speed up this process if music was active while ethernet disconnection? And also the other way around maybe? - |
|
Ok. Thanks for clarification 👍 |
|
One more thing, you are writing about stale connections in your commit message, could this origin from using wifi mac in snapcast hello message? I thought it was a good idea to use always the same mac because if not the server thinks we are two different devices. This would cause some issues for the user like volume and names being different suddenly. I guess this is still the best way of doing things though how about you? |
|
Another issue arised for me with current develop, I frequently get memory allocation errors/warnings on my devices with no PSRAM. In the past 750ms buffer was working without issues and I had to reduce this to 700ms maybe even less is needed I am not sure yet. Could you enable https://github.com/CarlosDerSeher/snapclient/blob/develop/main%2Fmain.c#L333 and so some comparison with current state vs your changes? How do those changes affect heap usage? |
|
See #197 |
|
@craigmillard86 So I'd really like to merge this but I am a bit overwhelmed by the tons of commit messages. Could you please squash this to a single commit with a proper commit messages, concentrating on the actual changes/improvements? |
This commit consolidates 86 commits of development work implementing: - Unified ethernet and WiFi network interface management - Ethernet with static IP configuration support - WiFi failover when ethernet disconnects - Ethernet priority with automatic failover to WiFi - IPv6 support with fallback to IPv4 - Improved network robustness and error handling - Global restart button for network recovery - Web UI for ethernet static IP configuration - Proper cleanup and resource management - Various bug fixes and stability improvements Key changes: - Merged separate ethernet and WiFi components into unified network interface - Added persistent Ethernet static IP configuration via Kconfig - Implemented proper connection state management with FreeRTOS EventGroups - Added gateway reachability checks with IPv6 fallback - Fixed race conditions in player startup/shutdown - Improved DHCP restart handling - Reduced buffer allocation for PSRAM-less devices (750ms -> 700ms)
craigmillard86
force-pushed
the
feature/unified-ethernet
branch
from
40af5de to
362ec8c
Compare
Squashed, hopefully thats a little better! |
|
closing this PR as moved to cherry pick branch and new PR |
3 participants
This PR consolidates and refactors the network interface components to provide seamless Ethernet/WiFi handover with static IP support. It replaces the separate eth_interface and wifi_interface components with a unified network_interface component that coordinates both interfaces.
Key Features
Technical Changes
New Components:
Modified Components:
Removed Components:
Fixes Included