chore(Solution): merging multiple projects#32
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds comprehensive testing, benchmarking, and library infrastructure to a Modbus communication project. The changes introduce new socket communication libraries (TCP/UDP), serial port support, and extensive test coverage.
Key Changes:
- New benchmark project for performance testing across multiple Modbus libraries
- Comprehensive test projects for Socket, TcpSocket, UdpSocket, SerialPorts, and Modbus functionality
- New library implementations for TcpSocket, UdpSocket, Sockets, and SerialPorts
- Build configuration files for multi-framework support (net6.0 through net10.0)
Reviewed Changes
Copilot reviewed 158 out of 159 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Benchmarks/Benmarks.cs | Misspelled filename - should be "Benchmarks.cs" |
| test/Version.props | Defines target framework version properties for tests |
| test/Directory.Build.props | Configures test project defaults and NuGet packages |
| test/UnitTest*/**.cs | Unit test files for various modules with comprehensive coverage |
| src/Longbow.UdpSocket/** | UDP socket communication library implementation |
| src/Longbow.TcpSocket/** | TCP socket communication library implementation |
| src/Longbow.Sockets/** | Base socket utilities and data converters |
| src/Longbow.SerialPorts/** | Serial port communication library |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,141 @@ | |||
| // Copyright (c) Argo Zhang (argo@live.ca). All rights reserved. | |||
There was a problem hiding this comment.
Filename is misspelled. The file is named 'Benmarks.cs' but should be 'Benchmarks.cs' to match the class name and namespace.
| for (var index = 0; index < ClientCount; index++) | ||
| { | ||
| var client = new TouchSocket.Modbus.ModbusTcpMaster(); | ||
| await client.SetupAsync(new TouchSocket.Core.TouchSocketConfig().SetRemoteIPHost("127.0.0.1:502")); |
There was a problem hiding this comment.
Disposable 'TouchSocketConfig' is created but not disposed.
|
|
||
| for (var index = 0; index < ClientCount; index++) | ||
| { | ||
| var client = nModbusFactory.CreateMaster(new System.Net.Sockets.TcpClient("127.0.0.1", 502)); |
There was a problem hiding this comment.
Disposable 'TcpClient' is created but not disposed.
|
|
||
| // 测试正常电文 | ||
| cst.Dispose(); | ||
| cst = new(); |
There was a problem hiding this comment.
Disposable 'CancellationTokenSource' is created but not disposed.
| var type = Type.GetType("Longbow.TcpSocket.Sender, Longbow.TcpSocket"); | ||
| Assert.NotNull(type); | ||
|
|
||
| var instance = Activator.CreateInstance(type, new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp)); |
There was a problem hiding this comment.
Disposable 'Socket' is created but not disposed.
| { | ||
| public static void ThrowIfNotOpen(this SerialPort? port) | ||
| { | ||
| if (port == null || port.IsOpen == false) |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
| _serialPort.ThrowIfNotOpen(); | ||
|
|
||
| // 取消等待读取的任务 | ||
| if (_readTaskCompletionSource.Task.IsCompleted == false) |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
| _readTaskCompletionSource = new TaskCompletionSource(); | ||
|
|
||
| // 取消接收数据的任务 | ||
| if (_receiveCancellationTokenSource.IsCancellationRequested == false) |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
|
|
||
| public ValueTask CloseAsync() | ||
| { | ||
| if (_receiveCancellationTokenSource.IsCancellationRequested == false) |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
| ret = true; | ||
| } | ||
| } | ||
| catch { } |
There was a problem hiding this comment.
Generic catch clause.
Link issues
fixes #31
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge