Disposing multiple symbol observers can cause instable ADS connection

[maxxie85]

In a situation that haves multiple observers polling values from the PLC. When those observers are disposed at the same time (when navigating away from a view that shows the data) the ADS connection can become instable. When observing the ADS Connection state changed event multiple connection Lost/Resurrected events are fired.

This is also a intermittend problem, because it doesn't always happen when disposing all observers at the same time.

I've created a demo application that in my testing will result in the problem. It creates and disposes 30 observers on 30 different PLC symbols. When the first Connection lost is detected, the automatic toggling is stopped.

In ADS5 when this happens and the observers are created nothing happens. In ADS6 when the connection has become unstable, but the observers are created, the PollValues2 method will cause an TimeOut exception. This can be tested by unchecking the "Stop on Lost" checkbox

DemoApplication.zip

[RalfHeitmann]

With your Demonstration Application I see a few issues that might be ok for a problem demonstration but some are not appropriate for 'productive' code. I'll try to sort this out a little bit (with first thoughts):

1. Around 40 separate Observers are used (2 for each Symbol) and with its own trigger (Task created by 'Interval'). This leads to a heavy use of async Tasks and Ads Request roundtrips. For example: Each Polling trigger is handled in its own Background task and triggers the ReadValue on each symbol (2 times within 500 ms). This leads to an ADS Network-Roundrip for each reading plus one roundtrip on handle creation on observer start and plus one roundtrip on Observer Dispose (DeleteHandle). Statistically we have a roundtrip every 500ms / 40 = 12.5 ms (Minimum time for a roundtrip is around 3 ms). This is already a significant load. For 'productive' code I would strongly recommend to reduce the roundtrips, e.g. by the folllowing overload

   public static IObservable<ResultSumValues> PollValues(this ISumSymbolRead sumRead, IObservable<Unit> trigger)

   This ensures you can use the same trigger for all observers and combines the Read Operation as SumRead in one Network roundtrip. Handle creation and Deletion will be done in one Roundtrip also.

2. Disposing the Observers as reaction of the ConnectionState handler is in the actual situation unfortunate. In my test runs of the DemoApplication from time to time the ReadRequests produced timeouts, that led via handling ConnectionState.Lost to a dispose of the Observers. Because that Dispose communicates (Deletion of Handles) that leads to further Communication errors (as long the integrated FailFastHandler is activated via the Resurrection Timeout. My proposal is to deactivate the FailFastHandler via setting the ResurrectionTimeout to Zero.

settings.ResurrectionTime = TimeSpan.Zero;

That removes the subsequent Timeouts of the next Ads Requests (at least for testing purposes)

3. One change from V5 -> V6 was that a Timeout doesn't break the Observer with an error. So if an Timeout occurs the Observer is still operable. The consequence is that you are not getting a value as long the Ads Request doesn't succeed (see also FailFastHandler and ResurrectionTimeout). With the PollValues overload shown in 1. you are still able to analyse the AdsErrorCode of each ADS request and stop your observer if appropriate.

Edit: Added code for setting ResurrectionTime. Added point 3.

[maxxie85]

Firts of all thanks for your comprehensive response.

1. I find this point interresting as the many timer background tasks can cause significant load. I was able to fairly easy modify my code so that the observers that poll values from the PLC are all triggered by the same observer. However if I monitor the statistics information provided by the AdsSession, the Cycles (or ADS roundtrips according to the comments) still increases heavily when I have many observers polling values from the PLC on different symbols. Suggesting that even triggering with a shared observeble as trigger, each individual observable does causes a ADS roundtrip when reading from the PLC.

2. Fortunately the AdsSession class is a lot more forgiving than the AdsClient class when it comes to disconnects. Using the SessionSettings.Default or SessionSettings.FastWriteThrough both show that errors have occured when disposing many observers when looking at the AdsSession statistics in my testing.

[RalfHeitmann]

However if I monitor the statistics information provided by the AdsSession, the Cycles (or ADS roundtrips according to the comments) still increases heavily when I have many observers polling values from the PLC on different symbols

Therefore I proposed the PollValues overload above (using a SumCommand), that gets the values of all announced values in one roundtrip. So you have one roundtrip for announcing the observer, one roundtrip with getting the values on each observer trigger and one roundtrip on freeing the internal symbol handles on dispose.

[maxxie85]

Do you mean that the ResultSumValues is a class/struct that contains all the members that I want to read from the PLC?

[RalfHeitmann]

Sorry the Documentation for that overload is not online yet (but in the next days). Code Sample:

The ISumSymbolRead object (here SumSymbolRead) would specifies all symbols, and the ResultSumValues object contains - within one roudtrip - the values of the symbols:

using (AdsClient client = new AdsClient())
{
    // Connect to target
    client.Connect(new AmsAddress(AmsNetId.Local, 851));

    // Create Symbol information
    var symbolLoader = SymbolLoaderFactory.Create(client, SymbolLoaderSettings.Default);
    IValueSymbol cycleCount = (IValueSymbol)symbolLoader.Symbols["TwinCAT_SystemInfoVarList._TaskInfo[1].CycleCount"];
    IValueSymbol lastExecTime = (IValueSymbol)symbolLoader.Symbols["TwinCAT_SystemInfoVarList._TaskInfo.LastExecTime"];
    List<ISymbol> symbols = new List<ISymbol>() { cycleCount, lastExecTime };

    //Create the SumCommand
    SumSymbolRead sumRead = new SumSymbolRead(client, symbols);

    // Reactive Notification Handler
    var sumCommandObserver = Observer.Create<ResultSumValues>(result =>
    {
        Console.WriteLine($"SumCommand Succeeded: {result.OverallSucceeded}, CycleCount: {result.Values[0]}, LastExecTime: {result.Values[1]}");
    }
    );

    // Take 20 Values/Result in an Interval of 500ms
    IDisposable subscription = sumRead.PollValues(TimeSpan.FromMilliseconds(500)).Take(20).Subscribe(sumCommandObserver);
    subscription.Dispose(); // Dispose the Subscription
}

[maxxie85]

I tried to implement this in the demo application. However I'm greeted with an exception. The constructor asks for an IList<ISymbol> however upon calling the PollValues I the exception System.InvalidCastException: Unable to cast object of type 'WpfDemo.Common.Models.ValueSymbolDecorator'1[System.Boolean]' to type 'TwinCAT.TypeSystem.IDynamicSymbol'.

My changed code:

private void Start()
{
    if (!_disposables?.IsDisposed ?? false)
    {
        _disposables.Dispose();
    }

    Console.WriteLine("Starting subscriptions");
    _disposables = new CompositeDisposable();

    IAdsConnection connection = _adsSession.Connection as IAdsConnection;

    if (connection == null)
    {
        return;
    }

    List<ISymbol> symbolList = new List<ISymbol>();

    foreach (ChildSymbol symbol in _childSymbols)
    {
        symbolList.Add(symbol.BoolValue);
        symbolList.Add(symbol.DoubleValue);
    }

    SumSymbolRead sumRead = new SumSymbolRead(connection, symbolList);

    _disposables.Add(sumRead.PollValues(TimeSpan.FromMilliseconds(500)).Take(1).Subscribe(OnNext));
}

[RalfHeitmann]

Actually the SumSymbolRead needs the Symbol instances instantiated by the SymbolLoader because the SumCommand expects a Type of class TwinCAT.Ads.TypeSystem.Symbol or a dynamical wrapped IDynamicSymbol (with wrapped instance of class Symbol again). This implementation of SumSymbolRead is suboptimal and bites you here. I will change that in the next version. In the meanwhile, you could put the the original symbol objects to SumSymbolRead instead of your Decorated replacements. That should work.

[RalfHeitmann]

By the way, the documentation of the actual V6 is now online. You can find the PollValues overload here:

https://infosys.beckhoff.com/content/1031/tc3_ads.net/12209228555.html?id=267498840446641199

[maxxie85]

Actually the SumSymbolRead needs the Symbol instances instantiated by the SymbolLoader because the SumCommand expects a Type of class TwinCAT.Ads.TypeSystem.Symbol or a dynamical wrapped IDynamicSymbol (with wrapped instance of class Symbol again). This implementation of SumSymbolRead is suboptimal and bites you here. I will change that in the next version. In the meanwhile, you could put the the original symbol objects to SumSymbolRead instead of your Decorated replacements. That should work.

The function of the decorator is to create a Generic IValueSymbol to make it easy to cast the ReadResult back to the expected Type. It would be awesome if that would be a native feature of ADS. But that's outside the scope of this issue. However if you ask for an ISymbol I expect it should handle anything that implements an ISymbol.

I does work when passing the IValueSymbol implementation from the decorated class. It would be nice though if the results returned the data linked to the ISymbol instance on which they came from. Now you get an array of objects. Instead for instance if it returned an array of

struct SomeStruct
{
  ISymbol symbol; //The ISymbol from the symbol collection passed on to the SumSymbolRead instance
  object? value; //The data from the PLC
}

This would make it easier to work with the result upon receiving them. Now it probably depends on the order of the IList<ISymbol> going into the constructor of the SumSymbolRead. But this means you have to keep that list persistent and immutable because it's the guide to link stuff back.

[RalfHeitmann]

The function of the decorator is to create a Generic IValueSymbol to make it easy to cast the ReadResult back to the expected Type.

Could you give an example what is the expected type? The SymbolLoader marshallers should already cast the value to the appropriate .NET Types. I have no clue what is missing.

It would be nice though if the results returned the data linked to the ISymbol instance on which they came from.

Thats a point. Actually PollValues supports different methods of access. Only one of them is the access via ISymbol. The others are IdxGroup/IndexOffset and InstancePath. All of them have the same return type (ResultSumValues). I could provide a further overload just for the Symbolic option with the ISymbol reference.

[Barteling]

Could you give an example what is the expected type? The SymbolLoader marshallers should already cast the value to the appropriate .NET Types. I have no clue what is missing.

Helping my colleague @maxxie85 here. An example where we would use a (generic) decorator is to have a strongly typed IValueSymbol So we can read values without the need for casting.

IValueSymbol<bool> symbol;
bool result = symbol.ReadValue();

I added the actual interface and decorator in the attachment. New Text Document.txt

So our problem here is that the SumSymbolRead accepts a list of IValueSymbols, but on the background apparently need some specific concrete implementation. (Which is by the way a violation of the Liskov substitution principle)

Do you see a way to change this?

[maxxie85]

Could you give an example what is the expected type? The SymbolLoader marshallers should already cast the value to the appropriate .NET Types. I have no clue what is missing.

Yes you are correct that in Runtime the results are in the appropiate .NET types. However in compile time it's handled as an object. Which requires you to still write code like:

bool result = symbol.ReadValue() as bool;
//or
bool result = (bool) symbol.ReadValue();
//or
if(symbol.ReadValue() is bool result)
{
  //do something with result
}

Then you also have to catch the possibility of a null or you get compiler errors. Using a StronglyTyped IValueSymbol will avoid all that and makes the code simpler, less error prone and easier to unit test.

Thats a point. Actually PollValues supports different methods of access. Only one of them is the access via ISymbol. The others are IdxGroup/IndexOffset and InstancePath. All of them have the same return type (ResultSumValues). I could provide a further overload just for the Symbolic option with the ISymbol reference.

That would be a powerfull function in order to map back the results. And would make using the SumSymbolRead a lot more versatile.

[RalfHeitmann]

An example where we would use a (generic) decorator is to have a strongly typed IValueSymbol So we can read values without the need for casting.

I have to sort this out a little bit - I understand your intention but the way for realization is different from my (first) expectations.

The intention behind the ValueSymbols (represented by the IValueSymbol and/or by the ValueSymbolExtensions) was to return somehow 'dynamic' types, where the PLC specifies the type (mapped from the ADS API automatically to .NET Type without further specifying).

What you are trying to do is (somehow) realized with what we call ANY_TYPE concept. E.g. for observers: AnyTypeExtensions: https://infosys.beckhoff.com/content/1031/tc3_ads.net/9408792331.html?id=3888897450962002941 https://infosys.beckhoff.com/content/1031/tc3_ads.net/9407526667.html?id=6009304797431087429

There is no SumCommand-Polling realized there (actually, what is somehow missing), but if you want to get Multiple-Values (with different types) in one go, then I don't see how you get rid of the cast.

If you get single values it seems that

public static IObservable<T> PollValues<T>(this IAdsConnection connection,string instancePath,IObservable<Unit> trigger)

could be what you want.

So our problem here is that the SumSymbolRead accepts a list of IValueSymbols, but on the background apparently need some specific concrete implementation. (Which is by the way a violation of the Liskov substitution principle)

Yes, actually we have a poor implementation here (because it depends on the more specific "Symbol" class). I will change this. But we cannot fullfill the Liskov principle here, because most of the used Symbol collection classes are basing on IList\<ISymbol> and a limitation to IList\<IValueSymbol> would leave these collections unusable. The SumSymbolRead can use also pure ISymbol objects, but not with dynamic IValueSymbol.ReadValue() access. Then it could downgrade to other access methods (just reading primitives, or just Raw byte arrays).

[maxxie85]

The intention behind the ValueSymbols (represented by the IValueSymbol and/or by the ValueSymbolExtensions) was to return somehow 'dynamic' types, where the PLC specifies the type (mapped from the ADS API automatically to .NET Type without further specifying).

We avoid dynamic types where whe can, as they pose problems that can be avoided by using concrete types. I configure the SymbolsLoadMode as SymbolsLoadMode.VirtualTree. When using this mode IValueSymbol<T> would make perfect sense, as the returned value is already marshalled as the expected type. The generic introduces here compile time validation of types and avoids casting and null check clutter. I don't see a use case for the dynamic here, as I know what I'm reading from the PLC and what type that is. Even if I use the dynamic I should still know what it is what I'm reading, in order to use the data returned by the observable.

What you are trying to do is (somehow) realized with what we call ANY_TYPE concept. E.g. for observers: AnyTypeExtensions: https://infosys.beckhoff.com/content/1031/tc3_ads.net/9408792331.html?id=3888897450962002941 https://infosys.beckhoff.com/content/1031/tc3_ads.net/9407526667.html?id=6009304797431087429

Interesting extension, but it lacks the PollValues2 which the ValueSymbol extensions do have. And as you have said it isn't supported by SumCommand. Which in powerfull in order to reduce ADS roundtrips, which I find interesting and have a use case for. Especially if the results contain the ISymbol instance.

If i would to use it I would add these extensions to actually make it usable with IValueSymbols

public static IObservable<T> PollValues<T>(this IValueSymbol symbol, TimeSpan period)
{
    return (symbol.Connection as IAdsConnection)?.PollValues<T>(symbol.InstancePath, period);
}

public static IObservable<T> PollValues<T>(this IValueSymbol symbol, IObservable<Unit> trigger)
{
    return (symbol.Connection as IAdsConnection)?.PollValues<T>(symbol.InstancePath, trigger);
}

public static IObservable<T> PollValues<T>(this IValueSymbol<T> symbol, TimeSpan period) where T : struct
{
    return (symbol.Connection as IAdsConnection)?.PollValues<T>(symbol.InstancePath, period);
}

public static IObservable<T> PollValues<T>(this IValueSymbol<T> symbol, IObservable<Unit> trigger) where T : struct
{
    return (symbol.Connection as IAdsConnection)?.PollValues<T>(symbol.InstancePath, trigger);
}

Yes, actually we have a poor implementation here (because it depends on the more specific "Symbol" class). I will change this. But we cannot fullfill the Liskov principle here, because most of the used Symbol collection classes are basing on IList and a limitation to IList would leave these collections unusable. The SumSymbolRead can use also pure ISymbol objects, but not with dynamic IValueSymbol.ReadValue() access. Then it could downgrade to other access methods (just reading primitives, or just Raw byte arrays).

Based on IList should be no problem as IValueSymbol is also an ISymbol

edit: typo

[RalfHeitmann]

There is now a new Version 6.0.129 released, which should solve all this issues above in some way. Please have a look at the following changes:

New overloads (also generics with the specified value type) in TwinCAT.Ads.Reactive.ValueSymbolExtensions and TwinCAT.Ads.Reactive.AnyTypeExtensions.

Furthermore, there is now

public static IObservable<ResultSumValues2<S>> PollValues2<S>(this ISumRead2<S> sumRead, IObservable<Unit> trigger)

which adds the Source Symbol into the result and can be used in combination with the SumSymbolRead or SumAnyTypeRead class:

// To Test the Observer run a project on the local PLC System (Port 851)
using (AdsClient client = new AdsClient())
{
    // Connect to target
    client.Connect(new AmsAddress(AmsNetId.Local, 851));

    // Create Symbol information
    var symbolLoader = SymbolLoaderFactory.Create(client, SymbolLoaderSettings.Default);
    IValueSymbol cycleCount = (IValueSymbol)symbolLoader.Symbols["TwinCAT_SystemInfoVarList._TaskInfo[1].CycleCount"];
    IValueSymbol lastExecTime = (IValueSymbol)symbolLoader.Symbols["TwinCAT_SystemInfoVarList._TaskInfo.LastExecTime"];
    List<ISymbol> symbols = new List<ISymbol>() { cycleCount, lastExecTime };

    //Create the SumCommand
    SumSymbolRead sumRead = new SumSymbolRead(client, symbols);

    // Reactive Notification Handler
    var sumCommandObserver = Observer.Create<ResultSumValues2<ISymbol>>(result =>
        {
            Console.WriteLine($"SumCommand ErrorCode: {result.ErrorCode}");

            if (result.Succeeded)
            {
                Console.WriteLine($"SumCommand OverallSucceeded: {result.OverallSucceeded}");
                foreach (var subResult in result.ValueResults)
                {
                    Console.WriteLine($"Source: {subResult.Source}, ErrorCode: {subResult.ErrorCode}, Value: {subResult.Value}");
                }
            }
        }
    );

    // Take 20 Values/Result in an Interval of 500ms
    IDisposable subscription = sumRead.PollValues2(TimeSpan.FromMilliseconds(500)).Take(20).Subscribe(sumCommandObserver);

    Console.ReadKey(); // Wait for Key press
    subscription.Dispose(); // Dispose the Subscription
}

The InvalidCastException should be history also ....

[maxxie85]

I tried to implement this in the demo application. However I'm greeted with an exception. The constructor asks for an IList however upon calling the PollValues I the exception System.InvalidCastException: Unable to cast object of type WpfDemo.Common.Models.ValueSymbolDecorator'1[System.Boolean]' to type 'TwinCAT.TypeSystem.IDynamicSymbol.

This is indeed resolved

ValueSymbolExtensions.PollValues<T>

This is a nice addition, thank you for implementing it!

It would also be a nice completion if PollValues2<T> would be introduced. This than would return a ResultReadValueAccess2<IValueSymbol, T> But that's just a nice to have.

public SumSymbolRead(IAdsConnection connection, IList<ISymbol> symbols).PollValues2

Thanks for adding the additional overload. This makes mapping back the results so much easier. I think ADS would still benefit from a native IValueSymbol<T>, that could than also be used by this overload. The return value of the observer generated by PollValues2 is ResultValue2<S, object> which will translate in ResultValue2<ISymbol, object>. However if there was a native IValueSymbol<T>, it then could become ResultValue2<ISymbol, T> which would remove casting in the processing of the PollValues2.

Returning to the original issue at hand. If I run the demo application from this issue with ADS 6.0.129, it still triggers connection lost state changes. This happens when:

• Every observable has it's own trigger
• A shared trigger
• Reading the symbols with SumSymbolRead

MainViewModel.txt

I attached the modified MainViewModel from the demo application. This is a drop in replacement, but the extension needs to be chagned back to .cs, as github doesn't allow that extension.

[RalfHeitmann]

In the meanwhile I was able to focus on the Connection Lost issue. The problem seems to be triggered by the Dispose of the Observables/Observers, which cancels any outstandig ADS requests internally with a CancellationToken. Unfortunately we use the AdsErrorCode.ClientSyncTimeOut also as return code for the cancellation, which is an indicator for the Lost state. I'm positive to fix the issue in the next release. In the meanwhile - if you don't stop your Observers like in the DemoApplication by Timer, you shouldn't see the Lost states.

[maxxie85]

I'm positive to fix the issue in the next release. In the meanwhile - if you don't stop your Observers like in the DemoApplication by Timer, you shouldn't see the Lost states.

The timer in the demo application is only there to automate the issue. My original encounter with this problem was when I was navigating away from a engineering screen in our application stack that contains a lot of observers. And noticed I would get very strange issues because Lost and Reconnect events triggers mechanisms that act upon losing and restoring the connection to the PLC.

[RalfHeitmann]

Please try the new 6.0.143. That should help.

[maxxie85]

Please try the new 6.0.143

Looks like it is working like it should. I can't trigger the connection state lost with the same testing as I did on the previous versions.