Implement soundtouch select source #31669

jtommi · 2020-02-09T17:08:10Z

Proposed change

I use Bose Soundtouch speakers and were missing the ability to select the input source.
I saw that the external package libsoundtouch 0.8 already support this, but Home-assistant not yet.
So I updated the requirements from libsoundtouch 0.7.2 to libsoundtouch 0.8 and implemented the feature in the component.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

I doubt that will help since you need the device, but here it is anyway

# Example configuration.yaml
media_player:
  - platform: soundtouch
    host: 192.168.1.2
    port: 8090
    name: Soundtouch speaker

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:

Documentation added/updated for www.home-assistant.io

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

homeassistant/components/soundtouch/media_player.py

jtommi · 2020-02-10T12:41:11Z

@springstan After reading CharlesBlonde/libsoundtouch#38 , I feel like it might be better to first solve the library issue. WDYT?

springstan · 2020-02-10T12:46:36Z

@springstan After reading CharlesBlonde/libsoundtouch#38 , I feel like it might be better to first solve the library issue. WDYT?

IMHO the library issue should be solved first, here is a list of requirements to accept a new library: #31647 (comment)

Kane610 · 2020-02-10T18:26:41Z

@springstan that comment is relevant when the topic is about forking the library to continue development. Switching to an async version of a library is a different thing. Maybe we should add a chapter to the development handbook about forking a library or replacing it with a different implementation?

springstan · 2020-02-10T19:44:15Z

@springstan that comment is relevant when the topic is about forking the library to continue development. Switching to an async version of a library is a different thing.

Oh I am sorry, did not know that. Falsly assumed that the process would be similar but thinking about it now it makes sense to have a different kind of discussion/process for switching to an async library.

Maybe we should add a chapter to the development handbook about forking a library or replacing it with a different implementation?

Yes, definitely. This would make future discussions and questions easier first of all and we would have a documented consensus towards such topics.

springstan · 2020-02-10T19:44:59Z

@Kane610 how would we go about deciding if we want to switch this library to an async version?

Kane610 · 2020-02-11T07:26:39Z

@Kane610 how would we go about deciding if we want to switch this library to an async version?

Well HASS generally prefer async implementations (see the integration quality scale), also if the developer of the changes are willing to take on a maintainer responsibility that should also weigh in.

MartinHjelmare · 2020-02-13T05:00:24Z

There are currently three open PRs for the soundtouch integration:
https://github.com/home-assistant/home-assistant/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+soundtouch

Members please coordinate these PRs when reviewing and/or considering merging.

We have already requested a library change here:
#31071 (comment)

jtommi · 2020-03-09T16:52:04Z

I don't feel comfortable changing the whole library.
But I think I can implement this feature.
The question is if it would be accepted or if the library changr is a must

MartinHjelmare · 2020-03-09T17:15:56Z

If the select source feature addition doesn't mean a breaking change later when we switch library it's ok, I think. Otherwise we should switch library first.

stale · 2020-04-08T18:07:32Z

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.

boxcee · 2020-04-13T17:04:46Z

Not related to code changes, but could help until this is merged: https://www.reddit.com/r/homeassistant/comments/fsliot/bose_soundtouch_how_to_change_the_source/fm8eyv0?utm_source=share&utm_medium=web2x

jtommi · 2020-04-13T18:12:48Z

@boxcee thanks, your code was added (select, select_source and the importing and usage of Source.AUX and Source.BLUETOOTH)
Feel free to test if you have Bose devices. My network is having some issues so I couldn't test yet.

I'm currently checking why I have functions like get_zone_info in my code but I didn't implement them and they're not on the dev branch (anymore)

jtommi · 2020-04-13T20:22:28Z

I'm currently checking why I have functions like get_zone_info in my code but I didn't implement them and they're not on the dev branch (anymore)

I was looking at my forked dev branch which wasn't up-to-date

jtommi · 2020-04-26T18:08:37Z

Functionality was tested with Soundtouch devices.
Now I have to improve test coverage. If someone could give me some help, that would be great, I'm trying to understand how that stuff works.

jtommi · 2020-04-28T07:44:43Z

My additions were covered by tests. for me this is ready to be merged.

MartinHjelmare

Please add a test where we mock device source status and assert that the entity sets the source state attribute correctly.

homeassistant/components/soundtouch/manifest.json

rytilahti · 2020-05-06T16:41:14Z

homeassistant/components/soundtouch/media_player.py

+        """List of available input sources."""
+        return [
+            Source.AUX.value,
+            Source.BLUETOOTH.value,


I presume Source is an enum? I'd recommend using something like:

return [x.value for x in list(Source)]

to avoid the need to keep this in sync when/if upstream changes and adds new potential sources.

It is indeed, but it looks like most of them don't have a method for selecting it (which kind of links to your second comment)

https://github.com/CharlesBlonde/libsoundtouch/blob/25163a7ae39de2724e2946e8b1b61484d2a590d6/libsoundtouch/utils.py#L43-50

rytilahti · 2020-05-06T16:42:24Z

homeassistant/components/soundtouch/media_player.py

+        """Select input source."""
+        if source == Source.AUX.value:
+            _LOGGER.debug("Selecting source AUX")
+            self._device.select_source_aux()


Here I think it would make sense to have a select_source(Source) method in the upstream lib and use that instead of having separate methods per source type.

Indeed, but the upstream lib isn't maintained anymore, so we're stuck for now.
There are efforts to replace the library, but some features are missing.
CharlesBlonde/libsoundtouch#38

MartinHjelmare · 2020-05-12T10:58:19Z

If the tests are added we can merge here.

…f a feature implementation

Added source and source_list

Rebased on dev

Co-authored-by: Martin Hjelmare <[email protected]>

MartinHjelmare

Looks good!

Krocko · 2020-05-13T09:50:01Z

Is there a chance to get the HDMI (PRODUCT) source for the SoundTouch 300 also?

homeassistant added the cla-signed label Feb 9, 2020

probot-home-assistant bot added integration: soundtouch small-pr PRs with less than 30 lines. labels Feb 9, 2020

springstan reviewed Feb 9, 2020

View reviewed changes

homeassistant/components/soundtouch/media_player.py Outdated Show resolved Hide resolved

springstan reviewed Feb 9, 2020

View reviewed changes

homeassistant/components/soundtouch/media_player.py Show resolved Hide resolved

jtommi mentioned this pull request Feb 10, 2020

Is it dead? CharlesBlonde/libsoundtouch#38

Closed

boxcee mentioned this pull request Apr 1, 2020

Add select source to soundtouch #33490

Closed

20 tasks

stale bot added stale and removed stale labels Apr 8, 2020

jtommi force-pushed the implement-soundtouch-select_source branch from 0d56c8b to d2c621c Compare April 13, 2020 14:41

jtommi force-pushed the implement-soundtouch-select_source branch from d2c621c to e4762ed Compare April 26, 2020 17:10

ocalvo approved these changes May 5, 2020

View reviewed changes

MartinHjelmare requested changes May 5, 2020

View reviewed changes

homeassistant/components/soundtouch/manifest.json Outdated Show resolved Hide resolved

rytilahti reviewed May 6, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

jtommi added 10 commits May 12, 2020 18:33

Update requirements for soundtouch from 0.7.2 to 0.8 in preparation o…

feb3215

…f a feature implementation

Implemented select source functionality in Soundtouch module

3f7672c

Corrected lint error

23ad0f6

Adapted the test to include additional supported feature

ee82fb3

Changed logging level. Removed unnecessary description

7337263

Correct lint error

92cba97

Changed test to expect SUPPORT_SELECT_SOURCE in supported features

d527645

Added source and source_list

Fixed 'not JSON serializable' issue.

7cc4e50

Rebased on dev

Added tests for select_source

ead29f1

Added tests for playing AUX and bluetooth

1dddebb

jtommi force-pushed the implement-soundtouch-select_source branch from 9b12740 to 1dddebb Compare May 12, 2020 18:34

Removed empty dependencies from manifest.json

349d2d9

Co-authored-by: Martin Hjelmare <[email protected]>

MartinHjelmare approved these changes May 12, 2020

View reviewed changes

MartinHjelmare merged commit 8f76f59 into home-assistant:dev May 12, 2020

home-assistant locked as resolved and limited conversation to collaborators May 13, 2020

Uh oh!

Implement soundtouch select source #31669

Implement soundtouch select source #31669

Conversation

jtommi commented Feb 9, 2020

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jtommi commented Feb 10, 2020

Uh oh!

springstan commented Feb 10, 2020

Uh oh!

Kane610 commented Feb 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

springstan commented Feb 10, 2020

Uh oh!

springstan commented Feb 10, 2020

Uh oh!

Kane610 commented Feb 11, 2020

Uh oh!

MartinHjelmare commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtommi commented Mar 9, 2020

Uh oh!

MartinHjelmare commented Mar 9, 2020

Uh oh!

stale bot commented Apr 8, 2020

Uh oh!

boxcee commented Apr 13, 2020

Uh oh!

jtommi commented Apr 13, 2020

Uh oh!

jtommi commented Apr 13, 2020

Uh oh!

jtommi commented Apr 26, 2020

Uh oh!

jtommi commented Apr 28, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rytilahti May 6, 2020

Choose a reason for hiding this comment

Uh oh!

jtommi May 6, 2020

Choose a reason for hiding this comment

Uh oh!

rytilahti May 6, 2020

Choose a reason for hiding this comment

Uh oh!

jtommi May 6, 2020

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

MartinHjelmare commented May 12, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Krocko commented May 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Example entry for `configuration.yaml`:

Kane610 commented Feb 10, 2020 •

edited

Loading

MartinHjelmare commented Feb 13, 2020 •

edited

Loading