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

Add static definition for Xiaomi Wireless 2-Button Switch #379

Merged
merged 6 commits into from
Feb 9, 2019
Merged

Add static definition for Xiaomi Wireless 2-Button Switch #379

merged 6 commits into from
Feb 9, 2019

Conversation

puzzle-star
Copy link
Contributor

This adds support for the non-standard Xiaomi lumi.sensor_86sw2, the two button wireless (battery powered) switches.

It adds three channels (switch_onoff cluster at endpoints 1, 2 and 3), for left button (switch_1), right button (switch_2) and both buttons at the same time (switch_3)

@hsudbrock
Copy link
Contributor

hsudbrock commented Feb 4, 2019

Thanks @puzzle-star !

Just to avoid misunderstandings: Using a static thing type definition for this switch is required, as the device does not correctly announce the clusters in its simple descriptors, right? I think it could be useful to have the reason documented with a comment in the thing type XML, wdyt?

@puzzle-star
Copy link
Contributor Author

Right. It reports the wrong set of clusters, but suports the (non reported) switch cluster

Ok to document in the XML (I assume there is a way it can be seen in the GUI). Will change it.

@t-8ch
Copy link
Member

t-8ch commented Feb 4, 2019

@puzzle-star , how did you find out, that it was these clusters?
Did the button presses show up in the zigbee traces even without binding these clusters, or do you have a way to bind all clusters and see what happens when pressing a button.

I have another PR (#346) that implements static definitions for another xiaomi switch and I am wondering if I missed the "normal" commands, because I did not bind the relevant cluster.

@cdjackson
Copy link
Contributor

cdjackson commented Feb 4, 2019 via email

@t-8ch
Copy link
Member

t-8ch commented Feb 4, 2019

@cdjackson , I know about these. Afaik I have to subscribe to these clusters to receive their commands.
Is there a way of subscribing all clusters while developing a new config. Then I could press the button a look at the zigbee debug log to see which command was actually sent.
(This is how I found out about the attribute reports of the other switch).
So I don't have to try each cluster own-by-one.

@cdjackson
Copy link
Contributor

cdjackson commented Feb 4, 2019 via email

@puzzle-star
Copy link
Contributor Author

Hi @hsudbrock and @cdjackson

Ok to document in the XML (I assume there is a way it can be seen in the GUI). Will change it.

done :-)

@cdjackson
Copy link
Contributor

Thanks @puzzle-star - but - I'm not sure this is the best approach.

Do we really think that users need to know about this? Personally, I don't think so - I would personally suggest to put this as a comment in the XML rather than putting it into the thing type description. Ideally, users don't need to know if this is a static thing type, or dynamic, or the reason why it's one or the other - they just need to know it works and I think this should not be provided to users.

If @hsudbrock thinks that users need to know this information, then maybe I can be convinced, but my preference is not to have this displayed in the UI.

@puzzle-star
Copy link
Contributor Author

Hi @cdjackson,

Ideally, users don't need to know if this is a static thing type, or dynamic, or the reason why it's one or the other - they just need to know it works and I think this should not be provided to users.

I tend to agree, but also I am seing the "add manually list" starting to grow, with lots of devices that cannot really be added manually, maybe a "Not to be added manually" note on a newline should anyway be added?

@cdjackson
Copy link
Contributor

cdjackson commented Feb 9, 2019 via email

@cdjackson
Copy link
Contributor

If we add listed="false" to the thing-type definition, it will not show up in the list -:

  <thing-type id="popp_700045_00_000" listed="false">

I actually suggest that we do this for all static thing types - I meant to start doing this a while back, but forgot...

@puzzle-star
Copy link
Contributor Author

puzzle-star commented Feb 9, 2019

OK. I think that approach is better, but I would suggest then to leave the comment in the description in a newline, separated by a "br" html tag. According to the thing xml documentation, this should be the way to have GUIs showing the first line in short representations.

I would suggest to keep the description separated by a BR, and adding the listed="false"

What do you think?

@cdjackson
Copy link
Contributor

cdjackson commented Feb 9, 2019 via email

@puzzle-star
Copy link
Contributor Author

OK, then, will leave it in a comment in the XML, out of the description

@cdjackson
Copy link
Contributor

cdjackson commented Feb 9, 2019 via email

@puzzle-star
Copy link
Contributor Author

Done. Once this is merged, I will do a PR to add the listed="false" to the other static thing definitions, to clean up the list

@cdjackson
Copy link
Contributor

Thanks Pedro.

Copy link
Contributor

@cdjackson cdjackson left a comment

Choose a reason for hiding this comment

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

LGTM

@cdjackson cdjackson merged commit 1d312f1 into openhab:master Feb 9, 2019
@puzzle-star puzzle-star deleted the xiaomi-lumi-sensor-86sw2 branch February 9, 2019 11:21
@hsudbrock
Copy link
Contributor

If @hsudbrock thinks that users need to know this information, then maybe I can be convinced, but my preference is not to have this displayed in the UI.

No, my suggestion above was also to merely add a comment to the XML - sorry if what I wrote was ambiguous.

Thanks for adding this thing type!

@puzzle-star
Copy link
Contributor Author

Hi @hsudbrock ,

Already discussed and agreed with @cdjackson (and fixed) :-)

@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