Skip to content

SongPal: do not crash if active_source is not (yet) available - fixes #20343#20344

Merged
rytilahti merged 3 commits intohome-assistant:devfrom
Censored3:songpal-active_source-fix
Jan 24, 2019
Merged

SongPal: do not crash if active_source is not (yet) available - fixes #20343#20344
rytilahti merged 3 commits intohome-assistant:devfrom
Censored3:songpal-active_source-fix

Conversation

@Censored3
Copy link
Copy Markdown
Contributor

Description:

Related issue (if applicable): fixes #20343

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

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

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@Censored3
Copy link
Copy Markdown
Contributor Author

With this implemented, source is populated after a while... Maybe it's a timing issue

@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Jan 23, 2019

I thought that this shoulnd't ever happen async_update is always called at least once, but looking at it now I think this can happen when there is no active source (lines 252-253). Good catch! Could you please update the description to explain the problem you are encountering? Do you get a stacktrace?

edit: stacktrace in the linked bug report, so ignore my questions.

def source(self):
"""Return currently active source."""
return self._active_source.title
return getattr(self._active_source, 'title', None)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you add a single-line comment here describing the necessity of using getattr instead of regular attribute access here, then it's okay to merge.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure - here you go. Is this comment sufficient?

Copy link
Copy Markdown
Member

@rytilahti rytilahti Jan 24, 2019

Choose a reason for hiding this comment

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

That's good, thanks! I changed the title to be a bit more descriptive :-)

@rytilahti rytilahti changed the title SongPal: handling if active_source can't be detected - fix #20343 SongPal: do not crash if active_source is not (yet) available - fixes #20343 Jan 24, 2019
@rytilahti rytilahti merged commit 0be922d into home-assistant:dev Jan 24, 2019
@ghost ghost removed the in progress label Jan 24, 2019
@Censored3
Copy link
Copy Markdown
Contributor Author

Great, happy to help.

Thx btw for adding active source reading; really something I was missing!

@rytilahti
Copy link
Copy Markdown
Member

rytilahti commented Jan 24, 2019

Glad to hear someone else besides me is using the platform :-) But wasn't that feature always there? Or do you mean the push-based, immediate updates? I think the problem itself may be caused partially by that codepath (if the device responds no active source during the initialization, the variable will get updated first after a source changed notification is received from the device).

@Censored3
Copy link
Copy Markdown
Contributor Author

Ah well I vaguely remember something about it being there, but at least wasn't working for my devices... Maybe the updated python-songpal dep took care of it. I'm happy either way ;-)

@balloob balloob mentioned this pull request Feb 6, 2019
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Feb 12, 2019
…ome-assistant#20343 (home-assistant#20344)

* SongPal: error handling if active_source can't be detected

* sonpal: Add comment to the use of getattr() for property source

* songpal: make comment single-line
kellerza pushed a commit to kellerza/ha-core that referenced this pull request Feb 24, 2019
…ome-assistant#20343 (home-assistant#20344)

* SongPal: error handling if active_source can't be detected

* sonpal: Add comment to the use of getattr() for property source

* songpal: make comment single-line
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SongPal acting up since 0.85

3 participants