Volume control logic could be improved!

[pavoni]

The roon api docs say:

 *     { "type": "db",    "min": -80, "max": 0,   "value": -50.5, "step": 0.5 }
 *     { "type": "db",    "min": -80, "max": 10,  "value": 4,     "step": 1.0 }
 *     { "type": "number" "min": 0,   "max": 100, "value": 80,    "step": 1.0 }
 *     { "type": "number" "min": 1,   "max": 99,  "value": 65,    "step": 1.0 }
 *     { "type": "incremental" }

The code in HA currently assumes that a number volume control is 0 to 100 and a db one is -80 to 0.

It should ask roon for the data before doing the calculation.

Probably best to add some utility conversion routines in the roon api.

Probably causing this issue: home-assistant/core#81138 (comment)

[juliusvaart]

Edit: sorry didn't notice this is the repo for the roon api interface!

Don't know all the code that should be changed for this but roon/media_player.py line 192 could be something like this maybe?

if volume_data["type"] == "db":

  # device volume range
  volume_range = volume_data["max"] - volume_data["min"] 

  # dB in one percent
  volume_percentage = 100 / volume_range

  # current dB value
  volume_db_level = volume_data["value"] 
  
  # current volume in percentage of range (convert dB to positive value)
  volume_percentage_level = volume_range - -volume_db_level  
  
  # volume percentage
  current_volume_level_percentage = volume_percentage * volume_percentage_level 

Example my case: -127dB - 0dB (range 127, current volume -56dB)
(100 / 127) * (127 - 56) = 55,91%

Example: -80dB - +10dB (range 90, current volume -56dB)
(100 / 80) * (80 - 56) = 37,78%

[pavoni]

That code looks sensible but there is also a similar problem here:

https://github.com/pavoni/pyroon/blob/master/roonapi/roonapi.py#L268

What probably makes sense is to move the volume calculation from media_player into roon api and then fix both issues in the library.

If you're a developer feel free to submit a roon_api PR - it's a lot easier working on the roon_api library than home assistant., and there are some python demo examples that you could adapt to check your code.

Very happy to help if you fancy having a go.

[juliusvaart]

I’m going to take a look next week! Thanks.

[juliusvaart]

So, tried to change te code but not really into Python. So not sure how to test...

[pavoni]

What I would do is to modify one of the demo apps.

Perhaps something that displays the volume of a zone, then reduces the volume, and then puts it back after a delay.

But happy to help - you can just open a pull request with what you have and I can take a look.

[juliusvaart]

Found out by running your demo.py that the Matrix Mini i3 reports the volume as 'type': 'number' instead of 'type': 'db'. So going to raise that issue with Roon first.

[pavoni]

Does roon remote show db on the scale?

Regardless it would be nice to make the roonapi handle this case if we can.

Sure there will be other examples!

[juliusvaart]

The missing dB scale is a bug. It doesn't show it in the remote. They passed it on to the dev-team.

[pavoni]

What's your current thinking on this?

I'm sure we could come up with a utility function that will work with the ranges (as long as they are correct) without needing to depend on the db setting being correct.

[juliusvaart]

It’s crazy that you mention this, because this morning i was thinking about it.

The example i gave above should work with every range because it converts the range to percentage.

edit:

Using the MIN and MAX volume from Roon API.

[juliusvaart]

Allright, so the following code works when changing the volume from Roon it updates the HA volume accordingly.

media_player.py from line 179

        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"]
            volume_current_level = volume_data["value"]

            volume_range = volume_max - volume_min
            volume_percentage_factor = 100 / volume_range

            if volume_current_level > 0:
              volume_percentage_level = volume_current_level * volume_percentage_factor
            else:
              volume_percentage_level = (volume_range + volume_current_level) * volume_percentage_factor

            volume_level = convert(volume_percentage_level, int, 0) / 100

I tested it with a Hifi Berry card (0-100 range) and the Matrix Mini i3 (-127,5 - 0 range):

Matrix Mini i3 Pro:

Hifiberry DAC+:

[juliusvaart]

The same logic should probably apply to the set_volume_level(), volume_up() and volume_down() functions for the other way round? (change from HA and update Roon)

[juliusvaart]

This works for me when changing the volume from HA on both devices:

set_volume_level()

    def set_volume_level(self, volume: float) -> None:
        """Send new volume_level to device."""
        volume = int(volume * 100)
        volume_data = self.player_data["volume"]
        volume_max = volume_data["max"]
        volume_min = volume_data["min"]
        volume_range = volume_max - volume_min
        volume_percentage_factor = volume_range / 100
        percentage_volume = volume_min + volume * volume_percentage_factor
        self._server.roonapi.change_volume(self.output_id, percentage_volume)

volume_up()

    def volume_up(self) -> None:
        """Send new volume_level to device."""
        volume_data = self.player_data["volume"]
        volume_max = volume_data["max"]
        volume_min = volume_data["min"]
        volume_range = volume_max - volume_min
        volume_percentage_factor = volume_range / 100
        volume_percentage_change = convert(3 * volume_percentage_factor, int, 0)
        self._server.roonapi.change_volume(self.output_id, volume_percentage_change, "relative")

volume_down()

    def volume_down(self) -> None:
        """Send new volume_level to device."""
        volume_data = self.player_data["volume"]
        volume_max = volume_data["max"]
        volume_min = volume_data["min"]
        volume_range = volume_max - volume_min
        volume_percentage_factor = volume_range / 100
        volume_percentage_change = convert(3 * volume_percentage_factor, int, 0)
        self._server.roonapi.change_volume(self.output_id, -volume_percentage_change, "relative")

[juliusvaart]

Sorry, this works in the Roon component in HA... so my solution is on the wrong repository!

[pavoni]

@doctorfree @Ramblurr

Do you have a view?

The question here is whether the 0-100 volume conversion should be in the library or outside.

My sense is to leave the current raw volume functions and add a second set - or some conversion functions.

The Roon wrapper in HA currently does this - but not always correctly for some endpoints!

[pavoni]

@juliusvaart no issues - wry easy for me to steal your hard work!

[Ramblurr]

My sense is to leave the current raw volume functions and add a second set - or some conversion function

This is good I think. Expose the raw control and some convenience functions

[juliusvaart]

wry easy for me to steal your hard work!

Haha, please do. I'm to lazy to create a pull request!

And wouldn't label this "hard work" ;-)

[juliusvaart]

• { "type": "incremental" }

Only don't know what this is, how this works. No min/max here... so an exception could be needed here to avoid errors in HA.

[pavoni]

@juliusvaart Having looked at this more carefully, it will need to be a breaking change, because the current pyroon change_volume method does a conversion if the endpoint uses db, which we don't want with your revised logic.

So my current plan is to do a release of pyroon which will contain the new socket library, and the revised play path parsing.

I can then do an HA library update - which is straightforward as it requires no code changes to HA.

I will then look at the volume logic - and revise the library and HA wrapper.

The HA guys prefer PRs that don't contain multiple changes - since they are easier to review.

[juliusvaart]

Sounds great! Thanks for all your work on the component and library!

[pavoni]

HA PR now here:-

home-assistant/core#84916