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 support for learning new commands #23888

Merged
merged 6 commits into from
Jun 5, 2019

Conversation

felipediel
Copy link
Contributor

This update creates a generic service in the 'remote' component to enable remote control platforms to learn new commands.

Description:

Related issue: #236

Example 1: Learn a single command

- service: remote.learn_command
  data:
    entity_id: remote.bedroom
    device: television
    command: 'Turn on'

Example 2: Learn a list of commands

- service: remote.learn_command
  data:
    entity_id: remote.bedroom
    device: television
    command:
      - 'Turn on'
      - 'Turn off'
      - 'Source'
      - 'Channel up'
      - 'Channel down'
      - 'Volume up'
      - 'Volume down'

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

This update creates a generic service in the 'remote' component to enable remote control platforms to learn new commands.
Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

For which platform will that be later?

EDIT: I saw it on architecture issue :)

@pvizeli pvizeli self-assigned this May 17, 2019
@pvizeli
Copy link
Member

pvizeli commented May 17, 2019

Please add a supported_feature functionality for that. I.e. Light/Climate/MediaPlayer

@felipediel
Copy link
Contributor Author

felipediel commented May 17, 2019

Please add a supported_feature functionality for that. I.e. Light/Climate/MediaPlayer

Hello friend. Do you mean something like this?

REMOTE_SERVICE_LEARN_COMMAND_SCHEMA = REMOTE_SERVICE_SCHEMA.extend({
    vol.Optional(ATTR_DEVICE): cv.string,
    vol.Optional(ATTR_COMMAND): vol.All(cv.ensure_list, [cv.string]),
    vol.Optional(ATTR_ALTERNATIVE): cv.boolean,
    vol.Optional(ATTR_TIMEOUT): cv.positive_int,
    vol.Optional(ATTR_SUPPORTED_FEATURES): vol.Any('Light', 'Climate', 'MediaPlayer')
})

Okay, I'll do it, just tell me the types that it needs to support, or if you want me to simply validate a string and leave for the platforms to choose.

Just out of curiosity, how will you implement this functionallity inside the remote control platforms?

I ask this because I'm developing a platform and I have the following JSON format in mind to store the codes:

{
    "Bedroom TV": {
        "type": "Television",
        "brand": "Philips",
        "model": "42PFL3403",
        "commands": {
            "Turn on": "JgA0ABsbHRo4Gh0aHBobHB0ZHRobNx0aOBocAA0F==",
            "Turn off": "JgA0ABscNhwbGxscGxsbHBscGxsbNxscNhwbAA0F=="
        }
    }
}

Here I've opted to include "type", "brand" and "model" information. These labels are not directly useful for sending and receiving codes, but I believe they are important for users to share their codes in the future and keep their collection of codes organized. But I decided not to receive these header entries through the learn_command service for two reasons:

  1. This information only needs to be set once per device, and I believe the best way would be through config entries or some kind of data flow. It would be a huge task to repeat the brand and model for every code the user wants to learn.

  2. This approach may lead to conflicts, let's assume that the user sends this payload:

- service: remote.learn_command
  data:
    - device: 'Bedroom TV'
      type: 'Television'
      brand: 'Philips'
      model: '42PFL3403'
      command: 'Turn on'

Followed by this another one:

- service: remote.learn_command
  data:
    - device: 'Bedroom TV'
      type: 'Television'
      brand: 'Philips'
      model: '52PFL3404'
      command: 'Turn off'

The models conflict for the same device, which one should be stored in the JSON file?

So maybe this also aplies to 'supported_features', what if I tell it's a 'MediaPlayer' when I capture the first command and then tell this is a 'Light' when I capture the second. How should platforms handle it?

I think storing the device type is a great idea to keep the codes organized, but maybe this information should be captured elsewhere and only once per device.

Alright, I've made my point, but it's up to you to decide. Just tell me and I'll do it.

@pvizeli
Copy link
Member

pvizeli commented May 18, 2019

@felipediel
Copy link
Contributor Author

I mean some things like that: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/light/__init__.py#L34-L41

Got it! What do you think of adding the following lines to the RemoteDevice class in the __init__.py file:

@property
def supported_features(self):
    """Return the list of supported features."""
    raise NotImplementedError()

I think it would be interesting to define some constants to be imported by platforms that will use this property, but I do not know what kind of features you want to support? Please give me a list of some features you would like to support at first and I will add the constants to the code.

@pvizeli
Copy link
Member

pvizeli commented May 26, 2019

- Add 'supported_features' property and a constant related to the 'learn_command' functionality.
- Redefine 'async_learn_command' function as a coroutine.
@felipediel
Copy link
Contributor Author

@felipediel

Default is: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/light/__init__.py#L480-L483

Your new feature is that what we need as SUPPORT_LEARN_COMMAND

Now I see the full picture. Done.

Adding the 'supported_features' attribute generated an assertion error on the 'Demo Remote' platform. This update fixes this.
This update fixes a typo that occurred at the last update.
@pvizeli pvizeli merged commit 0ed9e18 into home-assistant:dev Jun 5, 2019
@balloob balloob mentioned this pull request Jun 26, 2019
@felipediel felipediel mentioned this pull request Sep 9, 2019
8 tasks
alandtse pushed a commit to alandtse/home-assistant that referenced this pull request Oct 12, 2019
* Add support for learning new commands

This update creates a generic service in the 'remote' component to enable remote control platforms to learn new commands.

* Update __init__.py with the proposed changes

- Add 'supported_features' property and a constant related to the 'learn_command' functionality.
- Redefine 'async_learn_command' function as a coroutine.

* Update __init__.py

* Fix assertion error

Adding the 'supported_features' attribute generated an assertion error on the 'Demo Remote' platform. This update fixes this.

* Fix duplicated 'hass' object

This update fixes a typo that occurred at the last update.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants