[codex] Add read-only WiiM API probes#22
Conversation
There was a problem hiding this comment.
PR Summary:
Adds read-only WiiM API probes to pywiim:
get_audio_input_capability()for thegetAudioInputCapbilityfirmware command- Graphic EQ (Eq10HP LV2 plugin) read helpers:
get_graphic_eq_bands(),get_graphic_eq_preset_list() - Room correction read helper:
get_room_correction() - Diagnostics and verify CLI coverage for all new probes
- Corresponding models, constants, docs, and unit tests
Review Summary:
The PR is well-structured — new methods are async, properly documented, follow the existing mixin pattern, and are backed by unit tests. The main concerns are around capability gating and how the diagnostics probe reports support status for the new EQ endpoints.
Two medium-severity issues stand out: (1) The probe() loop in _gather_discovered_api_info always marks Eq10HP, Eq10HPPresetList, and RoomCorrGet as "supported": True because those methods return non-None dataclasses/dicts even on default/empty responses — unlike the other probed APIs that return None when unsupported. (2) Both get_graphic_eq_bands() and get_room_correction() gate on supports_peq, but GEQ (Eq10HP) and room correction are distinct LV2 plugins with potentially independent availability; the project's own contributing checklist and established patterns require each new WiiM-specific feature to have its own capability flag and probe_capabilities() entry. Finally, the new methods are missing capability-check unit tests (a pattern clearly established by the existing test_get_peq_bands_capability_check).
Suggestions
- Add dedicated
supports_geqandsupports_room_correctioncapability flags with probing logic incapabilities.py, matching thesupports_peqpattern. Apply - Extract the shared EQ-header parsing block (
EQLevel,EQStat,channelMode,Name,source_name) into a_parse_eq_header()helper used by both_parse_graphic_eq_settingsand_parse_room_correction_settings. Apply
⚡ Apply all quick fixes (2 quick fixes)
| await probe("GetAcousticCapability", self.client.get_acoustic_capability) | ||
| await probe("getAllRoutines", self.client.get_all_routines) | ||
| await probe("getSoundCardModeSupportList", self.client.get_sound_card_mode_support_list) | ||
| await probe("Eq10HP", self.client.get_graphic_eq_bands) |
There was a problem hiding this comment.
The Eq10HP and RoomCorrGet probes (lines 227–229) will always report "supported": True because get_graphic_eq_bands() / get_room_correction() never return None — they always return a dataclass. After asdict() serialization, the resulting dict is truthy, so value is not None and value != [] always passes.
On devices where _require_peq() is a no-op (empty _capabilities dict, not yet detected), the request goes to the device, may get a default-value response, and the probe happily marks the feature as supported.
Contrast with get_audio_input_enable() / get_audio_input_capability() which explicitly return None for unsupported responses, making them work correctly with the existing probe "supported" logic.
Consider wrapping these calls to return None when the feature is unsupported:
async def _peq_or_none(func):
if not self.client.capabilities.get("supports_peq", False):
return None
return await func()
await probe("Eq10HP", lambda: _peq_or_none(self.client.get_graphic_eq_bands))
await probe("Eq10HPPresetList", lambda: _peq_or_none(self.client.get_graphic_eq_preset_list))
await probe("RoomCorrGet", lambda: _peq_or_none(self.client.get_room_correction))Or alternatively make get_graphic_eq_bands() / get_room_correction() return None when unsupported, matching the pattern of the other discovered-API helpers.
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| return [GraphicEQBand(name=name, gain=gains.get(name, 0.0)) for name in GEQ_BAND_NAMES] | ||
|
|
||
|
|
||
| def _parse_graphic_eq_settings(raw: dict[str, Any]) -> GraphicEQSettings: |
There was a problem hiding this comment.
_parse_graphic_eq_settings and _parse_room_correction_settings are nearly identical — same EQLevel parsing block, same EQStat/channelMode/Name/source_name extraction. The only differences are the return type and plugin_uri field.
Consider extracting the common header-parsing into a shared helper to avoid the parallel maintenance burden:
def _parse_eq_header(raw: dict[str, Any]) -> dict[str, Any]:
"""Parse common EQ header fields shared by PEQ, GEQ, and room correction responses."""
eq_level = raw.get("EQLevel")
try:
parsed_eq_level = int(eq_level) if eq_level is not None else None
except (TypeError, ValueError):
parsed_eq_level = None
return {
"source_name": str(raw.get("source_name", "")),
"enabled": str(raw.get("EQStat", "Off")).strip().lower() == "on",
"channel_mode": str(raw.get("channelMode", PEQ_CHANNEL_MODE_STEREO)),
"name": str(raw.get("Name", "")),
"eq_level": parsed_eq_level,
}Then each parser is a one-liner assembling the result type from those fields.
actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| "preset": list(data.get("preset", [])), | ||
| } | ||
|
|
||
| async def get_room_correction(self) -> RoomCorrectionSettings: |
There was a problem hiding this comment.
get_room_correction() gates on _require_peq(), which checks supports_peq. However, room correction (RoomCorrGet) is a distinct device block from PEQ — it could plausibly be available on devices where supports_peq is False (e.g. a WiiM device with room correction but no PEQ), or conversely be absent on some WiiM devices with PEQ.
Using the PEQ capability flag as the gate for a separate feature risks both false-positives and false-negatives. Consider either:
- Adding a dedicated
supports_room_correctioncapability flag (preferred, mirrors how PEQ itself is handled), or - Adding a defensive
try/except WiiMErrorguard insideget_room_correction()itself and returningNoneon failure, consistent with theget_audio_input_capability/get_acoustic_capabilitypattern for "discovered" optional APIs.
Same concern applies to get_graphic_eq_bands() — GEQ (Eq10HP) is a sibling plugin of PEQ (EqNp) but they are independent LV2 plugins that may not have the same availability.
actions
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| call_args = mock_client._request.call_args[0][0] | ||
| assert "EQv2GetList" in call_args | ||
| assert "Eq10HP" in call_args | ||
|
|
There was a problem hiding this comment.
The TestGetGraphicEqBands class is missing a capability-check test, unlike TestGetPeqBands which covers the supports_peq=False path. Same gap exists for get_room_correction() and get_graphic_eq_preset_list().
The existing test_get_peq_bands_capability_check shows the pattern:
@pytest.mark.asyncio
async def test_get_graphic_eq_bands_capability_check(self, mock_client):
"""get_graphic_eq_bands raises WiiMError when supports_peq=False."""
from pywiim.exceptions import WiiMError
mock_client._capabilities = {"supports_peq": False}
with pytest.raises(WiiMError, match="supports_peq"):
await mock_client.get_graphic_eq_bands()Without this test, a regression that removes self._require_peq() from these new methods would go undetected.
actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
Summary
Adds read-only WiiM daily-use and diagnostic API surfaces in pywiim:
get_audio_input_capability()for firmware commandgetAudioInputCapbilityWhy
These APIs came out of the WiiM app endpoint sweep and are useful for capability discovery, diagnostics, and future Home Assistant integration work. Write paths remain intentionally out of scope until behavior is validated more broadly.
Validation
python -m pytest tests/unit/api/test_misc.py tests/unit/api/test_peq.py tests/unit/cli/test_diagnostics_cli.py -qResult:
89 passed.