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

[enocean] Add second action for two rocker switches #10769

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

CountBigBang
Copy link
Contributor

Added code to read a (potential) second action when using switches with two rockers (EEP F6-02-xx) according to the EnOcean specification.

This fixes issue #9750.

Signed-off-by: Dietmar Franzen [email protected]

@CountBigBang CountBigBang requested a review from fruggy83 as a code owner May 29, 2021 10:30
@fruggy83
Copy link
Contributor

fruggy83 commented Jun 1, 2021

HI @CountBigBang ,

thanks a lot for your work. I just have some questions before doing the review:

  • What kind of Rocker Switches do you own to test your changes?
  • For my standard PTM215 I can just trigger the second action if I switch both channels simultaneously. Is this true for your rocker?
  • If I pressed two channels simultaneously, I can only trigger a RELEASED message if I release all channels. Is this also true for your rocker?

If you answer my last questions with yes, I would suggest to rename "PRESSED_SEC" into "SEC_VALID" and just emit a "PRESSED" event in the convertToEventImpl function. Furthermore I am thinking about to introduce some new events or channels instead of handling this case the same as pressing both channels one after another. What do you think?

@CountBigBang
Copy link
Contributor Author

Hi @fruggy83, all my rocker switches are PTM200 switches. Almost all of them are two rocker switches. And some people in my household just love pressing the two buttons simultaneously. Therefore I was very much affected by the missing second action ;-)

The second action is only triggered if both rockers are pressed (or released) simultaneously. My understanding of the EEP specification is that this is a feature.

To be honest, I did not spend much time testing the RELEASED events, because to my knowledge they do not affect the item states. Is that right?

I am totally indifferent to the way the second action is handled. I just wanted to get the issue fixed and tried to be as "non-invasive" as possible with your code.

Would you take over and adjust the names (and procedures)? I don't have much experience with Java and are fairly new to openHAB (I am using Python for most of my projects).

@Skinah Skinah added the enhancement An enhancement or new feature for an existing add-on label Jun 30, 2021
@CountBigBang
Copy link
Contributor Author

@fruggy83 : Are you making progress with your changes? I would really like to see this issue closed as soon as possible.

@fruggy83
Copy link
Contributor

fruggy83 commented Sep 2, 2021

@CountBigBang sorry for my late answer, I was not happy with my implementation (could not add configuration options to the system channel types). However I found a solution and will push them to your PR.

 * Added two new channels:
   * secondActionPressed: Indicates if second action of rocker switch is pressed too
   * rockerSwitchAction: emits the combined rocker switch actions "ChannelADir|ChannelBDir", extensible channel, filter for each channel
* Added two new profiles for channel rockerSwitchAction:
   * rockerswitchaction-toggle-switch
   * rockerswitchaction-toggle-player
* EnOceanSensorHandler can now handle extensible channels too
* EEP F6-02 refactoring

Signed-off-by: Daniel Weber <[email protected]>
Copy link
Contributor

@fruggy83 fruggy83 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work. I have just added two new channels and profiles.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

I have two general questions that I do not yet understand:

  1. The referenced issue mentions that certain telegrams (when pressing rockers simultaneously) are not or wrongly interpreted and as such item states are not correctly reflected in openHAB. Why do we need to add additional channels to the thing type for this?
  2. Why does this PR contain binding specific profiles? How is that related to the issue?

package org.openhab.binding.enocean.internal.config;

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

Please add a short line of Javadoc here.

Copy link
Member

Choose a reason for hiding this comment

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

The javadoc has to go above the author tag (see other classes as a reference).

@fruggy83
Copy link
Contributor

fruggy83 commented Sep 9, 2021

Hi @kaikreuzer

  1. The current rocker switch implementation just ignores the second simultaneously pressed channel. If channel A and channel B rocker is pressed simultaneously the binding emits an event just for channel A. @CountBigBang changed this implementation in a way that an event for channel A and afterwards a second event for channel B is emitted. While I am ok with this implementation, one can discuss if this implementation is really correct as it does not differ between simultaneously or sequentially pressed channels. As the rocker switch thing type uses system rawrocker channel types I could not add a custom channel configuration. Therefore I introduced a state channel which indicates if second channel is pressed simultaneously. However to be able to use this channel you have to use rules. So I introduced another trigger channel which can be configured in which case it should emit a PRESSED or RELEASED event.
  2. The new trigger channel type uses a custom channel type. To be able to link these channels to an item I implemented two binding specific profiles.

I guess this is a little bit to much. I would suggest to revert @CountBigBang change, remove the state channel and just keep the trigger channel type with its profiles?

@CountBigBang
Copy link
Contributor Author

Hi @fruggy83 , @kaikreuzer, how do we continue from here? I must say that I am somehow frustrated. I opened the issue on 8 January. After nothing happened for more than four months, I started inspecting the code myself (having little experience with Java), found and solved the problem and made my (first) code contribution to the project in May. Four months on, and the issue is still open ...

* Channels "rockerswitchA" and "rockerswitchB" do not longer react if second action is pressed
* Added new parameter "handleSecondAction" to channel "rockerswitchListenerSwitch" and "rockerswitchListenerRollershutter" to define if classicDevice should react if second action is pressed too
* Added documentation
* EEP F6-02 refactoring

Signed-off-by: Daniel Weber <[email protected]>
@fruggy83
Copy link
Contributor

Hi @CountBigBang I changed the the behaviour as suggest in my last post. RockerSwitches do nt longer react on second action. If you want to handle a two button press, you have to use the new channel "rockerSwitchAction" or a classic device with corressponding listener channel. These channels can be configured to handle the second action press too.

I am really sorry that it took so long. To be honest I just forgot this issue.

@fruggy83 fruggy83 requested a review from kaikreuzer October 10, 2021 13:59
@CountBigBang
Copy link
Contributor Author

This is good news. Thank you. I'm looking forward to the next milestone release.
I'm not sure about the consequences of your implementation for my setup, though. Currently, a typical file-based thing definition including channel A of a two-rocker switch looks like this:
Thing classicDevice licht_arbeitszimmer "Licht Arbeitszimmer" [ senderIdOffset=21, sendingEEPId="F6_02_01", broadcastMessages=true, receivingEEPId="F6_02_01", suppressRepeating=false ] { Channels: Type virtualSwitchA : openHAB [duration=300, switchMode="rockerSwitch"] Type rockerswitchListenerSwitch : Arbeitszimmer [enoceanId="00A04563", channel="channelA", switchMode="rockerSwitch"] }
Would I need to change anything? In this case, the breaking change should be put in the release notes.

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Sorry, I also missed somehow @fruggy83's question or rather thought it was directed to @CountBigBang and was hence waiting for feedback on it.

@CountBigBang changed this implementation in a way that an event for channel A and afterwards a second event for channel B is emitted.

I personally would expect no events for channels A+B to be emitted, since the user wish is a simultaneous event. I don't think that a single user event should result in 3 technical events within openHAB (channel A + channel B + sim-Action).

Therefore I introduced a state channel which indicates if second channel is pressed simultaneously.

How would this channel be used? I don't really understand its purpose (unless it is only there to detect that events "channel A" and "channel B" should be ignored, but with the suggestion above, this would not be needed at all).

So I introduced another trigger channel which can be configured in which case it should emit a PRESSED or RELEASED event.

Hm, the code says that this channel emits DIR1|-, DIR1|DIR1, etc. events (which makes sense to me).

To be able to link these channels to an item I implemented two binding specific profiles.

If it emits DIR1|- events, the profile should actually have the filter configuration to say on which combinations it wants to react. This way, you only need the single trigger channel (emitting invariably the same events) and you can link different items to it, which each have a different profile configuration (and thus can react on different button press combinations). Does this make sense?

bundles/org.openhab.binding.enocean/README.md Outdated Show resolved Hide resolved
package org.openhab.binding.enocean.internal.config;

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

The javadoc has to go above the author tag (see other classes as a reference).

bundles/org.openhab.binding.enocean/README.md Outdated Show resolved Hide resolved
@fruggy83
Copy link
Contributor

@CountBigBang changed this implementation in a way that an event for channel A and afterwards a second event for channel B is emitted.

I personally would expect no events for channels A+B to be emitted, since the user wish is a simultaneous event. I don't think that a single user event should result in 3 technical events within openHAB (channel A + channel B + sim-Action).

That is exactly how it is implemented since my last commit. The old rockerSwitchA and rockerSwitchB channels just emit an event on a single button press now.

Therefore I introduced a state channel which indicates if second channel is pressed simultaneously.

How would this channel be used? I don't really understand its purpose (unless it is only there to detect that events "channel A" and "channel B" should be ignored, but with the suggestion above, this would not be needed at all).

I removed this state channel with my last commit. because of the same thoughts as you had :) Makes it even more simple now. However I forgot to remove this channel from the readme.

So I introduced another trigger channel which can be configured in which case it should emit a PRESSED or RELEASED event.

Hm, the code says that this channel emits DIR1|-, DIR1|DIR1, etc. events (which makes sense to me).

To be able to link these channels to an item I implemented two binding specific profiles.

If it emits DIR1|- events, the profile should actually have the filter configuration to say on which combinations it wants to react. This way, you only need the single trigger channel (emitting invariably the same events) and you can link different items to it, which each have a different profile configuration (and thus can react on different button press combinations). Does this make sense?

I did it the other way around. I have added extensible channel with a filter configuration. However your suggestion of adding just a single trigger channel and do the filtering in the profile is a better approach as it fits more. I will update the PR. Thanks a lot for your thoughts.

@fruggy83
Copy link
Contributor

Hi @CountBigBang , you just have to add enable the new parameter "handleSecondAction" in your channel config. Like

Thing classicDevice licht_arbeitszimmer "Licht Arbeitszimmer" [ senderIdOffset=21, sendingEEPId="F6_02_01", broadcastMessages=true, receivingEEPId="F6_02_01", suppressRepeating=false ] { Channels: Type virtualSwitchA : openHAB [duration=300, switchMode="rockerSwitch"] Type rockerswitchListenerSwitch : Arbeitszimmer [enoceanId="00A04563", channel="channelA", switchMode="rockerSwitch", handleSecondAction="true"] }

Would I need to change anything? In this case, the breaking change should be put in the release notes.

I will do a PR for the release notes after this issue got merged.

* Moved channel config for "rockerSwitchAction" into profiles
* Updated and cleaned README

Signed-off-by: Daniel Weber <[email protected]>
 * Updated and cleaned README

Signed-off-by: Daniel Weber <[email protected]>
@fruggy83 fruggy83 requested a review from kaikreuzer October 11, 2021 11:10
@kaikreuzer
Copy link
Member

Thanks for all your work @fruggy83!

@kaikreuzer kaikreuzer merged commit 4b13a26 into openhab:main Oct 12, 2021
@kaikreuzer kaikreuzer added this to the 3.2 milestone Oct 12, 2021
fruggy83 added a commit to fruggy83/openhab-distro that referenced this pull request Oct 13, 2021
kaikreuzer pushed a commit to openhab/openhab-distro that referenced this pull request Oct 14, 2021
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
…) (openhab#10769)

 * Added new channel:
   * secondActionPressed: Indicates if second action of rocker switch is pressed too
* Added two new profiles for channel rockerSwitchAction:
   * rockerswitchaction-toggle-switch
   * rockerswitchaction-toggle-player
* EnOceanSensorHandler can now handle extensible channels too
* EEP F6-02 refactoring

Also-by: Dietmar Franzen <[email protected]>
Signed-off-by: Daniel Weber <[email protected]>
dschoepel pushed a commit to dschoepel/openhab-addons that referenced this pull request Nov 9, 2021
…) (openhab#10769)

 * Added new channel:
   * secondActionPressed: Indicates if second action of rocker switch is pressed too
* Added two new profiles for channel rockerSwitchAction:
   * rockerswitchaction-toggle-switch
   * rockerswitchaction-toggle-player
* EnOceanSensorHandler can now handle extensible channels too
* EEP F6-02 refactoring

Also-by: Dietmar Franzen <[email protected]>
Signed-off-by: Daniel Weber <[email protected]>
Signed-off-by: Dave J Schoepel <[email protected]>
@wborn wborn changed the title [enocean] Add second action for two rocker switches (Fixes #9750) [enocean] Add second action for two rocker switches Dec 18, 2021
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
…) (openhab#10769)

 * Added new channel:
   * secondActionPressed: Indicates if second action of rocker switch is pressed too
* Added two new profiles for channel rockerSwitchAction:
   * rockerswitchaction-toggle-switch
   * rockerswitchaction-toggle-player
* EnOceanSensorHandler can now handle extensible channels too
* EEP F6-02 refactoring

Also-by: Dietmar Franzen <[email protected]>
Signed-off-by: Daniel Weber <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
…) (openhab#10769)

 * Added new channel:
   * secondActionPressed: Indicates if second action of rocker switch is pressed too
* Added two new profiles for channel rockerSwitchAction:
   * rockerswitchaction-toggle-switch
   * rockerswitchaction-toggle-player
* EnOceanSensorHandler can now handle extensible channels too
* EEP F6-02 refactoring

Also-by: Dietmar Franzen <[email protected]>
Signed-off-by: Daniel Weber <[email protected]>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
…) (openhab#10769)

 * Added new channel:
   * secondActionPressed: Indicates if second action of rocker switch is pressed too
* Added two new profiles for channel rockerSwitchAction:
   * rockerswitchaction-toggle-switch
   * rockerswitchaction-toggle-player
* EnOceanSensorHandler can now handle extensible channels too
* EEP F6-02 refactoring

Also-by: Dietmar Franzen <[email protected]>
Signed-off-by: Daniel Weber <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants