S7NetPlus / s7netplus

S7.NET+ -- A .NET library to connect to Siemens Step7 devices
MIT License
1.3k stars 581 forks source link

WriteClass bug #344

Closed ghost closed 3 years ago

ghost commented 3 years ago

Hi,

  1. The WriteClass method write the data into the PLC memory but never come back,stick into itself, so I have to terminate the program.
  2. THe WriteClass method can't handle the LREAL datatype.

Pls help Thanks

FalcoGoodbody commented 3 years ago

I am sorry but I actually did not tried to write a class back to the plc. Can check this week maybe

SalgadoDs commented 3 years ago

I had the same situation, I've modified a little bit the library to "Solve" this problem, I have two ways to kind of solve it, the first one is in the async process, im not quit experienced in async processes so i think this solution can be improved since it works good if you only do some writes, lets say write data with a button, but if you continuously write to plc it gets stuck after some write requests

here is what I did:

In PlcAsynchronous.cs file modified this way WriteBytesWithASingleRequestAsync Task

private async Task WriteBytesWithASingleRequestAsync(DataType dataType, int db, int startByteAdr, byte[] value, int dataOffset, int count, CancellationToken cancellationToken)
        {

            try
            {
                var stream = GetStreamIfAvailable();
                CancellationTokenSource cs = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var dataToSend = BuildWriteBytesPackage(dataType, db, startByteAdr, value, dataOffset, count);
                    await stream.WriteAsync(dataToSend, 0, dataToSend.Length, cs.Token);
                    cs.Dispose();
                }, cs.Token);

                CancellationTokenSource cs2 = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var s7data = await COTP.TSDU.ReadAsync(stream, cs2.Token);
                    ValidateResponseCode((ReadWriteErrorCode)s7data[14]);
                    cs2.Dispose();
                }, cs2.Token);
            }
            catch (OperationCanceledException)
            {
                throw;
            }
            catch (Exception exc)
            {
                throw new PlcException(ErrorCode.WriteData, exc);
            }
        }

As told before it works good if you are not continuosly writing however if you fire many times writing request will get stuck also. The problem is that await stream.WriteAsync and await COTP.TSDU.ReadAsync are never ending and that is the only way I found to stop them may be you can improve this solution.

The second solution and the one that Im using is making synchornous process, IDK why it is needed to by Async because for me it is working ok with Sync process, here is what I did:

In PlcSynchronous.cs created WriteClassSync void just by removing Async operators (Async, await) from WriteClassAsync Task

public void WriteClassSync(object classValue, int db, int startByteAdr = 0)
        {
            byte[] bytes = new byte[(int)Class.GetClassSize(classValue)];
            Types.Class.ToBytes(classValue, bytes);
            WriteBytes(DataType.DataBlock, db, startByteAdr, bytes);
        }

and changed in WriteClass void WriteClassAsync for WriteClassSync

public void WriteClass(object classValue, int db, int startByteAdr = 0, int timeout = 1000)
        {

            CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
            cancellationTokenSource.CancelAfter(TimeSpan.FromMilliseconds(timeout));

            //WriteClassAsync(classValue, db, startByteAdr, cancellationTokenSource.Token).GetAwaiter().GetResult();
            WriteClassSync(classValue, db, startByteAdr);
        }

for me the second solution is ok, however if you want to take a look and maybe you can implement and improve in your library so that it will be updated. Hope it helps someone

dhakh commented 3 years ago

I had the same situation, I've modified a little bit the library to "Solve" this problem, I have two ways to kind of solve it, the first one is in the async process, im not quit experienced in async processes so i think this solution can be improved since it works good if you only do some writes, lets say write data with a button, but if you continuously write to plc it gets stuck after some write requests

here is what I did:

In PlcAsynchronous.cs file modified this way WriteBytesWithASingleRequestAsync Task

private async Task WriteBytesWithASingleRequestAsync(DataType dataType, int db, int startByteAdr, byte[] value, int dataOffset, int count, CancellationToken cancellationToken)
        {

            try
            {
                var stream = GetStreamIfAvailable();
                CancellationTokenSource cs = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var dataToSend = BuildWriteBytesPackage(dataType, db, startByteAdr, value, dataOffset, count);
                    await stream.WriteAsync(dataToSend, 0, dataToSend.Length, cs.Token);
                    cs.Dispose();
                }, cs.Token);

                CancellationTokenSource cs2 = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var s7data = await COTP.TSDU.ReadAsync(stream, cs2.Token);
                    ValidateResponseCode((ReadWriteErrorCode)s7data[14]);
                    cs2.Dispose();
                }, cs2.Token);
            }
            catch (OperationCanceledException)
            {
                throw;
            }
            catch (Exception exc)
            {
                throw new PlcException(ErrorCode.WriteData, exc);
            }
        }

As told before it works good if you are not continuosly writing however if you fire many times writing request will get stuck also. The problem is that await stream.WriteAsync and await COTP.TSDU.ReadAsync are never ending and that is the only way I found to stop them may be you can improve this solution.

The second solution and the one that Im using is making synchornous process, IDK why it is needed to by Async because for me it is working ok with Sync process, here is what I did:

In PlcSynchronous.cs created WriteClassSync void just by removing Async operators (Async, await) from WriteClassAsync Task

public void WriteClassSync(object classValue, int db, int startByteAdr = 0)
        {
            byte[] bytes = new byte[(int)Class.GetClassSize(classValue)];
            Types.Class.ToBytes(classValue, bytes);
            WriteBytes(DataType.DataBlock, db, startByteAdr, bytes);
        }

and changed in WriteClass void WriteClassAsync for WriteClassSync

public void WriteClass(object classValue, int db, int startByteAdr = 0, int timeout = 1000)
        {

            CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
            cancellationTokenSource.CancelAfter(TimeSpan.FromMilliseconds(timeout));

            //WriteClassAsync(classValue, db, startByteAdr, cancellationTokenSource.Token).GetAwaiter().GetResult();
            WriteClassSync(classValue, db, startByteAdr);
        }

for me the second solution is ok, however if you want to take a look and maybe you can implement and improve in your library so that it will be updated. Hope it helps someone

it works,thanks!

TechByte-300 commented 3 years ago

As told before it works good if you are not continuosly writing however if you fire many times writing request will get stuck also. The problem is that await stream.WriteAsync and await COTP.TSDU.ReadAsync are never ending and that is the only way I found to stop them may be you can improve this solution.

Confirm, WriteClass simply became a disaster. It hangs up everything and everytime.

@SalgadoDs, do you use modified method as is or also being wrapped in Task.Run method?

thanks.

SalgadoDs commented 3 years ago

As told before it works good if you are not continuosly writing however if you fire many times writing request will get stuck also. The problem is that await stream.WriteAsync and await COTP.TSDU.ReadAsync are never ending and that is the only way I found to stop them may be you can improve this solution.

Confirm, WriteClass simply became a disaster. It hangs up everything and everytime.

@SalgadoDs, do you use modified method as is or also being wrapped in Task.Run method?

thanks.

I use it as it is, I dont need to continuously write, just write values once every user press button for example, I also created some other functions like dimensionStruct and readstructadvanced, writestructadvanced which reads also strings to TIA portal specific sizes, it is easier to use since works with arrays, structs, strings, etc, didnt test with Lreal, but for mw works, only created struct like the complete DB or part of it and reads/writes like a charm, also separated the measure function just to dimension structure once and then only read or write to it with that dimension with makes it faster, later I will post those functions that may help you or anyone else

mycroes commented 3 years ago

I had the same situation, I've modified a little bit the library to "Solve" this problem, I have two ways to kind of solve it, the first one is in the async process, im not quit experienced in async processes so i think this solution can be improved since it works good if you only do some writes, lets say write data with a button, but if you continuously write to plc it gets stuck after some write requests

here is what I did:

In PlcAsynchronous.cs file modified this way WriteBytesWithASingleRequestAsync Task

private async Task WriteBytesWithASingleRequestAsync(DataType dataType, int db, int startByteAdr, byte[] value, int dataOffset, int count, CancellationToken cancellationToken)
        {

            try
            {
                var stream = GetStreamIfAvailable();
                CancellationTokenSource cs = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var dataToSend = BuildWriteBytesPackage(dataType, db, startByteAdr, value, dataOffset, count);
                    await stream.WriteAsync(dataToSend, 0, dataToSend.Length, cs.Token);
                    cs.Dispose();
                }, cs.Token);

                CancellationTokenSource cs2 = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var s7data = await COTP.TSDU.ReadAsync(stream, cs2.Token);
                    ValidateResponseCode((ReadWriteErrorCode)s7data[14]);
                    cs2.Dispose();
                }, cs2.Token);
            }
            catch (OperationCanceledException)
            {
                throw;
            }
            catch (Exception exc)
            {
                throw new PlcException(ErrorCode.WriteData, exc);
            }
        }

As told before it works good if you are not continuosly writing however if you fire many times writing request will get stuck also. The problem is that await stream.WriteAsync and await COTP.TSDU.ReadAsync are never ending and that is the only way I found to stop them may be you can improve this solution.

The second solution and the one that Im using is making synchornous process, IDK why it is needed to by Async because for me it is working ok with Sync process, here is what I did:

In PlcSynchronous.cs created WriteClassSync void just by removing Async operators (Async, await) from WriteClassAsync Task

public void WriteClassSync(object classValue, int db, int startByteAdr = 0)
        {
            byte[] bytes = new byte[(int)Class.GetClassSize(classValue)];
            Types.Class.ToBytes(classValue, bytes);
            WriteBytes(DataType.DataBlock, db, startByteAdr, bytes);
        }

and changed in WriteClass void WriteClassAsync for WriteClassSync

public void WriteClass(object classValue, int db, int startByteAdr = 0, int timeout = 1000)
        {

            CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
            cancellationTokenSource.CancelAfter(TimeSpan.FromMilliseconds(timeout));

            //WriteClassAsync(classValue, db, startByteAdr, cancellationTokenSource.Token).GetAwaiter().GetResult();
            WriteClassSync(classValue, db, startByteAdr);
        }

for me the second solution is ok, however if you want to take a look and maybe you can implement and improve in your library so that it will be updated. Hope it helps someone

Just a quick reply in hope to save people from copying this nonsense. The only thing that might have an actual effect is removing of async calls, even though I'm not sure what's currently breaking the sync calls that use async calls underneath. The facade with Task.Run and the CancellationTokenSource is just utter bullshit. It might put enough of a delay on stuff so that concurrency no longer occurs, but the actual fix would be not to make concurrent calls to the PLC instance. It's not thread-safe, just like many classes in the .NET BCL. I might change that soon-ish, but please refrain from suggesting things you whole-heartedly acknowledge you don't understand.

SalgadoDs commented 3 years ago

I had the same situation, I've modified a little bit the library to "Solve" this problem, I have two ways to kind of solve it, the first one is in the async process, im not quit experienced in async processes so i think this solution can be improved since it works good if you only do some writes, lets say write data with a button, but if you continuously write to plc it gets stuck after some write requests here is what I did: In PlcAsynchronous.cs file modified this way WriteBytesWithASingleRequestAsync Task

private async Task WriteBytesWithASingleRequestAsync(DataType dataType, int db, int startByteAdr, byte[] value, int dataOffset, int count, CancellationToken cancellationToken)
        {

            try
            {
                var stream = GetStreamIfAvailable();
                CancellationTokenSource cs = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var dataToSend = BuildWriteBytesPackage(dataType, db, startByteAdr, value, dataOffset, count);
                    await stream.WriteAsync(dataToSend, 0, dataToSend.Length, cs.Token);
                    cs.Dispose();
                }, cs.Token);

                CancellationTokenSource cs2 = new CancellationTokenSource(TimeSpan.FromMilliseconds(2000));
                _ = Task.Run(async () => {
                    var s7data = await COTP.TSDU.ReadAsync(stream, cs2.Token);
                    ValidateResponseCode((ReadWriteErrorCode)s7data[14]);
                    cs2.Dispose();
                }, cs2.Token);
            }
            catch (OperationCanceledException)
            {
                throw;
            }
            catch (Exception exc)
            {
                throw new PlcException(ErrorCode.WriteData, exc);
            }
        }

As told before it works good if you are not continuosly writing however if you fire many times writing request will get stuck also. The problem is that await stream.WriteAsync and await COTP.TSDU.ReadAsync are never ending and that is the only way I found to stop them may be you can improve this solution. The second solution and the one that Im using is making synchornous process, IDK why it is needed to by Async because for me it is working ok with Sync process, here is what I did: In PlcSynchronous.cs created WriteClassSync void just by removing Async operators (Async, await) from WriteClassAsync Task

public void WriteClassSync(object classValue, int db, int startByteAdr = 0)
        {
            byte[] bytes = new byte[(int)Class.GetClassSize(classValue)];
            Types.Class.ToBytes(classValue, bytes);
            WriteBytes(DataType.DataBlock, db, startByteAdr, bytes);
        }

and changed in WriteClass void WriteClassAsync for WriteClassSync

public void WriteClass(object classValue, int db, int startByteAdr = 0, int timeout = 1000)
        {

            CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
            cancellationTokenSource.CancelAfter(TimeSpan.FromMilliseconds(timeout));

            //WriteClassAsync(classValue, db, startByteAdr, cancellationTokenSource.Token).GetAwaiter().GetResult();
            WriteClassSync(classValue, db, startByteAdr);
        }

for me the second solution is ok, however if you want to take a look and maybe you can implement and improve in your library so that it will be updated. Hope it helps someone

Just a quick reply in hope to save people from copying this nonsense. The only thing that might have an actual effect is removing of async calls, even though I'm not sure what's currently breaking the sync calls that use async calls underneath. The facade with Task.Run and the CancellationTokenSource is just utter bullshit. It might put enough of a delay on stuff so that concurrency no longer occurs, but the actual fix would be not to make concurrent calls to the PLC instance. It's not thread-safe, just like many classes in the .NET BCL. I might change that soon-ish, but please refrain from suggesting things you whole-heartedly acknowledge you don't understand.

Task.run didnt do the job for me, worked better with backgroundworker...

Just post your code master and that will prevent people trying to solve problem... Or do whatever you want, im working fine with my solution, im continuously reading and writing to plc and no problems, I also tried to solve with timeout in cancellation token in library functions but was pointless, so do what you want, fix it, use my solution or dont do anything, I dont care