Implement reconnection to ZWave serial stick (#979)

[dottedmag]

Keep probing the port if the stick is not ready on startup or an error has been
received from the serial port.

[dottedmag]

I haven't touched Java since 2002, so please let me know if something has changed since.

[cdjackson]

Thanks - just a few initial comments that will need to be resolved before proceeding.

I don't think this will work properly. How do you reinitialise the device once it is plugged in? There is a lot more to do than just to initialise the serial port. You probably need to close down the controller and restart it - or at leat ensure that the reinitialisation is completed. This could be quite messy to achieve (although I've not looked at it, so maybe it's not too hard).

Please sign your commit and PR.

Please ensure any PR is against the development branch, and not master.

[dottedmag]

I have signed my commit, and moved PR against "development" branch, but how do I sign PR?

When a serial port is closed, SerialPortConnectorThread kicks in, trying to connect until it succeeds. Once it succeeds it calls initializeNetwork() that cares about initialization of stuff.

In original PR I have missed that initializeNetwork() assigned some member variables in ZWaveControlerHandler. On a second initialization they were rewritten, and previous values were not canceled/deactivated/shutdown, so it created some resource leaks.

Updated PR calls dispose() on serial port error to close all resources used by both ZWaveControllerHandler and ZWaveSerialHandler.

[cdjackson]

How does it handle all the initialisation that will start again on every node. It looks like this will cause additional threads to start as I don’t think there is any protection against this? I’m just a bit worried that this hasn’t been properly designed to do this, and I’d really like to ensure that it’s fully tested against all (or as many as possible) scenarios. I don’t have the time to spend testing this right now so please try and check this. Thanks.

[dottedmag]

I don't see where ZWaveController ever removes ZWaveNodes from itself, so I'll work on it a little bit more to understand.

[cdjackson]

No - this is sort of my point. The software wasn’t designed to be restarted like this, so I want to be sure that it’s not going to cause any issue. It might be fine as it is, and I’d certainly be happy to avoid changing too much, but I’d like to be sure that if we merge this, it doesn’t cause a load of unwanted issues as soon as more people start using it ;) Thanks - let me know how you get on and if you have any questions, just fire away….

…

On 24 Aug 2018, at 00:10, Mikhail Gusarov ***@***.***> wrote: I don't see where ZWaveController ever removes ZWaveNodes from itself, so I'll work on it a little bit more to understand. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#980 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_kQ1USzzJhIWgcciAXHw6DgvSQp_-Iks5uTzZYgaJpZM4WIhQm>.

[bodiroga]

Hi @dottedmag!

Are you still interested in supporting this feature? It would be really cool if we could add it for the next version of openHAB (2.5, not 2.4, of course). I have implemented a reconnect feature in my "tcp_controller" branch (https://github.com/bodiroga/org.openhab.binding.zwave/tree/tcp-controller) and it is working great.

My suggestion is to also modify the ZWaveThingHandler's bridgeStatusChanged method like this:

ZWaveControllerHandler bridgeHandler = (ZWaveControllerHandler) getBridge().getHandler();

if (bridgeStatusInfo.getStatus() != ThingStatus.ONLINE) {
    updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE, ZWaveBindingConstants.OFFLINE_CTLR_OFFLINE);
    logger.debug("NODE {}: Controller is not online.", nodeId, bridgeStatusInfo.getStatus());
    synchronized (pollingSync) {
        if (pollingJob != null) {
            pollingJob.cancel(true);
            pollingJob = null;
        }
    }
    controllerHandler = null;
    if (bridgeHandler != null) {
        bridgeHandler.removeEventListener(this);
    }
    return;
}

logger.debug("NODE {}: Controller is ONLINE. Starting device initialisation.", nodeId);

This is necessary to stop all threads and pending jobs from each node when USB stick is disconnected. Otherwise, your changes look good.

Thanks for your work!

[dottedmag]

@bodiroga I definitely am, though I don't have the time right now to really read all the code and understand what else might be needed as @cdjackson has explained.

I'll rebase the patch to the new version, add with your changes and let it hang here for a while until I or someone else figures the rest out.

[openhab-bot]

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

https://community.openhab.org/t/share-z-wave-dongle-over-ip-usb-over-ip-using-ser2net-socat-guide/34895/165

[cdjackson]

Closing as no changes for 8 months and also superceded by #1210