mycroes / sally7

C# implementation of Siemens S7 connections with a focus on performance

sally7's Issues

Failed to return job ID exception on dispose

When disposing an active connection (with pending read/writes) the queues are disposed as well. This results in an exception that the job ID could not be returned to the queue. This is probably masking a more relevant exception.

Multiple connections to the same plc and handling a pool of connections

This is more of a 2 part question, the first being about Sally7:

Is it possible to have multiple connections to the same PLC with Sally7, assuming of course that some limit on maximum number of connections imposed by the PLC is not reached?
Is it possible by just opening multiple connections, or would that require adding some form of "connection number" before/while establishing the connection? (I know that the DeltaLogic AGLINK has such a concept, but am not sure if that is just internally in their library or if it is related to the S7 protocol).

The second part / the reason why I am asking is:
A single PLC connection with S7NetPlus/Sally7 still needs to be synchronized, even when used with async/await. To truly increase the bandwidth of accessing data I would either have access multiple data items at the same time, or use multiple connections to the same plc concurrently.

The first option with reading/writing multiple data items unfortunately does not fit at all in the existing software I have, and only solves scenarios where you can read multiple date items in a correlated fashion. For performing multiple independent actions on the same PLC, multiple connections would still be preferable.

So to get to the point:
My idea would be to have a pool of available, established connections. A worker grabs a connection, perform a read/write, and gives the connection back into the pool. Some other worker ensured that connections are initially established / re-established.
Alternatively, the pool could contain virtual connection handles and the normal workers are also responsible for establishing the connection itself, besides using it for read/write.

What do you think about that idea? What .NET collections and other mechanisms would you recommend for getting something like that to work?

Server side of S7 for emulation purpose

(As briefly discussed through email in the end of 2019)
Sally7 provides a great experience for the client part of the S7 protocol, but there is no server component at the moment. It means you need a real server or use another library to create an emulator if you want to test your code.

I've been using Snap7 in the mean time, but it would be nice to have a complete Sally7 experience for the S7 protocol.

Here an example adapted from the code provided by Snap7 for a basic S7 server emulator, using Snap7:

using Snap7.Managed;
using System;
using System.Collections.Generic;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace X
{
    class RobotAIServer
    {
        byte[] _database1 = new byte[260];  // Our DB1

        public event EventHandler ReceivedData;

        void eventCallback(IntPtr usrPtr, ref S7Server.USrvEvent Event, int Size)
        {
            Logger.Instance.Log("Event Callback");
            if (Event.EvtCode.Equals(S7Server.evcDataRead))
            {
                Logger.Instance.Log("Sent data to client");
            }
            if (Event.EvtCode.Equals(S7Server.evcDataWrite))
            {
                Logger.Instance.Log("Received data from client.");
                ReceivedData?.Invoke(this, EventArgs.Empty);
                hexDump(_database1, _database1.Length);
            }
        }

        void readEventCallback(IntPtr usrPtr, ref S7Server.USrvEvent Event, int Size)
            => Logger.Instance.Log("Read Event Callback");

        static void hexDump(byte[] bytes, int Size)
        {
            if (bytes == null)
            {
                return;
            }

            int bytesLength = Size;
            const int BYTES_PER_LINE = 16;

            char[] HexChars = "0123456789ABCDEF".ToCharArray();

            const int FIRST_HEX_COLUMN =
                  8                   // 8 characters for the address
                + 3;                  // 3 spaces

            const int FIRST_CHAR_COLUMN = FIRST_HEX_COLUMN
                + (BYTES_PER_LINE * 3)       // - 2 digit for the hexadecimal value and 1 space
                + ((BYTES_PER_LINE - 1) / 8) // - 1 extra space every 8 characters from the 9th
                + 2;                  // 2 spaces

            int lineLength = FIRST_CHAR_COLUMN
                + BYTES_PER_LINE           // - characters to show the ascii value
                + Environment.NewLine.Length; // Carriage return and line feed (should normally be 2)

            char[] line = ((new string(' ', lineLength - 2)) + Environment.NewLine).ToCharArray();
            int expectedLines = ((bytesLength + BYTES_PER_LINE) - 1) / BYTES_PER_LINE;
            var result = new StringBuilder(expectedLines * lineLength);

            for (int i = 0; i < bytesLength; i += BYTES_PER_LINE)
            {
                line[0] = HexChars[(i >> 28) & 0xF];
                line[1] = HexChars[(i >> 24) & 0xF];
                line[2] = HexChars[(i >> 20) & 0xF];
                line[3] = HexChars[(i >> 16) & 0xF];
                line[4] = HexChars[(i >> 12) & 0xF];
                line[5] = HexChars[(i >> 8) & 0xF];
                line[6] = HexChars[(i >> 4) & 0xF];
                line[7] = HexChars[(i >> 0) & 0xF];

                int hexColumn = FIRST_HEX_COLUMN;
                int charColumn = FIRST_CHAR_COLUMN;

                for (int j = 0; j < BYTES_PER_LINE; j++)
                {
                    if ((j > 0) && ((j & 7) == 0))
                    {
                        hexColumn++;
                    }

                    if (i + j >= bytesLength)
                    {
                        line[hexColumn] = ' ';
                        line[hexColumn + 1] = ' ';
                        line[charColumn] = ' ';
                    }
                    else
                    {
                        byte b = bytes[i + j];
                        line[hexColumn] = HexChars[(b >> 4) & 0xF];
                        line[hexColumn + 1] = HexChars[b & 0xF];
                        line[charColumn] = (b < 32) ? '·' : ((char)b);
                    }
                    hexColumn += 3;
                    charColumn++;
                }
                result.Append(line);
            }
            Logger.Instance.Log(result.ToString());
        }

        public async Task RunAsync(CancellationToken token)
        {
            var serverEvent = new S7Server.USrvEvent();
            var server = new S7Server();
            server.RegisterArea(S7Server.srvAreaDB,  // We are registering a DB
                                1,                   // Its number is 1 (DB1)
                                ref _database1,                 // Our buffer for DB1
                                _database1.Length);         // Its size

            var eventCallBack = new S7Server.TSrvCallback(eventCallback);
            var readCallBack = new S7Server.TSrvCallback(readEventCallback);
            server.SetEventsCallBack(eventCallBack, IntPtr.Zero);
            server.SetReadEventsCallBack(readCallBack, IntPtr.Zero);

            server.EventMask &= 1; // Mask 00000000
            server.EventMask |= S7Server.evcDataRead;
            server.EventMask |= S7Server.evcDataWrite;

            int Error = server.Start();
            if (Error == 0)
            {
                do
                {
                    while (server.PickEvent(ref serverEvent))
                    {
                        Logger.Instance.Log(server.EventText(ref serverEvent));
                        // TODO : EventText would throw on second command - push that in a separate thread ?
                    }
                    await Task.Delay(10).ConfigureAwait(true);
                }
                while (!token.IsCancellationRequested);
                server.Stop();
            }
        }

        public void WriteToDatabase(int index, byte value) => _database1[index] = value;

        public byte ReadFromDatabase(int index) => _database1[index];

        public string ReadStringFromDatabase(int index, int stringLength)
        {
            var str = new List<char>();
            for (int i = 0; i < stringLength; i++)
            {
                str.Add((char)ReadFromDatabase(index + i));
            }

            return new string(str.ToArray());
        }
    }
}

Extensions for reading/writing plain old byte arrays

For my Sally7 wrapper, I added some extension methods to just read/write a byte array as a single data item per request.

I also plan to add some magic to automatically split the byte array into smaller chunks that fit into the maximum length of a single request (PDU related), so that I can seamlessly support very large DBs in our product.

@mycroes Do you think Sally7 would benefit by having such extensions available, or do you think this is a slippery slope adding things too far away from the core competencies of Sally7? Similar to all the barely-maintained stuff in S7NetPlus which is not just reading/writing data items :-)

Stuck requests when receiveSignal is released after Dispose()

Consider following stack trace:

Sally7.Sally7Exception: Couldn't signal receive done.
   at Sally7.Sally7Exception.ThrowFailedToSignalReceiveDone()
   at Sally7.RequestExecutor.ConcurrentRequestExecutor.PerformRequest(ReadOnlyMemory`1 request, Memory`1 response, CancellationToken cancellationToken)
   at Sally7.S7Connection.ReadAsync(IDataItem[] dataItems, CancellationToken cancellationToken)
   at VLC.PLC.Sally7.Sally7PlcConnection.ReadIntermediateAsync(IEnumerable`1 readContainers)
   at VLC.PLC.PlcConnection.ReadAsync(IEnumerable`1 readContainers, Boolean triggerChanges)
   at VLC.PLC.Polling.PlcPoller.TimerCallback(Object state)

The following must have happened:

1. Request has entered PerformRequest
2. Request was sent
3. Response was received
4. ConcurrentRequestExecutor is disposed
5. receiveSignal.TryRelease() returns false
6. Sally7Exception.ThrowFailedToSignalReceiveDone() is invoked

If there was only one request, this results in the caller receiving the exception from point 6.

Now assume there was already a caller waiting:

1. Request A was sent
2. Request B was sent
3. Caller A receives response B
4. rec.Complete is called for Request B
5. Caller A awaits Request A (no cancellation)
6. Caller B receives response A
7. Caller B exceptions due to receiveSignal.Dispose()
8. Caller B receives the exception
9. Request A is never completed
10. Caller A is stuck

Possible solution:

1. On JobPool.Dispose()
2. Dispose() all Requests
   a. Request.Dispose() { _disposed = true; _continuation.Invoke(); }
   b. Request.GetResult() { ObjectDisposedException.ThrowIf(_disposed, this); return ... }
3. Add ObjectDisposedException.ThrowIf to ConcurrentRequestExecutor throw paths?

Disadvantages?

Guard against truncation of dataItems.Length

In

Sally7/Sally7/S7ConnectionHelpers.cs

Line 48 in d7e5b45

read.ItemCount = (byte) dataItems.Count;

and

Sally7/Sally7/S7ConnectionHelpers.cs

Line 63 in d7e5b45

span[1] = (byte) dataItems.Count;

then int-count is casted (= truncated) to byte. This may lead to unexpected results for the user, iif there are more than 255 dataItems.

I'd expect that a reasonable exception will be thrown, that indicates the cause of the failure.
"Fail early" is better than letting a user believe to be able to read more than 255 dataItems.

Endianness of the data payload

This is more of a question than an issue regarding endianness of the data. I've checked both pages you link in your readme file for hints about that and didn't find anything regarding data. Both doc are more concerned about the headers, like if the data was left to the implementers to decide about the endianness.

My use case is 3 data block all holding a short, I implemented two ways to read them:

                var activeFaultsCalibration = new DataBlockDataItem<short> { DbNumber = 1, StartByte = 200 };

                var activeFaultsLearning = new DataBlockDataItem<short> { DbNumber = 1, StartByte = 202 };

                var activeFaultsSorting = new DataBlockDataItem<short> { DbNumber = 1, StartByte = 204 };

                var activeFaultsCalibrationArray = new DataBlockDataItem<byte[]> { DbNumber = 1, StartByte = 200,Length = 2 };

                var activeFaultsLearningArray = new DataBlockDataItem<byte[]> { DbNumber = 1, StartByte = 202, Length = 2 };

                var activeFaultsSortingArray = new DataBlockDataItem<byte[]> { DbNumber = 1, StartByte = 204, Length = 2 };

This is the data I receive from the server:

This is the code that treat the values:

                Console.WriteLine($"Active faults calibration: {activeFaultsCalibration.Value}");

                Console.WriteLine($"Active faults learning: {activeFaultsLearning.Value}");

                Console.WriteLine($"Active faults sorting: {activeFaultsSorting.Value}");

                short value = BitConverter.ToInt16(activeFaultsCalibrationArray.Value, 0);

                Console.WriteLine($"Active faults calibration Array: {value}");

                value = BitConverter.ToInt16(activeFaultsLearningArray.Value, 0);

                Console.WriteLine($"Active faults learning Array: {value}");

                value = BitConverter.ToInt16(activeFaultsSortingArray.Value, 0);

                Console.WriteLine($"Active faults sorting Array: {value}");

and this is what get displayed:

The data is little endian, but the library is opiniated as big-endian for the data. I control the code of the server, so it's not an issue to do the conversion either on the server or the client.

I was wondering if there was something in the specs that defined this behavior? Or is it something that should be optional somehow if it's left to interpretation?

It's really not a big deal though, as your library exposes enough way to circumvent this.

The outro of the linked articles gives a hint that it might just be a weakness of the protocol:

This might be obvious now, but the S7 protocol is not a well designed one. It was originally created to simply query register values, which it did kind of all right, but then functionality was kept being added until it became this monstrosity. It is filled with inconsistencies and unnecessary redundancies and it only gets worse with Userdata messages. These irregularities and design flaws become way more obvious (and annoying) while trying to write a parser for the protocol.

But I wanted your opinion on this, as your library seems to be opiniated on this particular subject.

Use generic math for value conversion

Cf. #49 (comment)

Small cast error in ConvertFromLong

I tested you great software and i got errors with writing all long data types. I was looking and i found mistake in ConvertFromLong() in ToS7Conversions.cs line 64 ConvertFromInt((int) value >> 32, 1, output);. Now the cast is done before the shift but it should be fist shift and than cast: ConvertFromInt((int) (value >> 32), 1, output); I came after this by looking at the hexadecimal values in the PLC.

IsConnected

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?

Connection status PLC

The PLC.TcpClient.Connected does not return a valid value for whether the PLC is currently connected. The value returns true before data could be actually read. Is there a way to actually check the connection status that the PLC is ready for communication? Currently I am simply repeating the read until I do not receive an exception.

Verify requests are completed on dispose

The ConcurrentRequestExecutor permits multiple active requests. When the connection is closed and multiple requests are active they might not be completed and get stuck indefinitely.

Do you have any Documentation?

how to use it

ConcurrentRequest with too many large data items

If I use the new concurrent request things with multiple large data items (1000 byte each), I get the following exception:

System.Exception: 'An error was returned during communication:

Message type: Ack / 0x02
Error class: ErrorOnSupplies / 0x85
Error code: 0x0

Expose protocol overhead size for requests

I want to split my large data reading/writing requests into smaller chunks, similar to https://github.com/S7NetPlus/s7netplus/blob/051091919fa80b03989f4375e364ebc4b02aabe7/S7.Net/PlcAsynchronous.cs#L106
For that I need to know at what size to split my data into chunks.

I can get at the PDU from the IS7ConnectionParameters, that's great.

What I am missing are those magic numbers of protocol overhead for a request & per data item. I don't want to hardcode these, and since you have such well-organized data types for all your protocol code, would it be possible to expose the size of those structs somewhere, eg. in a static class?

[Question] Is there any standard way to query a PLC for its current program version or checksum?

(This is not an issue for Sally per se, but a question related to Sally7 devs and users field of work)

I would like to track if any changes or incompatibilities could happen between the programs I develop in C# using Sally, and the PLCs I communicate with.

Is there any standard way to query a PLC for its current program version or checksum?

I searched existing discussions and documentation without finding anything useful so far:

https://support.industry.siemens.com/forum/ca/en/posts/read-plc-program-checksum-over-tcp-ip/251950
https://support.industry.siemens.com/cs/mdm/109747174?c=96887724043&dl=nl&lc=fr-WW
https://stackoverflow.com/q/44668697/444469

Is the function GetChecksum good enough for this usage? How would I call this function with Sally?

Reconsider string handling

Strings are currently read and written inclyding both header bytes. The first byte however indicates the reserved area, as such it should technically never be written at all.

Renaming the async methods with Async suffixes

Considering the convention documented in the MSDN , it would feel more natural to have the async methods defined in the library renamed with an Async suffix:

        public Task Open();
        public Task Read(params IDataItem[] dataItems);
        public Task Write(params IDataItem[] dataItems);
        private async Task<int> ReadTpkt() /* would be not breaking */

to

        public Task OpenAsync();
        public Task ReadAsync(params IDataItem[] dataItems);
        public Task WriteAsync(params IDataItem[] dataItems);
        private async Task<int> ReadTpktAsync() /* would be not breaking */

It's a breaking change, so before submitting a PR, I wanted to know if it was something you had interest in, and the best course of action if that was the case.

since the library is still pre 1.0.0, breaking changes would be acceptable regarding semver, but there is always the possibility to keep the old signature for 1 version with an obsolete attribute too.

DateTime handling

I'm currently working on a project in which we need to read DTL (DateTimeLong) from the PLC. For now, we've created a small wrapper around Sally7. Would you like me to create a pull request in which I add the conversion from DateTime to DateTimeLong?

XML-comments

Do think you can include the comments in the next release?
Best regards!

Reading datatype string

I can read the datatype string with the following line:
DataBlockDataItem<byte[]> dataItemString = new DataBlockDataItem<byte[]> { DbNumber = 2, StartByte = 40, Length = 200 };
And converting it to a string with the next line:
Encoding.Default.GetString(dataItemString.Value)

Although I must say I expected to be able to read it like this:
DataBlockDataItem<string> dataItemStringString = new DataBlockDataItem<string> { DbNumber = 2, StartByte = 40 };
Which throws the following exception:
System.AggregateException HResult=0x80131500 Message=One or more errors occurred. (Read of dataItem Sally7.DataBlockDataItem1[System.String] returned DataTypeNotSupported) Source=Sally7 StackTrace: at Sally7.S7ConnectionHelpers.ParseReadResponse(ReadOnlySpan1 buffer, ReadOnlySpan1 dataItems) at Sally7.S7Connection.<ReadAsync>d__31.MoveNext() at Sandbox.Program.<Read>d__1.MoveNext() in D:\Projects\Sandbox\Sandbox\Program.cs:line 43

Inner Exception 1: Exception: Read of dataItem Sally7.DataBlockDataItem1[System.String] returned DataTypeNotSupported

This aside, reading the bytes and converting this to a string works.

My real problem starts when trying to read the string:
dataItemString.Value = Encoding.Default.GetBytes("Test");
Which writes the converted bytes to the DB:
X#(54 65 73 74 72 64 69 6E 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)
I can read them again using the working read method for the string, but I want the string to be decoded in the DB.

Writing through the string datatype for the DataBlockDataItem results in an empty string in the DB ('').
DataBlockDataItem<string> dataItemStringString = new DataBlockDataItem<string> { DbNumber = 2, StartByte = 40 }; dataItemStringString.Value = "Test";

How can I write the datatype string?

Q: Comparsion to S7netPlus

Hey,

we are using the S7NetPlus library in our project. Since you are one of the contributors to the S7NetPlus library and owner of Sally, there is no better person to ask.

• How do these two libraries compare to each other
• In which scenarios should I decide to use one over another

Thanks in advice
Best regards Miro