Improve handling of roon media players with fixed and incremental volume #99819

pavoni · 2023-09-07T12:23:13Z

Proposed change

The current HA roon integration assumes that roon media players have the ability to set volume, and increment volume (up and down). However in fact roon has endpoints with fixed volume, and incremental only volume, as well as more conventional volume.

This PR does two things:

Set the supported features correctly based on the abilities of the roon endpoint
Fix bugs when using incremental only roon endpoints

Incremental endpoints are not common (which is why the bugs haven't been reported) - but will be used if #99684 is merged.

This PR should be merged regardless of the PR above (as it fixes bugs). But it is required to release the PR above.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

emontnemery · 2023-09-28T09:41:04Z

homeassistant/components/roon/media_player.py

+        self._attr_volume_fixed = True
+        self._attr_volume_incremental = False


Please give other names to these to not confuse with the _attr- used as shorthand for entity properties

emontnemery · 2023-09-28T09:41:59Z

homeassistant/components/roon/media_player.py

+        if self._attr_volume_fixed:
+            _LOGGER.error("Cannot change volume for %s, volume is fixed", self.name)
+            return


This check should be unnecessary, set_volume_level won't be called for an entity which does not set MediaPlayerEntityFeature.VOLUME_SET

emontnemery · 2023-09-28T09:42:22Z

homeassistant/components/roon/media_player.py

+        if self._attr_volume_fixed:
+            _LOGGER.error("Cannot change volume for %s, volume is fixed", self.name)
+            return


This check should be unnecessary, volume_up won't be called for an entity which does not set MediaPlayerEntityFeature.VOLUME_SET or MediaPlayerEntityFeature.VOLUME_STEP

emontnemery · 2023-09-28T09:42:38Z

homeassistant/components/roon/media_player.py

+        if self._attr_volume_fixed:
+            _LOGGER.error("Cannot change volume for %s, volume is fixed", self.name)
+            return


This check should be unnecessary, volume_down won't be called for an entity which does not set MediaPlayerEntityFeature.VOLUME_SET or MediaPlayerEntityFeature.VOLUME_STEP

home-assistant · 2023-09-28T09:42:45Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

pavoni · 2023-09-28T13:57:16Z

Thanks for your help @emontnemery - I did wonder if those checks were not required.

Now updated.

emontnemery · 2023-09-28T14:48:33Z

homeassistant/components/roon/media_player.py

+        self.volume_fixed = volume["fixed"]
+        self.volume_incremental = volume["incremental"]
+        if not self.volume_fixed:
+            self._attr_supported_features = (
+                self._attr_supported_features | MediaPlayerEntityFeature.VOLUME_STEP
+            )
+            if not self.volume_incremental:
+                self._attr_supported_features = (
+                    self._attr_supported_features | MediaPlayerEntityFeature.VOLUME_SET
+                )


Does this need to be done every time the state is updated, or can we do it once, preferably in __init__?

The media_player number and state are updated in an async way by roon, so this can change as endpoints are enabled and disabled (this can be as simple as powering on a USB dac - which will enable the connected endpoint).

The volume type can also be changed by the user in roon - so I think it is safest to leave this as updating on every update, since that way it will never be wrong!

emontnemery · 2023-09-28T14:49:02Z

homeassistant/components/roon/media_player.py

+        self.volume_fixed = True
+        self.volume_incremental = False


Make these _protected or __private:

Suggested change

self.volume_fixed = True

self.volume_incremental = False

self._volume_fixed = True

self._volume_incremental = False

emontnemery · 2023-09-28T14:52:52Z

homeassistant/components/roon/media_player.py

+        self.volume_fixed = volume["fixed"]
+        self.volume_incremental = volume["incremental"]


How does this work?
volume["fixed"] and volume["incremental"] is only set in the else clause of the try-except above, is it OK to access them unconditionally here?

emontnemery · 2023-09-28T14:55:59Z

homeassistant/components/roon/media_player.py

        try:
            volume_data = player_data["volume"]
-            volume_muted = volume_data["is_muted"]
-            volume_step = convert(volume_data["step"], int, 0)
-            volume_max = volume_data["max"]
-            volume_min = volume_data["min"]
-            raw_level = convert(volume_data["value"], float, 0)

-            volume_range = volume_max - volume_min
-            volume_percentage_factor = volume_range / 100
+            volume_fixed = False
+            volume_incremental = volume_data["type"] == "incremental"
+            volume_muted = volume_data.get("is_muted", False)

-            level = (raw_level - volume_min) / volume_percentage_factor
-            volume_level = convert(level, int, 0) / 100
+            try:


Two nested try-blocks, both catching KeyError is a bit hard to read IMHO.
Could you try to rewrite it? Maybe add helpers fixed_volume +incremental_volume which return True if certain keys are missing?

I tried to leave the previous code (which was quite carefully crafted) - but agree that this now isn't at all easy to understand.

I'll take another look!

pavoni · 2023-10-12T10:21:56Z

@emontnemery Have refactored the scary block. I think it's much better!

emontnemery

LGTM, thanks @pavoni 👍

pavoni · 2023-10-12T10:52:59Z

Thanks for your help on this @emontnemery

…ume (#99819)

Improve volume handling of fixed and incremental endpoints.

852969f

home-assistant bot added bugfix cla-signed integration: roon new-feature by-code-owner Quality Scale: No score labels Sep 7, 2023

pavoni mentioned this pull request Sep 7, 2023

Add Roon volume hooks #99684

Closed

20 tasks

emontnemery added this to the 2023.10.0 milestone Sep 28, 2023

emontnemery requested changes Sep 28, 2023

View reviewed changes

home-assistant bot marked this pull request as draft September 28, 2023 09:42

Update from review comments.

0fb2cc0

pavoni marked this pull request as ready for review September 28, 2023 13:56

home-assistant bot requested a review from emontnemery September 28, 2023 13:56

emontnemery requested changes Sep 28, 2023

View reviewed changes

home-assistant bot marked this pull request as draft September 28, 2023 14:56

frenck modified the milestones: 2023.10.0, 2023.10.1, 2023.10.2 Oct 4, 2023

pavoni added 2 commits October 12, 2023 11:08

Refactor volume parsing.

a68e76c

Fix review comments.

74d4cb0

pavoni marked this pull request as ready for review October 12, 2023 10:19

home-assistant bot requested a review from emontnemery October 12, 2023 10:19

emontnemery approved these changes Oct 12, 2023

View reviewed changes

frenck merged commit dcb3dc2 into dev Oct 12, 2023
23 checks passed

frenck deleted the roon_volume_improvements branch October 12, 2023 10:52

frenck added the cherry-picked label Oct 12, 2023

frenck pushed a commit that referenced this pull request Oct 12, 2023

Improve handling of roon media players with fixed and incremental vol…

c11dd58

…ume (#99819)

frenck mentioned this pull request Oct 12, 2023

2023.10.2 #101871

Merged

github-actions bot locked and limited conversation to collaborators Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve handling of roon media players with fixed and incremental volume #99819

Improve handling of roon media players with fixed and incremental volume #99819

pavoni commented Sep 7, 2023

emontnemery Sep 28, 2023

emontnemery Sep 28, 2023

emontnemery Sep 28, 2023

emontnemery Sep 28, 2023

home-assistant bot commented Sep 28, 2023

pavoni commented Sep 28, 2023

emontnemery Sep 28, 2023

pavoni Sep 28, 2023

emontnemery Sep 28, 2023

emontnemery Sep 28, 2023

emontnemery Sep 28, 2023

pavoni Sep 28, 2023

pavoni commented Oct 12, 2023

emontnemery left a comment

pavoni commented Oct 12, 2023

		self._attr_volume_fixed = True
		self._attr_volume_incremental = False

		self.volume_fixed = volume["fixed"]
		self.volume_incremental = volume["incremental"]

Improve handling of roon media players with fixed and incremental volume #99819

Improve handling of roon media players with fixed and incremental volume #99819

Conversation

pavoni commented Sep 7, 2023

Proposed change

Type of change

Additional information

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

home-assistant bot commented Sep 28, 2023

pavoni commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavoni commented Oct 12, 2023

emontnemery left a comment

Choose a reason for hiding this comment

pavoni commented Oct 12, 2023