Added new commands and functionality to the harmony remote component. by everix1992 · Pull Request #7113 · home-assistant/core

everix1992 · 2017-04-14T23:05:00Z

-This includes the ability to optionally specify a number of times to repeat a specific command, such as pressing the volume button multiple times.
-Also added a new command that allows you to send multiple commands to the harmony at once, such as sending a set of channel numbers.
-Updated the unit tests for these changes.

Description:

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2432

Example entry for `configuration.yaml` (if applicable):

Checklist:

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

homeassistant · 2017-04-14T23:05:01Z

Hi @everix1992,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

mention-bot · 2017-04-14T23:05:02Z

@everix1992, thanks for your PR! By analyzing the history of the files in this pull request, we identified @iandday, @titilambert and @fabaff to be potential reviewers.

houndci-bot · 2017-04-14T23:05:08Z

line too long (98 > 79 characters)

houndci-bot · 2017-04-14T23:05:08Z

indentation contains mixed spaces and tabs
expected an indented block

houndci-bot · 2017-04-14T23:05:08Z

indentation contains tabs
TabError: inconsistent use of tabs and spaces in indentation

houndci-bot · 2017-04-14T23:05:08Z

indentation contains tabs
indentation contains mixed spaces and tabs
blank line contains whitespace

houndci-bot · 2017-04-14T23:05:08Z

line too long (92 > 79 characters)

houndci-bot · 2017-04-14T23:05:08Z

missing whitespace around operator

houndci-bot · 2017-04-14T23:05:08Z

missing whitespace around operator

houndci-bot · 2017-04-14T23:05:09Z

blank line contains whitespace

houndci-bot · 2017-04-14T23:05:09Z

blank line contains whitespace

houndci-bot · 2017-04-14T23:05:09Z

line too long (91 > 79 characters)

balloob · 2017-04-30T07:40:11Z

We should not add a new service. Instead, by using cv.ensure_list you can make the existing send_command support multiple commands and stay backwards compatible.

The default implementation of send_commands on the abstract base class should be to just call send_command once for each command.

I agree with the comment about the default implementation of send_commands. However, if we are keeping the num_repeats parameter as we were discussing below, I'm not sure if we'd want to fold send_command and send_commands into one service, unless we're fine with being able to repeat a list of commands as well. I don't actually see a problem with this, but I just hadn't thought of it originally.

Also, I'm slightly confused about the backwards compatible comment. These changes shouldn't break backward compatibility because the new parameters for the send_command service have defaults specified. And send_commands wouldn't break backwards compatibility because it's a new service.

You can setup the send_command schema validation in such a way that it is backwards compatible but still takes in multiple commands.

I prefer to keep it in 1 function.

Alright, I'll make this change as soon as I have some free time, which will be at latest tomorrow night.

It looks like merging everything back into one service is going to require some changes to pyharmony, so this'll be on hold until I can get another release out with that change in it.

pyharmony 1.0.16 is up with your changes @everix1992, thanks for your hard work!

balloob · 2017-04-30T07:43:48Z

Flakiness in a connection between remote and device is a concern for the platform. We should not have to add it to the main component.

I guess you can add it as platform config to the harmony platform ?

I'm curious, what do you mean by flakiness? If you're talking about the number of repeats parameter, that has nothing to do with flakiness. The intent of that it to allow you to repeat a command X number of times. For example, if you wanted to increase the volume by 5, you could send the volume up command with a number of repeats of 5.

We should not add this for now as it can already be achieved differently: by just sending multiple commands.

You are correct, but letting this be specified as a number allows for much easier integration. If I'm trying to trigger something through IFTTT, I can't generate a list of commands on the fly. I can, however, take the number given to me and pass that on to home assistant.

Alright, let's keep it in 👍

cdrage · 2017-05-01T16:06:18Z

Hi @everix1992

Before, I used a work-around to repeat the commands (using the script functionality of home assistant, so simply enter the command X amount of times). The unfortunate part was that it was delayed about 2-3 seconds each-time it inputted the command. My assumption was that it was either a built-in hass delay or an issue with communicating to the harmony remote back-end API.

Does this implementation experience that same issue? (I'm not home yet to try this PR out with my Harmony Hub)

cdrage · 2017-05-01T16:09:08Z

Harmony Hubs have built-in delays for each command. So each-command would be 400ms + Harmony's configured delay (for ex. mine is set to 300ms). So for each vol-up it'd be 700ms which would still be slower than using a physical remote.

Is there a way to change this delay value within configuration.yaml, also, have you tested it with a lower configured delay? I'm assuming HA is multi-threaded so it could handle these commands.

In response to your first question, no this PR doesn't experience that issue. The reason that behavior occurred within the script was due to how commands were being sent in the pyharmony library. I've worked with the developer of pyharmony to get that issue fixed.

With that issue gone, 0.4 seconds was the appropriate delay for my setup in which I was using the default delay for Harmony Hubs (didn't even realize there was a setting to change for that).

And yes, I did some testing when I was testing this and 0.4s seemed about right. Any faster (0.1 or 0.2) and I would drop commands. There was no benefit to going any slower.

Take this with a grain of salt since I'm fairly new to HASS development, but I think that it would be possible to add a default delay value to the configuration.yaml that would override the 0.4 seconds.

@everix1992 Awesome! Thanks a lot man. I'll test this PR on my personal Harmony Hub as well this week.

cdrage · 2017-05-03T13:32:35Z

This LGTM 👍

balloob

All remote platforms have to be updated to accommodate for a list of commands being passed in. Not just the demo and harmony platforms.

everix1992 · 2017-05-15T23:13:22Z

@balloob Ah, my bad. The commit I was working off of did not have any other remotes added. I'll rebase and get them fixed up.

houndci-bot · 2017-05-15T23:21:07Z

undefined name 'commands'

balloob

Ok to merge when tests pass 👍

houndci-bot · 2017-05-19T04:00:10Z

missing whitespace around operator

balloob · 2017-05-23T22:15:05Z

Ok to merge when conflicts resolved. Not sure what has changed.

-This includes the ability to optionally specify a number of times to repeat a specific command, such as pressing the volume button multiple times. -Also added a new command that allows you to send multiple commands to the harmony at once, such as sending a set of channel numbers. -Updated the unit tests for these changes.

… list of commands

…armony version in requirements_all.txt

everix1992 · 2017-05-23T23:04:00Z

Should be fixed now. Was a silly line endings conflict.

…

On Tue, May 23, 2017 at 5:15 PM, Paulus Schoutsen ***@***.***> wrote: Ok to merge when conflicts resolved. Not sure what has changed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7113 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACJAYG83znsNBWWRZCblVF3AtrtOthiHks5r81pxgaJpZM4M-KoY> .

homeassistant added integration: remote platform: remote.harmony cla-needed labels Apr 14, 2017

houndci-bot reviewed Apr 14, 2017

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Apr 14, 2017

balloobbot added platform requirement labels Apr 14, 2017

everix1992 mentioned this pull request Apr 19, 2017

Add documentation for harmony commands home-assistant/home-assistant.io#2432

Merged

balloob requested changes Apr 30, 2017

View reviewed changes

cdrage reviewed May 1, 2017

View reviewed changes

homeassistant added the cla-signed label May 14, 2017

balloob requested changes May 15, 2017

View reviewed changes

homeassistant added the cla-signed label May 15, 2017

houndci-bot reviewed May 15, 2017

View reviewed changes

Comment thread homeassistant/components/remote/kira.py Outdated

Copy link
Copy Markdown

houndci-bot May 15, 2017

Choose a reason for hiding this comment

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

undefined name 'commands'

balloob approved these changes May 17, 2017

View reviewed changes

houndci-bot reviewed May 19, 2017

View reviewed changes

Comment thread tests/components/remote/test_demo.py Outdated

Copy link
Copy Markdown

houndci-bot May 19, 2017

Choose a reason for hiding this comment

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

missing whitespace around operator

homeassistant added the cla-signed label May 19, 2017

robbiet480 added the cla-recheck label May 20, 2017

homeassistant added cla-signed and removed cla-recheck labels May 20, 2017

everix1992 added 6 commits May 23, 2017 17:55

Fix flake8 coding violations

4c8c07a

Remove send_commands command and make send_command handle a single or…

8b17224

… list of commands

Remove send_commands tests

e75c154

Update itach and kira remotes for new send_command structure. Fix pyh…

ed85542

…armony version in requirements_all.txt

Fix incorrect variable name

a4d2699

Fix a couple minor issues with remote tests

50e29a8

homeassistant added the cla-signed label May 23, 2017

balloob merged commit 3638b21 into home-assistant:dev May 24, 2017

balloob mentioned this pull request Jun 2, 2017

0.46 #7866

Merged

home-assistant locked and limited conversation to collaborators Sep 4, 2017

ghost added integration: harmony and removed platform: remote.harmony labels Mar 21, 2019

Uh oh!

Conversation

everix1992 commented Apr 14, 2017

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

homeassistant commented Apr 14, 2017

Uh oh!

mention-bot commented Apr 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everix1992 May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everix1992 May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdrage commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdrage May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everix1992 May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdrage commented May 3, 2017

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

everix1992 commented May 15, 2017

Example entry for `configuration.yaml` (if applicable):

everix1992 May 1, 2017 •

edited

Loading

everix1992 May 5, 2017 •

edited

Loading

cdrage commented May 1, 2017 •

edited

Loading

cdrage May 1, 2017 •

edited

Loading

everix1992 May 1, 2017 •

edited

Loading