home-assistant / architecture

Repo to discuss Home Assistant architecture
315 stars 99 forks source link

Support learning different command types with remote #438

Closed felipediel closed 3 years ago

felipediel commented 3 years ago

Context

Some devices support learning different command types, such as Broadlink RM pro and RM4 pro, which support both IR and RF commands. In such cases, it is necessary to specify the type of command that we are learning. So I made this PR to implement this feature by adding the command_type parameter to remote.learn_command service.

It would allow us to add RF functionality to Broadlink's remotes, but it was reverted here because we haven't discussed these architectural changes before. So here I am.

Proposal

New feature: Add the command_type option to remote.learn_command service.

Example entry for configuration.yaml:

# Example configuration.yaml
script:
  learn_tv_power_button:
    sequence:
      - service: remote.learn_command
        data:
          entity_id: remote.garage
          device: car
          command: unlock
          command_type: rf

Consequences

There are no breaking changes. Broadlink is the only integration using remote.learn_command so far. I was the one who proposed remote.learn_command a year ago. This interface still needs to be improved. We would also need to have a remote.delete_command, I see lots of users asking for this in the community, but this is another subject.

MartinHjelmare commented 3 years ago

Is it not good to encode the command type in the command name?

felipediel commented 3 years ago

That was my initial experiment. I can make it work, but I don't like it. Here are my reasons:

1. Confusing

We already have the b64: prefix for sending. This would introduce the ir: and rf: prefix for learning. One needs to be used before the code when sending and the others before the command name when learning. And they cannot be switched. Normal people don't know how to use these things. They would simply mix up everything and think it was my fault.

2. Redundant

The command_type is the same for all commands. Having a prefix would introduce redundancy when we want to learn a list of commands.

# Example ugly configuration.yaml
script:
  learn_tv_power_button:
    sequence:
      - service: remote.learn_command
        data:
          entity_id: remote.bedroom
          device: humidifier
          command:
            - rf:turn on
            - rf:turn off
            - rf:light on
            - rf:light off
            - rf:low
            - rf:medium
            - rf:high
# Example beautiful configuration.yaml
script:
  learn_tv_power_button:
    sequence:
      - service: remote.learn_command
        data:
          entity_id: remote.bedroom
          device: humidifier
          command:
            - turn on
            - turn off
            - light on
            - light off
            - low
            - medium
            - high
          command_type: rf

3. Not intent-friendly

The idea behind remote.learn_command is being able to create interactions like:

If we mix commands and types things will get messy.

4. Complex

Having a prefix for each command can bring us situations that we would like to avoid. Let's say the user does something like this:

# Example ugly configuration.yaml
script:
  learn_tv_power_button:
    sequence:
      - service: remote.learn_command
        data:
          entity_id: remote.bedroom
          device: humidifier
          command:
            - rf:turn on
            - ir:turn off
            - rf:light on
            - ir:light off
            - rf:low
            - ir:medium
            - rf:high

We can resolve this request, but it makes our logic much more complex and difficult to maintain. I think it would be better not to open this possibility, which once opened could hardly be undone without breaking changes.

The truth is that the command_type option is much more related to the type of remote than to the type of command itself (which derives from that). How about renaming it to remote_type?

felipediel commented 3 years ago

I think the downside of this decision is that if # 4 were really necessary, there would be no way to do it. But I have never seen a remote that sends both types. What do you think?

Edit: Actually there would be a way to do it. Just call the service twice. But that's tricky anyway.

MartinHjelmare commented 3 years ago

I think the command type service attribute makes sense.

Side note: The existing alternative service attribute does not make sense to me. It was never mentioned in the architecture issue that described the remote learn command service before we added it. It would be good to do a holistic oversight of this service. Maybe the broadlink integration needs a custom service instead if broadlink has special requirements?

felipediel commented 3 years ago

The alternative is common to all remotes. They usually use the same button for more than one thing (e.g. power = on / off), and these codes alternate (there are usually one or two bits in the code dedicated to this). If you send the same code twice it won't work. So if the user wants to learn a button like power, mute, or sometimes even volume, he needs to specify this flag so that we can learn an extra code and switch when sending. Any IR implementation that ignores this doesn't have a good response rate.

Just out of curiosity. The LIRC project has this feature and when the maintainers stopped paying attention to it the old users were pissed off because they knew it was necessary.

felipediel commented 3 years ago

But I agree that the name is not very good. How about renaming it to toggle?

MartinHjelmare commented 3 years ago

How is the alternate feature referenced in other remote implementations or devices? If there's a name that is common we should probably stick with that.

felipediel commented 3 years ago

LIRC calls it the toggle_bit. Since we are not referring to a single bit here (we capture the entire code again so as not to require user intervention), I think we can stick with toggle.

felipediel commented 3 years ago

I took this sample from their database:

begin remote

  name  FW_530C
  bits           13
  flags RC5|CONST_LENGTH
  eps            30
  aeps          100

  one           882   894
  zero          882   894
  plead         869
  gap          113783
  toggle_bit      2

      begin codes
          KEY_PLAY                 0x0000000000001535        #  Was: Play
          KEY_STOP                 0x0000000000001536        #  Was: Stop
          KEY_NEXT                 0x0000000000001520        #  Was: Next
          KEY_PREVIOUS             0x0000000000001521        #  Was: Prev
          KEY_VOLUMEUP             0x0000000000001410        #  Was: Vol+
          KEY_VOLUMEDOWN           0x0000000000001411        #  Was: Vol-
          KEY_REWIND               0x0000000000001532        #  Was: Rew
          Forw                     0x0000000000001534
          KEY_AGAIN                0x000000000000151D        #  Was: Repeat
          Shuffle                  0x000000000000151C
          KEY_PAUSE                0x0000000000001530        #  Was: Pause
          Program                  0x0000000000001524
#         DBB                      0x0000000000001537
          DBB2                     0x0000000000000406
          DSC                      0x000000000000040F
          INC_SURR                 0x0000000000000400
          KEY_1                    0x0000000000001537        #  Was: 1
          KEY_2                    0x0000000000001538        #  Was: 2
          KEY_3                    0x0000000000001539        #  Was: 3
          KEY_CD                   0x000000000000153F        #  Was: CD
          Tape1/2                  0x00000000000014BF
          Side                     0x00000000000014AF
          KEY_TUNER                0x000000000000147F        #  Was: Tuner
          KEY_AUX                  0x000000000000157F        #  Was: Aux
          KEY_SLEEP                0x000000000000154C        #  Was: StandBy
      end codes

end remote
felipediel commented 3 years ago

What about remote.delete_command? Do you think it is a good idea? Check this out. People need this.

felipediel commented 3 years ago

I think the remote entity would be much better with these three things:

MartinHjelmare commented 3 years ago

toggle sounds very general to me, but if that is the common name it's ok I think. Are there other projects or manufacturers that use this name or another name?

Command type and a remote.delete_command service makes sense to me.

We need some more opinions on this though before we proceed.

felipediel commented 3 years ago

Sometimes people call this a toggle_command. Better? I think that alternative wouldn't be wrong either, it's like a property.

elupus commented 3 years ago

The toggle bit is most often used to differentiate between long press (button held down) on the remote button and multiple presses (tapping). There is no difference in the interpretation meaning of the command. (it will keep on meaning volume up, turn on, turn off, standby) independent of the toggle bit.

The alternating/toggling function of the button is something the receiving device is in charge of.

felipediel commented 3 years ago

If the device receives two identical codes in a row for a toggle command, no matter the interval between them, it will consider it a long press and will not change the state. You need to switch the bit to have a good response rate.

felipediel commented 3 years ago

Sometimes you can solve this by cutting the code. But this requires user intervention. The idea here is to do something that works with one click.

elupus commented 3 years ago

yes, i just wanted to clearify that it's not about on/off.

felipediel commented 3 years ago

It never was. The name toggle may lead to this conclusion, perhaps its better to stick with alternative. What do you think?

elupus commented 3 years ago

I think toggle is the standard naming. So stick with that.

felipediel commented 3 years ago

Ok. Have we reached a consensus? Should I start doing these three things right now?

felipediel commented 3 years ago

Another need that may arise is to delete all commands related to a device. My suggestion is to use remote.delete_command interface with a special attribute (e.g. command: all) for this.

felipediel commented 3 years ago

I was thinking...

alternative means "capture an alternative code for this command". This makes sense. This is what we are doing.

toggle may lead to the idea of an "on/off" command or something that has a state, which is not always true. Sometimes volume, channel and other buttons also have this alternating bit.

So we should not change this. I see no reason for a breaking change. We are better with what we have.

My vote is to add remote.delete_command and command_type and leave alternative intact.

@frenck Do you agree with these changes?