Fix what field-testing the control channel turned up - #10
Merged
chrisuthe merged 8 commits intoAug 11, 2026
Merged
Conversation
Item 7 shipped with "nothing here has been driven against a real Sendspin server" and a list of what that left unproven. It has been now, against a live aiosendspin/Music Assistant instance on the LAN, so the list is replaced by what was observed: play and pause moving real playback, mute round-tripping and coming back as state on both the group and the player line, seek moving the position, seek past the server's published seek_max_ms (231000, exactly the track duration) refused locally, the status block filled in from real metadata, status reading `unknown` in the window before the first metadata arrives, and the socket at the macOS default path with no flags at all. Two findings that only a live server could have produced. Position is meaningless behind a sink that does not pace itself. Under -o null it saturates at the track duration within seconds, and that is not a status bug: the null sink consumes instantly and leaves on_frames_played unset, so nothing paces the stream, the server sends the whole track as fast as it can, and status faithfully reports a server that believes the track has finished. Behind -o portaudio the position advances at 1x. Worth knowing before reading a position off a device-less run. And seek-rel is accepted and does nothing. The evidence that this is the server's end rather than ours: the local gate refuses anything absent from supported_commands and seek-rel exits 0, so the server advertises seek_relative; format_client_command_message() writes offset_ms for SEEK_RELATIVE symmetrically with the position_ms it writes for SEEK; and absolute seek through that same path demonstrably moves playback. Three offsets, including a negative one, no effect. Per AI_POLICY.md no issue was opened from here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reported by the human, who listened: status said `player volume: 0` while the
player was audibly at full output.
The line was faithful to PlayerRole::get_volume(), and get_volume() was the
wrong source. The role stores 0 until a server sends a volume command -- and
advertises that 0 in client/state -- while sink.set_volume() is reached only
from on_volume_changed(), so every sink sits at full until one arrives. A
player no server has spoken to therefore plays at full while telling everyone
it is at zero, and status repeated the claim to the one person in a position
to hear otherwise. Which is the worst way for a status line to be wrong: it
was not vague, it was confidently false.
status now reports the gain the sink is really applying. PlayerListener tracks
it, being the only caller of set_volume() and so the only thing that knows what
the sink was told, and marks it "(default; no server has set it)" so a server
that deliberately chose full output stays distinguishable from one that never
spoke. DEFAULT_SINK_VOLUME replaces the bare {100} the three sinks each spelled
for themselves, with the disagreement written down beside it.
ControlDispatcher no longer needs PlayerRole at all, so it no longer holds it.
The underlying incoherence is left for item 13, which already owns advertised
state that does not match reality: we tell the server 0 while playing at full,
so the first volume command a server sends is heard as a cut from full rather
than a rise from silence. Fixing that means either seeding the role from the
sink at startup or the sink from the role -- the first changes what we
advertise, the second makes a fresh player silent until a server speaks. Both
are wider than a control channel.
Verified against the live server: a fresh player now reads
`player volume: 100 (default; no server has set it)`, and once Music Assistant
sets it the qualifier drops and the sink really is attenuated. 253 tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Finishing the field test turned up a gap: the server publishes repeat and shuffle in the same controller object as the volume, and status showed neither. So this CLI shipped two subcommands whose effect nobody could see and whose original value nobody could read back to restore -- which is also what had made them untestable without changing a user's settings blind. Both are now reported, beside the group volume they arrive with, and `unknown` until the server has sent controller state at all -- the same trap the volume has, since a default-constructed object is off and unshuffled. They carry a caveat the rest of the block does not, and it is written down rather than glossed: ServerStateControllerObject holds them as a plain enum and a plain bool, and the parser assigns them only when the field is present, so a server that omits `repeat` leaves the struct's own OFF behind and nothing downstream can tell that from a server that said `off`. seek_max_ms in the same object is an optional and does not have the problem. Having just fixed one line that presented a default as a fact, this one says what it actually means. Field test against the live server, with our sink first silenced via `vol 0` so nothing was audible this time: `next` and `prev` walk a real queue in both directions. `seek-rel`, `repeat` and `shuffle` are all accepted -- so the server advertises all three in supported_commands -- and all three do nothing, while play, pause, next, prev, seek and mute travel the identical path and work. That is three commands advertised and not implemented at the far end, recorded for reporting upstream; no issue opened from here, per AI_POLICY.md. 255 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The human turned the volume up and listened, which is the only thing that could have caught this. An earlier pass concluded that seek-rel, repeat and shuffle were advertised by the server and unimplemented, on the evidence that nothing in status changed after sending them. That was wrong, and wrong in a way worth naming: the spec does not oblige a server to republish state after acting, so absence of feedback is not absence of action. I had built a conclusion on a display and never checked it against the world. Driven again with sound: a relative seek back to a track's opening was audibly indistinguishable from the absolute seek used as a control. shuffle proven behaviourally -- off walked the album in order, on produced tracks from four different albums. repeat one held a track across its boundary instead of advancing. Every command in the surface works. What is actually missing is the server's reporting, and status now says so rather than presenting a stale cache as fact: - position is marked (estimated) while playing, because the library interpolates forward from the last progress the server sent -- so a seek the server does not re-report leaves the anchor stale and the figure drifts by however far the jump moved. That is exactly what happened: absolute and relative seeks both moved the audio while the number climbed straight through them. Paused, it is the server's own snapshot and carries no marker. - one note: line names state, position, repeat and shuffle as the server's word, because the misreading it prevents is the one that produced the wrong conclusion above. One line rather than a qualifier per field, so the block stays scannable and every line stays key: value. ROADMAP and README are corrected in place; there is nothing owed upstream after all. Whether a server should resend progress after a seek is a question for the spec, not a defect in either end. 257 tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The remainder of the surface, driven against the live server with the volume up and a human listening. vol and mute confirmed by ear -- alternating 12/50 twice with a mute between, loudness tracking the value and mute silencing it. Done audibly on purpose: the reported number is exactly what had been lying earlier, so confirming it against itself would have proved nothing. stop differs from pause in a way only the stream line shows. Both leave state: paused, but pause keeps stream: receiving while stop drops it to idle -- the server tears the stream down. That is the clearest justification yet for reporting the group's transport state and this endpoint's stream state as two separate lines: without the second, pause and stop are indistinguishable. switch acts, visible in the log as Stream ended followed by Stream started and a fresh codec header. With one group available the cycle returns where it began, so nothing else changes -- which is what the spec's switch cycle should do. And the three failure modes were exercised against real daemons rather than only unit-tested: all twelve transport commands return 4 against a daemon that is up and has never reached a server, each naming the connection rather than the command, while status on that same daemon returns 0 and prints what it knows locally -- the whole point of exempting it. 3 against a socket with no daemon. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading Sendspin/spec settled the question I had parked for item 13, and not in
the direction I expected: this was never a behaviour change to weigh up, it was a
spec violation with a visible symptom we had already been staring at.
- client/state's player `volume` MUST be included when a player advertises the
`volume` command, which this one does.
- "Group volume is the average of the volumes of players in the group that
support the `volume` command." Group volume is *derived from us*. So the
`group volume: 0` that confused this entire field test was our own misreport
averaged straight back to us -- not a server oddity, ours.
- Setting group volume works off "delta = requested_volume -
current_group_volume", so a wrong figure mis-applies every later group volume
change by exactly the error. A player claiming 0 while playing at full hears
a request for 30 as a cut from full rather than a rise from silence, which is
the symptom I had predicted without knowing the mechanism.
- "A server MUST NOT assume these values are unchanged after a reconnect" --
the player is the authority and the server depends on being told.
So main.cpp reports the sink's real gain before anything can connect.
update_volume() rather than reaching for the sink, because it does not invoke
on_volume_changed() -- that fires only for server-initiated changes -- so status
still tells a default apart from a volume a server chose.
Confirmed against the live server: group volume now reads 100 for a single player
at 100 where it read 0 before, and a following `vol 30` lands at a real 30 with
the default qualifier correctly gone.
Two deviations found in the same reading are left for item 13, now known to be
deviations rather than open questions: persisting volume and muted across
restarts is the spec's RECOMMENDED and needs item 8's store, and the software
taper is (volume/100)^2 where the spec says (volume/100)^1.5 -- roughly 3 dB
quiet at volume 50, 6 dB at 25, inherited from upstream and shared by both sinks.
The spec also says volume changes SHOULD be ramped, and nothing here ramps.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The entry had grown to 303 lines by accretion across four rounds of review and field testing, and it read like a log of how it was built: the volume story told in three places, position staleness in three, `stream`-versus-format in two, two overlapping "what was exercised" sections, and a running narrative of conclusions I had reached and then retracted. That last part is the one the repo already has a rule about -- describe the current state, not its history; the history belongs in the PR description, where it is. So the retraction narrative is gone while every fact it established stays: the commands work, the server does not always republish state, and an unchanged figure is not evidence a command did nothing. Restructured to the shape items 1-6 use: what was wrong, **Shipped** in <files> with one bullet per decision, three correctness points that are silent if got wrong, what is not in the slice, and a single consolidated account of what has and has not been exercised. 236 lines, every technical fact retained and each stated once -- checked by grepping for all nineteen of them. Two things gained rather than trimmed. The `status` bullet now collects all four "report what is true, mark what is not" decisions together, including the `repeat`/`shuffle` display that was previously only mentioned in passing. And item 13 now carries the three volume deviations item 7 handed it -- the `^1.5` taper, the missing ramp, and persistence -- with what each one costs, rather than leaving them findable only from item 7's deferral list. Also corrects the test count, which the entry had outrun again: 110 new, 147 to 257. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`q32_gain_for()` computed `(volume/100)^2`, inherited from upstream's `PortAudioSink::update_volume_multiplier_()`. The spec says `amplitude = (volume / 100)^1.5`, and that is not a taste call: it defines a volume as *perceived loudness* rather than amplitude -- "volume 50 should be perceived as half as loud as volume 100" -- and ^1.5 is the mapping that makes the number on a controller's slider mean that. This is audible for every existing user, and only in one direction: unity is unchanged and everything below it gets louder. vol old(^2) new(^1.5) change 10 0.01000 0.03162 +10.00 dB 25 0.06250 0.12500 +6.02 dB 50 0.25000 0.35355 +3.01 dB 75 0.56250 0.64952 +1.25 dB 100 1.00000 1.00000 0.00 dB Computed in floating point, because it runs once per volume change -- each sink caches the multiplier and apply_volume() is the only thing in the hot path -- so there is nothing to win by approximating a fractional power in integers and a good deal of clarity to lose. apply_volume() stays integer Q32 throughout. Pinned on the two volumes where the curve is exact rather than on rounded decimals: (1/4)^1.5 is exactly 1/8 and (1/25)^1.5 is exactly 1/125, so volume 25 and volume 4 land on round fractions of unity and either drifting means the curve changed. Plus a test asserting the perceptual property the exponent exists for -- inverting the curve recovers the volume ratio -- and one guarding the divergence from upstream so it cannot be tidied back. Also checked while here, and already correct: the spec makes `volume` and `muted` independent, "a volume change MUST NOT clear the mute state". Both setters store independently and each sink recomputes the multiplier from both, so a volume command arriving while muted stays muted. Item 13 keeps the two deviations that remain -- no ramp on volume changes, and no persistence of volume/muted across restarts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chrisuthe
marked this pull request as ready for review
August 11, 2026 15:25
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
1 participant
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.
Follow-up to #9, which merged while this was being driven against a real Music Assistant server. Five commits: two real bug fixes, one retraction, and the field-test record.
The theme: #9 shipped with 251 passing tests and three defects that only running it could find. Two of them were
statuspresenting a stale or defaulted cache as fact.statusclaimed a silent player you could hearThe line read
player volume: 0at a player audibly playing at full output. It was faithful toPlayerRole::get_volume(), and that was the wrong source:PlayerRole's stored volumeclient/statesink.set_volume()on_volume_changed()So an untouched player plays at full while telling everyone it is at zero.
statusnow reports the gain the sink is really applying, tracked inPlayerListener— the only caller ofset_volume(), so the only thing that knows what the sink was told — and marks it(default; no server has set it)so a server that deliberately chose full output stays distinguishable from one that never spoke.DEFAULT_SINK_VOLUMEreplaces the bare{100}the three sinks each spelled for themselves.The underlying incoherence is left for item 13, which already owns advertised state that does not match reality: we tell the server 0 while playing at full, so the first volume command a server sends is heard as a cut rather than a rise. Both candidate fixes (seed the role from the sink, or the sink from the role) change behaviour wider than a control channel.
A retraction, and the position field
#9's ROADMAP entry claimed
seek-rel,repeatandshufflewere advertised by the server and unimplemented. That was wrong. All three work. The evidence for the claim was that nothing instatuschanged after sending them — and the spec does not oblige a server to republish state after acting, so absence of feedback is not absence of action.Retested with the volume up and a human listening:
seek-rel— a relative jump to a track's opening was audibly indistinguishable from the absolute seek used as a controlshuffle— proven behaviourally: off walked the album in order, on produced tracks from four different albumsrepeat one— held a track across its boundary instead of advancingWhat is actually missing is the server's reporting, so
statusnow says so instead of presenting its cache as fact:positionis marked(estimated)while playing — the library interpolates forward from the last progress the server sent, so a seek the server does not re-report leaves the anchor stale and the figure drifts by however far the jump moved. Paused, it is the server's own snapshot and carries no marker.note:line namesstate,position,repeatandshuffleas the server's word. One line rather than a qualifier per field, so the block stays scannable and every line stayskey: value.repeatandshuffleare now reported at allThey arrive in the same controller object as the group volume and
statusshowed neither — so two subcommands had invisible effects and unreadable originals. Both reported now,unknownuntil the server sends controller state. They carry a caveat written down rather than glossed:ServerStateControllerObjectholds them as a plain enum and bool and the parser assigns them only when the field is present, so "server said off" and "server said nothing" are indistinguishable.seek_max_msin the same object is anoptionaland does not have the problem.The field-test record
13/13 subcommands confirmed acting against the live server. Highlights worth keeping:
stopdiffers frompausein a way only thestreamline shows — both leavestate: paused, butpausekeepsstream: receivingwhilestopdrops it toidle. That is the clearest justification for reporting the group's transport state and this endpoint's stream state separately.switchacts, visible asStream ended→Stream startedwith a fresh codec header.4against a daemon up but never connected, each naming the connection rather than the command, whilestatuson that same daemon returns0and prints what it knows locally.3against a socket with no daemon.Verification
257 tests (up from 251), clean under
-DSENDSPIN_CLI_WERROR=ON, shellcheck clean,scripts/smoke_test.shgreen, and rebased onto currentmain.Nothing is owed upstream after all — #9's claim to the contrary is retracted in this PR. Whether a server should resend progress after a seek is a spec question rather than a defect at either end.