S7 protocol: full refactor, threading architecture, and unified PLC state panel by EvaTheSalmon · Pull Request #5 · Semiteq/SemiStep

EvaTheSalmon · 2026-04-08T13:13:40Z

Summary

This branch delivers the complete S7/PLC integration overhaul across five commits:

Fix S7 protocol — corrected DataDbLayout offsets and S7Driver connection endpoint so the protocol wire format is bit-exact
Refactor S7 module — replaced exception-based control flow with Result<T> propagation throughout the S7 layer; introduced PlcSyncCoordinator, PlcSyncExecutor, sync loop with execution monitoring, reconnect reconciliation, recipe debounce write, and write verification
Surface PLC operational events in the message panel — PLC connect/disconnect, sync errors, and conflict events are displayed in the UI message panel
Fix threading architecture — STA entry point, unified dispatch model, self-marshalling MessagePanelViewModel (internal Post/CheckAccess pattern); removed premature AvaloniaScheduler.Instance assignment
Unify PLC state into Result<PlcSessionSnapshot> — IPlcSyncService.PlcState observable replaces LastError/ErrorChanged/StatusChanged; RecipeMutationCoordinator rebuilds the message panel from both recipe analysis reasons and PLC state reasons on every change; PlcSyncErrorText status-bar label removed

Key design decisions

MessagePanelViewModel is self-marshalling: all mutating methods dispatch to the UI thread internally; call sites do not wrap in Dispatcher.UIThread.Post
PLC state flows as Result<PlcSessionSnapshot> from PlcSyncCoordinator → DomainFacade → RecipeMutationCoordinator → MessagePanelViewModel
DomainFacade was split: PlcLifecycleManager handles enable/disable/reconnect; RecipeEditService handles recipe mutations
PlcSyncCoordinator was split: PlcSyncExecutor owns the sync execution loop

… endpoint DataDbLayout defaults were using overlapping WORD-sized offsets (0/2/4) for the two DWORD header fields, causing ArgumentOutOfRangeException on PLC read/write. Correct offsets are 0/4/8 (two 4-byte DWORDs before data start). Updated defaults in DataDbLayout.cs and all four connection.yaml files to match. S7Driver was passing a combined 'ip:port' string as the host argument; S7NetPlus requires ip and port as separate parameters.

…propagation - All public methods in PlcTransactionExecutor, RecipeConverter, S7Service, PlcSyncCoordinator, and PlcExecutionMonitor now return Result/Result<T> - Codec Decode methods (ExecutionStateCodec, ManagingAreaCodec) return Result<T> instead of throwing ArgumentException - Deleted PlcNotConnectedException and PlcWriteVerificationException; introduced NotConnectedError (FluentResults Error subclass) for disconnect discrimination - Collapsed redundant internal/public type pairs: PlcExecutionState->PlcExecutionInfo, ManagingAreaState->PlcManagingAreaState - OperationCanceledException never caught inside S7 module - Log.Warning entries are message-only; Log.Error entries include exception object - Extracted PlcRecipeDataComparer; IsRecipeActiveAsync delegates to ReadExecutionStateAsync - DomainFacade WRN logs converted to message-only - Updated tests: Result assertions replace exception assertions; added ExecutionStateCodecTests

Route PLC connection and sync events into the operator message panel under a "PLC" source label, so connection changes, sync results, and failures are visible without opening a log file. - DomainFacade.ConnectionStateChanged: Action? -> Action<PlcConnectionState> - MessagePanelViewModel: add AddWarning method - RecipeMutationCoordinator: wire OnConnectionStateChanged and OnSyncStatusChanged to the panel; surface EnableSync failures; thread-safe dispatch via Dispatcher.UIThread.Post and Subject lock - MainWindowViewModel: subscribe ToggleSyncCommand.ThrownExceptions; fix Log.Warning convention in HandleConflictAsync; surface conflict resolution failure to the panel

…self-marshalling MessagePanel

…iaScheduler access - Introduce PlcSessionSnapshot DTO and IObservable<Result<PlcSessionSnapshot>> PlcState on IPlcSyncService, replacing ErrorChanged event and LastError property - PlcSyncCoordinator publishes atomic snapshots via BehaviorSubject; sync execution extracted into PlcSyncExecutor to bring coordinator under 300-line limit - DomainFacade PLC lifecycle (EnableSync, DisableSync, reconnect reconciliation, conflict resolution) extracted into PlcLifecycleManager; recipe edit operations extracted into RecipeEditService, bringing DomainFacade under 300-line limit - RecipeMutationCoordinator subscribes to domainFacade.PlcState and rebuilds the message panel from combined _lastRecipeResult + _lastPlcState reasons on every state change, replacing the ad-hoc AddError/AddInfo call sites and the ConnectionStateChanged/ SyncStatusChanged dual-handler pattern - PlcSyncErrorText removed from status bar and MainWindowViewModel; sync errors now appear exclusively in the message panel as structured reasons - Fix startup crash: RxApp.MainThreadScheduler = AvaloniaScheduler.Instance was called before Avalonia initialised, causing PlatformNotSupportedException. Removed the manual assignment (UseReactiveUI() handles it) and moved InitializeServices into AfterSetup in App.Run, ensuring DI singletons are resolved after the scheduler is set

Copilot

Pull request overview

This PR delivers a broad refactor of the S7/PLC integration and UI threading model, shifting the PLC layer from exception-based control flow to FluentResults and unifying PLC state propagation into a Result<PlcSessionSnapshot> stream that feeds the UI message panel and status surfaces.

Changes:

Refactors PLC sync/execution/serialization paths to return Result/Result<T> (introducing PlcSyncExecutor, NotConnectedError, and codec decode results).
Reworks UI state + message panel updates to be UI-thread safe and driven by unified PLC state snapshots (removing legacy sync error label).
Updates connection/config layouts (DB offsets, endpoint handling) and adapts unit/integration tests to the new result + threading behavior.

Reviewed changes

Copilot reviewed 48 out of 48 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
SemiStep/UI/RecipeGrid/RecipeGridViewModel.cs	Ensures coordinator events are observed on the UI scheduler.
SemiStep/UI/MessageService/MessagePanelViewModel.cs	Makes message panel mutations self-marshalling to the Avalonia UI thread.
SemiStep/UI/MainWindow/MainWindowViewModel.cs	Adopts new PLC state change signal, adds error surfacing, updates sync enable flow.
SemiStep/UI/MainWindow/AppStatusBar.axaml	Removes legacy PLC sync error text from status bar.
SemiStep/UI/Coordinator/RecipeQueryService.cs	Removes legacy last-error exposure; hardens default action selection.
SemiStep/UI/Coordinator/RecipeMutationCoordinator.cs	Rebuilds message panel from combined recipe + PLC reasons; exposes PLC state observable.
SemiStep/UI/App.axaml.cs	Moves service initialization into Avalonia setup after scheduler initialization.
SemiStep/TypesShared/Plc/PlcSessionSnapshot.cs	Adds unified PLC session snapshot model + initial state.
SemiStep/TypesShared/Plc/Memory/DataDbLayout.cs	Fixes DB header/data offsets for protocol correctness.
SemiStep/TypesShared/Domain/IPlcSyncService.cs	Replaces event/LastError model with `PlcState` observable + sync/connection setters.
SemiStep/Tests/YamlConfigs/WithGroups/connection/connection.yaml	Updates YAML schema/offsets for new protocol layout fields.
SemiStep/Tests/YamlConfigs/Standard/connection/connection.yaml	Updates YAML schema/offsets for new protocol layout fields.
SemiStep/Tests/YamlConfigs/Standalone/UnknownYamlFields/connection/connection.yaml	Updates YAML schema/offsets while preserving unknown-field coverage.
SemiStep/Tests/UI/RecipeMutationCoordinatorTests.cs	Updates coordinator construction and flushes Avalonia dispatcher jobs in assertions.
SemiStep/Tests/UI/RecipeMutationCoordinatorLoadRecipeTests.cs	Updates load tests for UI-thread-marshalled message panel behavior.
SemiStep/Tests/UI/RecipeGridViewModelTests.cs	Updates coordinator construction to match new signature.
SemiStep/Tests/UI/MessagePanelViewModelTests.cs	Updates tests to flush dispatcher jobs after async UI-thread posts.
SemiStep/Tests/UI/Helpers/UIFixture.cs	Updates fixture coordinator construction to match new signature.
SemiStep/Tests/S7/S7ServiceTests.cs	Updates expectations/comments for result-based not-connected errors.
SemiStep/Tests/S7/PlcTransactionExecutorTests.cs	Updates tests to validate failed `Result` outcomes instead of exceptions; reduces duplication.
SemiStep/Tests/S7/PlcSyncCoordinatorTests.cs	Migrates status assertions to `PlcState` snapshot stream.
SemiStep/Tests/S7/PlcExecutionMonitorTests.cs	Renames execution state model references to `PlcExecutionInfo`.
SemiStep/Tests/S7/ManagingAreaCodecTests.cs	Updates decode tests to assert `Result` success/failure instead of exceptions.
SemiStep/Tests/S7/Helpers/FakeExecutionTransport.cs	Updates fake transport to use `PlcExecutionInfo`.
SemiStep/Tests/S7/ExecutionStateCodecTests.cs	Adds unit tests for new `ExecutionStateCodec` result-based decoding.
SemiStep/Tests/Helpers/StubPlcSyncService.cs	Updates stub to publish PLC state snapshots via observable.
SemiStep/S7/Sync/PlcWriteVerificationException.cs	Removes verification exception in favor of failed results.
SemiStep/S7/Sync/PlcTransactionExecutor.cs	Converts read/write operations to `Result` APIs; adds shared read+decode helper and verification comparer.
SemiStep/S7/Sync/PlcSyncExecutor.cs	Introduces dedicated debounce + write execution loop with status/time callbacks.
SemiStep/S7/Sync/PlcSyncCoordinator.cs	Publishes unified `PlcState` snapshots and delegates execution to `PlcSyncExecutor`.
SemiStep/S7/Sync/PlcRecipeDataComparer.cs	Adds bit-exact verification comparer for recipe payload round-trips.
SemiStep/S7/Sync/PlcExecutionMonitor.cs	Updates poll loop to consume `Result<PlcExecutionInfo>` from transaction executor.
SemiStep/S7/Serialization/RecipeConverter.cs	Converts converter operations to `Result` and propagates failures instead of throwing.
SemiStep/S7/Serialization/ManagingAreaCodec.cs	Converts decode to `Result<PlcManagingAreaState>` and centralizes layout validation.
SemiStep/S7/Serialization/ExecutionStateCodec.cs	Adds layout validation and returns `Result<PlcExecutionInfo>` on decode.
SemiStep/S7/S7Driver.cs	Fixes S7.Net PLC constructor endpoint usage (IP + port parameters).
SemiStep/S7/Protocol/PlcNotConnectedException.cs	Removes not-connected exception type (superseded by `NotConnectedError`).
SemiStep/S7/Protocol/PlcExecutionState.cs	Removes legacy execution state record (superseded by `PlcExecutionInfo`).
SemiStep/S7/Protocol/NotConnectedError.cs	Adds structured not-connected error for result-based flow control.
SemiStep/S7/Protocol/ManagingAreaState.cs	Removes legacy managing-area state record (superseded by `PlcManagingAreaState`).
SemiStep/S7/Facade/S7Service.cs	Adapts service APIs/logging to result-based transaction executor behavior.
SemiStep/Domain/Facade/RecipeEditService.cs	Extracts recipe mutation logic and result propagation from `DomainFacade`.
SemiStep/Domain/Facade/PlcLifecycleManager.cs	Extracts PLC enable/disable/reconnect reconciliation and conflict detection.
SemiStep/Domain/Facade/DomainFacade.cs	Splits responsibilities into lifecycle + edit services; exposes unified `PlcState`.
SemiStep/Domain/DomainDi.cs	Updates DI construction for new `DomainFacade` signature.
SemiStep/Domain/Domain.csproj	Adds System.Reactive dependency to the Domain project.
SemiStep/Application/Program.cs	Switches to STA entry point and refactors startup flow into a synchronous main.
ConfigFiles/connection/connection.yaml	Updates example/default connection YAML for new layout and keep-alive settings.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-08T13:20:27Z

SemiStep/UI/Coordinator/RecipeMutationCoordinator.cs

 	public async Task<Result> LoadRecipeFromPlcAsync()
 	{
 		var result = await domainFacade.LoadRecipeFromPlcAsync();

-		if (!result.IsFailed)
+		_lastRecipeResult = Result.Ok();
+
+		if (result.IsFailed)
 		{
-			messagePanel.Clear();
+			_lastRecipeResult = result;
 		}

-		RefreshMessagePanel(result);
+		RebuildMessagePanel();



LoadRecipeFromPlcAsync resets _lastRecipeResult to Result.Ok() when the load succeeds, which drops any non-error Reasons (e.g., warnings) returned from DomainFacade.LoadRecipeFromPlcAsync() on success. This means structural warnings from recipe analysis won’t be shown after a successful PLC load. Consider storing the actual result (success or failure) in _lastRecipeResult and relying on RefreshReasons to replace structural entries.

Copilot · 2026-04-08T13:20:28Z

SemiStep/UI/Coordinator/RecipeMutationCoordinator.cs

 	public async Task<Result> LoadRecipeAsync(string filePath)
 	{
 		var result = await domainFacade.LoadRecipeAsync(filePath);

-		if (!result.IsFailed)
+		_lastRecipeResult = Result.Ok();
+
+		if (result.IsFailed)
 		{
-			messagePanel.Clear();
+			_lastRecipeResult = result;
 		}

-		RefreshMessagePanel(result);
+		RebuildMessagePanel();



LoadRecipeAsync resets _lastRecipeResult to Result.Ok() when the load succeeds, which discards any success Reasons (e.g., warnings) produced by DomainFacade.LoadRecipeAsync() and prevents them from being rendered in the message panel. Consider keeping _lastRecipeResult = result regardless of success/failure so structural warnings still appear while stale structural entries are replaced.

Copilot · 2026-04-08T13:20:28Z

SemiStep/S7/Sync/PlcExecutionMonitor.cs

+					}
+					else
+					{
+						Log.Warning("Execution monitor poll error: {Message}", result.Errors[0].Message);
+					}
+
+					return;


PollLoopAsync returns (stops the execution monitor) on any failed ReadExecutionStateAsync result, including transient decode/read failures that are not NotConnectedError. Previously the loop logged and continued; with this change, the monitor can permanently stop publishing execution state until restarted. Consider continuing the loop after logging non-connection failures, and only exiting on cancellation or confirmed disconnect.

Suggested change

}

else

{

Log.Warning("Execution monitor poll error: {Message}", result.Errors[0].Message);

}

return;

return;

}

Log.Warning("Execution monitor poll error: {Message}", result.Errors[0].Message);

continue;

Copilot · 2026-04-08T13:20:29Z

SemiStep/S7/Sync/PlcSyncCoordinator.cs

+	public void Dispose()
 	{
 		if (_disposed)
 		{
 			return;
 		}

-		lock (_lock)
-		{
-			_pendingSnapshot = recipe;
-
-			if (_syncTask is not null && !_syncTask.IsCompleted)
-			{
-				Log.Debug("Sync in progress, queueing new snapshot");
-
-				return;
-			}
-
-			StartDebounce();
-		}
+		_disposed = true;
+		_executor.Dispose();
+		_subject.OnCompleted();
+		_subject.Dispose();
 	}


Dispose() completes and disposes _subject immediately after cancelling the executor, but the executor’s background task can still call the setStatus delegate (which invokes PublishSnapshot → _subject.OnNext). If that happens after _subject is completed/disposed, it can throw (and potentially surface as an unobserved task exception). Consider either waiting for pending executor work to finish before completing/disposing the subject, or guarding PublishSnapshot/Status updates so they no-op once _disposed is set.

Copilot · 2026-04-08T13:20:29Z

SemiStep/UI/MainWindow/MainWindowViewModel.cs

 		catch (Exception ex)
 		{
-			Log.Warning(ex, "Unexpected error while handling PLC recipe conflict");
+			Log.Warning("Unexpected error while handling PLC recipe conflict: {Message}", ex.Message);
+			MessagePanel.AddError("Failed to resolve PLC recipe conflict — sync disabled", "PLC");


The catch block logs only ex.Message and drops the exception object (Log.Warning("...{Message}", ex.Message)), which loses stack trace and exception type in logs. Consider logging the exception (Log.Warning(ex, ...)) while still showing the user-facing message via MessagePanel.AddError.

Copilot · 2026-04-08T13:20:29Z

SemiStep/UI/Coordinator/RecipeMutationCoordinator.cs

 	private void OnPlcRecipeConflictDetected(Recipe local, Recipe plc)
 	{
-		_plcRecipeConflictDetected.OnNext((local, plc));
+		Avalonia.Threading.Dispatcher.UIThread.Post(() => _plcRecipeConflictDetected.OnNext((local, plc)));
 	}


OnPlcRecipeConflictDetected posts an OnNext to the UI thread without guarding against disposal. If a conflict is detected, then the coordinator is disposed before the posted callback runs, _plcRecipeConflictDetected may already be disposed and the callback can throw on the UI thread. Consider checking _disposed inside the posted action (and returning early) before calling OnNext.

…, extract RecipeStepCoordinator - PlcSyncCoordinator: make _disposed volatile, acquire lock before setting in Dispose(), add TryPublish() helper, guard OnPlcStateChanged and OnPlcRecipeConflictDetected - PlcSyncExecutor: make _disposed volatile, set inside lock(stateLock), guard OnRecipeChanged inside lock, drain _syncTask in Dispose() with 5s timeout, replace dead catch(OperationCanceledException) with filtered AggregateException handler - PlcExecutionMonitor: continue on non-disconnect errors instead of return, extract CancelAndDetachPollTask() helper, rename StopTimeout to _stopTimeout per naming rules, remove redundant else - RecipeMutationCoordinator: assign _lastRecipeResult unconditionally in both load paths so success reasons are preserved, extract mutation methods to RecipeStepCoordinator, convert all expression-bodied methods to block bodies per editorconfig rules - Add regression tests: success-path warnings preserved after LoadRecipeAsync, transient error does not abort PollLoopAsync

Copilot

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-08T14:13:53Z

SemiStep/Domain/Facade/PlcLifecycleManager.cs

+		_connectionStateHandler = state =>
+		{
+			if (state == PlcConnectionState.Disconnected && _isSyncEnabled)
+			{
+				syncService.Reset();
+			}
+			else if (state == PlcConnectionState.Connected && _isSyncEnabled)
+			{
+				_ = PerformReconnectReconciliationAsync().ContinueWith(
+					t => Log.Error(t.Exception, "Unhandled error in reconnect reconciliation"),
+					TaskContinuationOptions.OnlyOnFaulted);
+			}
+
+			syncService.UpdateConnectionState(state);
+		};


In the connection state handler, syncService.Reset() is called before syncService.UpdateConnectionState(state). Since Reset() changes PlcSyncCoordinator.Status (and publishes a snapshot) using the previous connection state, subscribers can briefly receive an inconsistent PlcSessionSnapshot (e.g., ConnectionState=Connected while Status=Disconnected / “PLC connection lost”). Consider updating the connection state first (or adding an atomic Reset(state) / SetState(connectionState, status, ...) API) so the published snapshot is consistent.

Copilot · 2026-04-08T14:13:53Z

SemiStep/Domain/Domain.csproj

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.DependencyInjection.Abstractions" Version="10.0.5"/>
    <PackageReference Include="Serilog" Version="4.3.1" />
+    <PackageReference Include="System.Reactive" Version="6.0.1"/>


System.Reactive was added as a direct dependency, but there are no System.Reactive* usages in the Domain project. If it’s not required for compilation/runtime, consider removing it to reduce dependency surface and keep the project’s package graph minimal.

Suggested change

<PackageReference Include="System.Reactive" Version="6.0.1"/>

Copilot · 2026-04-08T14:13:54Z

ConfigFiles/connection/connection.yaml


 # -- Connection info --
-ip: "192.168.0.150:102"
+ip: "192.168.0.1:102"


ip was changed to 192.168.0.1:102, which is inconsistent with the rest of the repo (tests and PlcConnectionSettings.Default use 192.168.0.150). If this file is intended as the shipped/default config, consider reverting to the prior address or making it an explicit placeholder so developers/users don’t silently target the wrong PLC endpoint.

Suggested change

ip: "192.168.0.1:102"

ip: "192.168.0.150"

…ve dependency - PlcLifecycleManager: extract anonymous lambda to OnConnectionStateChanged() and call UpdateConnectionState before Reset() so every published snapshot carries the correct ConnectionState (was transiently inconsistent: Connected + Disconnected status) - Domain.csproj: remove System.Reactive package reference (IObservable<T> is BCL, no System.Reactive types are used in the Domain project)

EvaTheSalmon added 5 commits April 7, 2026 13:30

Fix threading architecture: STA entry point, unified dispatch model, …

9021b0f

…self-marshalling MessagePanel

Copilot AI review requested due to automatic review settings April 8, 2026 13:13

Copilot started reviewing on behalf of EvaTheSalmon April 8, 2026 13:14 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

EvaTheSalmon requested a review from Copilot April 8, 2026 14:05

Copilot AI reviewed Apr 8, 2026

View reviewed changes

EvaTheSalmon merged commit bbcaf42 into master Apr 8, 2026
1 check passed

EvaTheSalmon deleted the fix/s7-protocol branch April 8, 2026 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S7 protocol: full refactor, threading architecture, and unified PLC state panel#5

S7 protocol: full refactor, threading architecture, and unified PLC state panel#5
EvaTheSalmon merged 7 commits intomasterfrom
fix/s7-protocol

EvaTheSalmon commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

EvaTheSalmon commented Apr 8, 2026

Summary

Key design decisions

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants