Only connect TcpClient if not already connected

[schotime]

It appears we missed this one, but Connect needs to be called otherwise _frameBuffer and _networkStream etc won't get set.

[Apollo3zehn]

Thanks, I'll merge it on Monday :-)

[Apollo3zehn]

I am not sure if this is the right approach. The constructor accepts a TcpClient and an IPEndpoint:

FluentModbus/src/FluentModbus/Client/ModbusTcpClient.cs

Line 153 in 2616f90

public void Connect(TcpClient tcpClient, IPEndPoint remoteEndpoint, ModbusEndianness endianness)

So this constructor assumes that you are not yet connected and will connect on your behalf.

If you want to fully manage the TcpClient yourself, we would need another constructor overload which only accepts TcpClient (and ModbusEndianness).

What do you think?

Note: When we make new changes, I would also like to add IDisposable support ( #67).

[schotime]

I agree with using the constructor. We should also do this for SerialPort. It did feel a bit weird passing it to the connect method.
Also agree with IDisposable.
Don't know if you wanted to make the Close/Disconnect methods the same name so it can't be invoked from the ModbusClient generically also.

[Apollo3zehn]

I have changed the constructor signatures in v5 (will be released today). Thanks anyway for hinting me into the right direction.

[schotime]

Just had a look, and looks ok to me other than, ideally the IDisposable would be on the base class ModbusClient so that it doesn't have to be cast again to dispose.

Also you might have to have an exception thrown somewhere if you try and reuse it after calling dispose as you don't disconnect/close each time dispose is called.

[Apollo3zehn]

I thought about putting it into the base class but the base class itself does not need to be disposed so from an idealistic perspective it makes more sense to put it into the implementing classes. However maybe it is different from a practical point of view. I am not sure how a use case looks like where one works with the base class ModbusClient. Is it common to have a List<ModbusClient> where both, ModbusTcpClient and ModbusRtuClient are being part of at the same time? If that is a real use case, I agree that it would be more useful in the base class.

I am not sure I fully understand your second point. I know I should throw ObjectDisposedException when trying to reuse the instance after disposing but I do not really see the point doing so because when I dispose my instance, it is to be thrown away. I am hesitant because it clutters the code and the benefit is small I think.

[schotime]

1. I use the library to provide a generic modbus interface where the user can choose whether they're using TCP or Serial, and so its used generically after they're instantiated, hence why I also raised the Close/Disconnect naming disparity (which could be on the base class also).

2. I agree with that statement, but you might as well just call Close/Disconnect everytime then, rather than checking whether its disposed or not. All I was really thinking was either a) mark as disposed and throw if reused, or b) always call closed on dispose.

[Apollo3zehn]

Thanks for the explanations, I`ll come back to you next week.