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

Make dim buttons of the Philips Dimmer Switch usable. #352

Merged
merged 4 commits into from
Jan 16, 2019

Conversation

rbi
Copy link
Contributor

@rbi rbi commented Jan 9, 2019

This is a static thing definition for the Philips Dimmer Switch. When the Switch is autodiscovered only the On and Off buttons generate any events that can be used in OpenHab. With this PR both Dimm Buttons in the middle can be used too.

To get the right values I looked at this source code file:
https://github.com/dresden-elektronik/deconz-rest-plugin/blob/2f9c700cdb679de87666abe56c3477b31916e438/sensor.cpp#L96
I tried to use the proprietary definitions which would allow even more fine grained controling with this remote but I failed. I didn't know what I should use for the "*_parameter_name" properties. The commented out values work for me however.

@cdjackson cdjackson requested a review from hsudbrock January 9, 2019 21:32
@cdjackson
Copy link
Contributor

Thanks. Please can you sign your commit and the PR.

@rbi rbi force-pushed the philips_dim_switch branch from 1c0ea7a to 70e3cd6 Compare January 9, 2019 21:42
@rbi
Copy link
Contributor Author

rbi commented Jan 9, 2019

Thanks. Please can you sign your commit and the PR.

Do you mean I should add this "Signed-off-by" line to the commit? I've done it now and replaced the original commit.

@cdjackson
Copy link
Contributor

Yes, that's correct - as per the contributors guidelines, you are signing off that you accept the conditions.

Thanks.

@rbi
Copy link
Contributor Author

rbi commented Jan 10, 2019

The generically discovered version contains battery channels which work fine. This static thing version does not contain them yet. Should I add them?

At the moment, when you add a remote to the system you get the auto discovered device in the Inbox first. You add it and discovery completes while it is already added. Only then you get static thing as a new thing in the Inbox. When you add it you have 2 things for one remote. The temperature can then just be taken from the generic thing and the Dimm buttons from the static thing.
Sometimes discovery completes in the Inbox already. Than the static thing replaces the generic thing and you can only add the static thing. Then it would not be possible to use the battery channels.

@rbi
Copy link
Contributor Author

rbi commented Jan 10, 2019

May Zigbee standard knowledge is not that good but I do not understand why the parameter zigbee_shortpress_parameter_name is needed in the static thing definition. It is the only thing where I did not find an equivalent for here. The zigbee_shortpress_parameter_value is in that list however. If the zigbee_shortpress_parameter_name would not be needed maybe the proprietary clusters could be used which provide more information.

I also think there is something like a zigbee_longpress_released_* attribute set missing. In the innr rc-110 thing definition there is an extra channel just for long press release. Long press release should be possible on the channel that did the long press start.

@hsudbrock
Copy link
Contributor

Thanks for adding this, @rbi!

To my knowledge, it is currently not possible in the binding to have some channels of a thing from a static channel definition in the XML thing type, and to have some other channels being dynamically generated by the binding. Hence, it would be good if you could add the low-battery channel.

Regarding the attribute for the parameter name, and why this is needed: It is necessary to somehow specify the parameter value to distinguish different buttons for which the device emits the same command, just with different parameter values. The library used by the binding for ZigBee communication provides parameter values of commands via getter methods that include the parameter name, hence the use of the parameter name in the thing type. You are correct, this is nothing from the ZigBee standard. When implementing this feature, @cdjackson and I discussed briefly whether to extend the underlying ZigBee library to permit a different way to access the parameters, but decided that simply using the parameter name should be good enough. You probably do not find this in the link you provided, because this information is encoded by the hex values listed there.

Regarding the option to add additional triggers like 'long-press-release': When implementing this, I wanted to use the standard system channel types from Eclipse Smarthome, and not introduce custom channel types. I think that in ESH, there are currently no suitable system channel types that cover all the options (hence the 'workaround' for the release channel for the innr remote). I don't see any other option than introducing ZigBee-binding specific channel types for this, extending the list of system channel types in ESH after some discussion there, or just using a workaround similar to the one used in the innr remote thing type.

@rbi
Copy link
Contributor Author

rbi commented Jan 10, 2019

@hsudbrock Thanks for the explanation. I think I understand now the need for the for the parameter name.

The dimmer emits events on the cluster "0xfc00" too. I guess that is a proprietary one? As I understand it the underlying library has generated classes for well-known clusters. These classes contain the getters with the parameter name. I guess for proprietary clusters no class is generated so I can not access them at all? Or is there some kind of generic class which can be used in that case? What would the parameter_name be?

@hsudbrock
Copy link
Contributor

Yes, clusters with IDs in the range 0xfc00 – 0xffff are so-called 'manufacturer-specific' clusters; there is not so much support for manufacturer-specific clusters in the underlying ZigBee library - yet. I am currently working on that, maybe we can add support for those events from cluster 0xfc00 when this is done.

@rbi
Copy link
Contributor Author

rbi commented Jan 14, 2019

I've seen the batteryLevel and the batteryVoltage channel to have values at least once. They do only work sporadically for me however. The auto-discovered channels work only sporadically to so I guess it is no problem in the binding.

I've never seen a value for the batteryAlarm channel but I guess it only has a value if the battery is low?

I just recognized that I forgot the sign-off again. I fix this in the afternoon.

@hsudbrock
Copy link
Contributor

I've never seen a value for the batteryAlarm channel but I guess it only has a value if the battery is low?

This channel type is based on the attribute BatteryAlarmState of the power configuration cluster. As this attribute is an optional one, it could be that the dimmer switch does not support it - in this case it would be good to not add the battery alarm channel to the thing type. Do you happen to know if the attribute is supported by the device?

@rbi rbi force-pushed the philips_dim_switch branch from 052bad3 to fe38f93 Compare January 14, 2019 16:46
@rbi
Copy link
Contributor Author

rbi commented Jan 14, 2019

Unfortunately I don't know this so I have commented out it for now. I've added the Signed-Off Line.

With this definitions all button channels generate SHORT_PRESS events. When doing a long press on a dimmer button a SHORT_PRESS event is generated each second until the button is released. I guess when switching to the proprietary cluster that last behavior would change.

@@ -1,6 +1,7 @@
philips_sml001,vendor=Philips,modelId=SML001
philips_rwl012,vendor=Philips,modelId=RWL021
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, one last question, as I overlooked this before: Should it be 012 or 021 in the ID? It looks as if the digits were accidentally transposed on one side of this property entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be rwl021. Thanks for the hint. I've fixed it.

@hsudbrock
Copy link
Contributor

hsudbrock commented Jan 15, 2019

Unfortunately I don't know this so I have commented out it for now.

I will probably get one of those switches myself and take a look; then I can add the channel or remove the commented section for the channel with an additional PR at a later point.

Copy link
Contributor

@hsudbrock hsudbrock 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 to me, thanks @rbi!

@hsudbrock hsudbrock merged commit bc40b0f into openhab:master Jan 16, 2019
@rbi rbi deleted the philips_dim_switch branch January 17, 2019 15:29
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/zigbee-binding/15763/2087

@cdjackson cdjackson added this to the 2.5 milestone Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants