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 utility function to bulk set partial config parameters #176

Merged
merged 15 commits into from
Mar 24, 2021

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented Mar 23, 2021

This has been an in demand request for some time. The new function will either accept a raw integer value or a dictionary of {bitmask: value} and will convert the dictionary to the raw integer value before calling the node.set_value command. The logic all works, what I am not sure of is if we can use node.set_value to bulk set parameters. I asked @petro to test this for me because I don't have any nodes with partial parameters EDIT: Tests confirmed that we can in fact use node.set_value to bulk set parameters, so this PR is good to merge

@raman325
Copy link
Contributor Author

One thing to note is that Al has stated that he'd like to add native support for this type of action in zwave-js. That could eliminate or significantly reduce the amount of logic that has to live in the library. I personally think it's nice to have all of this logic in the library because we can raise relevant exceptions for the user rather than showing a generic error, but maybe that's just me

zwave_js_server/util/node.py Outdated Show resolved Hide resolved
zwave_js_server/util/node.py Outdated Show resolved Hide resolved
@raman325
Copy link
Contributor Author

FYI node.set_value does work for setting bulk values, so this PR is good to go whenever it's approved

@firstof9
Copy link
Contributor

firstof9 commented Mar 23, 2021

Results from the test if anyone wants them.

Test device Inovelli On/Off Red Series Switch
Data sent:

Value: 51972798
Parameter: 8
Command Class: 112
Test Results
{ type: 'result',
  success: true,
  messageId: 'set-parameter',
  result: { success: true } }
{ type: 'event',
  event:
   { source: 'node',
     event: 'value updated',
     nodeId: 27,
     args:
      { commandClassName: 'Configuration',
        commandClass: 112,
        property: 8,
        propertyKey: 255,
        newValue: 190,
        prevValue: 21,
        propertyName: 'LED Effect Color' } } }
{ type: 'event',
  event:
   { source: 'node',
     event: 'value updated',
     nodeId: 27,
     args:
      { commandClassName: 'Configuration',
        commandClass: 112,
        property: 8,
        propertyKey: 65280,
        newValue: 10,
        prevValue: 10,
        propertyName: 'LED Effect Brightness' } } }
{ type: 'event',
  event:
   { source: 'node',
     event: 'value updated',
     nodeId: 27,
     args:
      { commandClassName: 'Configuration',
        commandClass: 112,
        property: 8,
        propertyKey: 16711680,
        newValue: 25,
        prevValue: 10,
        propertyName: 'LED Effect Duration' } } }
{ type: 'event',
  event:
   { source: 'node',
     event: 'value updated',
     nodeId: 27,
     args:
      { commandClassName: 'Configuration',
        commandClass: 112,
        property: 8,
        propertyKey: 2130706432,
        newValue: 3,
        prevValue: 4,
        propertyName: 'LED Effect Type' } } }

zwave_js_server/util/node.py Outdated Show resolved Hide resolved
zwave_js_server/util/node.py Outdated Show resolved Hide resolved
zwave_js_server/model/node.py Show resolved Hide resolved
raman325 and others added 2 commits March 24, 2021 10:24
@raman325 raman325 requested a review from MartinHjelmare March 24, 2021 18:23
@MartinHjelmare
Copy link
Contributor

What do we need to change if zwave-js adds more native support for this feature?

@raman325
Copy link
Contributor Author

raman325 commented Mar 24, 2021

Not sure, because I am not sure what adding more native support means. I'm guessing that the main change would be to not calculate the raw value to send by doing the bit shift operations and just passing the individual bitmasks and values directly to zwave-js. All the other validation I would think we want to do in the library because we can raise more relevant exceptions to the caller

@MartinHjelmare
Copy link
Contributor

Ok, so there's no risk of breaking changes for the user if this would happen?

@raman325
Copy link
Contributor Author

raman325 commented Mar 24, 2021

I can't guarantee that there will be no breaking change in the future, but I don't believe so. This method should work as long as the property_key (bitmask for config parameters) and value format stay the same. If there was a change to that, I would suggest we handle it as a schema bump because that's a pretty significant change IMO

Copy link
Contributor

@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.

Ok, looks good!

@raman325 raman325 merged commit 02df775 into home-assistant-libs:master Mar 24, 2021
@raman325 raman325 deleted the bulk_set_config_params branch March 24, 2021 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants