CarlosDerSeher / snapclient

snapclient on ESP32
GNU General Public License v3.0
101 stars 14 forks source link

Enable fade out/mute in flac_task #25

Closed luar123 closed 8 months ago

luar123 commented 1 year ago

Fix #23 Enables fade out when using the flac task.

CarlosDerSeher commented 1 year ago

I just was thinking, as the passed parameter scSet is used in 3 different tasks, this probably isn't the best idea. Should we use a Semaphore or player_get_snapcast_settings() and player_send_snapcast_settings() or similar for this?

luar123 commented 1 year ago

I agree, it is probably better to use a semaphore for this.

luar123 commented 1 year ago

I gave this some thoughts and I think using something like:

player_get_snapcast_settings(&scSet)
scSet->volume = newValue;
player_send_snapcast_settings(scSet)

is problematic, as another task could change the settings in between get and send. So we either need a new atomic function player_update_snapcast_settings() (or one for each member) or use a Semaphore directly. Also I think it would be good to keep the settings in main and player separated because some updates are not even send to the player (e.g. in metadata_callback()).

CarlosDerSeher commented 1 year ago

So maybe we should make a single snapcast settings object which is accessed by all tasks using a semaphore. Tasks could be informed about changes by the queue, similar as I do it already with player_send_snapcast_settings(scSet)? Maybe they could even be informed directly about the change of specific member(s) in the object by sending a struct with information about what changed? This sounds like a lot of work (in comparison to just inserting a mutex for that specific case) though but maybe it is worth it in the long run?

luar123 commented 8 months ago

Hi, I have not worked on this in a long time and it diverged, so I closed it. At the moment I don't have the time to implement it the way you suggested and I don't think this feature is very important (and also needs a patched snapserver), so I would propose to create a new PR that will remove the fade-out feature completely and fix muting.

CarlosDerSeher commented 8 months ago

Sounds good to me