feat(ModbusCrc16): add modbus crc-16 algorithm#560
Merged
Conversation
Reviewer's GuideThis PR adds a table-driven Modbus CRC-16 utility with performance optimizations, renames and harmonizes the ReceivedCallback member throughout the TCP client API, strengthens error logging and activator extension resilience, extends the HexConverter to support ReadOnlySpan, and covers all new behaviors with focused unit tests. Class diagram for the new ModbusCrc16 utilityclassDiagram
class ModbusCrc16 {
+ushort Compute(ReadOnlySpan<byte> data)
+ReadOnlySpan<byte> Append(ReadOnlySpan<byte> data)
+bool Validate(ReadOnlySpan<byte> dataWithCrc)
-ushort[] CrcTable
}
Class diagram for ITcpSocketClient and DefaultTcpSocketClient callback renamingclassDiagram
class ITcpSocketClient {
+Func<ReadOnlyMemory<byte>, ValueTask>? ReceivedCallback
}
class DefaultTcpSocketClient {
+Func<ReadOnlyMemory<byte>, ValueTask>? ReceivedCallback
}
ITcpSocketClient <|.. DefaultTcpSocketClient
Class diagram for updated HexConverterclassDiagram
class HexConverter {
+string ToString(byte[]? bytes, string? separator = "-", bool upper = true)
+string ToString(ReadOnlySpan<byte> span, string? separator = "-", bool upper = true)
}
Class diagram for updated SocketLoggingclassDiagram
class SocketLogging {
+void Init(ILogger logger)
+void LogError(string message)
+void LogError(Exception ex, string? message = null)
-ILogger _logger
}
Class diagram for updated ActivatorExtensionsclassDiagram
class ActivatorExtensions {
+object? CreateInstance(this Type type, object?[]? args = null)
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a Modbus CRC-16 algorithm implementation to the BootstrapBlazor Socket library. The primary purpose is to provide CRC computation, validation, and data appending functionality for Modbus protocol communication.
Key changes include:
- Implementation of ModbusCrc16 utility class with lookup table-based CRC calculation
- Renaming of ReceivedCallBack to ReceivedCallback for consistency
- Enhanced HexConverter with ReadOnlySpan support and case-sensitive formatting
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/extensions/BootstrapBlazor.Socket/Utility/ModbusCrc16.cs | New ModbusCrc16 class implementing CRC-16 computation, validation, and data appending |
| src/extensions/BootstrapBlazor.Socket/DataConverter/HexConverter.cs | Added ReadOnlySpan overload for hex string conversion |
| src/extensions/BootstrapBlazor.Socket/Logging/SocketLogging.cs | Simplified LogError method with better parameter handling |
| ITcpSocketClient.cs and related files | Renamed ReceivedCallBack to ReceivedCallback for proper spelling |
| test/UnitTestTcpSocket/ModbusCrcTest.cs | Comprehensive unit tests for ModbusCrc16 functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Renaming ReceivedCallBack to ReceivedCallback is a breaking change in the public ITcpSocketClient API—please ensure all consumers are updated (and consider a major version bump).
- ModbusCrc16.Append still converts the stackalloc span to a heap array via ToArray—consider simplifying the API to directly return a byte[] to avoid extra allocations and make ownership clear.
- ActivatorExtensions.CreateInstance swallows all exceptions and returns null—consider providing a TryCreateInstance pattern or surfacing the underlying error to aid debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Renaming ReceivedCallBack to ReceivedCallback is a breaking change in the public ITcpSocketClient API—please ensure all consumers are updated (and consider a major version bump).
- ModbusCrc16.Append still converts the stackalloc span to a heap array via ToArray—consider simplifying the API to directly return a byte[] to avoid extra allocations and make ownership clear.
- ActivatorExtensions.CreateInstance swallows all exceptions and returns null—consider providing a TryCreateInstance pattern or surfacing the underlying error to aid debugging.
## Individual Comments
### Comment 1
<location> `src/extensions/BootstrapBlazor.Socket/Utility/ModbusCrc16.cs:69` </location>
<code_context>
+ /// </summary>
+ /// <param name="data">原始数据</param>
+ /// <returns>带 CRC 校验码的数据</returns>
+ public static ReadOnlySpan<byte> Append(ReadOnlySpan<byte> data)
+ {
+ ushort crc = Compute(data);
+
+ // 使用 stackalloc 避免堆分配（小数据时）
+ if (data.Length <= 256)
+ {
+ Span<byte> result = stackalloc byte[data.Length + 2];
+ data.CopyTo(result);
+ result[data.Length] = (byte)(crc & 0xFF);
+ result[data.Length + 1] = (byte)(crc >> 8);
+ return result.ToArray();
+ }
+ else
</code_context>
<issue_to_address>
Returning ToArray() from a stackalloc span defeats the purpose of stack allocation.
Returning result.ToArray() causes a heap allocation, which undermines the use of stackalloc. Consider returning the Span directly or consistently using arrays to avoid confusion.
</issue_to_address>
### Comment 2
<location> `src/extensions/BootstrapBlazor.Socket/Utility/ModbusCrc16.cs:105` </location>
<code_context>
+ return false;
+ }
+
+ ushort receivedCrc = (ushort)(dataWithCrc[^1] << 8 | dataWithCrc[^2]);
+ ushort calculatedCrc = Compute(dataWithCrc[..^2]);
+ return receivedCrc == calculatedCrc;
</code_context>
<issue_to_address>
Byte order for CRC extraction may be confusing.
Consider documenting the expected byte order or using BitConverter to make the endianness explicit and improve maintainability.
</issue_to_address>
### Comment 3
<location> `src/extensions/BootstrapBlazor.Socket/Utility/ModbusCrc16.cs:74` </location>
<code_context>
+ ushort crc = Compute(data);
+
+ // 使用 stackalloc 避免堆分配（小数据时）
+ if (data.Length <= 256)
+ {
+ Span<byte> result = stackalloc byte[data.Length + 2];
</code_context>
<issue_to_address>
Consider removing the stackalloc branch in Append and always use a single byte array allocation for simplicity.
Consider dropping the `stackalloc` + `ToArray()` branch entirely – it never actually saves you from a heap alloc, just adds extra code paths. A single `new byte[]` keeps everything straightforward:
```csharp
public static byte[] Append(ReadOnlySpan<byte> data)
{
ushort crc = Compute(data);
int len = data.Length;
var result = new byte[len + 2];
data.CopyTo(result);
result[len] = (byte)(crc & 0xFF);
result[len + 1] = (byte)(crc >> 8);
return result;
}
```
If you really want to keep the `ReadOnlySpan<byte>` signature, just return the array directly:
```csharp
public static ReadOnlySpan<byte> Append(ReadOnlySpan<byte> data)
{
ushort crc = Compute(data);
int len = data.Length;
var result = new byte[len + 2];
data.CopyTo(result);
result[len] = (byte)(crc & 0xFF);
result[len + 1] = (byte)(crc >> 8);
return result;
}
```
Both remove the extra branch, simplify the implementation, and preserve identical behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #559
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add Modbus CRC-16 algorithm support and related tests; standardize callback naming in TCP socket client interfaces; enhance logging, activator exception handling, and hex conversion utilities.
New Features:
Enhancements:
Tests: