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

Revbump to SoCo 0.13, add support for Night Sound and Speech Enhancement #10765

Merged
merged 1 commit into from
Dec 17, 2017

Conversation

rbdixon
Copy link
Contributor

@rbdixon rbdixon commented Nov 23, 2017

Ready to pull.

Description:

Sonos Playbar and Playbase devices support Night Sound and Speech Enhancement
effects when playing from sources such as a TV. Adds a new service "sonos_set_option"
whichs accepts boolean options to control these audio features.

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.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @rbdixon,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@property
def device_state_attributes(self):
"""Return device specific state attributes."""
return {ATTR_IS_COORDINATOR: self.is_coordinator}

attributes = {ATTR_IS_COORDINATOR: self.is_coordinator}

Choose a reason for hiding this comment

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

multiple spaces after operator

@balloob balloob changed the title Revbump to SoCo 0.13, add support for Night Sound and Speech Enhancement WIP: Revbump to SoCo 0.13, add support for Night Sound and Speech Enhancement Nov 23, 2017
@rbdixon rbdixon changed the title WIP: Revbump to SoCo 0.13, add support for Night Sound and Speech Enhancement Revbump to SoCo 0.13, add support for Night Sound and Speech Enhancement Dec 1, 2017
@rbdixon
Copy link
Contributor Author

rbdixon commented Dec 1, 2017

I've added docs PR and I think this is ready to merge. I cobbled together a simple test based on the patterns I saw for testing the other features. Local testing with my speakers works fine, too, of course.

@property
def night_sound(self):
"""Get status of Night Sound."""
if self._coordinator:
Copy link
Contributor

Choose a reason for hiding this comment

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

You defer to the coordinator. Are these options really per-group (like a playlist), not per-speaker (like volume)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some research using two speakers (Kitchen, Den) of my multi-unit setup. Den is a Playbar that supports the night_sound (NS) and speech_enhance (SE) features when playing from the TV source.

  1. When I build a group designating Kitchen as the coordinator and then switch to the TV source the Den then becomes the coordinator.
  2. When playing TV in a group the NS and SE effects are audible on the Den but not the Kitchen.
  3. SoCo will not send NS and SE commands to a non-Playbar speaker.

Based on #2 I agree with you that this is a per-speaker feature. This was masked by the Sonos behavior described in #1.

I'll push a new version that does not defer to the coordinator momentarily. It will not include a check to ensure that the speaker is the coordinator but given #3 this should not cause an error.

Sound good? Thank you for the close review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed an updated version per your comments. I also rebased to the current dev branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like your changes though if the Playbar always ends up being the coordinator, I guess it doesn't matter much.

…cement.

Sonos Playbar and Playbase devices support Night Sound and Speech Enhancement
effects when playing from sources such as a TV. Adds a new service "sonos_set_option"
whichs accepts boolean options to control these audio features.
Copy link
Contributor

@amelchio amelchio left a comment

Choose a reason for hiding this comment

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

Looks good 👍. I hope you will help move this to events when/if #10419 gets merged.

@rbdixon rbdixon mentioned this pull request Dec 16, 2017
@fabaff fabaff merged commit dfb8b5a into home-assistant:dev Dec 17, 2017
@fabaff fabaff added this to the 0.60 milestone Dec 17, 2017
fabaff pushed a commit that referenced this pull request Dec 17, 2017
…cement. (#10765)

Sonos Playbar and Playbase devices support Night Sound and Speech Enhancement
effects when playing from sources such as a TV. Adds a new service "sonos_set_option"
whichs accepts boolean options to control these audio features.
@home-assistant home-assistant locked and limited conversation to collaborators Mar 30, 2018
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.

6 participants