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 parameters in scripts #3538

Merged
merged 12 commits into from Nov 9, 2022
Merged

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Jun 9, 2022

What does this implement/fix?

Add support for parameters in scripts

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes esphome/feature-requests#241

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#2120

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266

Example entry for config.yaml:

# Example config.yaml
script:
  - id: control_light
    parameters:
      turn_on: bool
    then:
      # The script can receive and use the params e.g. turn_on
      - lambda: |-
          if (turn_on)
            id(status_light)->turn_on().perform();
          else
            id(status_light)->turn_off().perform();

binary_sensor:
  - platform: gpio
    pin: D2
    id: input2
    on_press:
      # call script through lambda, passing the args to execute
      - lambda: |-
          id(control_light)->execute(true);
    on_release:
      # call script through script.execute
      - script.execute:
          id: control_light
          turn_on: false

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

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

@probot-esphome
Copy link

probot-esphome bot commented Jun 9, 2022

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (script) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@jimtng jimtng force-pushed the script-param branch 2 times, most recently from 0da8050 to 7ca8b3b Compare June 9, 2022 04:51
@mmakaay
Copy link
Member

mmakaay commented Jun 9, 2022

Nice, striking this one from my personal TODO list 😄

@nagyrobi
Copy link
Member

nagyrobi commented Jun 9, 2022

Great!

esphome/components/script/script.cpp Outdated Show resolved Hide resolved
esphome/components/script/__init__.py Outdated Show resolved Hide resolved
esphome/components/script/__init__.py Outdated Show resolved Hide resolved
esphome/components/script/__init__.py Outdated Show resolved Hide resolved
esphome/components/script/__init__.py Show resolved Hide resolved
esphome/components/script/__init__.py Outdated Show resolved Hide resolved
esphome/components/script/script.h Show resolved Hide resolved
esphome/components/script/script.h Outdated Show resolved Hide resolved
@jimtng jimtng requested a review from OttoWinter June 14, 2022 16:07
@jimtng
Copy link
Contributor Author

jimtng commented Jul 9, 2022

ping @OttoWinter @jesserockz

@nagyrobi
Copy link
Member

nagyrobi commented Sep 27, 2022

@jimtng how could I test this in my addon version of ESPHome?

Trying with:

external_components:
  - source: github://pr#3538
    components:
      - script

fails with

ERROR Unable to import component script:
Traceback (most recent call last):
  File "/esphome/esphome/loader.py", line 162, in _lookup_module
    module = importlib.import_module(f"esphome.components.{domain}")
  File "/usr/lib/python3.9/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
  File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 790, in exec_module
  File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
  File "/config/esphome/.esphome/external_components/b61925cd/esphome/components/script/__init__.py", line 5, in <module>
    from esphome.const import CONF_ID, CONF_MODE, CONF_PARAMETERS
ImportError: cannot import name 'CONF_PARAMETERS' from 'esphome.const' (/esphome/esphome/const.py)
Failed config

@jimtng
Copy link
Contributor Author

jimtng commented Sep 28, 2022

@nagyrobi you'd need to check out my branch in order to test. This PR contains changes to esphome/const.py which is not a part of the script component, hence you couldn't just source the component.

@mmakaay
Copy link
Member

mmakaay commented Sep 29, 2022

For that reason, it's not a bad idea to keep new constants in your component, at least during development. That makes it possible to handle it as an external component.
After merging, these constants can be moved to const.py, or even kept within the component when they aren't likely to be reused.

@nagyrobi
Copy link
Member

For my use case I was offered a nice workaround, so I don't need to test anymore: esphome/feature-requests#241 (comment)

@jimtng
Copy link
Contributor Author

jimtng commented Sep 29, 2022

For that reason, it's not a bad idea to keep new constants in your component, at least during development. That makes it possible to handle it as an external component. After merging, these constants can be moved to const.py, or even kept within the component when they aren't likely to be reused.

@jesserockz WDYT?

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Very minor change request. Otherwise looks good, and tested locally

esphome/components/script/__init__.py Outdated Show resolved Hide resolved
esphome/components/script/__init__.py Outdated Show resolved Hide resolved
@jesserockz
Copy link
Member

I actually pushed some commits that "fix" having lists in the types. The docs mentioned [] becomes std::vector, but this was not the case in the code. It now is.

Also fixed the "crash" when parameters were not set on the execute action, this is a limitation of not being able to raise cv.Invalid inside to_code functions.

@jesserockz jesserockz merged commit 8c122aa into esphome:dev Nov 9, 2022
@jesserockz jesserockz mentioned this pull request Nov 9, 2022
@jimtng jimtng deleted the script-param branch November 11, 2022 10:14
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add variables to Scripts
5 participants