Conversation
…-PlcAdsConnectionService fix: fix potential deadlock in PlcAdsConnectionService
Member
Author
|
@copilot review the code |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces support for writing structs with CommandInputBuilder and PrimitiveCommandArgumentHandler, fixes a double-write bug, adds auto-reconnect functionality to PlcAdsConnectionService, improves PlcAdsStateReader robustness, upgrades dependencies, and migrates all test projects from FluentAssertions to AwesomeAssertions.
Key Changes:
- Added recursive struct writing support in
PrimitiveCommandArgumentHandlerfor PLC commands - Fixed double write issue in
WriteInputDatamethod - Implemented auto-reconnect feature with configurable timeout in
PlcAdsConnectionService - Enhanced
PlcAdsStateReader.StopSampling()with safer notification cleanup usingTryDeleteDeviceNotification - Upgraded Beckhoff.TwinCAT.Ads from 6.1.332 to 6.2.521 and bumped package versions to 5.1.0.0
- Migrated all test projects to AwesomeAssertions 9.* and updated assertion syntax
- Updated auto-generated resource files to English and newer tool versions
Reviewed changes
Copilot reviewed 47 out of 49 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| NET/Mbc.Pcs.Net.Command/PrimitiveCommandArgumentHandler.cs | Added struct support to WriteInputData, removed duplicate write call, enabled recursive struct handling |
| NET/Mbc.Pcs.Net/State/PlcAdsStateReader.cs | Enhanced StopSampling() with safer notification cleanup and handle validation |
| NET/Mbc.Pcs.Net/Connection/PlcAdsConnectionService.cs | Implemented auto-reconnect feature with cancellation token and configurable retry interval |
| NET/Mbc.Pcs.Net/Connection/PlcAdsConnectionProvider.cs | Updated to use ILoggerFactory instead of ILogger for proper logger instantiation |
| NET/Mbc.Pcs.Net/Connection/ConnectionResources.Designer.cs | Auto-generated file updated from German to English comments |
| NET/Mbc.Pcs.Net.Command/CommandResources.Designer.cs | Auto-generated file updated to version 17.0.0.0 |
| NET/Mbc.Pcs.Net/Mbc.Pcs.Net.csproj | Version bumped to 5.1.0.0 |
| NET/Mbc.Pcs.Net.Alarm/Mbc.Pcs.Net.Alarm.csproj | Version bumped to 5.1.0.0, Beckhoff.TwinCAT.Ads upgraded to 6.2.521 |
| NET/Mbc.Ads.Utils/Mbc.Ads.Utils.csproj | Version bumped to 5.1.0.0, Beckhoff.TwinCAT.Ads upgraded to 6.2.521 |
| NET/Mbc.Pcs.Net.Test/Mbc.Pcs.Net.Test.csproj | Replaced FluentAssertions with AwesomeAssertions 9.* |
| NET/Mbc.Pcs.Net.DataRecorder.Test/Mbc.Pcs.Net.DataRecorder.Test.csproj | Replaced FluentAssertions with AwesomeAssertions 9.* |
| NET/Mbc.Pcs.Net.Command.Test/Mbc.Pcs.Net.Command.Test.csproj | Replaced FluentAssertions with AwesomeAssertions 9.* |
| NET/Mbc.Pcs.Net.Alarm.Test/Mbc.Pcs.Net.Alarm.Test.csproj | Replaced FluentAssertions with AwesomeAssertions 9.* |
| NET/Mbc.Pcs.Net.Alarm.Service.Test/Mbc.Pcs.Net.Alarm.Service.Test.csproj | Replaced FluentAssertions with AwesomeAssertions 9.* |
| NET/Mbc.Ads.Utils.Test/Mbc.Ads.Utils.Test.csproj | Replaced FluentAssertions with AwesomeAssertions 9.* |
| NET/Mbc.Ads.Mapper.Test/Mbc.Ads.Mapper.Test.csproj | Replaced FluentAssertions with AwesomeAssertions 9.* |
| NET/Mbc.Pcs.Net.Command.Test/PlcCommandAsyncTests.cs | Updated assertion method from BeLessOrEqualTo to BeLessThanOrEqualTo for AwesomeAssertions compatibility |
| NET/Mbc.Ads.Mapper.Test/AdsMapperTestFakePlcData.cs | Updated StringMarshaler.UTF16 to StringMarshaler.UTF16.Encoding for clarity |
| NET/Changelog.md | Added version 5.0.1 entry documenting struct support and double-write bugfix |
| Various test files | Updated imports from FluentAssertions to AwesomeAssertions |
Files not reviewed (2)
- NET/Mbc.Pcs.Net.Command/CommandResources.Designer.cs: Language not supported
- NET/Mbc.Pcs.Net/Connection/ConnectionResources.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
NET/Mbc.Ads.Mapper.Test/AdsMapperTestFakePlcData.cs:261
- 'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
'MockMemberCollection' exposes the internal representation stored in field '_members'. The value may be modified through the variable fakeMembers.
public MockMemberCollection(List<IMember> members)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private bool _connected; | ||
| private IAdsConnection _connection; | ||
| private bool _autoReconnectEnabled; | ||
| private ILogger _logger = null; |
There was a problem hiding this comment.
[nitpick] The field _logger is initialized to null by default on line 24, but this explicit initialization is unnecessary in C# as reference types are null by default. Consider removing the = null for cleaner code.
| private readonly PlcAdsConnectionProvider _plcConnection; | ||
| private bool _connected; | ||
| private IAdsConnection _connection; | ||
| private bool _autoReconnectEnabled; |
There was a problem hiding this comment.
Field '_autoReconnectEnabled' can be 'readonly'.
- Removed Support for .NET Framework 4.7.1 (But still .NET Standard 2.0.) - Removed Dependency to Mbc.Hdf5Utils Nuget package. The code is now part of Mbc.Pcs.Net.DataRecorder.
Support for .NET 10.0.
Update cake build to version 6.2 now with dotnet tool .\build.ps1 -Target Build .\build.ps1 -Target Test # x64 .\build.ps1 -Target Test --x86=true # x64 & x86 .\build.ps1 -Target NugetPublish --apikey=<key>
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.
This pull request introduces a new feature for writing structs with
CommandInputBuilderandPrimitiveCommandArgumentHandler, fixes a bug with double writes, and upgrades several dependencies. Additionally, all test projects now useAwesomeAssertionsinstead ofFluentAssertions, and the Beckhoff TwinCAT.Ads package is updated across projects. Below are the most important changes grouped by theme.Features and Bugfixes
CommandInputBuilderandPrimitiveCommandArgumentHandler. Struct fields must be flagged with{attribute 'PlcCommandInput'}and be of typeCommandInputBuilderin C#.PrimitiveCommandArgumentHandlerwould perform a double write inWriteInputData.Dependency and Version Upgrades
Beckhoff.TwinCAT.Adspackage from version6.1.332to6.2.521in bothMbc.Ads.UtilsandMbc.Pcs.Net.Alarmprojects. [1] [2]Mbc.Ads.UtilsandMbc.Pcs.Net.Alarmprojects from5.0.0.0to5.1.0.0. [1] [2]Test Suite Updates
FluentAssertionswithAwesomeAssertions(version9.*) across all test projects and updated their package references accordingly. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]Minor Code Improvements
StringMarshaler.UTF16toStringMarshaler.UTF16.Encodingin test helpers for improved clarity and correctness. [1] [2]