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

More Osram smart+ switch support #366

Merged
merged 4 commits into from
Feb 6, 2019

Conversation

rbi
Copy link
Contributor

@rbi rbi commented Jan 17, 2019

With this pull request long press and long press release work for all 4 buttons. Short press work only for the right buttons at the moments because the library can not map the required cluster/commandId combination. I guess that is fixed with #363 ?

The switch reports voltage but now battery percentage. I added a mapping for the voltage too.

While creating the definition I got the following exception

2019-01-17 19:16:12.122 [WARN ] [c.ZigBeeConverterGenericButton:255  ] - 000D6F000FE4C49A: Could not read parameter moveMode for command MoveToSaturationCommand [Color Control: 28320/2 -> 0/1, cluster=0300, TID=17, saturation=254, transitionTime=2]java.lang.NoSuchMethodException: com.zsmartsystems.zigbee.zcl.clusters.colorcontrol.MoveToSaturationCommand.getMoveMode()
	at java.lang.Class.getMethod(Class.java:1786)
	at org.openhab.binding.zigbee.internal.converter.ZigBeeConverterGenericButton$CommandSpec.matchesParameter(ZigBeeConverterGenericButton.java:250)
	at org.openhab.binding.zigbee.internal.converter.ZigBeeConverterGenericButton$CommandSpec.matchesCommand(ZigBeeConverterGenericButton.java:241)
	at org.openhab.binding.zigbee.internal.converter.ZigBeeConverterGenericButton.getButtonPressType(ZigBeeConverterGenericButton.java:152)
	at org.openhab.binding.zigbee.internal.converter.ZigBeeConverterGenericButton.commandReceived(ZigBeeConverterGenericButton.java:128)
	at com.zsmartsystems.zigbee.zcl.ZclCluster$5.run(ZclCluster.java:845)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

I fixed it in the second commit.

rbi added 2 commits January 17, 2019 19:22
Short press only works for the right buttons.

Signed-off-by: Raik Bieniek <[email protected]>
… wrong command id anyway.

This fixes an exception.

Signed-off-by: Raik Bieniek <[email protected]>
@rbi
Copy link
Contributor Author

rbi commented Jan 18, 2019

I'm not happy with the four long press release channels. In another pull-request you said that the Eclipse smart home system channel you wanted to use does not define a long press release. Whoever invented that channel must have thought about this I guess.

The channel is a text channel. My guess is that it should contain the "LONG_PRESS" text as long as the the user holds the button and cleared afterwards. You could define nice rules then:

  • When channel receives LONG_PRESS, do dim up and schedule each second:
  • When channel still contains LONG_PRESS, do dim up and continue
  • Else break the loop.

My suggestion would be a longpress release attribute in the XML which would make the GenericConverter clear the text channel.

@hsudbrock
Copy link
Contributor

My suggestion would be a longpress release attribute in the XML which would make the GenericConverter clear the text channel.

The channel is a trigger channel, so, afaik, it does not have a state that could be cleared.

Shouldn't one rather extend the definition of the system channel type in Eclipse SmartHome? Then, after extending the channel type in Eclipse SmartHome, one could use the additional trigger payloads here in the binding. WDYT?

@hsudbrock
Copy link
Contributor

Short press work only for the right buttons at the moments because the library can not map the required cluster/commandId combination. I guess that is fixed with #363 ?

No, this won't be fixed with #363, as #363 is concerned with manufacturer-specific cluster support.

This would be fixed with zsmartsystems/com.zsmartsystems.zigbee#438 (an issue I created in the underlying ZSS ZigBee library last year, when I tried to integrate the Osram switch :) ).

@hsudbrock
Copy link
Contributor

One more comment (without any expectation to fix this with this PR): The Osram switch comes in three variants, with different model IDs. You have added the model ID Switch 4x EU-LIGHTIFY with this PR; other model IDs are Switch-LIGHTIFY and Switch 4x-LIGHTIFY. But I am not sure whether it is currently possible to configure multiple model IDs for one thing type in the discovery.txt file...

@rbi
Copy link
Contributor Author

rbi commented Jan 18, 2019

My suggestion would be a longpress release attribute in the XML which would make the GenericConverter clear the text channel.

The channel is a trigger channel, so, afaik, it does not have a state that could be cleared.

Shouldn't one rather extend the definition of the system channel type in Eclipse SmartHome? Then, after extending the channel type in Eclipse SmartHome, one could use the additional trigger payloads here in the binding. WDYT?

I read the documentation and created an issue to clarify it :-) eclipse-archived/smarthome#6871 . In the issue where they discussed the introduction of the trigger channels they have talked about release so they must have thought about it eclipse-archived/smarthome#1043 .

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.

LGTM, thanks @rbi!

Would you rather like to wait with merging until you know more about the issue with the system channel type in ESH? If not, I think that this could be merged now, and improved later.

@rbi
Copy link
Contributor Author

rbi commented Jan 18, 2019

I would like to wait a little longer at least until we know if it is just a missing documentation issue or a missing LONG_PRESS_RELEASED event issue. I think if it is the former the effort to adapt code in this binding is low.

If this would be merged now and the long press release channels would be removed later this would break OpenHAB rules using these channels that where created in the meantime.

@hsudbrock
Copy link
Contributor

OK, then let's wait with merging.

@rbi
Copy link
Contributor Author

rbi commented Feb 2, 2019

There has been no reaction to my question how to correctly handle long press release in two weeks. Maybe we should merge this as it is.

@hsudbrock
Copy link
Contributor

I agree that we could merge this now. With zsmartsystems/com.zsmartsystems.zigbee#438, we could later improve this by adding the missing short-pressed events, maybe @cdjackson gets to add these missing commands to the zsmartsystems library at some point.

@cdjackson
Copy link
Contributor

I hope these will be available very soon - once the new autocoder is ready...

@hsudbrock
Copy link
Contributor

Nice :) Thanks for the update, Chris!

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