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 arguments with effects #13500

Closed
wants to merge 6 commits into from
Closed

Conversation

starkillerOG
Copy link
Contributor

@starkillerOG starkillerOG commented Mar 27, 2018

Description:

Hyperion: Add posibility for arguments with effects.
If the turn on service is called with the effect parameter, now effect arguments of hyperion can be used in the call following this syntax:

  • service: light.turn_on
    data:
    entity_id: "light.tv_light"
    effect: "EFFECT_NAME||ARGUMENT1=VALUE1||ARGUMENT2=VALUE2"

so first the name of the effect, then a seperation sign "||" then the name of the first argument followed by the value of this argument seperated with an "=" sign. A unlimeted amount of argumets can follow each with a "||" between arguments and a "=" between the argument and its value.

For instance two working effects for Hyperion including arguments would be:
effect: "Cinema brighten lights||color-start=[ 0, 0, 0 ]||color-end=[ 255, 136, 0 ]||fade-time=60.0"
or
effect: "Knight rider||speed=1.0||fadeFactor=0.7||color=[0,255,0]"

Pull request in home-assistant.github.io with documentation : home-assistant/home-assistant.io#5037

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

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

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

Hyperion: Add posibility for arguments with effects.
If the turn on service is called with the effect parameter, now effect arguments of hyperion can be used in the call following this syntax:
- service: light.turn_on
   data:
       entity_id: "light.tv_light"
       effect: "EFFECT_NAME||ARGUMENT1=VALUE1||ARGUMENT2=VALUE2"

so first the name of the effect, then a seperation sign "||" then the name of the first argument followed by the value of this argument seperated with an "=" sign. A unlimeted amount of argumets can follow each with a "||" between arguments and a "=" between the argument and its value.

For instance two working effects for Hyperion including arguments would be:
effect: "Cinema brighten lights||color-start=[ 0, 0, 0 ]||color-end=[ 255, 136, 0 ]||fade-time=60.0"
or 
effect: "Knight rider||speed=1.0||fadeFactor=0.7||color=[0,255,0]"
json_mess['effect']['args'] = {}
for i in range(1,len(mess)):
arg = mess[i].split('=')
if len(arg)==2:

Choose a reason for hiding this comment

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

missing whitespace around operator

}
if len(mess)>1:
json_mess['effect']['args'] = {}
for i in range(1,len(mess)):

Choose a reason for hiding this comment

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

missing whitespace after ','

'command': 'effect',
'priority': self._priority,
'effect': {'name': self._effect}
})
}
if len(mess)>1:

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 homeassistant added platform: light.hyperion small-pr PRs with less than 30 lines. labels Mar 27, 2018
@starkillerOG
Copy link
Contributor Author

@MartinHjelmare @balloob could one of you review and merge this PR, its just a few lines?

@balloob
Copy link
Member

balloob commented Mar 28, 2018

😠 Don't tag people for comments, there are 121 open PRs.

We should never use literal_eval.

@starkillerOG
Copy link
Contributor Author

@balloob sorry, I found a solution, now using json.loads() instead of ast.literal_eval()

@thundergreen
Copy link

I hope there will be a good documentation about arguments

@pvizeli
Copy link
Member

pvizeli commented Mar 29, 2018

Make a new platform service for the Hyperion if you need a other format like we use on core like other light platform does

@fabaff fabaff changed the title Hyperion: Add arguments with effects Add arguments with effects Mar 29, 2018
@balloob
Copy link
Member

balloob commented Mar 30, 2018

Don't use json load either. Service data can already contain any data structure.

@starkillerOG
Copy link
Contributor Author

@thundergreen their is already documentation about the effect arguments on the hyperion site, I added these two lines to the documentation of Home Assistant to refer to that documentation:

More about effect arguments can be found on hyperion-project.org.
All specific effect arguments for each effect can be found in the json files on the Hyperion Github.

@starkillerOG
Copy link
Contributor Author

starkillerOG commented Apr 2, 2018

@pvizeli and @balloob I do agree that defining a new service that has multiple arguments were you can just input the effect arguments in a diffrent parameter would be ideal. But I have no idea how I could do that and how that works. Therefore I used the already build in effect parameter that can be supplied to the build in light.turn_on service.

The drawback is that I have to use one parameter to supply both the effect name and all effect arguments with their corresponding values. So that is multiple types of data that i have to supply (strings, numbers bolean etc in one parameter).

Is it possible to input a dictionarry in the build in light.turn_on effect parameter?
Then I could use a dictionarry to supply everything instead of a string with seperators ( || and = ).

Although I think that a dictonary would be a more complex syntax than the string with seperators that I have now.

What do you think the best approach would be @balloob?

Using a dict instead of a string as data input
Tryed using a dict but it did not work because effect forces a string so the dict wil be read as string
@starkillerOG
Copy link
Contributor Author

@balloob I tyred supplying a dict to the effect argument like this service call:

{
  "entity_id": "light.tv_light",
  "effect": {"name": "Knight rider", "color" : [0,255,0]}
}

However this gave me a string instead of the expected dict, so still having the same problem.
I tryed defining a custom service, but I do not how to do that, could you help, or provide a hint?

@balloob
Copy link
Member

balloob commented Apr 14, 2018

I guess you're right, it's currently not possible to do an effect with advanced data structures.

I suggest that we close this PR for now. Eventually we're going to remove effect from turn_on / turn_off and make it a standalone service, but that requires a bit more work.

@balloob balloob closed this Apr 14, 2018
@starkillerOG
Copy link
Contributor Author

@balloob can't we merge this PR as is with the json.loads, such that effect arguments will work for now?
And impove it in the future with a standalone service as you indicated.

@balloob
Copy link
Member

balloob commented Apr 16, 2018

No. We never allow services to receive data that has to be parsed as JSON, YAML or any other value.

@starkillerOG
Copy link
Contributor Author

Alright, thanks for your quick replay!
If you have time @balloob, could you take a look at my PR for the denonavr media player compent:
#13706
Submitted it 11 days ago after the denonavr library got updated with my new code supporting sound modes, but did not get any feedback yet on the HomeAssistant PR.

@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants