Apollo3zehn / FluentModbus

Lightweight and fast client and server implementation of the Modbus protocol (TCP/RTU).
MIT License
189 stars 71 forks source link

Relax protection modifiers to enable subclassing #37

Closed robeving closed 3 years ago

robeving commented 3 years ago

This PR reduces restriction on a number of classes to enable subclassing of the server component. A very simple TCP simple server can now be created with syntax like this:

    internal class InternalModbusServer : ModbusServer
    {
        private readonly ModbusTcpRequestHandler request;

        public InternalModbusServer(TcpClient client) : base(true)
        {
            request = new ModbusTcpRequestHandler(client, this);
        }

        protected override void ProcessRequests()
        {
            if (request.IsReady)
            {
                if (request.Length > 0)
                    request.WriteResponse();

                _ = request.ReceiveRequestAsync();
            }
        }
    }

This is useful for callers who have their own TcpServer implementation

robeving commented 3 years ago

https://github.com/Apollo3zehn/FluentModbus/issues/36

Apollo3zehn commented 3 years ago

Thanks for the PR. I have a few questions to understand your use case.

I ask this because with the new public access modifiers I need to understand the impact since they would now be part of the public API and need to be documented and supported properly. In any case, unprotecting the ProcessRequests method is good idea.

Thanks :-)

robeving commented 3 years ago

In my case I have a single TcpClient and I don't want any other connections handled by the 'InternalModbusServer'. I want to managed pretty much everything surrounding the transport layer and only want the modbus library to handle datagrams.

By inheriting from ModbusServer I can have a very small implementation which has no understanding of the transport layer BUT using some of the helper code provided by ModbusTcpRequestHandler really helps with the heavy lifting with a TcpClient. Implementing something without the ModbusTcpRequestHandler class would in reality mean a lot of copy and pasted code, hence why I changed it's protection as well.

Apollo3zehn commented 3 years ago

Sorry I needed several attempts to go through this pull request and think about its implications.

During that I came up with a different approach:

/// <summary>
/// Starts the server. It will use only the provided <see cref="TcpClient"/>.
/// </summary>
/// <param name="tcpClient">The TCP client to communicate with.</param>
public void Start(TcpClient tcpClient)
{
    // "base..." is important!
    base.Stop();
    base.Start();

    this.RequestHandlers = new List<ModbusTcpRequestHandler>()
    {
        new ModbusTcpRequestHandler(tcpClient, this)
    };
}

Would that work in your case, too? I.e. just pass your tcp client into a constructor that accepts just that client?

robeving commented 3 years ago

Yes I think that would work

Apollo3zehn commented 3 years ago

Okay, I have just published a new version. I am sorry that I could not merge your PR but I think it is better to not expose all the internal types. Anyway, making ProcessRequests available for subclassing is still a good idea so I have included that change. When there will be other uses cases that require ModbusTcpHandler and related types to be exposed, I will come back to this PR.