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

Add sound mode support #4470

Closed
wants to merge 6 commits into from
Closed

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Jan 20, 2018

Description:

  • Two aditional configuration options: sound_mode and sound_mode_dict
  • Aditional service: media_player/select_sound_mode

Front end pull request: home-assistant/frontend#815

Pull request in home-assistant (if applicable): home-assistant/core#13706

Checklist:

  • Branch: Fixes, changes and adjustments should be created against current. New documentation for platforms/components and features should go to next.
  • The documentation follow the standards.

- this time with the next branch as base
Added the documentation for the new general sound mode support service.
General basis is there for all platforms, but only the Denon AVR component is currently implemented (first release).
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

@starkillerOG I've reviews the docs and left some comments. Could you please take a look? Thx already! 👍

@@ -48,6 +48,8 @@ media_player:
name: NAME
show_all_sources: True / False
timeout: POSITIVE INTEGER
sound_mode: True
Copy link
Member

Choose a reason for hiding this comment

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

We are trying to keep the examples as minimal as possible. Please remove the added optional configuration paramters.

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 removed the sound_mode. But I would like to keep the sound_mode_dict in the example because this is a somewhat difficult parameter, for example if you put " " around it and make it a string it will not work anymore. And if I remember corectly the values in this dict schould be uppercase, otherwise the values received from the AVR will not match. And a dictionary is in general a little more difficult then just a string or a number for the basic users.

Furthermore the hole zones parameter is also in the example and you do not need that in general and is also optional if you don't use second zones on your AVR.

@@ -59,6 +61,8 @@ Configuration variables:
- **name** (*Optional*): Name of the device. If not set, friendlyName of receiver is used.
- **show_all_sources** (*Optional*): If True all sources are displayed in sources list even if they are marked as deleted in the receiver. If False deleted sources are not displayed (default). Some receivers have a bug that marks all sources as deleted in the interface. In this case this option could help.
- **timeout** (*Optional*): Timeout for HTTP requests to the receiver. Defaults to 2 seconds if not provided.
- **sound_mode** (*Optional*)(*boolean*): Flag that defines if sound mode is supported. Default value: True.
- **sound_mode_dict** (*Optional*): Dictonary containing the sound modes that are supported, the key needs to be identical with the command to set a specific sound mode and the corresponding value needs to be the sound mode as reported by the AVR. Default value: {'MUSIC':'PLII MUSIC', 'MOVIE':'PLII MOVIE', 'GAME':'PLII GAME', 'PURE DIRECT':'DIRECT', 'AUTO':'None', 'DOLBY DIGITAL':'DOLBY DIGITAL', 'MCH STEREO':'MULTI CH STEREO', 'STEREO':'STEREO'}
Copy link
Member

Choose a reason for hiding this comment

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

  • Dictonary -> Dictionary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for checking my bad spelling

@@ -68,5 +72,6 @@ A few notes:
- Additional option the control Denon AVR receivers with a builtin web server is using the HTTP interface with denonavr platform.
- denonavr platform supports some additional functionalities like album covers, custom input source names and auto discovery.
- Marantz receivers seem to a have quite simliar interface. Thus if you own one, give it a try.
- The key-value structure in the sound_mode_dict is needed because the commands to set an sound mode and the reported sound mode are diffrent. This structure matches the reported sound mode with the commands to set a sound mode.
Copy link
Member

Choose a reason for hiding this comment

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

  • diffrent -> different
  • Backquote sound_mode_dict keyword

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you make a backquote in this "code"?

@frenck frenck added the new-feature This PR adds documentation for a new Home Assistant feature to an existing integration label Jan 20, 2018
- fixed some spelling mistakes
@starkillerOG
Copy link
Contributor Author

@frenck Thanks for your review, I made changes based on your comments.
Could you maybe also look at the backend and frontend pull request for this sound mode support?
Front end: home-assistant/frontend#815
Back end: home-assistant/core#11764

@frenck
Copy link
Member

frenck commented Jan 20, 2018

Thanks for the adjustments @starkillerOG 🎖
This PR can be merged as soon as both parent PRs have been merged.

I'm sorry but I do not review those type of PRs.

@starkillerOG
Copy link
Contributor Author

@frenck could you check your request for changes as addressed, so the red cross will go away and it can be merged once the other 2 PR's are accepted?

@frenck
Copy link
Member

frenck commented Jan 20, 2018

@starkillerOG Sorry simply forgot to hit the approve button, my bad... 🤦‍♂️

@stale
Copy link

stale bot commented Mar 21, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale The PR had no activity for quite some time, and is marked as Stale label Mar 21, 2018
@starkillerOG
Copy link
Contributor Author

Still waiting until my PR of the denonavr library is merged.

@stale stale bot removed the Stale The PR had no activity for quite some time, and is marked as Stale label Mar 22, 2018
@frenck frenck self-assigned this Mar 23, 2018
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

@starkillerOG Could you please take a look at the merge conflicts?

@frenck frenck assigned starkillerOG and unassigned frenck Mar 23, 2018
@starkillerOG
Copy link
Contributor Author

@frenck I think I resolved the conflict, but I am still waiting until the backend code can be merged.

frenck
frenck previously approved these changes Apr 2, 2018
@starkillerOG
Copy link
Contributor Author

@frenck I think I might have undone your changes, sorry I just wanted to correct the syntax.
Can you do the changes again?
Sorry.

@frenck frenck self-assigned this Apr 6, 2018
@starkillerOG
Copy link
Contributor Author

Thank you @frenck

@frenck
Copy link
Member

frenck commented Apr 6, 2018

No! Thank you 👍

@frenck
Copy link
Member

frenck commented May 30, 2018

Closing this PR, since the parent PR has been closed as well.

@frenck frenck closed this May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature This PR adds documentation for a new Home Assistant feature to an existing integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants