Generic buttons defined by attribute changes

[t-8ch]

This PR extends the generic button functionality. It allows the definition of buttons based on attribute changed events.
An example configuration for a Xiaomi Aqara wallswitch is provided.

The logic and checks to validate the configuration should be cleaned up.
Also tests are need.
If there is interest in this feature I will provide them.

[cdjackson]

Thanks @t-8ch I'll probably let @hsudbrock review this since he wrote the initial implementation. From a quick look though it looks fine. As you say - tests are needed, and I might also suggest the occasional line or two of comments ;)

[t-8ch]

Will do!

[t-8ch]

Currently the value of the attribute is compared via via its string representation.
This works nicely around the fact, that the configuration only allows string values, while the actual attribute may have a wider range of types.
However this will make it impossible to specify integer values as hex, like the other values.
Do we want to:

• Force users to specify the exact string versions
• Special-case integer attribute values
• Only allow integer attribute values
  ?

[t-8ch]

How should the tests be grouped/ordered in relation to the existing command tests?
Should they by similar functionality or group by command/attribute?

[t-8ch]

It seems the channels can't be linked from the UI, as no profile can be selected.
Is this intentional/known or a bug in thing definition?

Edit: It's probably because the channel only triggers and has no state

[t-8ch]

@cdjackson One question about the configuration application:
Currently after pairing the switch from the inbox it will take some time until it is fully discovered. After the device is fully discovered a second Thing, with the properties and channels configured via the XML files, appears in the Inbox.
Adding this Thing will again trigger a full discovery process.
In the end there are two Things configured, one generic and one properly usable.
Is this intentional or is my configuration somehow broken?

[cdjackson]

No - this is not the intention. There should only be a single thing discovered - initially the generic device, and finally the specific device. I am reasonably sure that it used to work like this, but I also know there have been some changes in ESH recently to remove some inbox functionality, so maybe this is now broken.

…

On 26 Dec 2018, at 11:45, Thomas Weißschuh ***@***.***> wrote: @cdjackson <https://github.com/cdjackson> One question about the configuration application: Currently after pairing the switch from the inbox it will take some time until it is fully discovered. After the device is fully discovered a second Thing, with the properties and channels configured via the XML files, appears in the Inbox. Adding this Thing will again trigger a full discovery process. In the end there are two Things configured, one generic and one properly usable. Is this intentional or is my configuration somehow broken? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#346 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_kQys6ZXJ0HswekiyBrAzGHgJI2Eiiks5u82FggaJpZM4ZgAFp>.

[t-8ch]

Ok. Maybe someone with one of the other configured devices can try to reproduce the issue.
Otherwise I will have to do some digging.

[hsudbrock]

Hi @t-8ch, thanks for contributing!

I had a first look at the PR, and before I start a detail-level review, I have a general question. The reason for the question is that, at first glance, it looks a little odd to me to use trigger channels in combination with ZigBee attribute reporting - but maybe this is only due to how the Xiaomi Aqara wall switch (which I have not used so far) is implemented.

You have added three trigger channels for the wall switch, left, right, and range-check; consider left for now, assuming that this models button presses for the left button of the wall switch.

The channel triggers when the device reports a new value for the attribute 85 of cluster 0x12 (value 1 for single press, 0 for long press, and 2 for double press).

The question: Will the value of the attribute always indicate the last press that was performed on the left button of the device? I.e., imaging doing a double press, then waiting for 10 seconds, and then reading the attribute from the device. Will the attribute value still be 2? If not, which attribute value will the device show when reading the attribute after 10 seconds?

Background of the question: It seems strange to me that an attribute is used to model an action like a button press, and I wonder why this is modeled like this for the wall switch. Attributes smell like some state that can be retrieved from the device - which could, maybe, be better modeled with a state channel and not a trigger channel in openHAB.

[t-8ch]

To be honest, I only ever used the attribute changed commands emitted by the switch presses.
(That always and promptly are emitted on each button press, even when the same button was already pressed last time).
I will have to double check on the actual value of the attribute.

[t-8ch]

Also the physical device itself does not indicate any state to the user.
The buttons always (mechanically) reset back into the non-pressed state.

[hsudbrock]

Thanks for the feedback, @t-8ch. From what you write, it sounds like the wall switch uses attributes in a - for me - strange way (which might be completely normal when looking at it from another angle though...) From what you write, it sounds reasonable to cover this with the GenericButtonConverter.

[hsudbrock]

I had a look at the changes, see comments inline.

[t-8ch]

Thanks for the feedback. I will try to implement it soon.

[t-8ch]

@hsudbrock I update the PR to incorporate your suggestions.
Could you have another look?

FYI: Currently I don't have access to the switch, so this version of the PR is not actually tested with a physical device.

[t-8ch]

The CI seems to be happy now.

[hsudbrock]

@t-8ch Thanks for the update. Sorry, I did not get to have a look again yet, will do this soon!

Just one hint, as I see that you force-pushed to your branch: For the reviewer, it is often easier if you add new commits instead of changing commits, so that one can see what has changed in addition. (When merging the PR github will squash the commits anyway...)

[t-8ch]

If you prefer it this way sure.
I tried to create logical commits, as the PR does multiple things. Would it be ok, if I add more commits and just before the merge I reorganize them to be 3 (or 2) commits again?

[hsudbrock]

If you prefer it this way sure.

Thanks; no need to change anything now for the review though, just as hint for next time :)

Would it be ok, if I add more commits and just before the merge I reorganize them to be 3 (or 2) commits again?

Fine for me; as far as I recall, github will squash the commits though when merging, creating a commit that has the PR title as commit message, so the reorganization might not be visible in this project's commit history anymore. Not sure 100% though...

[t-8ch]

You should be able to choose when performing the merge.

[hsudbrock]

OK, thanks for the hint!

[t-8ch]

Hm, I will also perform some cosmetic cleanups. (Rewording, things that should not be changed, etc)

[hsudbrock]

Thanks for the update to the PR! I have added some small comments inline.

[t-8ch]

@hsudbrock I addressed your comments.

[hsudbrock]

Thanks, lgtm! OK to merge from your side, @cdjackson ?

[openhab-bot]

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

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

[hsudbrock]

Hi @t-8ch; could you please rebase the PR on the master branch, so that it can be merged?

I just had a look at the merge conflict, I think it is only two things to do:

• For the merge conflict in discovery.txt, simply use the line added by this PR and the line that was added by More Osram smart+ switch support #366
• For the merge conflict in ZigBeeConverterGenericButton, add the improvement from More Osram smart+ switch support #366 into the method CommandSpec#matches as follows:

        boolean matches(ZclCommand command) {
            if (commandId == null) {
                return false;
            }
            boolean commandIdMatches = command.getCommandId().intValue() == commandId;
            return commandIdMatches
                    && (commandParameterName == null || commandParameterValue == null || matchesParameter(command));
        }

WDYT?

[t-8ch]

@hsudbrock Done

[hsudbrock]

@t-8ch Thanks!