Add Modbus RTU/ASCII OVER TCP/UDP #149
Conversation
There was a problem hiding this comment.
Thanks for your PR! I've asked a few questions and it looks like there is at least one item that needs fixing before we can merge.
Note that I'm travelling at the moment, but will pull down your full PR into my local environment so I can better understand your changes and intent and may have more questions and comments at that point.
| <ItemGroup> | ||
| <ProjectReference Include="..\NModbus\NModbus.csproj" /> |
There was a problem hiding this comment.
What is the thought for removing this reference?
There was a problem hiding this comment.
This was a bad mistake, so I should have done it
| Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "NModbus.IntegrationTests", "NModbus.IntegrationTests\NModbus.IntegrationTests.csproj", "{1AA055F2-68E1-409F-A744-6081D7A27A57}" | ||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "NModbus.IntegrationTests", "NModbus.IntegrationTests\NModbus.IntegrationTests.csproj", "{1AA055F2-68E1-409F-A744-6081D7A27A57}" | ||
| EndProject | ||
| Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "ConsoleApp1", "H:\Users\Administrator\Desktop\MLToolkit\ConsoleApp1\ConsoleApp1.csproj", "{5F0BB26E-AB88-4D59-834D-5082653BF24B}" |
There was a problem hiding this comment.
This looks like a temporary change that probably shouldn't be here as it points to a project in your local environment.
There was a problem hiding this comment.
Yes, you are right, in fact I used “ConsoleApp1” for testing but I forgot to delete it
| public ModbusMasterRtuOverTcpConnection(TcpClient client, IModbusSlaveNetwork slaveNetwork, IModbusFactory modbusFactory, IModbusLogger logger) | ||
| : base(client,slaveNetwork,modbusFactory,logger, new ModbusRtuTransport(new TcpClientAdapter(client), modbusFactory, logger)) | ||
| { | ||
| //TODO |
There was a problem hiding this comment.
What is there left to do here?
There was a problem hiding this comment.
Sorry, this is some habit of mine, in fact nothing needs to be done here
| private readonly IModbusSlaveNetwork _slaveNetwork; | ||
| private readonly IModbusFactory _modbusFactory; | ||
| private readonly Task _requestHandlerTask; | ||
| protected internal TcpClient _client; |
There was a problem hiding this comment.
I'll need to pull this PR down to my local environment to follow exactly what's happening here, but what is your thought in making all of these protected? Also, did something change where we can't keep the readonly modifier?
There was a problem hiding this comment.
You are right, at first I was hoping to change it to "protected readonly" so that subclasses could inherit, but apparently I forgot to add "readonly"，
| @@ -38,6 +38,8 @@ public int WriteTimeout | |||
| set { _serialPort.WriteTimeout = value; } | |||
| } | |||
|
|
|||
| public string Name => _serialPort?.PortName; | |||
There was a problem hiding this comment.
What problem does exposing the port name solve?
There was a problem hiding this comment.
As you can see, in fact this is for use in Logger.Information, but they are not important
which will lead to some changes. For example, this will affect all UdpMasters in ModbusFactory.cs They must specify this Endpoint during initialization, but the slave does not need to. This may Not very good. The reason I added it is because when Udp serves as the master station, it needs to actively send requests, so it needs to set the Endpoint of the master station. When Udp serves as the slave station, it listens for requests. Receive automatically obtains the client Endpoint when sending. What are your thoughts on using this Endpoint?
|
No description provided.