Merge pull request #29 from balloobbot/task/identity-flags

Add the identity flags: --id, --manufacturer, --product-name

Author: chrisuthe

README.md

@@ -963,13 +963,14 @@ environment rather than as arguments:
 | `SENDSPIN_SERVER_ID` | the connected server's id |
 | `SENDSPIN_SERVER_NAME` | its friendly name |
 | `SENDSPIN_SERVER_URL` | the URL this run dialled — set only on an `-s` run |
+| `SENDSPIN_CLIENT_ID` | this player's id, when `--id` chose one |
 | `SENDSPIN_CLIENT_NAME` | this player's friendly name (`-n`) |
 
 The vocabulary is the Python `sendspin-cli`'s, deliberately: a hook script written
-against one player runs unchanged against the other. `SENDSPIN_CLIENT_ID` is part
-of that vocabulary and reserved here, but nothing arrives in it yet: the library
-derives this player's id from the interface MAC and does not expose it, so the
-honest value waits on a flag that chooses one. A variable whose value is
+against one player runs unchanged against the other. `SENDSPIN_CLIENT_ID` carries the id
+`--id` or the `id` config key chose: the library derives one from the interface
+MAC when neither did, and does not expose what it derived, so a run that
+configured no id leaves the variable unset. A variable whose value is
 unknown for the event is left unset rather than exported empty, so `[ -n
 "$SENDSPIN_SERVER_ID" ]` means what it says — and any `SENDSPIN_*` inherited from
 the player's own environment is cleared first, so a wrapper script's stale export
@@ -1082,14 +1083,21 @@ which is what a foreground run whose terminal has just closed should do.
 The flags follow squeezelite's: `-o` output device, `-l` list devices, `-n` name,
 `-s` server, `-z` daemonize, `-P` pidfile, `-d`/`-f` logging. All but `-l` and
 `-z` also have a long spelling — `--output --name --server --pidfile --logfile
---log-level` — so that every config key is a flag name. Eleven more are long-only
+--log-level` — so that every config key is a flag name. Fourteen more are long-only
 because they are not squeezelite's: `--port`, the port this player serves on,
 `--buffer-ms`, `--static-delay`, the two mDNS flags `--no-mdns` and `--mdns-name`,
 the two control-socket flags `--control-socket` and `--no-control`, the two stream
-hooks `--hook-start` and `--hook-stop`, and `--config` and `--state-dir` for the
-two files above. Run `--help` for the current state of each
-— a few still point at [`docs/ROADMAP.md`](docs/ROADMAP.md) for behaviour that is
-not built yet.
+hooks `--hook-start` and `--hook-stop`, the three identity flags `--id`,
+`--manufacturer` and `--product-name`, and `--config` and `--state-dir` for the
+two files above. `--id` is the one to know about: it is the *stable* id a server
+files this player's volume, group and pairing under, and without it the id is
+derived from the network interface MAC — so two players on one host share it, and
+each server-side setting lands on whichever connected last. A dual-mono pair needs
+its own `--id` per instance, and its own `--port` and `--state-dir` with it — the
+state file is namespaced by neither `--id` nor `--port`, so without `--state-dir`
+the two overwrite each other's volume, mute, delay and last server. Run `--help` for the current
+state of each — a few still point at [`docs/ROADMAP.md`](docs/ROADMAP.md) for
+behaviour that is not built yet.
 
 A **subcommand comes first**, before any flag: `sendspin-cli vol 50 --port 9000`,
 not `sendspin-cli --port 9000 vol 50`. That is not getopt permutation showing
@@ -1248,8 +1256,9 @@ mode `0600`, so a player that loses power mid-write leaves either the old file o
 the new one and never half of either.
 
 Two players on one host share this file unless you give each its own
-`--state-dir`. They already need different `--port`s; give them different state
-directories too, or the second one to save its volume overwrites the first's.
+`--state-dir`. They already need different `--id`s and `--port`s; give them
+different state directories too, or the second one to save its volume overwrites
+the first's.
 
 ## Tests
 

docs/ROADMAP.md

@@ -2098,9 +2098,9 @@ no workaround either — the control socket answers questions, it does not annou
   always; `SENDSPIN_SERVER_ID`, `SENDSPIN_SERVER_NAME`, `SENDSPIN_SERVER_URL` (the URL
   this run dialled — outbound only, as in Python) and `SENDSPIN_CLIENT_NAME` where
   known. A hook script written against one player runs unchanged against the other.
-  `SENDSPIN_CLIENT_ID` is reserved but never set yet: the library derives its id from
-  the interface MAC and does not expose it, so the honest value arrives with a future
-  `--id` flag. `SENDSPIN_SERVER_URL` is what this run *dialled*, which is not the same
+  `SENDSPIN_CLIENT_ID` carries the `--id` value item 23 chooses: the library's
+  MAC-derived default is not exposed, so without the flag it stays unset.
+  `SENDSPIN_SERVER_URL` is what this run *dialled*, which is not the same
   claim as which server answered: `-s` leaves the inbound listener up, and the library
   reports that a connection is up without saying where it came from — no connect callback,
   and nothing exposing a connection's URL or direction — so one that dialled in while an
@@ -2140,3 +2140,32 @@ no workaround either — the control socket answers questions, it does not annou
   reach into. SIGPIPE goes back to `SIG_DFL` for the same reason: the player ignores it, and
   an ignored disposition survives `execve()` where a caught one does not, so `… | head -1`
   inside a hook would otherwise report a failed write rather than ending.
+
+
+### 23. Identity flags (`--id`, `--manufacturer`, `--product-name`) — *shipped*
+
+The client id was the library's MAC-derived default with no way to choose one, and the
+`client/hello` device info was hardcoded. The id is the half that bites: it is the
+*stable* identity a server files volume, group membership and pairing under — `-n` is
+only what it displays — and two players on one host derive the same MAC, so each
+server-side setting lands on whichever connected last. The dual-mono
+two-daemons-one-host pattern needs a flag; the Python CLI has all three.
+
+**Shipped** in `src/cli.{h,cpp}`, `src/main.cpp`, `tests/cli_test.cpp` and
+`tests/config_file_test.cpp`:
+
+- **`--id <id>`** sets `SendspinClientConfig::client_id`, and its config key is `id`.
+  The default stays empty on purpose — the library derives the MAC-based id only when
+  nothing is set, and that remains the right identity for one fixed endpoint per host.
+  The flag also feeds item 22's `SENDSPIN_CLIENT_ID`, which until now was reserved but
+  never set: the derived id is not exposed by the library, so the flag's value is the
+  first honest one a hook can be handed.
+- **`--manufacturer` / `--product-name`** override the hello device info, for the
+  integrator whose product embeds this player and should be listed as itself. Defaults
+  stay `sendspin-cpp-cli` / `sendspin-cli`, declaring what this really is.
+- All three are refused empty, warned about on a subcommand run like every other
+  daemon-only flag, and settable from the config file through the same
+  `apply_option()` door.
+
+Verified against a real `aiosendspin` server: the connected client reports the `--id`
+value as its `client_id`, and the start hook exports it as `SENDSPIN_CLIENT_ID`.

docs/wiki/Configuration.md

@@ -49,6 +49,9 @@ already does that.
 |---|---|---|---|
 | `output` | `-o`, `--output` | a device: `hw:1,0`, `default`, `portaudio:2`, `null`, `stdout` | `default` where ALSA is built in, else `portaudio`, else `null` |
 | `name` | `-n`, `--name` | the friendly name a controller shows | this host's name |
+| `id` | `--id` | the stable client id a server files this player's settings under — two players on one host must not share it | derived from the interface MAC |
+| `manufacturer` | `--manufacturer` | the manufacturer `client/hello` reports | `sendspin-cpp-cli` |
+| `product-name` | `--product-name` | the product name `client/hello` reports | `sendspin-cli` |
 | `server` | `-s`, `--server` | `<host>[:<port>]`, a `ws://` URL, or `mdns:[<name>]` | none — wait to be discovered |
 | `port` | `--port` | the port this player's own WebSocket server listens on | `8928` |
 | `buffer-ms` | `--buffer-ms` | audio the output backend keeps queued, 10–2000 | `100` |
@@ -173,8 +176,8 @@ Writes go through a temporary, an `fsync` and a `rename` at mode `0600`, so a pl
 loses power mid-write leaves either the old file or the new one and never half of either.
 
 **Two players on one host share this file** unless you give each its own `--state-dir`. They
-already need different `--port`s; give them different state directories too, or the second
-one to save its volume overwrites the first's.
+already need different `--id`s and `--port`s; give them different state directories too, or
+the second one to save its volume overwrites the first's.
 
 ### `static-delay` versus a remembered `static-delay-ms`
 

docs/wiki/Controlling-the-Player.md

@@ -202,11 +202,12 @@ The event's facts arrive in the environment, in the same vocabulary the Python
 `sendspin-cli` uses — a hook script written against one runs unchanged against the other:
 `SENDSPIN_EVENT` (`start` or `stop`) always, and `SENDSPIN_SERVER_ID`,
 `SENDSPIN_SERVER_NAME`, `SENDSPIN_SERVER_URL` (`-s` runs only, and the URL this run
-dialled rather than a statement about which server answered — see below) and
-`SENDSPIN_CLIENT_NAME` where known. An unknown is left *unset* rather than exported
-empty, so `[ -n "$SENDSPIN_SERVER_ID" ]` means what it says. A stop event carries the
-same server facts as the start it pairs with — gathered when the stream started, because
-a stream usually ends when its connection goes and there is nothing left to ask by then.
+dialled rather than a statement about which server answered — see below),
+`SENDSPIN_CLIENT_ID` (when `--id` chose one) and `SENDSPIN_CLIENT_NAME` where known. An
+unknown is left *unset* rather than exported empty, so `[ -n "$SENDSPIN_SERVER_ID" ]`
+means what it says. A stop event carries the same server facts as the start it pairs
+with — gathered when the stream started, because a stream usually ends when its
+connection goes and there is nothing left to ask by then.
 
 `SENDSPIN_SERVER_URL` is the one to read carefully: it is the URL this run dialled, not
 which server answered. `-s` leaves the inbound listener up, so a server that dials *in*

docs/wiki/Getting-Started-on-a-Raspberry-Pi.md

@@ -150,8 +150,9 @@ nothing for a player running as you.
 - **`avahi-daemon` is what provides mDNS on a Pi**, and Raspberry Pi OS ships it running. If
   you have turned it off, the player warns and retries rather than failing — but nothing will
   discover it until it is back.
-- **One player per Pi.** Two on one host need different `--port`, different `--state-dir`
-  and different control sockets. It works; it is just not what a Pi is usually for.
+- **One player per Pi.** Two on one host need different `--id`, different `--port`,
+  different `--state-dir` and different control sockets. It works; it is just not what a
+  Pi is usually for.
 
 ## Next
 

docs/wiki/Troubleshooting.md

@@ -266,9 +266,10 @@ systemctl status sendspin-cli
 pgrep -a sendspin-cli
 ```
 
-To run a second player on purpose, give it its own `--port`, its own `--control-socket` and
-its own `--state-dir` — they share the state file otherwise, and the second to save its
-volume overwrites the first's.
+To run a second player on purpose, give it its own `--id`, its own `--port`, its own
+`--control-socket` and its own `--state-dir` — they share the state file otherwise, the
+server files both under one identity, and the second to save its volume overwrites the
+first's.
 
 ## `status` is telling me something odd
 

packaging/sendspin-cli.conf.example

@@ -20,12 +20,23 @@
 # here at all: -l, -z, --config, --help and --version are refused as unknown keys.
 #
 # Every line below is commented out, since a line that is not is a value chosen. Where an
-# option has a default, that is the value shown; `name` and `server` have none worth
-# printing, so those two are examples instead.
+# option has a default, that is the value shown; the keys with none worth printing --
+# `name`, `id`, `server` and the two hooks -- carry an example instead.
 
 # The name a controller shows for this endpoint. Defaults to this host's name.
 #name = kitchen
 
+# The stable client id a server files this player's volume, group and pairing under --
+# the name above is only what it displays. Defaults to an id derived from the network
+# interface MAC, which two players on one host would share: give each its own id (and
+# its own port and state-dir).
+#id = kitchen-left
+
+# The device info client/hello carries, shown in server device lists. For a product
+# that embeds this player and should be listed as itself.
+#manufacturer = sendspin-cpp-cli
+#product-name = sendspin-cli
+
 # The output device: a reserved name (null, stdout, -) or a <backend>:<device> pair.
 # `sendspin-cli -l` lists what this host has and what each device accepts. The default is
 # ALSA's own `default` PCM wherever this build has ALSA, which follows whatever the host is
@@ -53,8 +64,9 @@
 # Shell commands run when a stream starts and when it stops -- an amplifier relay, a
 # light. `/bin/sh -c`, with the event's facts in SENDSPIN_* environment variables:
 # SENDSPIN_EVENT (start|stop) always, and SENDSPIN_SERVER_ID, SENDSPIN_SERVER_NAME,
-# SENDSPIN_SERVER_URL and SENDSPIN_CLIENT_NAME where known. The hook runs without
-# blocking playback, its output goes to the log, and a non-zero exit is a warning.
+# SENDSPIN_SERVER_URL, SENDSPIN_CLIENT_ID and SENDSPIN_CLIENT_NAME where known. The hook
+# runs without blocking playback, its output goes to the log, and a non-zero exit is a
+# warning.
 #hook-start = amixer set Master unmute
 #hook-stop = amixer set Master mute
 

src/cli.cpp

@@ -70,6 +70,9 @@ enum LongOnly {
     OPT_CONFIG,
     OPT_HOOK_START,
     OPT_HOOK_STOP,
+    OPT_ID,
+    OPT_MANUFACTURER,
+    OPT_PRODUCT_NAME,
 };
 
 /// @brief An option a config file may set: its key, and how a diagnostic names it.
@@ -108,6 +111,9 @@ const std::vector<SettableOption>& settable_options() {
         {Opt::StateDir, "state-dir", "--state-dir"},
         {Opt::HookStart, "hook-start", "--hook-start"},
         {Opt::HookStop, "hook-stop", "--hook-stop"},
+        {Opt::ClientId, "id", "--id"},
+        {Opt::Manufacturer, "manufacturer", "--manufacturer"},
+        {Opt::ProductName, "product-name", "--product-name"},
     };
     return table;
 }
@@ -387,6 +393,24 @@ bool apply_option(const SettableOption& option, const std::string& value, Option
             }
             out.hook_stop = value;
             break;
+        case Opt::ClientId:
+            if (empty_value()) {
+                return false;
+            }
+            out.client_id = value;
+            break;
+        case Opt::Manufacturer:
+            if (empty_value()) {
+                return false;
+            }
+            out.manufacturer = value;
+            break;
+        case Opt::ProductName:
+            if (empty_value()) {
+                return false;
+            }
+            out.product_name = value;
+            break;
         case Opt::NoMdns:
             if (!parse_bool(value, out.no_mdns)) {
                 error = "invalid --no-mdns '" + value + "' -- expected true or false";
@@ -581,6 +605,9 @@ bool parse_options(int argc, char* argv[], Options& out, std::FILE* err) {
         {"state-dir", required_argument, nullptr, OPT_STATE_DIR},
         {"hook-start", required_argument, nullptr, OPT_HOOK_START},
         {"hook-stop", required_argument, nullptr, OPT_HOOK_STOP},
+        {"id", required_argument, nullptr, OPT_ID},
+        {"manufacturer", required_argument, nullptr, OPT_MANUFACTURER},
+        {"product-name", required_argument, nullptr, OPT_PRODUCT_NAME},
         {nullptr, 0, nullptr, 0},
     };
 
@@ -741,6 +768,15 @@ bool parse_options(int argc, char* argv[], Options& out, std::FILE* err) {
             case OPT_HOOK_STOP:
                 apply(Opt::HookStop, optarg);
                 break;
+            case OPT_ID:
+                apply(Opt::ClientId, optarg);
+                break;
+            case OPT_MANUFACTURER:
+                apply(Opt::Manufacturer, optarg);
+                break;
+            case OPT_PRODUCT_NAME:
+                apply(Opt::ProductName, optarg);
+                break;
             case ':':
                 fail("option '" + offending_option(flag_argv, optind) + "' needs a value");
                 break;
@@ -908,9 +944,10 @@ bool parse_options(int argc, char* argv[], Options& out, std::FILE* err) {
         // listen on anything. Left it out and it would silently produce a "this player was
         // started with --no-control" message about the wrong process.
         static constexpr Opt DAEMON_ONLY[] = {
-            Opt::Device,   Opt::Name,     Opt::Server,   Opt::Daemonize, Opt::Pidfile,
-            Opt::Logfile,  Opt::LogLevel, Opt::BufferMs, Opt::NoMdns,    Opt::MdnsName,
-            Opt::NoControl, Opt::StateDir, Opt::StaticDelay, Opt::HookStart, Opt::HookStop,
+            Opt::Device,    Opt::Name,         Opt::Server,      Opt::Daemonize, Opt::Pidfile,
+            Opt::Logfile,   Opt::LogLevel,     Opt::BufferMs,    Opt::NoMdns,    Opt::MdnsName,
+            Opt::NoControl, Opt::StateDir,     Opt::StaticDelay, Opt::HookStart, Opt::HookStop,
+            Opt::ClientId,  Opt::Manufacturer, Opt::ProductName,
         };
         for (Opt opt : DAEMON_ONLY) {
             if (out.was_given(opt)) {
@@ -1018,6 +1055,12 @@ void print_usage(std::FILE* out, const char* prog) {
     std::fprintf(out, "  -l            List output devices with their capabilities, and exit\n");
     std::fprintf(out, "  -n, --name <name>\n");
     std::fprintf(out, "                Friendly name (default: this host's name)\n");
+    std::fprintf(out, "  --id <id>     Stable client id, which is what a server files this\n");
+    std::fprintf(out, "                player's volume, group and pairing under -- -n is only\n");
+    std::fprintf(out, "                what it displays. Defaults to an id derived from the\n");
+    std::fprintf(out, "                network interface MAC, which two players on one host\n");
+    std::fprintf(out, "                would share: give each its own --id (and its own\n");
+    std::fprintf(out, "                --port and --state-dir)\n");
     std::fprintf(out, "  -s, --server <server>\n");
     std::fprintf(out, "                Connect out to <host>[:<port>] or a ws:// URL\n");
     std::fprintf(out, "                (the server's port defaults to %u), retrying until it\n",
@@ -1112,14 +1155,20 @@ void print_usage(std::FILE* out, const char* prog) {
     std::fprintf(out, "                unit has neither, so pair StateDirectory= with this\n");
     std::fprintf(out, "                flag; with none of the three the player still runs and\n");
     std::fprintf(out, "                simply remembers nothing\n");
+    std::fprintf(out, "  --manufacturer <text>\n");
+    std::fprintf(out, "  --product-name <text>\n");
+    std::fprintf(out, "                The device info client/hello carries, shown in server\n");
+    std::fprintf(out, "                device lists (defaults: sendspin-cpp-cli, sendspin-cli).\n");
+    std::fprintf(out, "                For a product that embeds this player and should be\n");
+    std::fprintf(out, "                listed as itself\n");
     std::fprintf(out, "  --hook-start <command>\n");
     std::fprintf(out, "  --hook-stop <command>\n");
     std::fprintf(out, "                Run <command> through /bin/sh when a stream starts or\n");
     std::fprintf(out, "                stops -- an amplifier relay, a light. The event's facts\n");
     std::fprintf(out, "                arrive as SENDSPIN_EVENT (start|stop) and, where known,\n");
     std::fprintf(out, "                SENDSPIN_SERVER_ID, SENDSPIN_SERVER_NAME,\n");
-    std::fprintf(out, "                SENDSPIN_SERVER_URL (outbound only) and\n");
-    std::fprintf(out, "                SENDSPIN_CLIENT_NAME. The hook runs without blocking\n");
+    std::fprintf(out, "                SENDSPIN_SERVER_URL (outbound only), SENDSPIN_CLIENT_ID\n");
+    std::fprintf(out, "                and SENDSPIN_CLIENT_NAME. The hook runs without blocking\n");
     std::fprintf(out, "                playback; its output goes to the log, and a non-zero\n");
     std::fprintf(out, "                exit is a warning, not a player failure\n");
     std::fprintf(out, "  -h, --help    Show this help\n");