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

Support SELECT_SOURCE for Bose soundtouch #31071

Closed
wants to merge 4 commits into from

Conversation

zhumuht
Copy link
Contributor

@zhumuht zhumuht commented Jan 22, 2020

Breaking change

Proposed change

support SELECT_SOURCE for Bose soundtouch, and show friendly_name

Type of change

  • New feature (which adds functionality to an existing integration)

Example entry for configuration.yaml:

media_player:
  - platform: soundtouch
    host: 192.168.100.99
    name: bose_soundtouch_083944
    resources:
      - name: "BluRayPlayer"
        source: "HDMI_3"
      - name: "XBox360"
        source: "HDMI_1"
      - name: "TMall Box"
        source: "HDMI_4"

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
  • 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:

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@zhumuht zhumuht force-pushed the soundtouch_source branch 2 times, most recently from 68cec0d to 8b3f053 Compare January 22, 2020 08:22
@MartinHjelmare MartinHjelmare changed the title support SELECT_SOURCE for Bose soundtouch Support SELECT_SOURCE for Bose soundtouch Jan 22, 2020
homeassistant/components/soundtouch/const.py Outdated Show resolved Hide resolved
homeassistant/components/soundtouch/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/soundtouch/media_player.py Outdated Show resolved Hide resolved
@@ -78,11 +83,16 @@
| SUPPORT_PLAY_MEDIA
)

SOURCE_SCHEMA = vol.Schema(
{vol.Required(CONF_SOURCE): cv.string, vol.Required(CONF_NAME): cv.string}
Copy link
Member

Choose a reason for hiding this comment

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

Please check that the source is one of the supported sources of the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/CharlesBlonde/libsoundtouch , this library is not updated 2 years. no api to check source

Copy link
Member

Choose a reason for hiding this comment

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

We have the enum Source available from the library. Can't we use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HDMI_1 ~ HDMI_6, and something else, not belong to source, but sourceAccount.
sourceAccount not enum in the library.

sources from device:
image

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(
{
vol.Required(CONF_HOST): cv.string,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
vol.Optional(CONF_RESOURCES): vol.All(cv.ensure_list, [SOURCE_SCHEMA]),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this option? If we know what the device supports, we can just return that list of sources in the media player entity source list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/CharlesBlonde/libsoundtouch , this library is not updated 2 years. no api to get sources that device supported.
so config sources to configuration.yaml

Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to bump libsoundtouch to 0.8.0 since HA is currently using an even older version?

Copy link
Contributor

Choose a reason for hiding this comment

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

btw - this might be an alternative in future? Just found it https://github.com/trunet/aiobosest

Copy link
Member

Choose a reason for hiding this comment

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

We like asyncio! If that works it might be a very nice replacement! The repo looks a bit dormant though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would it make sense to bump libsoundtouch to 0.8.0 since HA is currently using an even older version?

in this pr, libsoundtouch lib already be updated to 0.8.0

Copy link
Member

Choose a reason for hiding this comment

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

Since the sources could be fetched from the api if the library would support it, I don't think adding this config option is a good step forward. We want to avoid config options as much as possible.

Please update or change library first, in a separate PR, and then we see if this PR is still needed.

Copy link
Contributor

@da-anda da-anda Jan 23, 2020

Choose a reason for hiding this comment

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

there is a pending PR over at libsoundtouch which would add a source list, but the maintainer didn't respond to PRs in over a year. CharlesBlonde/libsoundtouch#34

The repo looks a bit dormant though.

Yes, the author of it commented that he wrote it for HA back then but the guys implementing the HA component went with libsoundtouch instead, which is why he stopped working on it. But he seems to be interested in working on it and implement the missing features if we (HA) considers to switch to it.
CharlesBlonde/libsoundtouch#38

I'm not saying we have to or should, especially since libsoundtouch is well documented and it's always a pitty if well used projects either have to be forked or abandoned.

Copy link
Member

@MartinHjelmare MartinHjelmare Jan 23, 2020

Choose a reason for hiding this comment

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

If the maintainer of https://github.com/trunet/aiobosest is active and the maintainer of https://github.com/CharlesBlonde/libsoundtouch is not, I suggest we change to the active library.

homeassistant/components/soundtouch/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/soundtouch/media_player.py Outdated Show resolved Hide resolved
homeassistant/components/soundtouch/media_player.py Outdated Show resolved Hide resolved
@stale
Copy link

stale bot commented Mar 21, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Mar 21, 2020
@stale stale bot closed this Mar 28, 2020
@da-anda da-anda mentioned this pull request Apr 1, 2020
20 tasks
@lock lock bot locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants