New media player service that send a key to the device

tulindo commented 4 years ago

Context

All televisions (but not only TVs) do have a remote controller that basically sends keys to the device in order to control it

Proposal

Add to the media player component a service that sends control keys. I already made a pull request (https://github.com/home-assistant/home-assistant/pull/27587) with the service defined and implement for the samsungtv component

Consequences

The first that came into mind is that a frontend developer will be able to build a remote control to manage every TV.

dgomes commented 4 years ago

I support this, can be used with mediaroom (already had a specific service) and more generally broadlink

balloob commented 4 years ago

That's why we have the remote integration . It is pretty common for integrations to support multiple platforms.

tulindo commented 4 years ago

@balloob If I've understood correctly: instead of adding to media player the send key feature you suggest to implement the https://www.home-assistant.io/integrations/remote (send_command I suppose) at component level. Is that correct?

dgomes commented 4 years ago

I might be the one misunderstanding but:

Remote integration, integrates remotes
The proposed issue standardises services made available by media_players

So the remote integration is used to get events from remotes, and this proposal would provide services available by media players. Ultimately I would create an automation to to trigger such services based on the events of a remote.

I think they are opposite sides of the coin.

Sorry If I'm not getting this right

MartinHjelmare commented 4 years ago

The remote integration provides a service to send a command. This is what this issue is after, right? To be able to send a command from a remote of a tv or similar device. Since we already provide this in the remote integration, the solution is to implement a remote platform for the device that needs this feature.

https://github.com/home-assistant/home-assistant/blob/1cae6e664c39f50c7254c80adf228e362e8099b9/homeassistant/components/remote/__init__.py#L130-L131

We don't need to further complicate the media_player integration.

tulindo commented 4 years ago

That's clear @MartinHjelmare. I'm going to implement in a new PR closing the current one. Few questions:

Since both remote and media_player integration do have turn on/off and toggle service. I can easily implement them in the remote platform. It's better to copy code from the media player or put the code in a single place... (if so... where?)
The main parameter of the send_command is a string describing the command. I'd like to make this command "standard" across all remotes (that was the idea behind the PR... otherwise I could have simply implemented a new service for my TV). What do you suggest? Try to explain better... if the aim is to implement a frontend component that can control any kind of TV it will have to use a standard command set (this is what I made in my original PR for the media_send_key service). Isn't send_command too generic?

Regards, Paolo

tulindo commented 4 years ago

@MartinHjelmare, @balloob and @dgomes: I do agree with the remote integration suggestion but, IMHO, I think that if we don't define a standard set of commands like I did in my PR for the media_send_key service (where I defined a list of supported keys), it will be useless (in order to have a generic behaviour between platforms)

tulindo commented 4 years ago

What about "simply" moving my media_send_key service from media_player to the remote integration?

tulindo commented 4 years ago

@MartinHjelmare and @balloob what's your opinion? Should I go for a Samsung media player send_key service (that will expose to frontend and the Samsungctl functionality) make a generic send_control_key service at remote level (with fixed generic key set) or use the already defined send_command and implement it for Samsung TVs. Thanks

MartinHjelmare commented 4 years ago

I suggest making a remote platform for the samsungtv integration. Note that this will require a refactor of the existing media_player platform first. to extract the connection to the samsungtv component.

Also note other ongoing samsung PRs.

tulindo commented 4 years ago

@MartinHjelmare. Implementing the remote platform will also require the refractor of the media player platform for sure. My question was {and still is) : is the send_command service the right place to expose the Samsungctl send_key functionality?

IMHO a command is too much generic (it means all and nothing at the same time} ... I would go for a more specific one (maybe with a set of supported basic keys) What do you think? Thanks, Paolo

MartinHjelmare commented 4 years ago

That's my suggestion.

tulindo commented 4 years ago

@MartinHjelmare and @balloob sorry I closed the issue by mistake. I did not exactly understand what Martin's suggestion. Implement the remote platform as well as refactoring the media_player platform for the samsungtv compoment is clear. But, what about the original intent of exposing the samsungctl's send_key service?

Implement the already defined send_command
Create a samsung_send_key service (inside the remote platform)
Add a send_remote_key in the remote integration and implement it in the samsungtv remote platform

I would go for the third, what is your opinion? Regards, Paolo

MartinHjelmare commented 4 years ago

1 is my suggestion.

tulindo commented 4 years ago

OK. In the send command parameter:

Entityid will be the ID of the remote itself (i.e. remote.mytv)
command will be the name of the key (as exposed by samsungctl)... well a list of keys
what about device?

Here's my doubt about the device: I've two samsung TV (media_player.liginvroom and media_player.bedroom) Would you have a single remote entity (i.e. remote.samsungtv) and in the device identity specify one of the 2 TV? or 2 remote entities (in this case the device parameter will be useless) Thanks, Paolo

MartinHjelmare commented 4 years ago

Each device should represent its feature set as one or more entities. So if you have two samsung TVs, ie two devices, they should each spawn their own remote entity, representing the remote feature. So two TVs should spawn two remote entities.

tulindo commented 4 years ago

@escoand I created this issue in order to find the best way for the exposal of the Samsungctl send key service. As @MartinHjelmare said "Note that this will require a refactor of the existing media_player platform first. to extract the connection to the samsungtv component" Since you are working on the media player platform I think it will be better to get in touch. Little bit off topic... Do you have any idea on how to detect if the TV is actually displaying a TV program?

escoand commented 4 years ago

AFAIK not possible with samsungctl. Especially the legacy protocol is a one-way. With the websocket I'm not familiar. But there are other libraries which can maybe fill the gap: https://github.com/roberodin/ha-samsungtv-custom

escoand commented 4 years ago

@tulindo what do you mean by "require a refactor"?

tulindo commented 4 years ago

I think that basically @MartinHjelmare wants to keep common code in a single shared component in order to avoid code duplication.

tulindo commented 4 years ago

@escoand since your PR related to config flow for Samsung TV.has been accepted. Do you think that that part can be put in a shared file in order to reuse it in the remote platform I'd like to implement for samsungtv? Can you briefly write me down 2 lines of documentation about your work that will help me to understand it better? (I'm new to HA and python) Thanks

escoand commented 4 years ago

My merged PR was about automatic protocol/port detection. My next PR (currently in preparation) is about the config flow. The send_key service was for me just an easy way to test new and additional functionality for more extensions. My PR was for make the internal send_key method accessible from the UI. Your thoughts about this all are already further than I was ever.

tulindo commented 4 years ago

@escoand What's changed from a user point of view with your PR? (less things to configure by hand I suppose)

escoand commented 4 years ago

Yes, you just don't need to set the port, but it's still possible. So no breaking changes.

tulindo commented 4 years ago

Clear. Off-topic: I've seen that in order to turn on the TV thare are currently 2 options. If user configured mac address the wake on lan feature is used, instead it sends the power on key using samsung ctl. I'm using wakeonlan on my setup. I don't know if the poweronkey works (simply never tried) I've seen that there is a package called getmac (https://pypi.org/project/getmac/)... do you think we can use it in order to have always wakeonlan used? Can be useful?

Going back in topic: Speaking about refactoring I'm thinking of adding a new class (named SamsungTVController) that will act as a wrapper for the samsungctl component. This class wil be a property of SamsungTVDevice and will expose the send key service and also deal with configuration stuff. Same service will be used in the remote platform. What do you think about my idea?

escoand commented 4 years ago

@tulindo not sure. I would probably do it with multi inheritance. So we could have one base SamsungTV class and two deriving, maybe class SamsungTVPlayer(MediaPlayerDevice, SamsungTVBase) and your remote class SamsungTVRemote(RemoteDevice, SamsungTVBase). So we could move just some internal methods to the base class and don't need to change quite much in the media player. This would also help me for config flow to have a base class which is able to detect devices and their correct configuration. But don't know if this is best practice. Just know that Java explicitly does not support such things because of understanding problems for the programmer/compiler.

To the off topic: also thought about it already. But knowing the mac address does not guarantee power on is working. For my legacy device it's not.

Would be interested what @balloob or @MartinHjelmare say to all this.

tulindo commented 4 years ago

I wasn't aware of python supporting multi inheritance. For me it's a better option than my original idea. Let's see what they say about this. @escoand we can also proceed in parallel with the two different approaches and see what we get at the end. @MartinHjelmare and @balloob what do you think?

tulindo commented 4 years ago

I just found that appletv has both media player and remote. And looks like the shared code is in the init.py file

escoand commented 4 years ago

My code is before initializing the media player. It's the discovery. So it should not get in touch with your suggested changes. Is appletv using global functions or does it use our discussed approach?

tulindo commented 4 years ago

Looks like they use a common class. But I gave just a quick look using my mobile. I think discovery can too be shared between media player and remote. The controlled device is always the same.

bendavid commented 4 years ago

Something to keep in mind is that the use case for this sort of thing goes beyond just sending button presses. Many of these TV's/receivers have websocket/http/etc api's supporting a wide range of commands.

The remote.send_command call seems to have some infrared-centric stuff, and "device" as a required parameter doesn't really make sense in this context, since the controlled device will be specified through the entity_id as already discussed. "device" could be abused for what I have called "command_type" in my PR, but in this case at least the remote api would have to be modified to make this parameter optional for send_command.

It's also not clear that it makes sense for all of these tv/receiver/etc integrations to have to support the remote api, with the associated redundancy of turn_on and turn_off and associated state with the primary media_player component.

So I still think there is an argument for adding a generic command call to the media_player api.

tulindo commented 4 years ago

@bendavid your observations make sense to me. IMHO the remote integration is something obscure... for instance speaking about media player, what is a remote? is it the HA representation of the standard remote controller device that every TV has or it is another way to see the TV itself? At home I have 2 Samsung TV. both can be controlled by the same physical device. In the HA "world" I can see 2 options

single remote entity that will control 2 media player entity: In this example the device_id of the send command will be the entity id of the controlled media player. turn on and turn off service will not make sense (they will act against the remote itself and not against the TVs)
a remote entity every media player. in this example turn on and off will act just like the media player services and the device parameter will be simply useless...

I think that we need clarifications from @balloob

escoand commented 4 years ago

The first option doesn't make sense to me. You have 1 remote somebody has maybe 3. Should he config also 3 of the remote entities?

The whole remote platform is a bit confusing to me. Why is the remote interesting for HA, it's just a way to communicate with a device. And we already have a way to handle the TV, the media player component.My TV has also buttons at the backside. But nobody would implement a platform for this.

An exception could be something like a Logitech Harmony but more in the use of a gateway.

bendavid commented 4 years ago

The harmony has state (activities) and can be controlled (switching activities or sending commands to other devices defined in the harmony configuration but not necessarily in HA), so the remote platform makes perfect sense in that context.

JPHutchins commented 4 years ago

For context, I work in residential AV installation where customers spend $$ on "logitech harmony on steroids". These systems have a long legacy, are old and buggy, have difficulty with IP commands, etc but what they get right is their "representation of a media system."

A media system would be represented as sources (media_player components) that are routed to receivers (av_receiver components or display components) and subsequently routed to a display if the receiver is not also display. A representation of its physical connections.

Therefore, when adjusting the volume of the source the media_player component would ask the av_receiver component or display to adjust the volume. This media system has one interface for changing sources which would be handled by the receiver or display.

To the end user this representation is monolithic - they have a single interface that controls a single media system in a single location and the controls are consistent and simple regardless of whether their commands are sent to a source, receiver, or display.

Most of the community questions around media_player implementations are due to how cumbersome it would be to implement EVERY command available to a particular AV receiver. There are hundreds. @bendavid is suggesting to make available to the end user a means to expose commands that are important to them. In his case a toggling mute, in my case dynamic range control. For other users this is a matter of simply fixing one broken command since they have a slightly different version. The user should be able to expose their command in configuration.yaml:

a_media_player_platform:
  host: 10.10.10.101
  commands:
    type: http_get
      - name: "Mute"
        command: "/goform/formiPhoneAppDirect.xml?RCKSK0410370"

Where the end user or community has directed them to the magic IR or telnet bandaid that the API failed to support.

bendavid commented 4 years ago

(As an aside, one of the things that breaks this source, receiver, display model is the advent of "smart tv's" and receivers with built in streaming functionality, such that the display or receiver also acts as a source.)

tulindo commented 4 years ago

@escoand the remote I was meaning in my first option was intended as one virtual device to control more Samsung TVs (even if you're 10 physical remotes at home)

bendavid commented 4 years ago

Coming back to this, there are at least two distinct ways this ~same use case is currently dealt with in home assistant for integrations which are primarily accessed through the media_player domain.

1) Generic commands handled through a separate domain. E.g. kodi.call_method https://github.com/home-assistant/home-assistant/blob/78b83c653a6ffe8ce6cd62ccea067f3c146163e3/homeassistant/components/kodi/services.yaml#L22

2) Independent commands handled through remote domain, e.g. apple_tv through remote.send_command

I have personally contributions submitted or to be submitted for denonavr and webostv integrations to address a similar use case.

I can see a few possibilities for how to support this going forward

a) Add platform dependent service calls in a separate domain for each integration (as for kodi currently)

b) Support remote.send_command service in these cases (can we skip implementing redundant state and turn_on/turn_off services in this case?) -I see that the device parameter is already optional here, so ok. -Supporting all use cases might require adding a "command_type" or similar parameter to remote.send_command, which would of course make this service increasingly platform dependnet

c) Add send_command or equivalent to media_player domain

poroping commented 4 years ago

For a real-world example here is a PR I made which is a good example of service calls that could easily fit in both the remote or media_player domains. (calling play_media to launch youtube app and play a video on an lg smart tv).

https://github.com/home-assistant/home-assistant/pull/27283

MartinHjelmare commented 3 years ago

We use the remote integration with platforms for this purpose:

https://developers.home-assistant.io/docs/core/entity/remote

home-assistant / architecture