Add snapcast state and refactor inter task communication by luar123 · Pull Request #203 · CarlosDerSeher/snapclient

luar123 · 2026-02-15T16:43:22Z

This PR adds snapcast state + callbacks and refactors the inter task communication.

snapcast state: STOPPED, IDLE, PLAYING or PAUSED
callbacks can be registered and fire on state change
functions to pause/stop/restart/start the snapclient from main app or any other task. Can be used to free i2s and play from another source, or to restart the connection
player task is not aware of mute/volume state anymore. This simplifies player_send_snapcast_setting
handle dac sleepmode from main task. Use state callback to decide wake up/ shutdown.
dac is only unmuted if snapcast mute state and player mute state is false, avoiding unnecessary mute/unmute.
discard chunks before decoding if player is paused
use mutex instead of player_shutdown_in_progress from feat: Unified Ethernet interface with settings manager and web UI #201

I am unsure about the task notifications, had a race conditions when using them in the callbacks (that's why the main task callback uses a mutex). So maybe it would be better to use mutex or queues instead.

~~Todo: Either change notifications or set configTASK_NOTIFICATION_ARRAY_ENTRIES=2 in sdkconfigs.~~

@craigmillard86 Using this PR network_playback_stopped/network_playback_started from #201 could be replaced by a state callback.

craigmillard86 · 2026-02-24T22:45:33Z

I have merged these together and created a branch and PR here:
https://github.com/craigmillard86/snapclient/tree/feature/unified-eth-player-state
luar123#1

@luar123 not sure how best to handle this with all the changes going on at the moment!

luar123 · 2026-02-25T22:55:19Z

@craigmillard86 Let's discuss your changes in luar123#1 until #204 and this PR are merged.

CarlosDerSeher · 2026-02-28T20:31:17Z

There seems to be a problem

I (5428) PLAYER: DMA completely loaded
I (6119) PLAYER: initial sync age: 7us, chunk duration: 26122us

assert failed: ulTaskGenericNotifyTake tasks.c:5734 (uxIndexToWait < 1)


Backtrace: 0x4008c299:0x3ffe4ec0 0x4008c22d:0x3ffe4ee0 0x400912e9:0x3ffe4f00 0x4018f689:0x3ffe5020 0x400f8e84:0x3ffe5040
--- 0x4008c299: panic_abort at /home/karl/espressif/esp-idf-v5.5.2/components/esp_system/panic.c:477
--- 0x4008c22d: esp_system_abort at /home/karl/espressif/esp-idf-v5.5.2/components/esp_system/port/esp_system_chip.c:87
--- 0x400912e9: __assert_func at /home/karl/espressif/esp-idf-v5.5.2/components/newlib/src/assert.c:80
--- 0x4018f689: ulTaskGenericNotifyTake at /home/karl/espressif/esp-idf-v5.5.2/components/freertos/FreeRTOS-Kernel/tasks.c:5734
--- 0x400f8e84: player_task at /home/karl/luar123_snapclient/components/lightsnapcast/player.c:2194

When I set CONFIG_FREERTOS_TASK_NOTIFICATION_ARRAY_ENTRIES = 2 this goes away. Not sure why this assert failed: ulTaskGenericNotifyTake tasks.c:5734 (uxIndexToWait < 1) fails though if set to 1

luar123 · 2026-02-28T20:53:25Z

Yes, I wrote this in the first post:

Todo: Either change notifications or set configTASK_NOTIFICATION_ARRAY_ENTRIES=2 in sdkconfigs.

I added a second notification to player task (index 1, 0 is already used) so this needs to be set to 2. We could also change it to mutex.

CarlosDerSeher · 2026-02-28T21:20:11Z

Either sdkconfig.defaults should be adjusted or use mutex

Ok sorry should have read your post more thoroughly. I see this is still open for discussion

luar123 · 2026-02-28T22:54:31Z

Added it to the sdkconfigs. Mutex is not working, because it can't be given from multiple tasks.

luar123 · 2026-03-17T20:43:47Z

While working on #217 I discovered a few deficiencies:

dac control: Only set volume/mute when snapcast is playing and (re-)apply when it starts playing (already implemented in Add Bluetooth A2DP Sink Component #217)
the player state as is implemented is good enough for i2s management, but for general resource management we need the snapcast state. Then we can know if snapcast wants to play and stop bluetooth / release memory for the decoder to work.
Additionally, I want to use an explicit i2s lock to make sure just one task is trying to use it.

CarlosDerSeher · 2026-03-17T21:46:12Z

I like you pushing those features forward. Some input that came to my mind when I saw you'll refactor snapcast setting related data exchange / inter task communications.

Currently the tasks store their own scSetings object. Wouldn't it be better to have them operate on the same data?
tasks could then be notified about a change through queue / event loop. Not sure which one is better. Queue would be more freertos generic than event loop, which is an IDF thing or am I wrong?

luar123 · 2026-03-17T22:20:03Z

Good idea, will have a look.

luar123 · 2026-03-29T21:39:20Z

Back-ported volume control, snapcast state instead of player state and i2s lock from BT PR #217
Will look into scSetings next.

luar123 · 2026-03-30T10:07:21Z

* Currently the tasks store their own scSetings object. Wouldn't it be better to have them operate on the same data?

* tasks could then be notified about a change through queue / event loop. Not sure which one is better. Queue would be more freertos generic than event loop, which is an IDF thing or am I wrong?

Currently we have:

one object owned by http_get task. This is also written in the flac callbacks without protection. Does flac use its own task?
a global copy in player.c currentSnapcastSetting.
a local copy in player task. -> this will always be needed for fast access and for comparison.

I see two options to improve:

Use a mutex protected global scSettings object directly in http_task and send a change notification to player task.
Use a local scSettings object in http_task as is done now and send it to player task through a queue. Would be easier because scSettings could be used without mutex in http task but requires a queue with space for the whole object.

@CarlosDerSeher what do you think?

CarlosDerSeher · 2026-03-30T10:37:07Z

All codecs are called within http_task context, so no mutex needed here.

I'd opt for option 2, as I am almost certain that scSetings object could be reduced in size. I am not sure which variables are absolutely needed in player task.

CarlosDerSeher · 2026-03-30T18:37:03Z

Had a quick look and I think this is much cleaner already

luar123 · 2026-03-30T20:13:07Z

Thanks. Please have a closer look when you have time. In the meantime I will use it as base for my other work.

I removed everything that is not needed from snapcastSetting_t and renamed to playerSetting_t. Then I created a new snapcastSetting_t that is playerSetting_t+mute+volume, because those two are not needed in the player task.

Please also have a closer look at the changes in main.c regarding snapcast state and sending commands to http_task. I decided to use a task notification with an enum as value to send the commands. This could be improved by setting individual bits for the commands or using a event group to avoid overwriting previous commands (for example restart requested from network interface change).

luar123 · 2026-04-24T21:01:06Z

@CarlosDerSeher do you think you could look into this soon?
@craigmillard86 has already rebased #201 on this in luar123#1 so this is also ready.

I can then maintain the a2dp sink component in my repo so you don't have to review it.
And to further modularize I think it would be great to move all snapclient stuff from main into a component (like in the preview #207), so others could import it more easily. I can prepare a PR for that without any functional changes once this is merged.

CarlosDerSeher · 2026-04-25T06:04:46Z

I've actually thought about bringing in a collaborator.
Since you are very committed to the project anyway your name popped up in my head. Maybe we can get together in another channel and talk about this? Discord or similar?

luar123 · 2026-04-25T13:36:10Z

Yes sure, I don't have a ton of free time to look into other PRs myself, but could certainly help. You can find me on discord with the same name.

CarlosDerSeher

After reviewing (but not testing) the changes I think this is mostly OK. See my comments.

CarlosDerSeher · 2026-05-05T18:58:52Z

+    if (sc_state == STOPPED) {
+      xSemaphoreGive(snapcastStateMux);
+      command = STOP;
+      while(command != START) {


Is this save to do? Will turn false for sure some time?

The idea is that after init sc_state=STOPPED so it will block until main calls sc_start_snapcast(). This sets command=START. After this sc_state will only be STOPPED if sc_stop_snapcast() is called. This allows the main app to stop and disconnect snapcast completely.
Thinking about it, I should probably modify the OTA component to make use of this.

Added sc_stop_snapclient() before killing http_task for OTA, so the connection is terminated properly.

CarlosDerSeher

I like the renaming to snapclient

CarlosDerSeher · 2026-05-13T10:37:48Z

  // KillAllThreads();
  // dsp_i2s_task_deinit();
+  sc_stop_snapclient();
+  vTaskDelay(1000 / portTICK_PERIOD_MS);  // give snapclient some time to stop before we kill the http task that might be using the player


I am pretty sure this is safe to do but isn't there a way to get state information with the new implementation? Just to be 100% sure we can kill the http task.

Yes, will add it.

also please ensure the usage of pdMS_TO_TICKS throughout

Fixed both: ota will now wait for the snapclient state callback and only kills http_task if it times out. Also snapclient_stop stops the player task now so we don't have to wait for the queue to drain. Overall, way less delay than before.
Had to increase the ota task stack size slightly.

CarlosDerSeher · 2026-05-20T20:30:40Z

Alright I think this is ready? Increased OTA stack won't matter much as it is only active after snapclient is off.

luar123 · 2026-05-20T20:41:30Z

I think this is good to go

CarlosDerSeher · 2026-05-20T20:48:09Z

Okcould you have a look at the commit message? It seems there are some unrelated ones there. Especially the one about partition size.

luar123 · 2026-05-21T18:09:07Z

Could you merge with squash and simply replace the description(commit messages) with:

* Add snapcast state and player state
* refactor inter task communication
* add commands to controll the snapclient
* Set CONFIG_FREERTOS_TASK_NOTIFICATION_ARRAY_ENTRIES=2
* Add i2s lock
* Apply volume/mute only while playing
* Refactor handling of scSettings object
* Use sc callback in ota, stop player task early on sc_stop

manually squashing is a bit annoying because I merged develop a few times.

luar123 · 2026-05-21T20:34:54Z

Thank you!

…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>

Add player state and refactor inter task communication

11d6c49

CarlosDerSeher reviewed Feb 18, 2026

View reviewed changes

Comment thread components/lightsnapcast/player.c Outdated

Comment thread components/lightsnapcast/player.c Outdated

Comment thread main/main.c Outdated

luar123 added 2 commits February 20, 2026 18:28

maximize partitions

dfe124a

Fix dead lock in deinit_player. Refactor callbacks and check for NULL

2074ba9

luar123 added 2 commits February 28, 2026 16:34

Merge branch 'develop' into player_state

81b97a7

Keep volume in settings struct

d4923a9

luar123 changed the base branch from refactor_parser to develop February 28, 2026 15:40

luar123 added 3 commits February 28, 2026 16:46

Merge remote-tracking branch 'upstream/develop' into player_state

4d4f250

Move log message so it does not spam the logs when paused

61a54a7

fix

029bf60

Set CONFIG_FREERTOS_TASK_NOTIFICATION_ARRAY_ENTRIES=2

b3c9532

luar123 mentioned this pull request Feb 28, 2026

Preview: Snapclient component with reduced dependencies #207

Draft

fix state cb

b6daa59

CarlosDerSeher reviewed Mar 3, 2026

View reviewed changes

Comment thread components/lightsnapcast/player.c Outdated

luar123 added 3 commits March 3, 2026 22:56

Merge remote-tracking branch 'upstream/develop' into player_state

00dcd40

fix

db77bee

Merge remote-tracking branch 'upstream/develop' into player_state

4a9335d

luar123 mentioned this pull request Mar 17, 2026

Add Bluetooth A2DP Sink Component #217

Draft

luar123 added 3 commits March 29, 2026 23:19

Apply volume/mute only while playing

cf4f984

Use snapcast state instead of player state

5ec9a48

Add i2s lock

4915458

Refactor handling of scSettings object

a274fa1

CarlosDerSeher reviewed May 5, 2026

View reviewed changes

Stop snapclient on OTA, rename functions, add docu.

b3907e7

CarlosDerSeher reviewed May 13, 2026

View reviewed changes

luar123 added 2 commits May 17, 2026 21:11

Use sc callback in ota, stop player task early on sc_stop

d0bc694

small delay and increase task stack

22aeabb

luar123 changed the title ~~Add player state and refactor inter task communication~~ Add snapcast state and refactor inter task communication May 21, 2026

CarlosDerSeher merged commit c69ecac into CarlosDerSeher:develop May 21, 2026
2 checks passed

luar123 mentioned this pull request May 21, 2026

feat: Unified Ethernet interface with settings manager and web UI #201

Closed

Conversation

luar123 commented Feb 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

craigmillard86 commented Feb 24, 2026

Uh oh!

luar123 commented Feb 25, 2026

Uh oh!

CarlosDerSeher commented Feb 28, 2026

Uh oh!

luar123 commented Feb 28, 2026

Uh oh!

CarlosDerSeher commented Feb 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luar123 commented Feb 28, 2026

Uh oh!

Uh oh!

luar123 commented Mar 17, 2026

Uh oh!

CarlosDerSeher commented Mar 17, 2026

Uh oh!

luar123 commented Mar 17, 2026

Uh oh!

luar123 commented Mar 29, 2026

Uh oh!

luar123 commented Mar 30, 2026

Uh oh!

CarlosDerSeher commented Mar 30, 2026

Uh oh!

CarlosDerSeher commented Mar 30, 2026

Uh oh!

luar123 commented Mar 30, 2026

Uh oh!

luar123 commented Apr 24, 2026

Uh oh!

CarlosDerSeher commented Apr 25, 2026

Uh oh!

luar123 commented Apr 25, 2026

Uh oh!

CarlosDerSeher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarlosDerSeher May 5, 2026

Choose a reason for hiding this comment

Uh oh!

luar123 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

luar123 May 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CarlosDerSeher left a comment

Choose a reason for hiding this comment

Uh oh!

CarlosDerSeher May 13, 2026

Choose a reason for hiding this comment

Uh oh!

luar123 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

CarlosDerSeher May 17, 2026

Choose a reason for hiding this comment

Uh oh!

luar123 May 17, 2026

Choose a reason for hiding this comment

Uh oh!

CarlosDerSeher commented May 20, 2026

Uh oh!

luar123 commented May 20, 2026

Uh oh!

CarlosDerSeher commented May 20, 2026

Uh oh!

luar123 commented May 21, 2026

Uh oh!

luar123 commented Feb 15, 2026 •

edited

Loading

CarlosDerSeher commented Feb 28, 2026 •

edited

Loading