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

Disconnect on alive signal error #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ChrisDoernen
Copy link
Contributor

@ChrisDoernen ChrisDoernen commented Nov 18, 2020

Hallo Christoph,

wir haben festgestellt, dass im Falle eines Kontaktabbruchs (Strom aus, Stecker wird rausgezogen, Netzwerk wird gewechselt, etc.) zwischen dem Client und der SPS kein "disconnect" Event gefeuert wird. Scheinbar bekommt der client von node-snap7 das nicht mit und daher wird in Zeile 133 bei client.Connected() noch true zurückgegeben. Damit ist die Verbindung dauerhaft weg, auch wenn die SPS wieder online ist.

Wenn manuell disconnected wird, kann das Problem umgangen werden.

Das Problem kann z.B. reproduziert weden, indem der Server in einem anderen Prozess gestartet und dann gekillt wird.

Ich werde versuchen, unit tests dafür zu schreiben, wäre aber dankbar, wenn du Zeit findest dir das kurz anzuschauen.

Viele Grüße
Chris

@mathiask88
Copy link
Contributor

@ChrisDoernen
Copy link
Contributor Author

Hi Mathias, thanks for the reply and for the amazing work with node-snap7!

Having read your comment, the call this.client.PlcStatus() should actually be a message that would time out after the connection was lost, right? Does it make a difference that the method is used asynchronous?

@mathiask88
Copy link
Contributor

The issue with raw sockets is that the software does not get any information by the system if the socket is timed out or disconnected until you actually try to write or read to that socket. Most user software or higher level network APIs implement a Ping/pong mechanic to get information about the status of the connection. That is what @psi-4ward implemented here with the PlcStatus().

@ChrisDoernen
Copy link
Contributor Author

Ok, but the problem is that the node-snap7 client doesn't seem to change the status itself. However, when s7client writes with the PlcStatus(), it gets an error, which it is currently ignoring. I feel we should not ignore this error.

Do you think that the proposed change is useful then?

@psi-4ward
Copy link
Owner

I think about a more generic solution:

  protected _getErr(s7err: number | string[]): Error {
    if (Array.isArray(s7err)) return new Error(`${ this.opts.name } Errors: ` + s7err.join('; '));
    const errStr = this.client.ErrorText(s7err).trim();
    if(errStr.includes('TCP : ') && this.client.Connected()) {
      debug(`Set disconnected cause of TCP Error`);
      this.client.Disconnect();
    }
    return new Error(`${ this.opts.name }: ` + errStr);
  }

Ie, manually disconnect on every single TCP error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants