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

chris · June 9, 2018, 1:45pm

So it should be the same for ZWave - and I believe that ZWave complies with the logging guidelines for this level.

Well, that may be, but you mean that only I should suffer?

If we put ALL debug down as TRACE messages, then everytime I ask someone for a DEBUG log, I need to ask them to provide a TRACE log, and this will generate really HUGE files . This will cause many people problems I think as the logging will really be big. Yes, I will suffer as I’ll need to process these logs, but also others I think will have problems as they will need to manage these logs as well.

It’s not that I dislike it at all. This goes against the logging guidelines. It will generate a lot of data in a log configuration that is not meant to generate much at all. We already heard above that people will not like this (not just me).

Sorry, but I’m not sure what to suggest. I’ve already said that I plan to reduce logging before this gets merged. That was always the plan stated a long time ago in this thread. The intensive logging here was mainly to allow transactions, and new features to be fully debugged. I don’t think we can increase the logging to INFO level or I will get in trouble, and if I reduce all debug logging down to TRACE level, it will cause problems with development.

I guess you don’t like my suggestion to manage this in your logback config file as you didn’t comment?

mstormi · June 9, 2018, 2:42pm

To be honest I didn’t understand it.
You want me to essentially run at debug and write a log4j filter to do on-the-fly processing/compression of current debug level output to one-liners ?
I wouldn’t mind, but I’ve got no experience with that and no idea how to do that really…

chris · June 9, 2018, 4:58pm

No - not at all. Simply that you can use the log config file to only log certain class debug if you wanted to eliminate some of the information. I’m not sure that you can produce the one line summary (I don’t even know what that would look like), but simply that if you want to reduce the logging, it is easily possible using the log config file (or even Karaf for that matter).

Instead of enabling logging for the whole binding with -:

log:set DEBUG org.openhab.binding.zwave

you could just enable logging from the transaction manager with -:

log:set DEBUG org.openhab.binding.zwave.internal.protocol.ZWaveTransactionManager

or if you only wanted to see the converter output to see the processed data

log:set DEBUG org.openhab.binding.zwave.internal.converter

So it’s possible for you to reduce the logging significantly by doing this. Sure - you can’t get the summary log entries like this, but you can tailor what you do see in the logs.

Dim · June 11, 2018, 7:00am

@chris Clarification needed

Post #1 uses org.openhab.binding.zwave-2.3.0-SNAPSHOT.jar (shouldn’t that be renumbered to 2.4.0 ? )
File size: 2.028.108 bytes, build date: 04-Jun-2018 17:52

Post #2873 uses org.openhab.binding.zwave_2.4.0.201805312220.jar (with the modified dead nodes handling)
File size: 2.017.323 bytes, build date: 01-Jun-2018 00:21

Q: should I use this one (#2873) or the one from the first post which has a newer timestamp?
Have you implemented the changes wrt the dead nodes handling in the one from the first post?

My (ongoing) testing on the 2.4.0.201805312220 (#2873) showed no issues. Things didn’t go OFFLINE and all was good.

sihui · June 11, 2018, 7:03am

chris · June 11, 2018, 7:12am

Yes - I just haven’t updated the build as something broke -:

The one in the first post hasn’t changed the DEAD node strategy - mainly as @5iver said that half of his system stopped working with that version. I don’t think I’ve had any other negative comments though so I should look to change this soon.

Dim · June 11, 2018, 7:16am

cool!

I will continue with the adventurous version (from Post #2873) of the binding and report back if I see any issues

Thanx @sihui. I missed that reply from @chris

I also found out how to modify the nodeID of the Z-Wave Serial Controller: I had to use Manual addition (my bad)

druciak · June 12, 2018, 6:53am

Finally I found some time to experiment with command class version overriding using thing-type properties and it works great. I am overriding COMMAND_CLASS_ASSOCIATION version to “1” and after reinitializing this reincluded device it has lifeline association properly set to the controller and reports are back!

How can we proceed with this issue? I don’t want to rebuild the binding everytime you publish new version. Is there a way to include this setting in your database?

chris · June 12, 2018, 7:00am

Did you try setting the configuration options to override this, or are you changing the code? It is possible to set the version, and it’s possible to add the command class, so maybe by setting the right parameters you can make this work? I think that just setting the “force version” config option should work.

I need to check the database option to configure this, but if you look at line 600 in the initialisation file you will see the code. If you are making the change there, then the database update will work.

github.com

openhab/org.openhab.binding.zwave/blob/development/src/main/java/org/openhab/binding/zwave/internal/protocol/initialization/ZWaveNodeInitStageAdvancer.java#L600


for (Map.Entry<String, String> entry : properties.entrySet()) {
    String key = entry.getKey();
    String value = entry.getValue();


    String cmds[] = key.split(":");
    if ("commandClass".equals(cmds[0]) == false) {
        continue;
    }
    String args[] = value.split("=");


    if ("setVersion".equals(args[0])) {
        if (zwaveVersionClass.getCommandClass().toString().equals(cmds[1])) {
            logger.debug("NODE {}: Node advancer: VERSION - Set {} to Version {}", node.getNodeId(),
                    CommandClass.getCommandClass(cmds[1]), args[1]);


            // TODO: This ignores endpoint
            try {
                zwaveVersionClass.setVersion(Integer.parseInt(args[1]));
            } catch (NumberFormatException e) {
                logger.error("NODE {}: Node advancer: VERSION - number format exception {}",
                        node.getNodeId(), args[1]);

druciak · June 12, 2018, 7:38am

I’ve added a property to xml with thing-type definition (qubino/zmnhda_0_0.xml), something like this:

<property name="commandClass:COMMAND_CLASS_ASSOCIATION">setVersion=1</property>

druciak · June 12, 2018, 7:40am

Can you explain how and where I need to set this option? I am not sure what you are talking about.

chris · June 12, 2018, 7:44am

Did it work?

As I said above, I need to check how this is set. There will be an option in the database which is set in the command class options - I just need to remember what it is (I think it is FORCEVERSION). This will then generate the XML that I think should look the same as you defined above.

druciak · June 12, 2018, 8:02am

Yes, it did. This is all I’ve done and now the problematic device is initialized properly. There are also other command classes that report version 0, so I am going to override all of them. However ASSOCIATION was the most important and I started my experiments with it.

chris · June 12, 2018, 8:16am

Ok, I just did a test and confirmed that FORCEVERSION is the correct option. This needs to be added to the command class options -:

druciak · June 12, 2018, 12:00pm

Can I edit this by myself? My user name is the same like here…

chris · June 12, 2018, 12:39pm

You can now

druciak · June 12, 2018, 5:18pm

Thanks. I just have added this setting for two more command classes: POWERLEVEL and MANUFACTURER_SPECIFIC. I also updated some other info. Please let me know when a new build is ready.
I will continue testing on my own build until then.

fremel75 · June 14, 2018, 7:09pm

Isnt this related to this thread Status updates for Qubino Relays/Dimmers generally do not work
And that you must activate extra channels or add a temperature sensor and exclude unit from controller and then add it back again
*or maybe not didnt read the whole thread before i posted sorry

chris · June 15, 2018, 8:08am

No - the issue is different. The issue is related to the fact that device reports that it supports version 0 of the command class, and the standards state that if a device reports version 0 it means it doesn’t support the command class. The binding therefore removes it.

We had a discussion at the time on this - if you want to reopen the discussion then please have a read above first in case it explains your questions already.

chris · June 16, 2018, 1:09pm

I’ve created a new version of the test binding with the new DEAD node code here.

This only has a few small changes relating to the number of retries during the initial PING stage.