IsConnected

[scamille]

Currently, in Sally7.Plc.S7Connection the value of client.IsConnected is not exposed publicly at all.

Is this intentional?

My current pattern in how I use S7 drivers is like this:

First, setup plc/connection object without actually connecting to the PLC.

Then, usually in some kind of event loop, executed from time to time:

void DoStuffWithPlc()
{
  if (!plc.IsConnected)
    plc.Connect();
  plc.Read/WriteData();
}

This allows me to not worry on how to establish the connection the first time, or if it was closed at any time during operations (either by the remote host, my OS, or by myself calling plc.Close() somewhere else)

I know that I can't know if the connection is still operational at all just by calling IsConnected, and that the read/write call will just fail in case the connection is down. That means accepting failure and recovering from it, which will happen when the function is called the next time, either because the event loop executes again or I have something like Polly set up which tries multiple times.

How do you handle that with the current state of Sally7? Do you just Dispose the whole S7Connection on any Exception? If not, how do you recover from the situation that a connection has been closed?

Or put differently: Do you have anything against a PR exposing TcpClient.IsConnected?

[mycroes]

Hi @scamille,

We have PLC infrastructure that allows us to connect to different types/brands of PLC's, all we need to do to read from a different PLC type is change the PLC connection configuration and change variable addresses. As such, we have some custom code per PLC and some shared code across all PLC implementations.

I connect at application startup, but most of the time we read (almost) continuously (though based on different groups, with their own timer and a single queue per PLC for all requests). Actually I currently don't apply any connection check at all I think, so if a read fails it often is retried a couple hundred times while the connection is being established 😆

For our Sally7 wrapper I'm using the following to catch communication exceptions:

public override bool IsCommunicationException(Exception exception)
{
    return exception is ObjectDisposedException || exception is IOException ||
        base.IsCommunicationException(exception);
}

The base method is implemented as follows:

public virtual bool IsCommunicationException(Exception exception)
{
    var aggregateException = exception as AggregateException;
    if (aggregateException != null)
    {
        return aggregateException.Flatten().InnerExceptions.All(IsCommunicationException);
    }

    return exception is InvalidOperationException
        || exception is PlcCommunicationException
        || exception is TimeoutException;
}

PlcCommunicationException is a custom exception, in case of our Sally7 wrapper it never gets used I think. We basically use this method to classify exceptions as transient or permanent. If the exception is permanent, we throw it and in case of the poller we normally use we just stop polling that group of addresses. If it's transient, we retry the action.

As far as exposing the IsConnected property, I'm not sure if I'd approve. I've been thinking about exposing the entire TcpClient though, because that would allow for far more extensibility. Also keep in mind that while this is my own library and I fully own the copyright personally, I'm using this professionally so any changes that have impact on that will probably be rejected anyway 😛

[scamille]

So you classify a communication exception as permanent, and then just dispose the whole Sally7.S7Connection and reconnect, correct?

I guess I can also just dispose the connection on all exceptions except some whitelisted, transient ones. But since I don't except many exceptions at all once everything is set up, that might not even be worth classifying.

For my application the only important goal is to be able to recover the connection eventually, even if it means being sometimes a bit too blunt and killing stuff off unnecessarily.

And sure transient errors will continue to appear as long as the problem persists. As long as it eventually reconnects once they no longer persist, I am happy.

[mycroes]

Actually, I classify communication exceptions as transient, in which case I still reconnect though. The permanent exceptions are when the PLC reports the datablock doesn't exist for instance, in that case I no longer poll that address in the PLC.

As for the original request, I now expose a TcpClient property (of equally named type) in v0.9.0 which you can use to check for IsConnected. I also added an S7CommunicationException, I'm not promising it's thrown on every connection error, but at least it's thrown on invalid TPKT's.

Feel free to reopen this issue if you think it's not properly resolved yet.

[scamille]

It allows me to replace this:

        private bool IsConnected()
        {
            // Get at the TcpClient of the library
            var field = typeof(Sally7.S7Connection).GetField("client",
                BindingFlags.NonPublic | BindingFlags.GetField | BindingFlags.Instance)?.GetValue(_plc);
            var tcpClient = (TcpClient?)field;
            return tcpClient?.Connected ?? false;
        }

So thank you :-) I'll try out the exceptions, but it will probably take some weeks/months until I get the time to do all this and try it out.