search

bazwilliams / openhomedevice

Python library to access services on Linn Ds and other Openhome devices
MIT License
7 stars 3 forks source link

Errors trying to adjust volume when not enabled. #14

Closed von-Chaps closed 5 years ago

von-Chaps commented 5 years ago

Hi there,

I see Device.VolumeEnabled() and I get what it's for. Device.VolumeLevel() and Device.IsMuted() also check if service is None, so are proteced if they are called when volume is not enabled. However, SetVolumeLevel(), IncreaseVolume(), DecreaseVolume() and SetMute() don't make this check so errors are raised if called when service is None.

openhome/media_player.py is doing exactly this when connected to a device that does not support volume adjustment. I was going to fix it, by patching media_player.py to check VolumeEnabled() first, but thought I best check with you whether the behaviour you have in Device.py is deliberate, or whether you'd rather protect those methods yourself, thus rendering changes to media_player.py unnecessary.

Cheers and thanks for the good work!

bazwilliams commented 5 years ago

Thanks for reporting this.

I seem to recall an issue around volume a few years back but I couldn’t recreate it. Now I know how.

I’ll have a look to see how best it can be handled in the library. Presumably protecting for None and doing nothing is the safest action?

bazwilliams commented 5 years ago

@von-Chaps Feel free to review/comment on the above PR - I think this should cover the problem at the library level. Once merged, I'll release a newer package to PyPi.

Can you create an issue on the home assistant repo reporting the volume control problem as well please? Media player components are meant to advertise volume control capability to Home Assistant only if the service is enabled and I suspect the openhome component isn't working correctly. Worst case the fix is a version bump of the library - but I'll take a look at that component shortly.

von-Chaps commented 5 years ago

Thanks for the quick response. Your suggested protections are, I think, right! Commented on the PR.

I raised an issue in the HA repo a while ago, before I dug in deeper and found you. It's here; https://github.com/home-assistant/home-assistant/issues/25531

You're right, openhome/media_player.py is not doing the right thing.

Cheers!

bazwilliams commented 5 years ago

I've published a new version of the library https://pypi.org/project/openhomedevice/

von-Chaps commented 5 years ago

@bazwilliams Nicely done! Many thanks.