OH2 Z-Wave refactoring and testing... and SECURITY

Add-ons Bindings
,
2652

Just found a while to analyze the code. Decapsulating multiinstance code looks weird to me…

We have a notification:

2018-04-02 14:44:31.265 [DEBUG] [WaveSerialHandler$ZWaveReceiveThread] - Receive Message = 01 0C 00 04 00 1A 06 60 06 02 25 03 FF 56

So this is:

2018-04-02 14:44:31.281 [DEBUG] [nal.protocol.ZWaveTransactionManager] - Received msg (0): Message: class=ApplicationCommandHandler[4], type=Request[0], dest=26, callback=0, payload=00 1A 06 60 06 02 25 03 FF

Which is sent from node 26 with multiinstance message: 60 06 02 25 03 FF
This is what we have in ZWaveCommandClassPayload as payload bytes.

Then the control goes to ZWaveNode.processCommand(60 06 02 25 03 FF): https://github.com/openhab/org.openhab.binding.zwave/blob/development/src/main/java/org/openhab/binding/zwave/internal/protocol/ZWaveNode.java#L1126

Here: https://github.com/openhab/org.openhab.binding.zwave/blob/development/src/main/java/org/openhab/binding/zwave/internal/protocol/ZWaveNode.java#L1203
we check payload.getCommandClassId() which takes byte nbr 0 of the payload -> 60 (COMMAND_CLASS_MULTI_CHANNEL)
and payload.getCommandClassCommand() which takes byte nbr 1 of the payload -> 06

Then we go into ‘if’ statement and here: https://github.com/openhab/org.openhab.binding.zwave/blob/development/src/main/java/org/openhab/binding/zwave/internal/protocol/ZWaveNode.java#L1219
we realize this is MULTI_INSTANCE_ENCAP
AND (!) we get instance number from byte nbr 1 (from which we were taking class command) -> 06.

Shouldn’t we take byte nbr 2 (02)? This is what I spotted just browsing sources, haven’t had time to debug this yet, but definitely will do.

Is this a bug or I am just missing something?

2653

I will take a look - thanks (but this is not related to the issue where the device is not responding to the MC requests).

2654

Strictly speaking, it’s a bug, but the code is not used so can be deleted. It will not change the operation of the binding at all since it is assigned to a value that is never used. Probably this was a line for debugging at some stage in the past…

2655

I am talking about Node 13 and 26 issue, this log is for node 26.

Are you sure? Please take a closer look. There is another bug: endpoint number is assigned indeed to wrong variable, but the code is definitely used.

I just tested it and now - woohoo - reports are back for node 13 and 26. I am going to make a PR in a few minutes.

1 Like
2656

I’m a bit confused what you’re talking about with this? What log are you talking about - your report above didn’t have a log :confused: .

Well, I just deleted the line and there were no errors as the variable is not used, so yes, I’m pretty sure.

Cool - I’ll be interested to see this :wink: .

2657

Just to be sure… @chris, did you see my post?

2658

No - it’s lost in this thread. It doesn’t seem to be related to security in any way is it? Maybe a separate thread would be better as we don’t need to discuss every zwave issue here :wink: .

I’m not really sure I can answer much though - probably something strange in the lower layers outside of the binding, but I don’t know.

2659

Read my post carefully. Indeed instanceNumber is not used, but everything inside this ‘if’ (lines 1219-) is needed. So there are actually 2 bugs.

Here is PR: Fix for MULTI_INSTANCE_ENCAP by druciak · Pull Request #865 · openhab/org.openhab.binding.zwave · GitHub

1 Like
2660

Nope. I thought this thread was about this developement binding in general, not just the security part. I guess if you can’t answer there aren’t much hope here though. I’ll see if I get any further information out and post in a new thread if I do :frowning:

2661

Sorry - it was not so easy to understand.

2662

PR looks good - thanks. It should help solve some of the issues with old devices that support the older multi-instance class.

Please can you sign the commit and the PR though. Thanks.

2663

Yes, I know. I was in the middle of building/installing/analysing the binding and focusing on all of these so my posts here are little bit chaotic. :slight_smile:

The commit has “signed-off” do I need to do something more?

2664

Generally it should be on the PR as well, but that’s fine - I didn’t see it on the commit. I’ve merged and I’ll try and do an update later or tomorrow.

2665

Hi, thank you for a good refactoring session :wink: Im having issues with some of my fibaro magnet sensor and smoke detectors and hopes that this will be the remedy.
Is there any upgrade path explained somewhere? Perhaps a wiki? Second, when do you think this will be mainstreamed and will it require manual steps?

Keep up the good work! :slight_smile:

2666

The first 5 or 6 messages in this thread have the update method.

I will try and look at it once I finish some other work in a month or so.

Yes.

2667

It would be helpful if you could include steps in the first post for removing/rediscovering existing Things, and remove the link to the old jar in post 6. And since this thread gets so much traffic, maybe add some helpful links, like the database and OH zwave binding documentation.

2668

I’ve load the newest development snapshot of 2.April 2018.
Now all my Z-Wave devices are marked as UNINITIALIZED in Paper UI.

Before was installed the snapshot 2.3.0 of this link:
https://openhab.ci.cloudbees.com/job/openHAB2-Bundles/lastStableBuild/org.openhab.binding%24org.openhab.binding.zwave/

Do you know something about this problem?

2669

Yes, read the first couple of posts in this thread: you need to delete all things and readd them if you want to switch from snapshot to development.

2670

I have a GE Zwave Fan switch and since switching to Openhab2 it no longer has speed control. It only does on/off.It is being recognized as a ‘ZWave 4002 In-Wall Smart Fan Control’

Any thoughts?

2671

I guess you need to link two items, one switch and one dimmer, to the switch_dimmer channel:
http://www.cd-jackson.com/index.php/zwave/zwave-device-database/zwave-device-list/devicesummary/281