Cap the level a session starts at when the slider is routed - #52
Conversation
With hardware_volume=1 the component passes the slider level as an explicit --volume, and choose_start_volume returns an explicit level before it ever looks at max_start_volume. The safe start therefore existed, was passed on the paths that did not need it, and was skipped on the one that did: a slider left high after an evening of listening became the level the next track opened at, on material whose loudness nobody knew yet. start_volume_max caps the first helper of a session and lifts as soon as one reports PLAYING, so a seek cannot turn down a level chosen mid session. It is not a volume limit - the slider still governs everything after the start and reaches the speaker in about a second. The slider now also follows the level the helper reports, because a cap that leaves the slider pointing somewhere the speaker is not just defers the jump to the first touch of it. The mapping itself is exonerated. Measured by ear on the M5 over a slider mapped onto 0..10: 1 inaudible, 3 a little more, 5 distinct, 6 cuts through conversation, 7 comfortable. No cliff in it. Reaching 7 by dragging is fine; reaching it cold is not, so this caps the starting point rather than bending the curve.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🐤 Kanarek · 🟣 merged
Documentation, Foobar, Tests · 4 files |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Triage of the three open PRs, not a new finding on this one: the slider-sync race Devin flagged is the blocker here. A second 55001 client during active playback is the topology Merge order matters: #48, #51 and #52 all branch from Generated by Claude Code |
Two real problems in the previous commit, both found in review. Moving the slider sends the level back out, and at PLAYING the control channel has not been announced yet - it arrives on the next line. The send therefore found no socket and fell back to launching a process, opening a second connection to 55001 while audio streams, which AGENTS.md calls out as able to starve the stream. The level is now held until CONTROL_PORT connects and applied there, over the socket the helper already holds. It is generation-checked too, so a PLAYING left in a retired helper pipe cannot move the slider for a helper being killed. The cap also only covered the routed-slider branch. A first helper with no slider reading and no configured volume fell through to no clamp at all and used the helper default of 10 - the exact loud start this setting exists to stop, arriving through the one branch with nothing watching it. That path now carries start_volume_max. A configured INI volume stays uncapped on purpose: it is a choice somebody made, where a slider position is leftover state. Also: a malformed volume= no longer parses as 0, which is a level and would have silenced the speaker; volume_max reports an out-of-range value instead of falling back in silence, which is what the docs already claimed; and start_volume_max joins the key reference.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Four from review, all correct. The slider sync could still spawn a control process: connect_control_ channel can fail, and the sync ran regardless, so the level went out over a newly launched second connection to 55001 mid-stream. It now reports whether the socket came up and the sync waits on that. Disabling the cap made starts louder than before this branch existed. The disabled case passed the speaker maximum, overriding the helper own default clamp of 10; it now omits the argument, which is what the code did before. The cap also reached setups where its justification does not hold. With routing off the slider is a host-side gain that never touches the speaker, so "the slider governs everything after the start" is false and a capped start would be raisable only one menu press at a time. It is now gated on hardware_volume. And the sync is skipped when the reported step is above volume_max: the inverse mapping clamps to the ceiling, so syncing a higher level would write the ceiling back and turn the speaker down.
Found while answering the ordering question rather than raised by it. The level was cleared on CONTROL_PORT, so a helper that reached PLAYING and died before announcing its channel left it behind for the next helper to apply on its own CONTROL_PORT line. The generation check cannot catch that: by then the generation is legitimately current. It now resets with the rest of the per-helper state in start_child. Also corrects the guard comment, which claimed parity with the test set_protocol_state_if_current applies. It deliberately omits the flush check - that one exists so state cannot restart a clock for a stream being torn down, and a slider position is not clock state.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The cap held only until the first seek. A replacement helper reads the remembered slider step and passes it straight through, so a slider still sitting at last night level handed the speaker that level and undid the quiet start. The slider sync cannot carry this on its own - it is conditional on the control socket and on volume_max, and this has to hold whether or not it ran - so the capped level is now what gets remembered, rather than what the slider said.
The declaration claimed only the pipe-reading thread does, which stopped being true when start_child began clearing it. They still cannot overlap - start_child calls stop_child first and that joins the protocol thread - so no atomic is needed, but the reason is the join rather than a single owner.
The test asserted on the routed branch while calling itself unrouted, which would send the next reader looking for a bug that is not there. And the paragraph claiming nothing is ignored quietly did not mention volume_max or start_volume_max - volume_max being the one this PR stopped ignoring quietly.
|
Verified on the physical M5 before merging, with the build from e944eb1 installed (69/69 hashes matched, nothing outside the package).
One thing the run exposed that this PR does not fix and does not claim to. foobar’s volume slider is not linear in pixels: a small movement near the left edge went from −42 dB to −21.5 dB. Our mapping spreads 60 dB over 10 steps, so those 20 dB are three raw steps — measured earlier the same day as the difference between "a little more" and "loud enough to cut through conversation". The start is no longer a surprise; the first adjustment after it still can be. Tracked separately. |
1 participant
The owner has reported the speaker starting far too loud several times. Three explanations were tried and measured away today; this is the one that survived.
What it is
With
hardware_volume=1the component passes the slider level as an explicit--volume.choose_start_volumereturns an explicit level on its first line, before it ever looks atmax_start_volume. So the safe start exists, is passed on the paths that do not need it, and is skipped on the one that does — routing the slider silently disables the protection it looks like it respects.A slider left high after an evening of listening then becomes the level the next track opens at, on material whose loudness nobody knows yet.
What changes
start_volume_max, raw step0..30, default3,0disables. It caps the first helper of a playback session and lifts as soon as one reportsPLAYING, so a seek cannot turn down a level chosen mid-session. Not a volume limit: the slider governs everything after the start and reaches the speaker in about a second over the held connection.The component also moves the slider to the level the helper reports in
WAMBRIDGE PLAYING volume=<step>. Without that a capped start leaves the slider pointing at a level the speaker is not playing, and the first pixel of movement jumps there — the same surprise, deferred.What was measured, and how well
By ear on the M5, slider mapped onto
0..10, stepping up one at a time:No cliff in it — the mapping was never the problem. Reaching 7 by dragging sounds fine; reaching it cold is what made the owner jump. Hence a cap on the starting point rather than a different curve.
Weaknesses worth knowing: this is a listener judgement on live radio, and the owner flagged mid-run that one step coincided with a louder passage. Whether the M5 raw steps are even in dB is still unmeasured and the docs still say so. An instrumented rerun on a steady tone is planned.
Not verified
281 tests pass. No build has been installed, so nothing here has been heard yet — including whether starting at
3is pleasant in practice or merely safe.