Feature/unified Wifi and Ethernet#1
Open
craigmillard86 wants to merge 15 commits intoluar123:player_statefrom
Open
Feature/unified Wifi and Ethernet#1craigmillard86 wants to merge 15 commits intoluar123:player_statefrom
craigmillard86 wants to merge 15 commits intoluar123:player_statefrom
Conversation
…into feature/unified-eth-develop
Merge branch 'luar123/player_state' into 'feature/unified-eth-develop', combining refactored parser architecture with unified Ethernet management. Critical fixes applied to merged codebase: CRITICAL-3: Fix reentrancy deadlock on playerStateMux (introduced by merge) - pause_player() was invoking callbacks while holding mutex - Moved call_state_cb() outside mutex-protected section - Prevents callback from reacquiring same mutex CRITICAL-1: Fix deadlock in deinit_player() (pre-existing in luar123) - deinit_player() held mutex while waiting for player task - Now acquires mutex only to signal shutdown, releases before waiting - Prevents task from being blocked during shutdown sequence CRITICAL-2: Fix double network_if_init() with wrong ordering (pre-existing in feature/unified-eth-develop) - Removed redundant first network_if_init() call - Reordered: network_events_init() must come before network_if_init() - Critical for event group initialization CRITICAL-4: Fix task notification return value check (pre-existing in luar123) - ulTaskNotifyTake() returns uint32_t notification value, not pdTRUE - Changed: if (ulTaskNotifyTake(...) == pdTRUE) → if (ulTaskNotifyTake(...) != 0) - Correctly detects notification receipt
luar123/player_state code uses strcmp() and vTaskDelay() which require <string.h> and FreeRTOS headers. Without these explicit includes, the code fails to compile with implicit declaration errors. These are essential dependencies, not optional.
luar123's simpler approach: don't take playerStateMux at all during deinit, avoiding the deadlock risk entirely. More elegant than our previous fix.
…ents - Implement unified MAC address so Ethernet uses WiFi eFuse MAC for consistent Snapcast client identity across interfaces - Defer MAC unification during active playback to avoid audio interruption - Add WiFi suppression during MAC transition to prevent switch MAC flapping - Fix boot-time MAC unification when Ethernet links up before WiFi - Fix Ethernet disconnect failover with proper server cleanup delays - Harden thread safety for takeover state with semaphore protection - Fix player mutex corruption, state callback, and pause/resume handling - Fix hostname save showing false error in web UI (undeclared variable) - Respect default netif set by takeover logic in connection handler - Add Ethernet grace period before committing to WiFi at boot
There was a problem hiding this comment.
Pull request overview
This pull request implements unified MAC addressing between WiFi and Ethernet interfaces to enable seamless network transitions in a Snapcast audio client on ESP32. The main goal is to prevent duplicate client appearances in the Snapcast server when switching between network interfaces and to provide smooth Ethernet takeover without interrupting audio playback.
Changes:
- Introduces unified MAC address system where WiFi and Ethernet share the same MAC address, enabling seamless IP takeover via DHCP
- Implements Ethernet takeover state machine with playback-aware deferral to avoid interrupting active audio
- Adds Ethernet configuration UI (Disabled/DHCP/Static IP) with IP address, netmask, gateway, and DNS settings
- Fixes player state callback issues including pause/resume handling and mutex corruption prevention
- Adds network event coordination system using FreeRTOS EventGroups for reconnect requests and playback state signaling
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| main/main.c | Adds network playback state tracking callback and updates MAC address retrieval to use runtime MAC from netif; reorders initialization to set up network events before interfaces |
| main/connection_handler.c | Updates network interface selection logic with Ethernet priority and boot-time grace period; fixes mDNS initialization issue; improves netif selection during reconnection |
| components/ui_http_server/ui_http_server.c | Adds HTTP handlers for Ethernet settings (mode, static IP, netmask, gateway, DNS) with validation |
| components/ui_http_server/html/index.html | Adds restart button in navigation bar with JavaScript handler |
| components/ui_http_server/html/general-settings.html | Adds Ethernet configuration UI section with validation for static IP settings; includes netmask validation with comprehensive valid mask list |
| components/ui_http_server/html/dsp-settings.html | Adds DOMContentLoaded event listener initialization |
| components/settings_manager/settings_manager.c | Implements Ethernet settings storage/retrieval with IPv4 and netmask validation functions |
| components/settings_manager/include/settings_manager.h | Adds public API for Ethernet settings management and documents common return codes |
| components/network_interface/wifi_interface.c | Adds WiFi suppression mechanism for Ethernet takeover to prevent MAC flapping; logs WiFi MAC for verification |
| components/network_interface/priv_include/network_interface_priv.h | Defines internal network interface functions for inter-component coordination |
| components/network_interface/network_interface.c | Implements unified MAC address system, network event group for coordination, and playback state tracking |
| components/network_interface/include/network_interface.h | Adds public API for network events, playback state signaling, and IP checking |
| components/network_interface/include/eth_interface.h | Adds functions to check Ethernet takeover status and enabled state |
| components/network_interface/eth_interface.c | Implements comprehensive Ethernet management including static IP configuration, MAC unification, takeover state machine, playback monitor task, gateway reachability checks, and auto-disable on failure |
| components/network_interface/CMakeLists.txt | Conditionally includes Ethernet sources and ping dependency based on build configuration |
| components/lightsnapcast/snapcast_protocol_parser.c | Fixes log message formatting for unsupported codec |
| components/lightsnapcast/player.c | Fixes pause/resume state handling, moves state callbacks outside mutex to prevent deadlock, adds early playback state synchronization, and improves queue creation error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Restore WiFi as default netif when MAC unification fails (both immediate and deferred takeover paths) to prevent broken routing - Remove 100ms vTaskDelay that blocked the ESP system event loop task - Add missing network_playback_stopped() on start_player failure paths so the network layer doesn't think playback is still active - Mark wifi_suppressed_for_takeover as volatile for cross-task visibility - Remove shadowed 'paused' variable in http_get_task inner scope - Remove 0.0.0.0 from JS validMasks to match C-side netmask rejection
luar123
reviewed
Feb 25, 2026
Owner
luar123
left a comment
luar123
left a comment
There was a problem hiding this comment.
Here are a few comments. It seems you use both, the player callback system and network_playback_started() directly at the same time. Is this intended?
Adopt luar123's major architectural improvements: - Command-based state machine (STOP/START/RESTART/PAUSE/UNPAUSE) - snapcastSetting_t wrapping playerSetting_t with muted/volume - Refactored init_snapcast() with i2s lock callback - Parser rewrite: macros to inline functions, PARSER_OK/PARSER_RESTART_CONNECTION - i2s locking via function pointer, ESP_PM_APB_FREQ_MAX power management - Callback-only state notification (sc_add_state_cb) per PR review feedback Preserve our unique features: - Unified Ethernet/WiFi MAC via runtime netif lookup - ETH priority/takeover logic in connection_handler.c - network_state_cb bridging snapcast state to network events - Reconnect checking in http_get_task inner and outer loops - network_events_init() before network_if_init() ordering - mDNS fix (no repeated mdns_init)
- Remove stale player.h include and get_player_state() fallback from network_is_playback_active() — luar123 removed these APIs; the event-bit path (kept in sync by network_state_cb) is sufficient - Debounce network_state_cb: only signal playback_stopped when transitioning from PLAYING/PAUSED, preventing transient IDLE during reconnect cycles from triggering premature ETH takeover - Change outer-loop reconnect log from ESP_LOGD to ESP_LOGI for visibility at default log level - Make playback_old non-static and reset on reconnect to prevent stale state from skipping volume/mute reapplication
Author
|
I have updated based on your latest changes and suggestions - I havent tested this yet but looked good.. Let me know your thoughts @luar123 |
Owner
|
Great, thanks! I also tried the same :-) but was not able to test, so not sure if it is complete, see https://github.com/luar123/snapclient/tree/feature/unified-eth-player-state. Anyway, your clean-up looks more complete. Could you still have a look at my branch, I removed the whole |
Remove the network_check_and_clear_reconnect() event-bit polling mechanism and use direct sc_restart_snapcast() calls instead. The command-based state machine already has a RESTART command, so a separate polling system is unnecessary. - Replace 5 network_request_reconnect() calls in eth_interface.c with sc_restart_snapcast() - Remove network_request_reconnect() and network_check_and_clear_reconnect() from network_interface.c and headers - Remove 3 polling call sites from main.c inner/outer loops - Move process_data() before command xTaskNotifyWait for better RESTART responsiveness - Add 2s server cleanup delay to RESTART path with cross-reference comments - Add NULL checks after xQueueCreate in player.c (start_player, player_task, and snapcastSettingQueueHandle in init_player) - Add fallthrough annotation for STOP->RESTART switch case - Improve unsupported codec error log with string length
Author
Ah great! Sorry been busy moving house :) Have updated to follow as makes sense and much cleaner. Will try and test over the weekend |
luar123
reviewed
Apr 4, 2026
Owner
luar123
left a comment
luar123
left a comment
There was a problem hiding this comment.
Just some small comments. Will also try to test more.
- Add timer/PM cleanup on queue creation failure in start_player() - Move queue info log inside else block in player_task - Only treat PLAYING (not PAUSED) as active in network_state_cb - Preserve paused state across reconnects - Reorder inner loop: check commands before process_data, add post-recv RESTART detection with 2s delay - Move network init before audio/codec init for faster startup - Restore develop branch PR text in README
Author
|
Still yet to test on actual device! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
of which network interface is active, preventing duplicate clients appearing when switching between WiFi and Ethernet.
without interrupting active audio playback. On Ethernet disconnect, it falls back to WiFi gracefully.
unnecessary disconnect/reconnect cycle.
netmask, gateway, and DNS configuration.
stopped caused "latency buffer full" with no audio on reconnect.
and notification mishandling.