crowbarz / ha-pioneer_async

Home Assistant media_player component for Pioneer AVRs, rewritten in asyncio and supporting the UI config flow.
Apache License 2.0
29 stars 9 forks source link

Refactor media_player.volume_set for AVRs that don't support setting volume #4

Closed Varming73 closed 3 years ago

Varming73 commented 3 years ago

Hi - and thanks for your great work! I got a Pioneer receiver using your addon and it's mostly working great. I noticed that media_player.volume_set doesn't work on the player. This means I can't use a slider to control the volume or make it work with Google Assistant. Am I missing something?

crowbarz commented 3 years ago

What model Pioneer AVR do you have? It turns out that not all Pioneer AVRs are capable of setting the volume to a specific level via the serial API. If your model doesn't support it, then there is probably an error logged in the Home Assistant log for volume set command (VL for Zone 1)

To configure the integration to emulate volume set using volume up/down steps, enable "Emulate volume set by stepping volume up/down" (params > volume_step: true } in configuration.yaml) and set "Volume units changed" (params > volume_step_delta) appropriate for your unit.

Varming73 commented 3 years ago

I have the VSX-528. Both media_player.volume_down and media_player.volume_up works perfectly and it's only media_player.volume_set that doesn't work. I've tested with and without volume_step: true but it doesn't seem to do any diffence at all with my (old) receiver.

crowbarz commented 3 years ago

Can you enable debug logging on aiopioneer to show all the commands sent and responses received when volume_set is sent, with and without volume_step configured?

Varming73 commented 3 years ago

Sure - but I've never tried that before. How do I enable the logging and where do I find the logs?

Varming73 commented 3 years ago

Oh, I just found this: https://community.home-assistant.io/t/customizing-existing-component-pioneer-receivers/26369/2 where it says: "Volume set do not work on the VSX-528 so I use VOLUME_STEP instead"

I guess that explains it.

crowbarz commented 3 years ago

Can you share your configuration? Enabling params > volume_step should allow media_player.volume_set to function properly without the AVR needing to directly support volume set.

Varming73 commented 3 years ago

Sure thing, with params it looks like this:

- platform: pioneer_async
  name: "VSX-528"
  host: 192.168.1.69
  port: 8102
  params:
    volume_step: true
    volume_step_delta: 5
  sources:
    'CD': '01'
    'Tuner': '02'
    'DVD': '04'
    'TV': '05'
    'Sat/Cbl': '06'
    'DVR/BDR': '15'
    'iPod/USB': '17'
    'HDMI/MHL': '48'
    'BD': '25'
    'Adapter': '33'
    'Netradio': '38'
    'Media Server': '44'
    'Favorites': '45'
    'Game': '49'
crowbarz commented 3 years ago

I see the problem - the documentation was wrong, that param is actually called volume_step_only. Sorry about that, I've updated the documentation. Let me know if correcting the param name fixes the issue.

If you do get it working, then please let me know what volume_step_delta should be for model VSX-528 so I can add a profile for this model to set the default params appropriately.

crowbarz commented 3 years ago

Also, to answer an earlier question, enable debugging by adding the following to configuration.yaml and then restarting HA:

logger:
  logs:
    aiopioneer: debug # enable debug logging for aiopioneer package
    # custom_components.pioneer_async: debug # enable debug logging for integration

Debug logging shows you every command that is sent, and the response that is received back from the AVR.

crowbarz commented 3 years ago

Note: I edited the earlier post to correct the param to volume_step_only (removing an extra s).

Varming73 commented 3 years ago

You're right - that fixed it! I'm unsure how to find the right volume_step_delta but if I ask Google Assistent to go up 10% and then down 10% the value: 2 seems to come closest to get to the same value. Any other inputs for how to find the "right" delta?

... and thanks A LOT for helping me - and so fast even!

Varming73 commented 3 years ago

Input: In the documentation maybe it would be worth mentioning that a bigger delta is a smaller delta in actual volume.

I just tested a bit more and I think 1.8 would have been perfect - but with an integer that's not an option. So the profile for the VSX-528 should have a delta of 2. One is way too much and three is too small steps for it to be fairly accurate. This time I tested by moving a slider from 10 to 25 and see on the receiver where it goes. It hit 23 on the receiver but hit 10 when I put the slider to 10.

crowbarz commented 3 years ago

No problem, glad you find this integration useful! (I essentially wrote it out of angst with the existing integration.)

I'll put volume_step_delta of 2 for now for platform VSX-528, thanks for testing.

Also, I'll think about how to refactor this functionality to remove the need for a delta at all. One idea would be to keep hitting volume up/down and check the volume, stopping when desired volume is reached/exceeded. That is a bit more complicated because I would need to start a separate thread to perform a step then check it is at the right level (Edit: after a quick look at the code I may be able to do this in place, need to check whether cached volume attribute is already updated on return from volume_up/down possibly yielding to the attribute update thread), and deal with concurrency issues like starting another media_player.volume_set action when a stepping thread is already active. Let's leave this issue open to track that.

Varming73 commented 3 years ago

Your integration is great, it's the first I've tried that actually works and that don't bomb my receiver into not responding for hours. I was close to buying a different (newer) one - but it actually does what I need it to so your integration just made the old VSX-528 live longer in this house.

I really like your idea which would be a lot more elegant than the delta value - and a lot more precise. If you actually implement it I would of course be happy to help test (and pull logs if needed).

Varming73 commented 3 years ago

Just a short feedback: Everything works great and the VSX-528 works great with the integration. No more time-outs due to memoryleaks etc. I'm really happy with it and can now use my receiver for more years.

Your improvement would make it even greater and I can't complain about what is already here - thanks!!

crowbarz commented 3 years ago

Thanks @Varming73 for your feedback, and great to hear it's working well for you!

I'm in a similar situation as you with my VSX-930: as an AVR it works perfectly for me, but when I used the original HA Pioneer integration I had to also set up automation to power cycle it (via a smart plug) 15 mins after the system was turned off, as well as at 3am every day, just to keep it mostly stable and responding.

I'll be a little busy for the next while due to work commitments, but this improvement is on my list of things to fix/implement.

Varming73 commented 3 years ago

I just noticed the new version - anything you would like me to test?

crowbarz commented 3 years ago

Thanks for the offer to test :)

0.4 mainly fixed some issues with Zone 3, scan_interval and source discovery. I just released 0.5-pre1 based on the latest code, it bumps aiopioneer to 0.1.5 which has the updated volume step emulation code that is a better fix this issue.

Apparently you should be able to upgrade to beta versions via HACS if you select Show Beta, although I couldn't see how to do this in the latest version of HACS. I'm still testing this release on my own AVR and will push a release in a day or two if there are no major issues.

Varming73 commented 3 years ago

No matter if it fix the issue or not please tell if you need something tested :) I've upgraded to 0.4 already but only today so haven't really tested it yet.

crowbarz commented 3 years ago

I've released 0.5, which should fix this issue properly without having to guess a volume_step_delta.

Please test and let me know whether it sets the volume more accurately. In particular, if the integration is stepping the volume and after the volume step command the volume is unchanged or goes the other way (reached max limit/max volume, or something else is changing the volume at the same time), or it steps more than (new volume - old volume) times, it should stop stepping and log a warning.