home-assistant / architecture

Repo to discuss Home Assistant architecture
319 stars 100 forks source link

Sound mode support media_player general and denonavr #23

Closed starkillerOG closed 6 years ago

starkillerOG commented 6 years ago

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 commented 6 years ago

sound_mode_support

sound_mode_support2

starkillerOG commented 6 years ago

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

balloob commented 6 years ago

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

starkillerOG commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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) :

denonavr_sound_mode_list

starkillerOG commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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:

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

starkillerOG commented 6 years ago

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 commented 6 years ago

@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:

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

madmicio commented 6 years ago

Marantz Sr-7009 supported?