Skip to content

Sonos idle #32712

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.

Sign up for GitHub

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

Merged
balloob merged 4 commits into from
Mar 12, 2020
Merged

Sonos idle #32712

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5edf61a
Sonos idle
balloob Mar 12, 2020
9aa2bcd
F-string
balloob Mar 12, 2020
b197a2b
Add to properties
balloob Mar 12, 2020
47b595a
Fixes
balloob Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 23 additions & 20 deletions homeassistant/components/sonos/media_player.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@
ATTR_SPEECH_ENHANCE = "speech_enhance"
ATTR_QUEUE_POSITION = "queue_position"

UNAVAILABLE_VALUES = {"", "NOT_IMPLEMENTED", None}


class SonosData:
"""Storage class for platform global data."""
Expand Down Expand Up @@ -330,7 +332,7 @@ def wrapper(entity, *args, **kwargs):

def _timespan_secs(timespan):
"""Parse a time-span into number of seconds."""
if timespan in ("", "NOT_IMPLEMENTED", None):
if timespan in UNAVAILABLE_VALUES:
return None

return sum(60 ** x[0] * int(x[1]) for x in enumerate(reversed(timespan.split(":"))))
Expand Down Expand Up @@ -427,7 +429,10 @@ def device_info(self):
@soco_coordinator
def state(self):
"""Return the state of the entity."""
if self._status in ("PAUSED_PLAYBACK", "STOPPED"):
if self._media_title is not None and self._status in (

@amelchio amelchio Mar 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure that it can fail but honestly, this makes me feel odd. It seems to conflate concerns. Maybe we paused some untagged media? Even if that cannot happen with Sonos (I am unsure), it seems like a situation the frontend should be able to handle?

@balloob balloob Mar 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I consider it a bug in Home Assistant if a media player is claiming to have media loaded (being in a paused/playing state) and it cannot describe that media and it normally can.

Especially since Sonos does have the transitioning state to indicate that it's between songs, which we will still consider being playing.

Even if you play MP3s there is a title (the filename)

@amelchio amelchio Mar 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, in that case we have a bug in update_media_linein() :)

Maybe it would look less out of place as a guard.

@balloob balloob Mar 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh sorry, with MP3 I meant one from a local server. You're right linein is different. Do we know the transport info if it's linein / TV ?

@balloob balloob Mar 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

With a guard, do you mean that you want to see the if be inside the other if ?

@amelchio amelchio Mar 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Line-in currently has

        self._media_artist = source

that we can just change to

        self._media_title = source

and it is hard-coded as "TV" or "Line-in".

@amelchio amelchio Mar 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I meant

    if self._media_title is None:
        return STATE_IDLE

but don't change it just for me, I'm fine with whatever :)

@balloob balloob Mar 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done and done!

"PAUSED_PLAYBACK",
"STOPPED",
):
return STATE_PAUSED
if self._status in ("PLAYING", "TRANSITIONING"):
return STATE_PLAYING
Expand Down Expand Up @@ -511,16 +516,14 @@ def _set_favorites(self):

def _radio_artwork(self, url):
"""Return the private URL with artwork for a radio stream."""
if url not in ("", "NOT_IMPLEMENTED", None):
if url.find("tts_proxy") > 0:
# If the content is a tts don't try to fetch an image from it.
return None
url = "http://{host}:{port}/getaa?s=1&u={uri}".format(
host=self.soco.ip_address,
port=1400,
uri=urllib.parse.quote(url, safe=""),
)
return url
if url in UNAVAILABLE_VALUES:
return None

if url.find("tts_proxy") > 0:
# If the content is a tts don't try to fetch an image from it.
return None

return f"http://{self.soco.ip_address}:1400/getaa?s=1&u={urllib.parse.quote(url, safe='')}"

def _attach_player(self):
"""Get basic information and add event subscriptions."""
Expand Down Expand Up @@ -621,9 +624,9 @@ def update_media_radio(self, variables, track_info):
media_info = self.soco.avTransport.GetMediaInfo([("InstanceID", 0)])
self._media_image_url = self._radio_artwork(media_info["CurrentURI"])

self._media_artist = track_info.get("artist")
self._media_artist = track_info.get("artist") or None

@amelchio amelchio Mar 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to add these or None to the property getters for less duplication. We already do that for media_image_url.

@amelchio amelchio Apr 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like this change made the PR miss its target because the state property reads from self._media_title which is no longer set to None.

I have included a proposal to fix that in #34311 (by using self.media_title).

self._media_album_name = None
self._media_title = track_info.get("title")
self._media_title = track_info.get("title") or None

if self._media_artist and self._media_title:
# artist and album name are in the data, concatenate
Expand All @@ -640,7 +643,7 @@ def update_media_radio(self, variables, track_info):

# For radio streams we set the radio station name as the title.
current_uri_metadata = media_info["CurrentURIMetaData"]
if current_uri_metadata not in ("", "NOT_IMPLEMENTED", None):
if current_uri_metadata not in UNAVAILABLE_VALUES:
# currently soco does not have an API for this
current_uri_metadata = pysonos.xml.XML.fromstring(
pysonos.utils.really_utf8(current_uri_metadata)
Expand All @@ -650,7 +653,7 @@ def update_media_radio(self, variables, track_info):
".//{http://purl.org/dc/elements/1.1/}title"
)

if md_title not in ("", "NOT_IMPLEMENTED", None):
if md_title not in UNAVAILABLE_VALUES:
self._media_title = md_title

if self._media_artist and self._media_title:
Expand Down Expand Up @@ -704,11 +707,11 @@ def update_media_music(self, update_media_position, track_info):
self._media_position = rel_time
self._media_position_updated_at = utcnow()

self._media_image_url = track_info.get("album_art")
self._media_image_url = track_info.get("album_art") or None

self._media_artist = track_info.get("artist")
self._media_album_name = track_info.get("album")
self._media_title = track_info.get("title")
self._media_artist = track_info.get("artist") or None
self._media_album_name = track_info.get("album") or None
self._media_title = track_info.get("title") or None

self._source_name = None

Expand Down