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 zwave_js.set_config_parameter service #46673

Merged

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Feb 17, 2021

Proposed change

Adds new zwave_js.set_config_parameter service

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @home-assistant/z-wave, mind taking a look at this pull request as its been labeled with an integration (zwave_js) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

homeassistant/components/zwave_js/const.py Outdated Show resolved Hide resolved
homeassistant/components/zwave_js/const.py Outdated Show resolved Hide resolved
homeassistant/components/zwave_js/services.py Outdated Show resolved Hide resolved
@marcelveldt
Copy link
Member

Nice work on this! I actually started working on this but you've beat me to it haha :-)

I was just thinking.. How would users know what values exist and what values to set ?

  • Should we let them set a value by it's exact name as found in the UI (and we deal with how to handle the value) ?
  • Should we also provide a get_config_values service that dumps the possible values to log or file ?
  • Or do we consider this something for advanced users only, and assume they will be able to grab the values from a dump file etc. ?

BTW: I also agree with you that we should try to mimic the behavior of previous integrations. We now have a chance to do this right. In that light, I kind of prefer the first option where we just accept the value label as it appears in the UI

@raman325
Copy link
Contributor Author

raman325 commented Feb 17, 2021

Attempting to summarize the Discord conversation as a reference. We want to support setting config parameters with the following service data schemes, using an enumerated/bitwise parameter as an example (https://devices.zwave-js.io/?jumpTo=0x000c:0x0203:0x0001:0.0 parameter 31)

Scenario 1: Update a partial value by bit, accepts a value of either the number or the label:

parameter: 31
bitmask: 0x01
value: 1/Blink

Scenario 2: Update a partial value by label, accepts a value of either the number or the label:

parameter: LED 1 Blink Status (bottom)
value: 1/Blink

Scenario 3: Update all bitwise values using a value dict (key can be either the bit or the label, value can be either the number or the label):

parameter: 31
value:
  0x01: 1/Blink
  0x02: 1/Blink
parameter: 31
value:
  "LED 1 Blink Status (bottom)": 1/Blink
  "LED 2 Blink Status": 1/Blink

Scenario 4: Update all bitwise values using raw value:

parameter: 31
value: 3

Most of these will require additional upstream changes to support, but hopefully that encapsulates all use cases

@kpine
Copy link
Contributor

kpine commented Feb 17, 2021

I like the proposal. A couple observations.

parameter: LED 1 Blink Status (bottom)
value: 1/Blink

This assumes that no parameters (or even partials for different parameters) for a node have the same label (property). I hope that's true in all cases, but should we confirm?

parameter: 31
value:
  "LED 1 Blink Status (bottom)": 1/Blink
  "LED 2 Blink Status": 1/Blink

If I was setting only "LED 1 Blink Status (bottom)", then I don't care about parameter 31, but now when I want to set more than one I know need to know that. I don't have a better idea though, just an observation. At least this does avoid any problems with possible duplicate labels (a partial can't possibly have duplicate labels right?).

@raman325
Copy link
Contributor Author

I like the proposal. A couple observations.

parameter: LED 1 Blink Status (bottom)
value: 1/Blink

This assumes that no parameters (or even partials for different parameters) for a node have the same label (property). I hope that's true in all cases, but should we confirm?

parameter: 31
value:
  "LED 1 Blink Status (bottom)": 1/Blink
  "LED 2 Blink Status": 1/Blink

If I was setting only "LED 1 Blink Status (bottom)", then I don't care about parameter 31, but now when I want to set more than one I know need to know that. I don't have a better idea though, just an observation. At least this does avoid any problems with possible duplicate labels (a partial can't possibly have duplicate labels right?).

I think the duplicate label is OK since the user can just use the parameter/property number as a fallback.

Regarding your second point, we can make parameter optional if the value is a dictionary/mapping if that's really a concern

@kpine
Copy link
Contributor

kpine commented Feb 17, 2021

Regarding your second point, we can make parameter optional if the value is a dictionary/mapping if that's really a concern

I don't have a real strong opinion on it. As proposed, it's probably fine.

@raman325
Copy link
Contributor Author

OK so we now have parameter and bitmask as attributes, and we have support for Scenario 1 and Scenario 2, but all of the logic lives in the service, and I think we should instead send the inputs as is to the library and let it validate and transform the data before sending

@raman325 raman325 changed the title Create zwave_js.set_config_value service Create zwave_js.set_config_parameter service Feb 17, 2021
@raman325 raman325 force-pushed the zwave_js_set_config_value_service branch 2 times, most recently from 9bf2a44 to 761b54f Compare February 20, 2021 17:15
@raman325 raman325 marked this pull request as ready for review February 20, 2021 17:15
@raman325 raman325 changed the title Create zwave_js.set_config_parameter service Add zwave_js.set_config_parameter service and zwave_js/set_config_parameter WS API command Feb 21, 2021
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Are we sure the helpers that were moved from the service module will be used elsewhere?

homeassistant/components/zwave_js/helpers.py Outdated Show resolved Hide resolved
homeassistant/components/zwave_js/helpers.py Outdated Show resolved Hide resolved
@raman325 raman325 force-pushed the zwave_js_set_config_value_service branch from 30fec97 to 7d17381 Compare February 23, 2021 02:26
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Looks good!

@MartinHjelmare
Copy link
Member

There's a merge conflict. Sorry about that.

@raman325 raman325 merged commit 5a3bd30 into home-assistant:dev Feb 23, 2021
@raman325 raman325 deleted the zwave_js_set_config_value_service branch February 23, 2021 16:35
@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2021
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.

7 participants