Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makinkg swyh a service #118

Closed
wants to merge 5 commits into from
Closed

Makinkg swyh a service #118

wants to merge 5 commits into from

Conversation

ein-shved
Copy link
Contributor

@ein-shved ein-shved commented Feb 21, 2024

I am going to use swyh as audio streaming service at home onboard of allwiner-based OrangePI Zero 2 device.

For this, I updated CLI version to accept its configuration directly as file, change configuration reading to accept defaults and changed some of its types to be more rust-styled Options.

Added module for NixOS, so you can fully configure swyh-rs as several services in couple of lines:

    services.swyh = {
        my-default-swyh = {
            auto_resume = true;
            ssdp_interval_mins = 1.5;
        };
        extra-swyh = {
            server_port = 5902;
            sound_source = 2;
        };
    };

Additionally added simple tests based on nixos-test framework: two for testing configurations and one - for migration.
Run them with nix build .#test, or add github action for auto-testing with nix

@dheijl
Copy link
Owner

dheijl commented Feb 21, 2024

Question (without looking at the changes in detail): what happens if you apply these changes and start swyh-rs and swyh-rs-cli with their existing configurations? Does it work as before and the config files get silently "upgraded" (as has been the case for almost every release) ?

@ein-shved
Copy link
Contributor Author

ein-shved commented Feb 21, 2024

I'm hoping nothing will break and the new version will work well with previous and older configurations. At least that's what I'm working on.

The best way - is to perform migration test (e.g. using nixosTest framework, see my nix-test branch), but I failed to find a way to simulate an input audio device with qemu.

This patch allows to specify configuration in command-line arguments as
file, and allows it to be read-only and partly completed.

Change-Id: Ie7eb5d1124072d24665cc197b6ff772d7af53c40
This module implement cross-platform detecting of local address without
outgoing connections. This useful to run swyh when you do not any
internet connection (e.g. test).

Change-Id: I53dcd481bd03e8be45f6c388c09721090422cb91
Change-Id: I224f301881f1d0c422f8ad0a19a2c23942d62404
This module introduce options to create multiple instances of SWYH as
systemd services with very same configuration options its accepts in
native for Nix way. E.g:

```nix
services.swyh = {
    my-default-swyh = {
        auto_resume = true;
        ssdp_interval_mins = 1.5;
    };
    extra-swyh = {
        server_port = 5902;
        sound_source = 2;
    };
};
```

Change-Id: Ie4e1a5989e58bf00ff73fb4bc83553a4554a5507
Change-Id: Ib6a82a41d7989ba3245ab227143456a1e89a2fba
@dheijl
Copy link
Owner

dheijl commented Mar 6, 2024

As there were too many conflicts to merge directly I cherrypicked your commits into my main branch (with a few minor changes).

I may have messed up the nix files, so you'd better check those, as I am not into nix myself (it didn't work out of the box on Ubuntu 22.04 without messing with groups and permissions and I couldn't be bothered).

I put the local-ip-address crate behind a feature of the CLI, so that the GUI (most users) is not affected.

I also removed some old cruft (the old ini file and old config folder migration code) from the configuration.

I intend to release 1.10.0 shortly.

Could you provide some short guidance on the nix and service stuff for me to put in the readme?

The code, both gui and cli, behave as desired and nothing seems to break, thanks for the excellent work!

@ein-shved
Copy link
Contributor Author

I will check and fix(if needed) the nix stuff shortly, and suggest patch to Readme.

Still you can see here, how I use swyh in my firmware for allwiner OrangePi Zero2 PC - the pcm-to-chromecast streaming device.

Can you please provide more information about problems, you have with nix on Ubuntu? Maybe, better to try this instead of ubuntu-builtin?

@dheijl
Copy link
Owner

dheijl commented Mar 6, 2024

I did a simple sudo apt install as you suggested earlier and got "socket permission" errors. I suppose that the standard package install resulted in a multi-user setup that caused the problems.

I will follow your advice and install the single-user setup from here, and try again after you have looked at my nix files.

Thanks!

@dheijl
Copy link
Owner

dheijl commented Mar 8, 2024

A question: why the change

    let mut audio_output_device =
        get_default_audio_output_device().expect("No default audio device");

into

    let mut audio_output_device_opt = get_default_audio_output_device();

Are there situations where there is no default audio device?

@ein-shved
Copy link
Contributor Author

Of course. The device without built-in audio input device, but with plugged-in usb. Just like orpi zero2 in my pcm-chromecast project.

@dheijl
Copy link
Owner

dheijl commented Mar 8, 2024

Of course

A single comment line would have been useful then, unless you think everyone uses your set-up.

I'll change it to your code.

dheijl added a commit that referenced this pull request Mar 8, 2024
handle the case were there is no default output device (PR #118)
@ein-shved
Copy link
Contributor Author

A single comment line would have been useful then

Yes, you right, I missed that.

I put the local-ip-address crate behind a feature of the CLI

I think, it is bad approach to have different implementations for same funcional without real reason. But I don't insist.

@dheijl
Copy link
Owner

dheijl commented Mar 9, 2024

The Google DNS connection is in there because on Windows 10 with Hyper-V or multi-homed the Windows API tends to return a wrong default local network, so I'm leaving that in for the GUI. For the CLI it doesn't matter that much.

@dheijl
Copy link
Owner

dheijl commented Mar 9, 2024

I suppose this is now obsolete, so I'm closing it.

@dheijl dheijl closed this Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants