Skip to content

Support TAS58xx 'volume' capability for ALSA volume control - #228

Merged
balloob merged 4 commits into
mainfrom
fix/tas58xx-volume
Apr 15, 2026
Merged

Support TAS58xx 'volume' capability for ALSA volume control#228
balloob merged 4 commits into
mainfrom
fix/tas58xx-volume

Conversation

@balloob

@balloob balloob commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Detect volume (not just pvolume) in ALSA mixer capabilities, fixing hardware volume control for TAS58xx-based DAC HATs like the Sonocotta Louder Raspberry
  • Add parametrized tests covering mono (volume volume-joined) and stereo (volume) modes, plus discovery and get_volume flows
  • Fix test portability by monkeypatching AVAILABLE so discovery tests pass on non-Linux CI

Based on the work in #214 by @chaudis, with simplified tests.

Closes #214

Test plan

  • test_find_mixer_element_tas58xx[mono] — detects volume volume-joined
  • test_find_mixer_element_tas58xx[stereo] — detects volume
  • test_louder_raspberry_discovery — full device→card→element flow
  • test_louder_raspberry_get_volume — reads back correct volume percentage

🤖 Generated with Claude Code

The Sonocotta Louder Raspberry HAT (TI TAS5825M) reports 'volume' or
'volume volume-joined' instead of the standard 'pvolume' capability.
Detect both so these devices get working hardware volume control.

Closes #214

Co-Authored-By: chaudis <chaudis@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 15, 2026 01:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates ALSA mixer discovery to support TAS58xx-based DAC/amplifier HATs whose amixer sget output reports volume (and volume-joined) instead of pvolume, and extends the test suite to cover these cases and end-to-end discovery/get-volume flows.

Changes:

  • Extend _has_playback_volume() to treat volume as a valid playback-volume capability alongside pvolume.
  • Update ALSA volume discovery docs/comments to reflect the expanded capability handling.
  • Add TAS58xx/Sonocotta Louder Raspberry test cases (mono + stereo capability detection, discovery flow, get_state parsing).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
sendspin/alsa_volume.py Accept volume capability during mixer-element capability checks and update related docs/comments.
tests/test_alsa_volume.py Add TAS58xx-focused capability/discovery/get-volume tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/test_alsa_volume.py Outdated
Comment on lines +486 to +492
# -- TAS58xx / Sonocotta Louder Raspberry HAT --------------------------------
# The TAS58xx driver reports "volume" (or "volume volume-joined") instead of
# the standard "pvolume" capability.

import pytest

_TAS58XX_SCONTROLS = (

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import pytest is placed mid-file (after many test definitions). Ruff's E402/I001 rules typically require imports to be at the top of the module; this will likely fail linting. Move the pytest import into the main import block at the top of the file.

Copilot uses AI. Check for mistakes.
Comment thread tests/test_alsa_volume.py Outdated
Comment on lines +529 to +537
("digital_output", "desc"),
[
(_TAS58XX_SGET_DIGITAL_MONO, "mono (volume volume-joined)"),
(_TAS58XX_SGET_DIGITAL_STEREO, "stereo (volume)"),
],
ids=["mono", "stereo"],
)
def test_find_mixer_element_tas58xx(monkeypatch, digital_output: str, desc: str) -> None:
"""TAS58xx 'volume' capability is detected — {desc}."""

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parametrized test takes desc but never uses it, and the docstring contains {desc} without being an f-string. With Ruff enabled, this is likely to trigger an unused-argument lint error. Either drop the desc parameter (the parametrization ids already cover readability) or reference it in the test so it’s actually used.

Suggested change
("digital_output", "desc"),
[
(_TAS58XX_SGET_DIGITAL_MONO, "mono (volume volume-joined)"),
(_TAS58XX_SGET_DIGITAL_STEREO, "stereo (volume)"),
],
ids=["mono", "stereo"],
)
def test_find_mixer_element_tas58xx(monkeypatch, digital_output: str, desc: str) -> None:
"""TAS58xx 'volume' capability is detected — {desc}."""
"digital_output",
[
_TAS58XX_SGET_DIGITAL_MONO,
_TAS58XX_SGET_DIGITAL_STEREO,
],
ids=["mono", "stereo"],
)
def test_find_mixer_element_tas58xx(monkeypatch, digital_output: str) -> None:
"""TAS58xx 'volume' capability is detected."""

Copilot uses AI. Check for mistakes.
Comment thread tests/test_alsa_volume.py
Comment on lines +549 to +552
async def exercise() -> tuple[int, str] | None:
monkeypatch.setattr(_alsa_mod, "AVAILABLE", True)
monkeypatch.setattr(asyncio, "create_subprocess_exec", _tas58xx_exec(_TAS58XX_SGET_DIGITAL_MONO))
device = SimpleNamespace(

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test monkeypatches _alsa_mod.AVAILABLE = True to make the discovery path runnable off-Linux. However, other tests in this module also call async_check_alsa_available(...) expecting a non-None result and don’t patch AVAILABLE, so the suite can still fail on non-Linux CI. Consider a shared fixture (or module-level setup) that forces AVAILABLE=True for all tests that exercise ALSA discovery logic.

Copilot uses AI. Check for mistakes.
Comment thread sendspin/alsa_volume.py Outdated
Comment on lines 134 to 135
# Fallback: first element with playback volume (e.g. USB DACs with non-standard names).
selected = pvolume_elements[0]

Copilot AI Apr 15, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pvolume_elements now collects elements with either pvolume or volume capability, so the name is misleading. Renaming it (and related variables like selected) to something like volume_elements would better reflect the broadened behavior and reduce future confusion.

Copilot uses AI. Check for mistakes.
balloob and others added 3 commits April 14, 2026 21:52
- Move `import pytest` to top-of-file import block
- Drop unused `desc` parameter from parametrized test
- Rename `pvolume_elements` → `volume_elements` to reflect broadened capability check

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@balloob
balloob merged commit ae9dece into main Apr 15, 2026
1 check passed
@balloob
balloob deleted the fix/tas58xx-volume branch April 15, 2026 01:59
@balloob balloob added the bugfix Fixes a bug label Apr 15, 2026
selleronom pushed a commit to selleronom/sendspin-cli that referenced this pull request Apr 15, 2026
…#228)

## Summary
- Detect `volume` (not just `pvolume`) in ALSA mixer capabilities,
fixing hardware volume control for TAS58xx-based DAC HATs like the
Sonocotta Louder Raspberry
- Add parametrized tests covering mono (`volume volume-joined`) and
stereo (`volume`) modes, plus discovery and get_volume flows
- Fix test portability by monkeypatching `AVAILABLE` so discovery tests
pass on non-Linux CI

Based on the work in Sendspin#214 by @chaudis, with simplified tests.

Closes Sendspin#214

## Test plan
- [x] `test_find_mixer_element_tas58xx[mono]` — detects `volume
volume-joined`
- [x] `test_find_mixer_element_tas58xx[stereo]` — detects `volume`
- [x] `test_louder_raspberry_discovery` — full device→card→element flow
- [x] `test_louder_raspberry_get_volume` — reads back correct volume
percentage

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: chaudis <chaudis@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants