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

Sound mode support media_player general and denonavr #23

Closed
starkillerOG opened this issue May 4, 2018 · 24 comments
Closed

Sound mode support media_player general and denonavr #23

starkillerOG opened this issue May 4, 2018 · 24 comments

Comments

@starkillerOG
Copy link

see this PR for the already implemented code.

I would like to add general support for "sound mode" to the media player component. I already implemented the general code and the specific code for the denonavr media players (see PR above).
It is just based on the source_input and source_input_list architecture and bassically just a coppy of that.

I will upload some pictures of how the working fronted looks like for the denonavr component:

@starkillerOG
Copy link
Author

sound_mode_support

sound_mode_support2

@starkillerOG
Copy link
Author

@balloob How do you think sound mode schould be incorperated in the general media_player component?

@balloob
Copy link
Member

balloob commented May 8, 2018

We should not add features to the base component if there is just 1 platform that needs it.

@starkillerOG
Copy link
Author

@balloob I think many media_player platforms can actually use this code.
@rytilahti wants to implement support using this code for the songpal component.
@ludeeus was talking about the yamaha component.
And on the form I talked to someone that wanted to implement sound mode support for the sonos component.

I definitly think this could be usefull for more than only the denonavr component, that is also the reason why I made it general.

Of course their are no concrete impemented code for those other platforms yet, so indeed at first this would be for the denonavr platform, but doing it this way it allows for future growth and expansion to other components. I think it is better to have one general way of doing sound mode than have each component come up with its own slightly diffrent approach.

@rytilahti are you serious about implementing sound mode support for the songpal component? How long do you guess it will take you to implement it / when do you have time? If you want I can try to help you a bit, but I dont have songpal wich makes it a bit harder.

@rytilahti
Copy link
Member

@starkillerOG adding those two methods is not much work and songpal devices allow enumerating them over the API, so I can definitely do that pretty quickly. It may require changing some bits in songpal backend code, so getting it into a releaseable state will take a bit longer, but I'm mostly waiting for an OK from @balloob ATM. If an OK is given, I'll make it happen this week.

@starkillerOG
Copy link
Author

@balloob What is the next step / what needs to be done?
Do I need to convert the sound mode support to a denonavr specific service, or do you want to go ahead with general sound mode support?

@starkillerOG
Copy link
Author

@balloob could you please give your opinion and provide some directions? I would really apreciate it!

I would really like to continue to work on getting this implemented, but I need or your approval of the original PR, or directions on how to addapt the original PR or directions for a new approuch.

@starkillerOG
Copy link
Author

@pvizeli since you where the last to work on the media_player/init.py, what do you think?
If we would implement general sound mode support, what do you think is the best way?

I was thinking about the same structure as the input_source structure:
A list attribute that contains all possible sound modes for that component.
And a string attribute that contains the current sound mode.
In the frontend than have a dropdown menu that shows which sound mode is active and that allows you to select a diffrent sound mode (see pictures above).
Of course also the support_sound_mode attribute that can be used to enable/disable the sound mode code for components that have implemented sound mode support or not.

Any input would be much apreaciated, I am open to other structures or any feedback.

@starkillerOG
Copy link
Author

@fabaff You also worked on the media_player/init.py and you helped me before with another PR.
Could you please please provide input, I would really like to move forward with (general) sound mode support, but I don't get much feedback from the core devs.

What do you think about a sound_mode_list structure just like the input_source attribute uses?

@balloob
Copy link
Member

balloob commented May 30, 2018

So how fixed are sound modes ? Can you list some ?

If we want to allow controlling sound modes in the future via voice assistants, the values need to be from a defined list.

@starkillerOG
Copy link
Author

@balloob in principle each component supplies is't own list of sound modes in the current way I did it (just like the input source attribute).
But the sound modes are fairly standard, all Denon and Marantz receivers (DenonAVR component) have the same sound modes that have all been implemented in the denonavr library and my PR.
These sound modes are (also see picture below) :

  • MUSIC
  • MOVIE
  • GAME
  • DOLBY DIGITAL
  • STEREO
  • MCH STEREO (multi channel stereo)
  • AUTO
  • VIRTUAL
  • PURE DIRECT

denonavr_sound_mode_list

@starkillerOG
Copy link
Author

@balloob may I ask how the defined list is done for the input source attribute?
I thought input source was already implemented in voice control?

@balloob
Copy link
Member

balloob commented May 30, 2018

Not that I know off. Smart home skills for Alexa/Google usually have their own values they allow, which they will pass to us. We can only map that if we have a defined list as well. Trying to do some wonky string replaces won't work with other languages.

@starkillerOG
Copy link
Author

I found the PR that was merged that I meant:
alexa: Add media_player InputController support (#11946)
I have no clue how this works, but it looked to me like their is some sort of input source implementation for Alexa (voice control).

@starkillerOG
Copy link
Author

@balloob would you like to allow each component to define their own sound mode list (just like the input source attribute does), or would you like to have one fixed list for all components, (propably hard code that list into media_player/init.py)?

@balloob
Copy link
Member

balloob commented May 30, 2018

Well, it's not about what I want perse, it's what about makes most sense.

For other things building on top of Home Assistant (like Alexa/Google Assistant), fixed values are required. I just don't know if it would make sense because I am also assuming users can define their own sound modes

@starkillerOG
Copy link
Author

@balloob I think it would be better to let each component define their own sound mode list.

Although their are general sound modes that almost all media players will have such as: music, movie, game, stereo. Their are also modes that might not be supported by all media players, for instance MCH STEREO, VIRTUAL or DOLBY DIGITAL only makes sence for surround systems and not for single speakers. Besides some media players might have aditional not common modes like Denon and Marantz have "PURE DIRECT" that might not be so common.

The songpal sound bar from @rytilahti supports:

  • movie
  • music
  • game
  • sports
  • clearaudio
  • standard

So for instance the "clearaudio" mode is not supported for Denon/Marantz but is for Songpal.

@starkillerOG
Copy link
Author

In general users can not specifie their own sound modes, but each component might have diffrent sound modes, so users will have diffrent sound modes based on what media player they have and thus which component they use.

@starkillerOG
Copy link
Author

starkillerOG commented May 30, 2018

@balloob what could be done is hard code an extensive list in the media_player/init.py and then let components pick from that list wich sound modes it supports.

For the Denonavr & Songpal example the general list in media_player/init.py would be:

  • MUSIC
  • MOVIE
  • GAME
  • DOLBY DIGITAL
  • STEREO
  • MCH STEREO (multi channel stereo)
  • AUTO
  • VIRTUAL
  • PURE
  • SPORT

The "clearaudio" from Songpal can then be mapped to "PURE", The "standard" from Songpal to "AUTO", the SPORTS from Songpal to "SPORT" and the "PURE DIRECT" from Denonavr to "PURE".

Each component can then supply a sub-list of this main hard coded list to the media_player/init.py to indicate which modes are supported.
This gains you some generality in the sound modes because it enforces the same spelling, for instance if one component has SPORTS and the other SPORT it will be mapped to the same SPORT value. Besides you will have one complete list in the media_player/init.py that can be used for other things that build on top of HomeAssistant (like Alexa).

Do you think this is worth implementing @balloob, or would you stick to the same thing the input souce attribute does (let the components have full control of the value list) ?

@balloob
Copy link
Member

balloob commented May 30, 2018

Media Player is a component.
Denon and Songpal are platforms.

Let's stick with a dynamic list. No need to create a 1000 constants for one off use by random platforms.

@rytilahti
Copy link
Member

Thank you @starkillerOG for being persistent with this, and my apologies not following so quickly with the implementation for songpal on this.

I see that there could be some use for a static list of specific modes (if downstream forces such), but in generally, I think we need a dynamic list here; although my test setup has those modes @starkillerOG mentioned, there are also others available (but these are device dependent, similar to inputs/outputs), search https://developer.sony.com/develop/audio-control-api/hardware-overview/api-references/svs/getsoundsettings for "soundField" (and even with that, there are other modes someone could count in, e.g., the night mode).

Furthermore, in the case of my sound bar, there is also a separate "sports mode" which will also affect the picture settings. I also feel that things like "MCH STEREO" or "VIRTUAL" are not generic enough to be included. So in my opinion it's either a dynamic list (my preference, I assume not so many people will have a variety of different types of devices, and if so, they'll have to manage it) or a consensus should be formed after downstream (alexa, google assistant etc.) users.

@balloob
Copy link
Member

balloob commented May 31, 2018

Alright, dynamic list it is. Can someone open a PR for the developer website to start documenting the new fields and methods for the media player entity? https://developers.home-assistant.io/docs/en/entity_media_player.html

starkillerOG added a commit to starkillerOG/developers.home-assistant that referenced this issue May 31, 2018
see this Issue in the architecture fork: home-assistant/architecture#23
@starkillerOG
Copy link
Author

@balloob I just created an PR for the developer website.
Could jou have a look at it?

rytilahti added a commit to rytilahti/home-assistant that referenced this issue May 31, 2018
balloob pushed a commit to home-assistant/developers.home-assistant that referenced this issue Jun 1, 2018
* Media player: sound mode structure

see this Issue in the architecture fork: home-assistant/architecture#23

* remove select_sound_mode_supported
@madmicio
Copy link

Marantz Sr-7009 supported?

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Oct 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants