Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for versioned methods used by newer devices #132

Merged
merged 18 commits into from
Nov 4, 2023

Conversation

allistermaguire
Copy link
Contributor

@allistermaguire allistermaguire commented Oct 23, 2023

I have a Sony STR-AN1000 which isn't fully supported. This PR adds support.

Updated to allow the support of multiple API method versions returned by getMethodTypes. It will also check if the version currently being used is a specifically supported version (getSupportedApiInfo) for the device and log a debug message if not.

Adds support for the following specific API Methods

  • version 1.2 for getCurrentExternalTerminalsStatus. Includes what should be a non-breaking change for notifyPlayingContentInfo notification.
  • version 1.3 for getSourceList.

Update method parameters for the following, as newer devices can be stricter with the API spec. Should be non-breaking.

  • getCustomEqualizerSettings.
  • getSpeakerSettings.

Limited backward compatibility testing has been done with a STR-DN1080.

Conversation can be found #130

@allistermaguire allistermaguire marked this pull request as ready for review October 23, 2023 21:24
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @allistermaguire! I don't currently have much time to go through this PR, but I thought it'd make sense to provide some very brief feedback. I'll try and see if my old soundbar still boots and give it a quick test, hopefully some point during the upcoming weekend.

songpal/containers.py Outdated Show resolved Hide resolved
songpal/notification.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
songpal/device.py Outdated Show resolved Hide resolved
songpal/device.py Outdated Show resolved Hide resolved
songpal/device.py Show resolved Hide resolved
songpal/device.py Outdated Show resolved Hide resolved
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few code readability improvements & one open question wrt get_supported_methods and then this is good to go! 👍

Maybe this debug message could be improved to include the version number (or maybe simply print out method instead of method name which makes those visible):

DEBUG:songpal.service:getSystemInformation got called with args (()) kwargs ({})

I see it's available when raising the verbosity (-dd), but it would be useful to have it shown in the regular debug logs just in case.

songpal/notification.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
songpal/device.py Outdated Show resolved Hide resolved
songpal/device.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
songpal/device.py Outdated Show resolved Hide resolved
songpal/method.py Outdated Show resolved Hide resolved
Copy link
Owner

@rytilahti rytilahti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after that one nit is done, thanks again for the PR! 🥇

songpal/method.py Outdated Show resolved Hide resolved
@allistermaguire
Copy link
Contributor Author

Done I think. Thank you for your patience :)

@rytilahti
Copy link
Owner

Nah, thanks for your contribution and patience! Although I don't use this myself anymore, it's nice to see improvements that can help others :-)

Do you want to have a new release right away, or do you have some other changes you'd like to get in before I prepare a release with this PR included?

@rytilahti rytilahti changed the title Add support for newer devices Add support for versioned methods used by newer devices Nov 4, 2023
@rytilahti rytilahti merged commit ad8b10d into rytilahti:master Nov 4, 2023
4 checks passed
@rytilahti rytilahti mentioned this pull request Nov 4, 2023
@allistermaguire
Copy link
Contributor Author

allistermaguire commented Nov 4, 2023

Thanks.

I would like to make it so that all sources (lack of a better term) are available in Hass. At the moment it is only inputs, so missing sources like Spotify, AirPlay, ChromeCast, Tunner etc.

I am not sure where the line is to do it front or back end? There are object differences between get_inputs() and get_source_list(), some URI query string handling with notifications is also necessary.

@rytilahti
Copy link
Owner

Good question, I'm not sure.. Maybe rather on the homeassistant's side, as that would perhaps make it cleaner to implement zone support, given a single input can be assigned to a single media player?

@allistermaguire
Copy link
Contributor Author

I had a quick look over the weekend and I think you are right. I was looking to see if it made sense / possible to use a parent class for Source and Input to inherit, but it would need some wiring in the Source child, and what would it be called :)

So I don't have any other changes in the works, so if you could release a new version that would be great. Will you bump it in Hass? If you do can you add _attr_device_class = MediaPlayerDeviceClass.RECEIVER to media_player.py as well.?

rytilahti added a commit that referenced this pull request Nov 7, 2023
This release brings support for versioned method calls which are required by newer hardware, like TA-AN1000, thanks to @allistermaguire!

[Full Changelog](release/0.15.2...0.16)

**Implemented enhancements:**

- Add defining source-address for discover [\#133](#133) (@rytilahti)
- Add support for versioned methods used by newer devices [\#132](#132) (@allistermaguire)

**Closed issues:**

- Support for TA-AN1000 [\#130](#130)
- Support for SRS-XB23?  [\#127](#127)
- Apparently missing some dependency, bunch of errors [\#126](#126)
- App doesn't work on latest python 3.11.1 [\#125](#125)
- can't find the device [\#116](#116)

**Merged pull requests:**

- Use ruff for linting and formatting [\#139](#139) (@rytilahti)
- Configure to use CI as trusted publisher [\#137](#137) (@rytilahti)
- Drop importlib\_metadata dependency [\#136](#136) (@rytilahti)
- Drop Python 3.7 support [\#135](#135) (@rytilahti)
- Add updated devinfo with version info for HT-XT3 [\#134](#134) (@rytilahti)
- Add devinfo for STR-AZ5000ES receiver [\#129](#129) (@ohmantics)
@rytilahti rytilahti mentioned this pull request Nov 7, 2023
rytilahti added a commit that referenced this pull request Nov 7, 2023
This release brings support for versioned method calls which are required by newer hardware, like TA-AN1000, thanks to @allistermaguire!

[Full Changelog](release/0.15.2...0.16)

**Implemented enhancements:**

- Add defining source-address for discover [\#133](#133) (@rytilahti)
- Add support for versioned methods used by newer devices [\#132](#132) (@allistermaguire)

**Closed issues:**

- Support for TA-AN1000 [\#130](#130)
- Support for SRS-XB23?  [\#127](#127)
- Apparently missing some dependency, bunch of errors [\#126](#126)
- App doesn't work on latest python 3.11.1 [\#125](#125)
- can't find the device [\#116](#116)

**Merged pull requests:**

- Use ruff for linting and formatting [\#139](#139) (@rytilahti)
- Configure to use CI as trusted publisher [\#137](#137) (@rytilahti)
- Drop importlib\_metadata dependency [\#136](#136) (@rytilahti)
- Drop Python 3.7 support [\#135](#135) (@rytilahti)
- Add updated devinfo with version info for HT-XT3 [\#134](#134) (@rytilahti)
- Add devinfo for STR-AZ5000ES receiver [\#129](#129) (@ohmantics)
@rytilahti
Copy link
Owner

Okay, new release is now and and I created home-assistant/core#103561 to bump the version. I'll add the device class, let's see if it's okay to do as a part of the version bump or if a separate PR is required :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants