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
Adding ZWave CentralScene activation handler. #9178
Conversation
Hi @sirmalloc, 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! |
@sirmalloc, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @turbokongen and @andrey-git to be potential reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good find! This is a pretty highly requested feature.
Please also add a test for this event. You should be able to use this as a template: https://github.com/home-assistant/home-assistant/blob/bd039b8c5332e9225ef2bd8291c9fd12309bebd9/tests/components/zwave/test_node_entity.py#L35
@@ -830,6 +830,25 @@ def __init__(self, values, domain): | |||
|
|||
def network_value_changed(self, value): | |||
"""Handle a value change on the network.""" | |||
if (value.command_class == const.COMMAND_CLASS_CENTRAL_SCENE and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this logic into the ZWaveNodeEntity. These events should only fire once per node, not for each entity on the node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started down that path originally, but in my testing there wasn't a ZWaveNodeEntity instance for the CentralScene value that OZW was sending the ValueChanged notification for. I'm not familiar enough with the codebase to determine if I should be automatically creating the entity earlier on during the initialization process, but I added the code here as it was the furthest I could trace the ValueChanged from OZW before it was discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I see part of the problem. The network_node_changed
is subscribed to a bunch of Z-Wave events.
What you'll want to do is copy this line into the ZWaveNodeEntity constructor, and add the method as it's defined here, only including your new logic.
That existing method could probably use some cleanup at some point, since it's probably not working as expected.
value.node.node_id == self.node.node_id and | ||
self.hass is not None): | ||
|
||
scene_id = int(str(value.index) + str(value.data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing this concatenation? Shouldn't these be individual event attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really performing math, so much as concatenating the two individual integers for the scene number and scene key that OZW is passing us in the ValueChanged notification. For example, with the HS-WD100+ dimmer and a double tap for the on button, the value.index
I get from OZW is 1
, and the value.data
I get is 3
, which I concatenate into a scene_id
of 13
. I was looking for a way I could pass them along separately to the event I fire and utilize that in an automation, but I didn't notice any way to do so with the existing code that fires the ZWave scene events, so this seemed like a suitable compromise to create a unique scene from both pieces of data I get from OZW.
|
||
scene_id = int(str(value.index) + str(value.data)) | ||
|
||
_LOGGER.info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can skip the log output if there's nothing else to add. Hass core will already log the event.
self.hass.bus.fire(const.EVENT_SCENE_ACTIVATED, { | ||
ATTR_ENTITY_ID: self.entity_id, | ||
const.ATTR_NODE_ID: self.node.node_id, | ||
const.ATTR_SCENE_ID: scene_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just change this to
const.ATTR_VALUE_INDEX: value.index,
const.ATTR_VALUE_DATA: value.data,
In const.py, you'll need to define ATTR_VALUE_DATA
ATTR_VALUE_DATA = "value_data"
Removed extraneous logging Modified scene_activated event to send the scene_id and scene_data separately
@armills - I've integrated your requested changes with a few differences. In node_entity.py I hook into the existing network_node_changed method and if a central scene value is detected, I call a new central_scene_activated method and pass it the scene id and data. In the new central_scene_activated method I felt it was more logical to go with "scene_id" and "scene_data" as the two parameters to the zwave.scene_activated event. As such, I added ATTR_SCENE_DATA to const.py. I've updated my original PR comments with the HS-WD100+ values for scene_id and scene_data, and an example automation trigger making use of the event. I've not yet had a chance to add the test for this event, I may be able to get to that later today. |
assert events[0].data[const.ATTR_NODE_ID] == 11 | ||
assert events[0].data[const.ATTR_SCENE_ID] == scene_id | ||
assert events[0].data[const.ATTR_SCENE_DATA] == scene_data | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation contains tabs
indentation contains mixed spaces and tabs
blank line contains whitespace
value = mock_zwave.MockValue( | ||
command_class=const.COMMAND_CLASS_CENTRAL_SCENE, | ||
index=scene_id, | ||
data=scene_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace
value = mock_zwave.MockValue( | ||
command_class=const.COMMAND_CLASS_CENTRAL_SCENE, | ||
index=scene_id, | ||
data=scene_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace
@@ -116,7 +116,59 @@ def listener(event): | |||
assert events[0].data[const.ATTR_NODE_ID] == 11 | |||
assert events[0].data[const.ATTR_SCENE_ID] == scene_id | |||
|
|||
@asyncio.coroutine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected 2 blank lines, found 1
@armills - I added the unit test to test_node_entity.py. I do apologize however as this is my first pull request, and I inadvertently did a pull of the latest dev branch before committing my changes, and I believe GitHub has included all the intermediate committers as part of this pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Appreciate the follow up. Just one more small request.
if (value is not None and | ||
value.command_class == COMMAND_CLASS_CENTRAL_SCENE): | ||
self.central_scene_activated(value.index, value.data) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should get rid of this return, and let the node statistics also update after this message is received.
… message is received
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge once documentation PR is ready.
@armills Where can I add said documentation/what specifically would be good to document on this feature? Pretty stoked to have just received my HomeSeer dimmer and have a PR ready to do with it 23 hours before I received it :) |
Documentation needs to be updated in the home-assistant.github.io repo. Specifically this file needs to be updated to include the new event. The scene activated section can be copied as a template. |
@armills How does that look. I am not 1000% sure exactly what the code behind this specific PR does, but I know what it means and hopefully the doc PR reflects that :) BTW, thanks @sirmalloc for making the code side of this! I (and as you know MANY others) will greatly appreciate that! |
I'll submit the documentation today so we can get this integrated. Apologies for the delay, I've got a very loud newborn yelling at me whenever I sit down to work.
… On Aug 31, 2017, at 3:39 PM, Addo Solutions ***@***.***> wrote:
@armills How does that look. I am not 1000% sure exactly what this PR does, but I know what it means and hopefully the doc PR reflects that :)
BTW, thanks @sirmalloc for making the code side of this! I (and as you know MANY others) will greatly appreciate that!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@sirmalloc No problem. @AddoSolutions actually jumped in with home-assistant/home-assistant.io#3288 so you can just note anything you'd like to see changed/included there. |
@sirmalloc You must have the same newborn I do! Docs updated per @armills revisions |
@AddoSolutions - Thanks for taking care of that. I've added a single note on the documentation re: the entity_id sent when the scene is activated. Other than that, looks great. Can't wait to have this in the codebase. |
@sirmalloc I know right? The box literally just landed on my doorstep from Amazon for my first HomeSeer switch and you had just thrown this PR up. Talk about timing! |
@armills not to be a pain (as I'm sure you have many other far more important things going on outside this PR) but is there anything else you need documentation wise or from me on this? Would love to get this polished off! |
Okay, documentation is aproved 👍 |
Thanks for the contributions! 🎉 |
Just confirmed it works with my HomeSeer HS-100+ |
I tried this with a Z-Wave.Me WALLC-S and it is partly working. Single tap works for buttons 1 and 2 at least. Buttons 3 and 4 don't seem to trigger in HA. I can see them logging in OWZ_Log though.
|
@cgtobi - Take a look in your zwcfg for that device and make sure the COMMAND_CLASS_CENTRAL_SCENE class has the value entities inside of it. If not, I'd suggest manually adding them while HA is stopped, similar to my example for the HS-WD100+ in the original comment above. I believe the value discovery code in OpenZWave for the central scene values isn't fully implemented so they need to be added manually. I'll be submitting a PR to OpenZWave soon to incorporate the central scene command class in the XML for the HS-WD100+.
… On Sep 1, 2017, at 6:22 PM, cgtobi ***@***.***> wrote:
I tried this with a Z-Wave.Me WALLC-S and it is partly working. Single tap works for buttons 1 and 2 at least. Buttons 3 and 4 don't seem to trigger in HA. I can see them logging in OWZ_Log though.
2017-09-02 00:04:55.276 Detail, Node012, Received: 0x01, 0x08, 0x00, 0x04, 0x00, 0x0c, 0x02, 0x98, 0x40, 0x25 2017-09-02 00:04:55.276 Info, Node012, Received SecurityCmd_NonceGet from node 12 2017-09-02 00:04:55.276 Info, NONCES: 0xa2, 0x98, 0x8b, 0x2c, 0xed, 0xf9, 0x32, 0x57 2017-09-02 00:04:55.276 Info, NONCES: 0x8a, 0x24, 0x36, 0xca, 0xdc, 0x9c, 0x71, 0x91 2017-09-02 00:04:55.277 Info, NONCES: 0x8c, 0x88, 0xfe, 0x0f, 0x07, 0x6e, 0x52, 0xf9 2017-09-02 00:04:55.277 Info, NONCES: 0xbf, 0x71, 0xa5, 0x07, 0xce, 0x63, 0x91, 0x0e 2017-09-02 00:04:55.277 Info, NONCES: 0x09, 0xc6, 0xca, 0x09, 0x8a, 0x54, 0x6d, 0x05 2017-09-02 00:04:55.277 Info, NONCES: 0x6a, 0x90, 0xf2, 0xdf, 0x41, 0xb7, 0x66, 0xa6 2017-09-02 00:04:55.277 Info, NONCES: 0xb5, 0xee, 0x16, 0x6c, 0x83, 0x7f, 0x70, 0x43 2017-09-02 00:04:55.277 Info, NONCES: 0xf1, 0x16, 0x4a, 0xbf, 0x79, 0xdc, 0xcd, 0x82 2017-09-02 00:04:55.277 Info, Node012, Sending (WakeUp) message (Callback ID=0x01, Expected Reply=0x00) - Nonce_Report - 0x01, 0x11, 0x00, 0x13, 0x0c, 0x0a, 0x98, 0x80, 0x8c, 0x88, 0xfe, 0x0f, 0x07, 0x6e, 0x52, 0xf9, 0x05, 0x01, 0xd0: 2017-09-02 00:04:55.285 Detail, Received: 0x01, 0x04, 0x01, 0x13, 0x01, 0xe8 2017-09-02 00:04:55.285 Detail, ZW_SEND_DATA delivered to Z-Wave stack 2017-09-02 00:04:55.319 Detail, Received: 0x01, 0x07, 0x00, 0x13, 0x01, 0x00, 0x00, 0x03, 0xe9 2017-09-02 00:04:55.319 Detail, ZW_SEND_DATA Request with callback ID 0x01 received (expected 0x01) 2017-09-02 00:04:55.383 Detail, Node012, Received: 0x01, 0x1f, 0x00, 0x04, 0x00, 0x0c, 0x19, 0x98, 0x81, 0xcc, 0x78, 0x7f, 0x01, 0xc2, 0xee, 0x23, 0xfe, 0xd8, 0xf4, 0x85, 0xfc, 0x79, 0x60, 0x8c, 0x1b, 0x62, 0x74, 0x34, 0x42, 0x7e, 0x8d, 0xf1, 0x6a 2017-09-02 00:04:55.383 Info, Raw: 0x98, 0x81, 0xcc, 0x78, 0x7f, 0x01, 0xc2, 0xee, 0x23, 0xfe, 0xd8, 0xf4, 0x85, 0xfc, 0x79, 0x60, 0x8c, 0x1b, 0x62, 0x74, 0x34, 0x42, 0x7e, 0x8d, 0xf1, 0x6a 2017-09-02 00:04:55.383 Detail, Node012, Decrypted Packet: 0x00, 0x5b, 0x03, 0xa0, 0x00, 0x05 2017-09-02 00:04:55.383 Detail, 2017-09-02 00:04:55.383 Info, Node012, Received Central Scene set from node 12: scene id=5 in 0 seconds. Sending event notification. 2017-09-02 00:04:55.383 Warning, Node012, No ValueID created for Scene 5
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@sirmalloc - Of course I read that and this is what I put in place:
|
@cgtobi - Try setting the scenecount to 5, both in the attribute of the command class and the value attribute of the first value in the command class, then add another value at the end of the command class with an index of 5. The log you posted showed it was trying to process a value for Scene 5 and wasn't finding the ValueID. I don't know for certain but I'm guessing it'll issue the ValueChanged event if there is a matching value with an index of 5.
… On Sep 2, 2017, at 2:58 AM, cgtobi ***@***.***> wrote:
@sirmalloc - Of course I read that and this is what I put in place:
<CommandClass id="91" name="COMMAND_CLASS_CENTRAL_SCENE" version="1" request_flags="5" issecured="true" innif="true" scenecount="0"> <Instance index="1" /> <Value type="int" genre="system" instance="1" index="0" label="Scene Count" units="" read_only="true" write_only="false" verify_changes="false" poll_intensity="0" min="-2147483648" max="2147483647" value="0" /> <Value type="int" genre="user" instance="1" index="1" label="Top Left Button Scene" units="" read_only="false" write_only="false" verify_changes="false" poll_intensity="0" min="-2147483648" max="2147483647" value="0" /> <Value type="int" genre="user" instance="1" index="2" label="Top Right Button Scene" units="" read_only="false" write_only="false" verify_changes="false" poll_intensity="0" min="-2147483648" max="2147483647" value="0" /> <Value type="int" genre="user" instance="1" index="3" label="Bottom Left Button Scene" units="" read_only="false" write_only="false" verify_changes="false" poll_intensity="0" min="-2147483648" max="2147483647" value="0" /> <Value type="int" genre="user" instance="1" index="4" label="Bottom Right Button Scene" units="" read_only="false" write_only="false" verify_changes="false" poll_intensity="0" min="-2147483648" max="2147483647" value="0" /> </CommandClass>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I was thinking the same and tried that but it didn't help. I'm still getting |
Ok, got the single taps working. I had to match the scene IDs. This is the zwcfg part:
|
Description:
This adds the ability to use ZWave CentralScene activations as you would any other ZWave scene activation. Thus far I've only tested with the HomeSeer HS-WD100+ dimmer, allowing me to bind automations to single, double, and triple taps on both the on and off buttons, as well as tap and hold.
Original discussion here: https://community.home-assistant.io/t/central-scene-command-class-fibaro-swipe-homeseer-ws-100d-switches/6114
Notes (updated):
I fire the scene_activated event with the scene_id, and the additional data from OZW's ValueChanged event using the key
scene_data
. For the HS-WD100+, the button sequences map to:Additionally, I had to edit my zwcfg file while Home Assistant was stopped to modify the config for the HomeSeer switches. I replaced the COMMAND_CLASS_CENTRAL_SCENE block for both with the following:
If Home Assistant is running when you modify the zwcfg file, your changes will be overwritten the next time it shuts down or restarts.
For an example of how to use these in an automation, use an event trigger:
Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
tox
run successfully. Your PR cannot be merged unless tests passNew dependencies have been added to theREQUIREMENTS
variable ([example][ex-requir]).New dependencies are only imported inside functions that use them ([example][ex-import]).New dependencies have been added torequirements_all.txt
by runningscript/gen_requirements_all.py
.New files were added to.coveragerc
.