Add support for round Xiaomi Mijia wireless switch. #707
Conversation
The Xiaomi Mijia wireless switch is a round button. Like a lot of the other LUMI devices, it does not report its clusters correctly and needs to be defined via xml to register correctly. This patterns an xml file based on the other similar Xiaomi devices. I verified that it works with a Xiaomi Mijia button that I have. Signed-off-by: Chris Elford <[email protected]> (github: elfman03) Signed-off-by: Chris Elford <[email protected]>
|
|
||
| <thing-type id="xiaomi_lumisensor-switch" listed="false"> | ||
| <label>Xiaomi LUMI Mijia Button</label> | ||
| <description>Xiaomi LUMI Mijia Button (round)</description> |
There was a problem hiding this comment.
Ideally the description should be different to the label :). Can we say what this is (eg Single round button) - I'm no idea, but it would be good to describe the device. We also shouldn't have the manufacturer name in the description when it's in the label.
| </channel> | ||
| </channels> | ||
|
|
||
| <properties> |
There was a problem hiding this comment.
I don't believe you need to add these properties - they are automatically added by the binding. I know they are also in some of the other files and I plan to remove these when I do some major refactoring to split the Tuya bundle out.
There was a problem hiding this comment.
Without it the button never gets the endpoint to register for me despite how many attempts or how long I hold down the link button. Having this xml seems to pre-seed the device endpoint "1" as being known so it won't get stuck at with an unknown remote endpoint early return point in com.zsmartsystems.zigbee ZigBeeNetworkManager.java(line 1086) -- receiveZclCommand().
My other attempt (which also worked for me) was to change the ZigBeeNetworkManager to ignore that short circuit return and add the endpoint in that unknown situation but you indicated that would violate compliance to do it at that level.
There was a problem hiding this comment.
Without it the button never gets the endpoint
Really? I'm surprised since this information is used to detect the device so adding it to the XML, which is used AFTER the device is detected, doesn't make sense. We don't normally add this information on other devices, although I appreciate it has crept in to some of the Xiaomi devices.
Just to be clear - the binding reads the properties such as the vendor name, and model, and logical type. It then sets these properties, and uses this information to decide the thing type. You adding these properties here can only be used AFTER the binding has already read them - before this, the XML is not used.
Having this xml seems to pre-seed the device endpoint "1" as being known
Sure - but these properties (vendor, modeId and localtype) should not be related to the channel definition.
My other attempt (which also worked for me) was to change the ZigBeeNetworkManager to ignore that short circuit return and add the endpoint in that unknown situation but you indicated that would violate compliance to do it at that level.
Absolutely - what you are doing here with this PR is the correct approach and I'm perfectly happy with the channel definition. I'm just surprised that it doesn't work with these properties added. I can't test this without the device, but these properties are not used in the binding so i really don't understand why you say they are required?
There was a problem hiding this comment.
I haven't 100% tracked it thru the codepath but I get the impression that the modelID property comes thru correctly during device detection but the endpoint/channel definitions don't. I got this impression that this xml file gets applied after the modelId match is detected which then safelists the endpoint definition in the xml file (i.e., indicating that endponit 1 is valid) thus letting it get bypass the flawed portion of the endpoint/channel definition reporting from the device.
There was a problem hiding this comment.
but the endpoint/channel definitions don't.
Sure - but I'm not asking you to remove the endpoint / channel definitions. I'm talking about this property section here that defines the model, vendor, and logical type. These are not used in the binding so I really can't understand how you say it doesn't work without them. Maybe you're right - but it doesn't make sense given these are not used - I'm just confused. Also if it was required, why do other definitions not have this.
Sorry - I'm just a bit confused about this and need to look through it to confirm this really isn't used, but from my quick search I can't see anything in the code.
There was a problem hiding this comment.
Ah. I understand now. I'll see how much I can trim it back. sorry for the spam.
Signed-off-by: Chris Elford [email protected] (github: elfman03) Signed-off-by: Chris Elford <[email protected]>
Signed-off-by: Chris Elford [email protected] (github: elfman03) Signed-off-by: Chris Elford <[email protected]>
The remaining channel level properties (endpoint=1 is definately needed as otherwise ZigBeeThingHandler.java:306 has a null problem.) (input_clusters=6 might be able to be omited and detected if desired) Signed-off-by: Chris Elford <[email protected]> (github: elfman03) Signed-off-by: Chris Elford <[email protected]>
|
I believe I removed all the ones you were concerned about. If desired, I could probably also remove the |
|
I'm a bit surprised that you say you can also remove the cluster - if you remove that, then this XML is not really doing anything. What does this device actually return for the simple descriptor? Is there really a need for this XML? |
|
I'll do some more iterations of testing early next week with different modes and report back. My impression from the few iterations yesterday was that even empty it still provides a visual indicator that pairing has progressed to a point you can stop fiddling with the pairing button in scan mode because a new named line shows up which indicates the necessary data has propagated rather than the zigbee:generic which shows up on earlier attempts. |
|
I don't want to add a static thing definition just so that you get a specific thing type - that's not the way this binding works. It is meant to automatically detect channels - not to use static thing types unless something isn't working correctly with the channel detection. In ZWave we have to maintain a database, and it's a real pain and give the zigbee binding was written after new features were added to OH to support dynamic channel generation, that's what we use. Once the data that is used for thing selection is received, it is added as properties - this includes the modelId, vendor, and some version identifiers, so there is already data to identify the device and the only extra thing you get is the thing type. Please provide the full debug log of the initialisation so we can see what's happening and see what is defined. If there's no need for the static thing then we shouldn't have it. |
|
On further testing, it looks like the endpoint and channel definition are both needed as in the latest patch from last week. Without them while the device gets close to completely detecting, but it gets stuck thinking it is a switch_level rather than a switch_onoff and doesn't seem to eventually fall back to switch_onoff. log attached. |
Well, the log shows this should not be needed as the simple descriptor already defines the endpoint -:
The binding is designed to provide channels based on the devices features, and it does sound that's working ok? As we see above, the simple descriptor defines that it supports both the level control and onoff clusters, so we end up with a level type channel. I'm not sure if you really gain anything you adding a static definition just to change the level to onoff? I just prefer to avoid static definitions if at all possible and it seems here that there is no functionality missing - or am I missing something obvious? :) |
|
As nearly as I can tell, it doesn't actually support the level control despite it's detection. The device doesn't come online as a device_level. If you don't want the patch, it's fine, I'll just build from source whenever I need to update openhab. |
|
Again though, I’m trying to work out what this gains? You have the onoff cluster, so it should work just fine shouldn’t it? I appreciate that changing this from level to onoff is a bit neater, but it doesn’t change anything functionally.
Maybe it doesn’t support level control, although you say “as far as you can tell”. Adding this PR to remove it means that a) if you are wrong, or b) if there is a different version that does support it, users will not be able to. I just think that if the device works as it is detected, then why make this aesthetic change?
… On 28 Nov 2021, at 12:06, elfman03 ***@***.***> wrote:
As nearly as I can tell, it doesn't actually support the level control despite it's detection. The device doesn't come online as a device_level.
If you don't want the patch, it's fine, I'll just build from source whenever I need to update openhab.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
As near as I can tell, the underlying binding removes the onoff channel when its all auto detected. [er.ZigBeeChannelConverterFactoryImpl] - 00158D000186EC2E: Removing channel zigbee:switch_onoff in favor of zigbee:switch_level |
Yes, of course, but that doesn't matter. You have the level control channel and in OH channels inherit their functionality - so a color channel inherits the functions of a level control and on off channels, and a level control inherits the functionality of an onoff channel. So you can still send an OnOff command to a dimmer channel, and if the onoff command is received in a level control channel, the state will be set to on or off. Functionally, it should be the same. We don't just remove the onoff channel and not give users any way to interact with onoff functions of a device. |
|
I don't know what else to log to help understand why it doesn't come online without this xml. |
|
What is the actual problem? It sounds like it is detected and you get the channel, so what is the problem you’re seeing? Do you have a log showing what is received (not during initialisation, but in operation). If the device is offline then that could be caused for other issues.
…Sent from my iPhone
On 28 Nov 2021, at 14:01, elfman03 ***@***.***> wrote:
I don't know what else to log to help understand why it doesn't come online without this xml.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
I attached a ziplog that included both detection and then button presses this afternoon. At button press time, it gets |
Sorry - your comment earlier was about discovery and I was focussed on that when I looked at that log.
Ok, I think I understand the problem with this device. The device is using attribute reporting when it is running as a client - normally a client should use the various commands (eg the OnOff command). When running as a server, the binding therefore does not listen to attribute updates - it expects the client to send the respective commands, and this device doesn't do that. Leave this with me for a few days to think about. |
|
Thanks! |
FYI this seems to be a recurring theme with Xiaomi devices. (See #346 and the other static thing definitions for xiaomi) |
|
Any update on this? Any additional changes needed by me? |
The Xiaomi Mijia wireless switch is a round button. Like a lot of the
other LUMI devices, it does not report its clusters correctly and needs
to be defined via xml to register correctly. This patterns an xml file
based on the other similar Xiaomi devices.
I verified that it works with a Xiaomi Mijia button that I have.
Signed-off-by: Chris Elford [email protected] (github: elfman03)
Signed-off-by: Chris Elford [email protected]