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

New component: bluesound #7192

Merged
merged 11 commits into from Aug 2, 2017

Conversation

Projects
None yet
5 participants
@thrawnarn
Copy link
Contributor

commented Apr 20, 2017

Description:

New platform for bluesound devices (hifi speakers and streamers)

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2460

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: bluesound
    hosts:
      - host: 192.168.1.132

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.
@mention-bot

This comment has been minimized.

Copy link

commented Apr 20, 2017

@thrawnarn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @robbiet480 to be potential reviewers.

@thrawnarn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

I'm having trouble pulling the latest version of the dev branch. Hence the conflict... Will try to resolve it later.
Also having trouble with tox in my environment...

thrawnarn added some commits Apr 20, 2017

Merge branch 'bluesound' of https://github.com/thrawnarn/home-assistant
… into bluesound

# Conflicts:
#	homeassistant/components/discovery.py
@balloob
Copy link
Member

left a comment

Could you extract the protocol specific information into a third party library on which this platform can depend ?

In such a way that you can do something like

device = Device(websession)
yield from device.update()

@property
def name(self):
    return device.name
return self._preset_items

@asyncio.coroutine
@Throttle(UPDATE_SERVICES_INTERVAL)

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

Throttle doesn't work with async.

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Apr 20, 2017

Author Contributor

I don't want to say you are wrong. But in my tests throttle did do what I wanted it to. It delayed the request for the time I asked for.

This comment has been minimized.

Copy link
@balloob

balloob Apr 21, 2017

Member

That's interesting, I did not expect that but I just tested it and it seems like you're right. Okay, you can leave them in.

return self._capture_items

@asyncio.coroutine
@Throttle(UPDATE_PRESETS_INTERVAL)

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

Please remove Throttle.

self._internal_update_sync_status(on_updated_cb, raise_timeout=False)

@asyncio.coroutine
@Throttle(UPDATE_CAPTURE_INTERVAL)

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

Please remove.

yield from response.release()

@asyncio.coroutine
@Throttle(SYNC_STATUS_INTERVAL)

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

Please remove.

return None
finally:
if response is not None:
yield from response.release()

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

No longer need to release since aiohttp 2

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Apr 20, 2017

Author Contributor

ah, I will remove this

"""Initiate the player async."""
if what is None:
# if it's the first request.
# let's make the request in a new thread

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

Why would you delay this a second and do it in a new job , Is this to debounce ?

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Apr 20, 2017

Author Contributor

I wanted to make this request in a separate thread, I want this component to be fully async and not hold a thread at startup. This was the way I found out. I'm happy to do this in some other way.

This comment has been minimized.

Copy link
@balloob

balloob Apr 21, 2017

Member
hass.async_add_job(self.async_init(True))

But I rather have you not call yourself. Instead have 1 method for startup that then enqueues the connection to be setup.

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Apr 21, 2017

Author Contributor

I think I tried this, but it locked the main thread. I will try again.

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2017

Member

This comment still needs to be addressed.

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Jul 24, 2017

Author Contributor

Hi, finally I have been looking into this.

I can't get it to be a non blocking thread when using hass.async_add_job. The call will be blocking the startup of HA. Any ideas on how to solve this? I don't want this component to block the startup.

This is the log when using hass.async_add_job

2017-07-24 16:57:31 INFO (MainThread) [homeassistant.setup] Setting up media_player
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.setup] Setting up discovery
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.setup] Setup of domain discovery took 0.0 seconds.
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event component_loaded[L]: component=discovery>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.components.media_player] Setting up media_player.bluesoundotepa
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.components.media_player.bluesound] calling URL: http://192.168.1.132:11000/SyncStatus
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=turn_on>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=turn_off>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=toggle>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_up>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_down>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_play_pause>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_play>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_pause>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_stop>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_next_track>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_previous_track>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=clear_playlist>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_set>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_mute>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_seek>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=select_source>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=play_media>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=shuffle_set>
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.setup] Setup of domain media_player took 0.2 seconds.
2017-07-24 16:57:31 INFO (MainThread) [homeassistant.core] Bus:Handling <Event component_loaded[L]: component=media_player>
2017-07-24 16:57:51 INFO (MainThread) [homeassistant.components.media_player.bluesound] Timeout with Bluesound: 192.168.1.132
2017-07-24 16:57:51 INFO (MainThread) [homeassistant.components.media_player.bluesound] Bluesound node 192.168.1.132 is offline, retrying later
2017-07-24 16:57:51 INFO (MainThread) [homeassistant.bootstrap] Home Assistant initialized in 20.72s

And this is the log when using async_track_time_interval

2017-07-24 16:58:53 INFO (MainThread) [homeassistant.setup] Setting up media_player
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.setup] Setting up discovery
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.setup] Setup of domain discovery took 0.0 seconds.
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event component_loaded[L]: component=discovery>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.components.media_player] Setting up media_player.bluesound
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=turn_on>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=turn_off>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=toggle>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_up>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_down>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_play_pause>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_play>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_pause>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_stop>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_next_track>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_previous_track>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=clear_playlist>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_set>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=volume_mute>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=media_seek>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=select_source>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=play_media>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event service_registered[L]: domain=media_player, service=shuffle_set>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.setup] Setup of domain media_player took 0.2 seconds.
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event component_loaded[L]: component=media_player>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.bootstrap] Home Assistant initialized in 0.81s
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Starting Home Assistant core loop
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Starting Home Assistant
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Bus:Handling <Event homeassistant_start[L]>
2017-07-24 16:58:53 INFO (MainThread) [homeassistant.core] Timer:starting
2017-07-24 16:58:54 INFO (MainThread) [homeassistant.components.http] Serving / to 10.225.70.119 (auth: True)
2017-07-24 16:58:54 INFO (MainThread) [homeassistant.components.http] Serving /api/websocket to 10.225.70.119 (auth: True)
2017-07-24 16:58:54 INFO (MainThread) [homeassistant.components.http] Serving /api/websocket to 10.225.70.119 (auth: True)
2017-07-24 16:58:54 INFO (MainThread) [homeassistant.components.http] Serving /api/themes to 10.225.70.119 (auth: True)
2017-07-24 16:58:54 INFO (MainThread) [homeassistant.components.media_player.bluesound] calling URL: http://192.168.1.132:11000/SyncStatus
2017-07-24 16:58:55 INFO (MainThread) [homeassistant.components.http] Serving /api/websocket to 10.225.70.119 (auth: True)
2017-07-24 16:59:14 INFO (MainThread) [homeassistant.components.media_player.bluesound] Timeout with Bluesound: 192.168.1.132
2017-07-24 16:59:14 INFO (MainThread) [homeassistant.components.media_player.bluesound] Bluesound node 192.168.1.132 is offline, retrying later

This comment has been minimized.

Copy link
@balloob

balloob Aug 1, 2017

Member

It's because we track all tasks during startup and will wait for them to finish.

If you want to enqueue a task by avoiding this, do not use a time tracking thing. That's a huge hack. Instead, listen for the EVENT_HOMEASSISTANT_START event to start the initialization. If Home Assistant is already running you can start init right away.

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Aug 1, 2017

Author Contributor

This seems to be working as expecting. Will try this latest version out at home with my speakers and then push after. Thanks :)

""""Loop which polls the status of the player."""
try:
while True:
yield from self.async_update_status()

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

You should not non-stop be querying the device. It is way better to just:

  • define SCAN_INTERVAL at root of file. That way Home Assistant will poll from time to time and allow users to change it
  • Remove all Throttle
  • Set should_poll to True.

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Apr 20, 2017

Author Contributor

It is long polling requests againts /Status. This is how they do it in their own apps. This way I get a responsiveness that is to die for :) . Everything is updated immediately when a change is made. This way is much better than making a request every 10 sec. If there isn't a change (a new song, change volume etc. etc.) this component will only make a request per minute

This comment has been minimized.

Copy link
@thrawnarn

thrawnarn Apr 20, 2017

Author Contributor

I don't want to set the same scan interval for every request. There is no need to make every request every time. I could achieve this with timestamps, but I thought this was a better and more easy to read.

My thought with this was to use the poll request (which I accidentally turned off, I will change this) as timer together with the throttle attribute

This comment has been minimized.

Copy link
@balloob

balloob Apr 21, 2017

Member

Oh, if it's long polling then it's all ok 👍

self._name)
raise
finally:
if response is not None:

This comment has been minimized.

Copy link
@balloob

balloob Apr 20, 2017

Member

Can remove this.

@thrawnarn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

I had a hunch that you would like it to be a separate library, but I was lazy and wanted to use some of the HA functionality. I could change this up in the next version if this would be ok with you? My plan is to implement join and unjoin of devices and to mimic the snapshot functionality of the SoCo (Sonos) library. This will take some time for me to achive since I'm not a python developer, if I could do it in C# instead I would be done tomorrow ;)

Removed response.release()
Fixed update_sync_status bug
Changed should_poll to True
def async_update_sync_status(self, on_updated_cb=None,
raise_timeout=False):
"""Update sync status."""
yield from self._internal_update_sync_status(on_updated_cb, raise_timeout=False)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 20, 2017

line too long (88 > 79 characters)

@balloob

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

I would be down to take it in for now and we move it later. However, the next step should be then to extract it into a lib before we should add join/unjoin.

Most of the HA functionality are just wrappers. So for example, if you want to 'create a thread' (which is really just to enqueue another task on the event loop) you can do loop.create_future(self.ensure_init())

@thrawnarn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

I totally agree that the next step should be to move it to a library and after that is done and working, I woll implement join/unjoin and snapshot.

@balloob

This comment has been minimized.

Copy link
Member

commented May 5, 2017

Documentation PR: home-assistant/home-assistant.io#2460

(still waiting for comments to get addressed)

@balloob

This comment has been minimized.

Copy link
Member

commented Jun 5, 2017

This PR seems to have gone stale. Closing it. You can reopen it when you're ready to finish it.

Comment that still needed addressing: #7192 (comment)

@balloob balloob closed this Jun 5, 2017

@thrawnarn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

How can I reopen this? @balloob

@balloob balloob reopened this Aug 1, 2017

@balloob

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

By saying the magic words 👍

_add_player(hass, async_add_devices, discovery_info.get('host'),
discovery_info.get('port', None))
return
else:

This comment has been minimized.

Copy link
@balloob

balloob Aug 1, 2017

Member

Unnecessary else (we're updating our linter and it will complain about this)

thrawnarn added some commits Aug 1, 2017

_init_player
)

@asyncio.coroutine

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Aug 1, 2017

expected 2 blank lines, found 1

thrawnarn added some commits Aug 1, 2017

@balloob

balloob approved these changes Aug 2, 2017

@balloob balloob merged commit 86c06ad into home-assistant:dev Aug 2, 2017

4 checks passed

cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 93.734%
Details
hound No violations found. Woof!
@balloob

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

Awesome 🐬 🎉

@fabaff fabaff referenced this pull request Aug 12, 2017

Merged

0.51 #8919

dethpickle added a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017

New component: bluesound (home-assistant#7192)
* New component: bluesound

* New component: bluesound

* Removed response.release()
Fixed update_sync_status bug
Changed should_poll to True

* Fix lint error

* Changes to init

* Fixed blank line

* updated requirements

* bump to xmltodict 0.11.0

@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017

@thrawnarn thrawnarn deleted the thrawnarn:bluesound branch Feb 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.