Noser by bqstony · Pull Request #13 · mbc-engineering/net-pcs

bqstony · 2025-11-26T19:05:49Z

This pull request introduces a new feature for writing structs with CommandInputBuilder and PrimitiveCommandArgumentHandler, fixes a bug with double writes, and upgrades several dependencies. Additionally, all test projects now use AwesomeAssertions instead of FluentAssertions, and the Beckhoff TwinCAT.Ads package is updated across projects. Below are the most important changes grouped by theme.

Features and Bugfixes

Added support for writing structs (including recursive structs) to the PLC using CommandInputBuilder and PrimitiveCommandArgumentHandler. Struct fields must be flagged with {attribute 'PlcCommandInput'} and be of type CommandInputBuilder in C#.
Fixed an issue where PrimitiveCommandArgumentHandler would perform a double write in WriteInputData.

Dependency and Version Upgrades

Updated the Beckhoff.TwinCAT.Ads package from version 6.1.332 to 6.2.521 in both Mbc.Ads.Utils and Mbc.Pcs.Net.Alarm projects. [1] [2]
Bumped the version numbers for Mbc.Ads.Utils and Mbc.Pcs.Net.Alarm projects from 5.0.0.0 to 5.1.0.0. [1] [2]

Test Suite Updates

Replaced all usages of FluentAssertions with AwesomeAssertions (version 9.*) 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

Updated usages of StringMarshaler.UTF16 to StringMarshaler.UTF16.Encoding in test helpers for improved clarity and correctness. [1] [2]

…uilder

…tData

…-PlcAdsConnectionService fix: fix potential deadlock in PlcAdsConnectionService

bqstony · 2025-11-26T19:06:39Z

@copilot review the code

Copilot · 2025-11-26T19:06:52Z

@bqstony I've opened a new pull request, #14, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

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 PrimitiveCommandArgumentHandler for PLC commands
Fixed double write issue in WriteInputData method
Implemented auto-reconnect feature with configurable timeout in PlcAdsConnectionService
Enhanced PlcAdsStateReader.StopSampling() with safer notification cleanup using TryDeleteDeviceNotification
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.

Copilot · 2025-11-26T19:09:56Z

NET/Mbc.Pcs.Net/Connection/PlcAdsConnectionService.cs

+        private void StopReconnectTask()
+        {
+            _reconnectCancellationTokenSource?.Cancel();
+            _reconnectCancellationTokenSource?.Dispose();
+            _reconnectCancellationTokenSource = null;
+            _reconnectTask = null;


Potential resource leak: _reconnectTask is set to null after cancellation but is never awaited. If the task is still running when StopReconnectTask() is called, it could continue executing in the background. Consider awaiting the task (with a timeout) before nullifying it to ensure proper cleanup:

_reconnectTask?.Wait(TimeSpan.FromSeconds(5));

or using async disposal pattern.

Copilot · 2025-11-26T19:09:56Z

NET/Mbc.Pcs.Net/Connection/PlcAdsConnectionService.cs

            {
                _connected = e.Connected;
                _connection = e.Connection;
-                ConnectionStateChangedInternal?.Invoke(this, e);
+
+                // Execute auto-reconnect if enabled
+                if (_autoReconnectEnabled && _serviceStarted && !_connected)
+                {
+                    StartReconnectTask();
+                }
+                else if (_connected)
+                {
+                    StopReconnectTask();
+                }
            }
+
+            ConnectionStateChangedInternal?.Invoke(this, e);
+        }
+
+        private void StartReconnectTask()
+        {
+
+            // Stop existing reconnect task if running
+            StopReconnectTask();
+
+            _reconnectCancellationTokenSource = new CancellationTokenSource();
+            var cancelationToken = _reconnectCancellationTokenSource.Token;
+
+            _reconnectTask = Task.Run(async () =>
+            {
+                while (!cancelationToken.IsCancellationRequested)
+                {
+                    try
+                    {
+                        _logger?.LogInformation("Wait {time} to try a reconnection to PLC.", ReconnectionTime);
+                        await Task.Delay(ReconnectionTime, cancelationToken);
+
+                        if (!cancelationToken.IsCancellationRequested && !_connected)
+                        {
+                            _logger?.LogInformation("Try a reconnection to PLC.");
+                            _plcConnection.Connect();
+                        }
+                    }
+                    catch (TaskCanceledException)
+                    {
+                        // Expected when cancellation is requested
+                        break;
+                    }
+                    catch (Exception)
+                    {
+                        // Continue reconnection attempts even if one fails
+                    }
+                }
+            }, cancelationToken);
+        }
+
+        private void StopReconnectTask()
+        {
+            _reconnectCancellationTokenSource?.Cancel();
+            _reconnectCancellationTokenSource?.Dispose();
+            _reconnectCancellationTokenSource = null;
+            _reconnectTask = null;


Potential race condition: The fields _connected, _serviceStarted, _reconnectCancellationTokenSource, and _reconnectTask are accessed from multiple threads (the reconnect task thread and the main thread) without synchronization. This can lead to race conditions. Consider using locks or making these fields volatile, or better yet, use thread-safe patterns for managing the reconnect task lifecycle.

Copilot · 2025-11-26T19:09:56Z

NET/Changelog.md

+ICommandInput inputStructValues = CommandInputBuilder.FromDictionary(new Dictionary<string, object>()
+{
+    ["StructValue1"] = 123,
+    ["StructValue1"] = 3333,


Duplicate key in dictionary example. "StructValue1" appears twice in the dictionary with different values (123 and 3333). This will cause a runtime error. The second occurrence should likely be "StructValue2".

Suggested change

["StructValue1"] = 3333,

["StructValue2"] = 3333,

Copilot · 2025-11-26T19:09:57Z

NET/Changelog.md

+{
+    ["value1"] = (byte)123,
+    ["stAbc"] = inputStructValues,
+    ["alue2"] = (byte)222,


Typo in dictionary key. The key is "alue2" but should likely be "value2" (missing leading 'v').

Suggested change

["alue2"] = (byte)222,

["value2"] = (byte)222,

NET/Mbc.Pcs.Net.Command/PrimitiveCommandArgumentHandler.cs

Copilot · 2025-11-26T19:09:58Z

NET/Mbc.Pcs.Net/State/PlcAdsStateReader.cs

+
+                if (result.Failed())
+                {
+                    _logger.LogWarning("Could not deregister Device Notification for '{variable_path}' because no connection is available.", _config.VariablePath);


The error message suggests "no connection is available" but this is misleading. The code is called when _adsConnectionService.IsConnected is true, so a connection exists. The error occurred during TryDeleteDeviceNotification, which can fail for other reasons (e.g., invalid handle, communication error). Consider a more accurate message like "Could not deregister Device Notification for '{variable_path}' (Error code: {result})."

Copilot · 2025-11-26T19:09:58Z

NET/Mbc.Pcs.Net/Connection/PlcAdsConnectionService.cs

        private bool _connected;
        private IAdsConnection _connection;
+        private bool _autoReconnectEnabled;
+        private ILogger _logger = null;


[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.

Copilot · 2025-11-26T19:09:58Z

NET/Mbc.Pcs.Net.Command/PrimitiveCommandArgumentHandler.cs

-                marshaledValues.Add(marshaledValueBuffer.Slice(0, size).ToArray());
+                    if (!converter.TryMarshal(item.DataType, encoding, value, marshaledValueBuffer.Span, out int size))
+                    {
+                        throw new PlcCommandException(string.Format("Input variable {0} has invalid PLC data type {1} to serialize with PrimitiveTypeMarshaler.", item.InstanceName, item.DataType.ToString()));


Redundant call to 'ToString' on a String object.

Copilot · 2025-11-26T19:09:58Z

NET/Mbc.Pcs.Net/Connection/PlcAdsConnectionService.cs

        private readonly PlcAdsConnectionProvider _plcConnection;
        private bool _connected;
        private IAdsConnection _connection;
+        private bool _autoReconnectEnabled;


Field '_autoReconnectEnabled' can be 'readonly'.

Copilot · 2025-11-26T19:09:59Z

NET/Mbc.Pcs.Net/Connection/PlcAdsConnectionService.cs

        private bool _connected;
        private IAdsConnection _connection;
+        private bool _autoReconnectEnabled;
+        private ILogger _logger = null;


Field '_logger' 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.

bqstony and others added 8 commits May 20, 2025 14:34

Support structs in PrimitiveCommandArgumentHandler with CommandInputB…

6fe5ba2

…uilder

Remove double write of PrimitiveCommandArgumentHandler in WriteInpu…

a9c05c4

…tData

change from FluentAssertions to AwesomeAssertions

61a2c4b

fix: fix potential deadlock in PlcAdsConnectionService

3fb3df1

Merge pull request #12 from mbc-engineering/fix-potential-deadlock-in…

6a33a13

…-PlcAdsConnectionService fix: fix potential deadlock in PlcAdsConnectionService

beder handling in PlcAdsStateReader when stop is called bevore start

59b4684

Update to Beckhoff.TwinCAT.Ads version 6.2.521

7ac1cb0

Add Automatic recconection behavior in PlcAdsConnectionService

414f4a8

Copilot AI review requested due to automatic review settings November 26, 2025 19:05

bqstony self-assigned this Nov 26, 2025

Copilot started reviewing on behalf of bqstony November 26, 2025 19:06 View session

Copilot AI mentioned this pull request Nov 26, 2025

Add struct support for PrimitiveCommandArgumentHandler, auto-reconnect, and dependency updates #14

Draft

Copilot finished reviewing on behalf of bqstony November 26, 2025 19:08

Copilot AI reviewed Nov 26, 2025

View reviewed changes

bqstony added 2 commits January 4, 2026 20:04

- Support for .NET 10.0.

3ef7b8e

- 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.

Merge pull request #16 from mbc-engineering/support_net10

ec0f13a

Support for .NET 10.0.

Conversation

bqstony commented Nov 26, 2025

Features and Bugfixes

Dependency and Version Upgrades

Test Suite Updates

Minor Code Improvements

Uh oh!

bqstony commented Nov 26, 2025

Uh oh!

Copilot AI commented Nov 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments