Feature/unified eth with player state#225
Open
craigmillard86 wants to merge 37 commits into
Open
Conversation
craigmillard86
commented
May 26, 2026
craigmillard86
commented
Contributor
- Unified MAC address across Ethernet and WiFi — The Snapcast server now sees a consistent client identity regardless of which network interface is active, preventing duplicate clients appearing when switching between WiFi and Ethernet.
- Seamless Ethernet takeover — When Ethernet is connected, the device automatically switches from WiFi to Ethernet without interrupting active audio playback. On Ethernet disconnect, it falls back to WiFi gracefully.
- Ethernet boot priority — At boot, the device waits briefly for Ethernet before committing to WiFi, avoiding an unnecessary disconnect/reconnect cycle.
- Ethernet configuration UI — New web UI settings for Ethernet mode (Disabled / DHCP / Static IP) with IP address, netmask, gateway, and DNS configuration.
- Fix: Audio stall after Ethernet disconnect — Resolved an issue where disconnecting Ethernet while playback was stopped caused "latency buffer full" with no audio on reconnect.
- Updated the Restart Button Location — Fixed the web UI to show restart in banner and resolved an error when saving the hostname despite the change succeeding.
…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
- 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
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
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
- 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
Static variables in update_state() and process_data() persisted across connection cycles, causing the playback state tracker to remain PLAYING after a disconnect. On reconnect, update_state() never transitioned to PLAYING again so the codec was never started and audio was silent. Move received_wire_chnk, state tracker, and last-update timestamp from static locals to connection-scoped variables in http_get_task(), passed by pointer through process_data() and update_state(). They now reset naturally each time the outer connection loop iterates.
Two issues prevented reliable Ethernet reconnection:
1. DHCP was stopped in the DISCONNECTED handler but never restarted in
the CONNECTED handler for DHCP mode. On reconnect, ESP-IDF's internal
handler took the static-IP path ("invalid static ip") and no IPv4
address was obtained.
2. IPv6 link-local addresses were not removed on disconnect. On the next
Link Up, esp_netif_create_ip6_linklocal() operated on stale IPv6
state, increasing stack usage and causing a sys_evt stack overflow.
…r), CarlosDerSeher#218, CarlosDerSeher#219 (TAS5825M), CarlosDerSeher#220 (MA12070P + CONFIG_I2S_SLOT_32BIT) Resolved by adopting upstream's snapclient_* naming throughout (reverting our branch's earlier snapcast_* rename) to minimise long-term drift, while keeping our network_state_cb registration in init for the unified-ethernet feature. Renamed sc_restart_snapcast -> sc_restart_snapclient call sites in eth_interface.c to match. Preserved local additions on top of upstream: - network_state_cb + sc_add_state_cb registration for ethernet integration - Runtime MAC read from netif (matches unified MAC on the wire) - Early bring-up of settings_manager_init / network_events_init / network_if_init - 2-second back-off after netconn_close on RESTART/STOP - RESTART/STOP check + 2s delay after process_data error - reset_connection_state() helper + hoisted statics so update_state / process_data state is cleared between snapclient connections (prevents stale "playing" state from previous connection surviving ~1s into the new one) Verified ethernet flow end-to-end via code-reviewer + Explore agents. Build: idf:v5.5.1 / esp32s3 OK (snapclient.bin 0x134bb0 bytes, 38% free). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The reset helper was added in the upstream/develop merge to clear update_state / process_data state between snapclient connections (the previously-removed connection-scoped locals had served this purpose). On review, upstream's function-local statics work fine in practice -- sc_state remains the source of truth, callbacks self-correct on next chunk, and the ~1s staleness window after reconnect has no observable impact. Reverting reduces drift from upstream and removes a future merge-conflict hotspot. Build: idf:v5.5.1 / esp32s3 OK (snapclient.bin 0x134ba0 bytes). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves Ethernet/WiFi coexistence and adds Ethernet static-IP configuration through the web UI, aiming to reduce reconnect loops and make interface selection more deterministic.
Changes:
- Add Ethernet “takeover” logic (default netif selection, unified MAC handling, playback-aware deferrals) and connection backoff behavior.
- Add NVS-backed Ethernet mode/static IP settings and expose them in the HTTP UI (GET/POST/DELETE + UI forms).
- Improve robustness in a few places (queue creation checks, safer string formatting, MDNS init behavior on reconnect).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| main/main.c | Initializes settings/network earlier; adds playback-state callback; improves MAC retrieval and reconnect backoff. |
| main/connection_handler.c | Prefers default netif, adds ETH takeover-aware waiting logic, avoids repeated mdns_init. |
| components/ui_http_server/ui_http_server.c | Adds HTTP endpoints to set/get/clear Ethernet settings in NVS. |
| components/ui_http_server/html/index.html | Adds a “Restart Device” button calling /restart. |
| components/ui_http_server/html/general-settings.html | Adds Ethernet configuration UI + validation + save/reset flows. |
| components/ui_http_server/html/dsp-settings.html | Minor initialization fix (load capabilities on DOMContentLoaded). |
| components/settings_manager/settings_manager.c | Adds NVS keys + getters/setters/validators for Ethernet mode/static IP + JSON export/import. |
| components/settings_manager/include/settings_manager.h | Documents common return codes and adds Ethernet settings APIs. |
| components/network_interface/wifi_interface.c | Adds WiFi suppression helpers for Ethernet takeover and logs unified MAC. |
| components/network_interface/priv_include/network_interface_priv.h | Introduces internal/private network-interface APIs used within the component. |
| components/network_interface/network_interface.c | Adds event-group coordination, network_has_ip, and unified MAC initialization. |
| components/network_interface/include/network_interface.h | Exposes new network APIs (events init, playback signaling, has_ip). |
| components/network_interface/include/eth_interface.h | Exposes takeover status helpers. |
| components/network_interface/eth_interface.c | Adds Ethernet mode, static-IP support, gateway ping check, deferred MAC unification/takeover logic. |
| components/network_interface/CMakeLists.txt | Conditionally builds Ethernet sources and deps; adds private include dir. |
| components/lightsnapcast/snapcast_protocol_parser.c | Improves unsupported codec logging detail. |
| components/lightsnapcast/player.c | Adds error handling for queue creation (partial). |
| README.md | Updates configuration instructions and fixes/updates links (plus minor wording changes). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
craigmillard86
added a commit
to craigmillard86/snapclient
that referenced
this pull request
May 26, 2026
- main.c: replace two sprintf() MAC formatters with snprintf() - settings_manager.c: treat ESP_ERR_NVS_NOT_FOUND as success on erase across the four eth_* setters (idempotent clear) - ui_http_server.c: switch on returned err in five eth_* POST handlers so ESP_ERR_INVALID_ARG maps to 400 Bad Request and storage failures map to 500 Internal Server Error - general-settings.html: always send eth_dns on save (empty string is treated by backend as erase) so clearing the field actually clears the persisted value - eth_interface.c: defer s_eth_handles assignment until after all netif setup succeeds, avoiding dangling-pointer use-after-free in eth_apply_unified_mac() if a later init step fails and cleans up the handle array - player.c: use temp + swap when recreating pcmChkQHdl in player_task; abort the task on creation failure rather than continuing with a NULL handle (matches existing pattern after player_setup_i2s failure) - README.md: typo (environnement -> environment) Not changed: the xTaskNotifyWait(0, 0, ...) call at main.c:1521. Notifications here are value-semantics (eSetValueWithOverwrite); FreeRTOS clears the notification pending state on successful xTaskNotifyWait return regardless of ulBitsToClearOnExit, so the comment's "re-observed repeatedly" concern does not apply here. Build: idf:v5.5.1 / esp32s3 OK (snapclient.bin 0x134cf0 bytes, 38% free in app partition). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- main.c: replace two sprintf() MAC formatters with snprintf() - settings_manager.c: treat ESP_ERR_NVS_NOT_FOUND as success on erase across the four eth_* setters (idempotent clear) - ui_http_server.c: switch on returned err in five eth_* POST handlers so ESP_ERR_INVALID_ARG maps to 400 Bad Request and storage failures map to 500 Internal Server Error - general-settings.html: always send eth_dns on save (empty string is treated by backend as erase) so clearing the field actually clears the persisted value - eth_interface.c: defer s_eth_handles assignment until after all netif setup succeeds, avoiding dangling-pointer use-after-free in eth_apply_unified_mac() if a later init step fails and cleans up the handle array - player.c: use temp + swap when recreating pcmChkQHdl in player_task; abort the task on creation failure rather than continuing with a NULL handle (matches existing pattern after player_setup_i2s failure) - README.md: drop all README changes vs upstream/develop -- the file is upstream-owned content and this feature branch should not be modifying it. Copilot's environnement->environment typo catch is valid but should be addressed via a separate upstream PR rather than carried here. Not changed: the xTaskNotifyWait(0, 0, ...) call at main.c:1521. Notifications here are value-semantics (eSetValueWithOverwrite); FreeRTOS clears the notification pending state on successful xTaskNotifyWait return regardless of ulBitsToClearOnExit, so the comment's "re-observed repeatedly" concern does not apply here. Build: idf:v5.5.1 / esp32s3 OK. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
craigmillard86
force-pushed
the
feature/unified-eth-player-state
branch
from
May 26, 2026 23:01
ee90082 to
9afbedd
Compare
Contributor
Author
|
@CarlosDerSeher / @luar123 I believe this is now in a good place to merge, let me know if there is anything else! |
luar123
reviewed
May 29, 2026
| if (result != 0) { | ||
| break; // restart connection | ||
| // Check if a RESTART arrived during the blocking recv | ||
| if (xTaskNotifyWait(0, 0, &command, 0) == pdTRUE && |
Contributor
There was a problem hiding this comment.
This will clear the notification but not handle the command. Can you move the whole command handling part (from if (xTaskNotifyWait(0, 0, &command, 1) == pdTRUE) {... to if (restart) {...}) just before if (result != 0) {?
Then add another bool stopped = false and set this to true in the STOPPED case. If restart or stopped -> close netconn and if restart or result!=0 delay(2s). Then we also avoid the delay in the stopped case.
Contributor
|
Do you think you could also add esp_wifi_set_ps(WIFI_PS_MAX_MODEM)/esp_wifi_set_ps(WIFI_PS_NONE) based on snapclient state? Would this fit in here? |
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.