v2026.8.28 - Reliability, validation & housekeeping (596 tests, warning-free build) - #158
Open
nokkies wants to merge 20 commits into
Open
v2026.8.28 - Reliability, validation & housekeeping (596 tests, warning-free build)#158nokkies wants to merge 20 commits into
nokkies wants to merge 20 commits into
Conversation
Owner
Author
|
Code review:
|
nokkies
changed the title
Owner
Author
|
I checked out the branch and fixed the blockers:
I kept the CustomEntry.Trend auto-enabling Monitor and the redundant Trend = false initializers. If you want those adjusted too, let me know. |
…ap links - Move 8 obsolete docs (v2.x improvement notes, unexecuted coordinator refactoring plans, v3.4.x one-shot release instructions, completed Unit ID isolation plan, obsolete metallic theme guide, stale feature roadmap) into docs/archive/ with an ARCHIVE.md index explaining each - Point README roadmap links at the living VERSIONED_ROADMAP.md - Fix README versioning section (CalVer, not semver) and stale ISCC example version
- UpdateService: guard nullable ReleaseUrl assignment (CS8601) - ModbusFrameLog: drop unused INotifyPropertyChanged/PropertyChanged event (CS0067) - frames are immutable once logged, the owning ObservableCollection notifies the UI - VisualNodeEditorViewModel.ConnectionLine: explicitly initialize PathFigure.Segments/PathGeometry.Figures (CS8602) instead of relying on lazy null collections Solution now builds with 0 warnings; 482/482 tests pass.
…e version From the UI/ViewModel code review (batch A): - Marshal LastErrorTime/HasConnectionError writes that ran on the poll thread (HandleAreaReadFailureAsync, ClearMonitorFailure) to the UI dispatcher - fixes cross-thread PropertyChanged on first read failure - PromptAddress (write path) now enforces the 0..65535 Modbus address range via shared ModbusAddressValidator constants, with status feedback - Project metadata Version no longer hardcodes 2026.7.24; reads the executing assembly version so it cannot drift - ConnectionManagerViewModel.BaudRates is now static readonly (was allocating a sorted list on every ItemsSource access) - Remove dead _fallbackConsoleMessages field, dead AdvancedFunctions_Click/DeviceScanner_Click handlers in MainView (live copies live in MainWindow code-behind) + orphaned usings - VisualNodeEditorView pan start point: drop the duplicate assignment that would NRE when the canvas scroll viewer is null 0 build warnings; 482/482 tests pass.
… harden startup From the headless/tests/CI code review: - HeadlessPollingService: detect connection loss (IsConnected or 3 consecutive failed reads) and reconnect with backoff instead of zombie-polling forever; backoff configurable via Polling:ReconnectBackoffMs - HeadlessCustomService: same reconnect handling; stop the host (StopApplication) when the watch file is missing/empty or malformed instead of idling forever; guard shutdown disconnects - Program: reject unknown -- options and options missing values; validate Polling:Count/IntervalMs/StartAddress and Custom:TickMs at startup; case-insensitive Serilog level parsing (incl. Trace); resolve relative log paths against the content root - HeadlessProfileFactory: headless MQTT ClientId defaults to ModbusForge-Headless (matches shipped appsettings and help text) - New ModbusForge.Headless.Tests project (net8.0, 45 tests): profile factory transport/defaults, argument + config validation, log level parsing, polling reconnect behavior, and custom-watch failure paths (the headless project previously had zero test coverage)
- ModbusTcpService: bound socket + transport I/O at 5s (NModbus defaults to infinite, so a silent device hung every operation holding the I/O lock); disable NModbus transport-level retries (TCP retransmits at protocol level; a dead peer now fails in ~5s instead of ~21s and blocks the poll queue); dispose previous client/socket on reconnect (leak fix); chunk coil and discrete reads >2000 points instead of throwing; single writes now rethrow SlaveException/IO failures instead of silently reporting success - ModbusSerialService: same chunking fix for bit areas; single writes rethrow; timeouts promoted to a named constant - PollingEngine: a null read (no device response) is now surfaced as an error instead of a successful empty result - ScriptRunner: null reads reported as command failure, not (success, null) - ScriptRuleService: Equals/NotEquals now compare semantically (boxed ushort vs parsed double never matched before); reentrancy guard so overlapping timer ticks cannot double-fire a rule action; dispose-safe evaluation; named timer constant - ModbusServerService: reads for an unconfigured unit ID no longer silently return the primary unit's data - ConnectionManager: RemoveProfile disconnects synchronously before disposing (no more fire-and-forget race); implements IDisposable so remaining profile services are released on shutdown - MqttGatewayService: gated client creation vs disconnect (reconnect no longer leaks a client after disconnect); bounded synchronous dispose instead of fire-and-forget; named reconnect-delay constant - RetryPolicyService: Random.Shared (was per-instance, non-thread-safe), overflow-safe exponential backoff, jitter range clamped (Next(0,0) could throw) - CircuitBreakerService: exactly one half-open recovery probe at a time (was every concurrent caller); probe failure reopens for a fresh period - ApiApplicationService: GetStatus no longer sync-over-async blocks the API thread; interface/callers/tests updated to GetStatusAsync - TagService.DeleteGroupAsync: rollback now restores in-place re-parenting mutations (list snapshots shared object references) and root groups from a pre-mutation snapshot; MovedTagCount no longer counts tags that were not actually relocated - DeviceIdentificationReader: FC43 MoreFollows pagination with a 16-transaction safety cap (long vendor names pushed other objects into follow-up responses) - Tests: regression coverage for the TCP timeout fix, PollingEngine error surfacing, and ScriptRuleService value comparison
- ModbusMultiUnitServer: validate the MBAP protocol ID (0x0000) and close non-compliant connections (the field was read but never checked) - FC05 now strictly accepts only 0xFF00/0x0000; any other 16-bit value returns Illegal Data Value instead of silently writing OFF - Unit ID 0 (broadcast) writes are applied to every configured unit's data store and receive NO response, per the Modbus spec (previously a data store was created for unit 0 and answered); broadcast reads are ignored - Cap FC43 object payload at 246 bytes so the response PDU stays within the 253-byte protocol limit (250 would produce an oversize frame) - Reject new connections beyond MaxClients (10) and drop clients that go idle for 10 minutes (per-request read timeout); active client count is tracked so rejected/closed clients release their slot - FC processing errors log at Warning instead of Debug - FC23: client (TCP + serial) and server services now enforce the spec's 121-register write cap (was FC16's 123) - Tests: protocol-compliance suite for FC05 strict values, broadcast semantics, and MBAP protocol ID rejection
- LoggingStreamResource: parse Unit ID / function code deterministically per transport type (the old heuristic treated any frame with raw[2..3]==0x00 as an MBAP header and misparsed legitimate RTU frames, e.g. reads at low addresses); construction sites now pass their TransportType - ModbusFrameLogger: delta-timestamp update and ring-buffer insert now happen under a single lock (two separate lock sections let concurrent logs interleave and corrupt deltas); new FrameLogged event fired outside the lock for UI subscribers that must marshal to the UI thread - Atomic persistence: new AtomicFileWriter (same-directory temp file + rename) used by SettingsService, ConnectionManager.SaveProfiles, and RegisterTemplateStore so a crash mid-write no longer corrupts settings or connection profiles - ModbusSerialService.RunDiagnosticsAsync now runs on the thread pool (it opens a COM port and does blocking I/O; previously it blocked the calling thread); diagnostic port close/dispose log instead of swallowing - SerialPortStreamResource.InfiniteTimeout returns SerialPort.InfiniteTimeout (255) - the sentinel NModbus compares against - instead of the current ReadTimeout value - Empty catch blocks in disconnect/diagnostics cleanup now log at Debug - SerialSettingsDetector probe timeouts and ModbusSerialService diagnostic timeouts promoted to named constants Deferred to the UI batch: AutoReconnect preference is still settings-only; the reconnect behavior lives in MainViewModel and will be implemented there.
- Dispose now unhooks the custom-entry PropertyChanged hooks, clears the MQTT gateway SnapshotProvider, and unsubscribes the VisualNodeEditor PropertyChanged - the gateway is a long-lived singleton whose captured delegate kept the whole view-model graph alive - Implemented the previously dead AutoReconnect preference: on unexpected connection loss the VM retries ConnectProfileAsync on the configured interval; deliberate user disconnects, profile switches, project reloads, and dispose all stop it - Connection events (ActiveProfileChanged / ProfileConnected / ProfileDisconnected / ActiveProfile.PropertyChanged) are now marshaled to the UI thread before touching view-model state - they are raised from thread-pool threads by ConnectionManager - Poll loop restart after an error now backs off 1 s instead of spinning a tight error -> immediate-restart loop; clean stops restart immediately - Left navigation list is now data-driven (NavigationItems with per-item TabIndex and IsVisible mirroring the tab flags): hidden tabs disappear from the nav list, the fragile static NavigationIndexConverter and its hardcoded index tables are deleted, and the two ScriptEditor_Click code-behind handlers plus the ActiveUnitIdComboBox handler become direct command/two-way bindings (OpenScriptEditorCommand) - Tab indices are named constants; IsTabIndexVisible / EnsureSelectedTabIsVisible use them - Console tab now binds to the shared IConsoleLoggerService collection (the same sink the Modbus services, script engine, and API already log into), honoring the MaxConsoleMessages setting; repeated identical status messages (poll cycles) no longer spam the console - String writes are capped at the FC16 limit (123 registers) with clear user feedback in both the register grid and custom watch paths - ApplyRegisterValues no longer throws on duplicate grid addresses (TryAdd, first entry wins); metadata deduped the same way - OpenUrl validates http(s) URLs and logs/announces failures instead of swallowing them - async-void event handlers in RegisterTemplateImportDialog, TagBrowserWindow, WatchWindow, and VisualNodeEditorView now surface exceptions instead of silently swallowing them
- TrendViewModel: samples now queue and flush to the UI in one batch (per-sample blocking dispatcher.Invoke per series per poll); retention trimming and live-window alignment run once per batch; queue is bounded and lost-wakeup safe - ScriptEditorViewModel: now IDisposable (unsubscribes the shared script runner events and the script Commands collection), resubscribes to the Commands list when a script is loaded, Run never leaves IsRunning stuck on runner errors, and load/save errors are logged - SignalGeneratorViewModel: dropped the no-op 'await Task.CompletedTask', a generation counter stops a superseded run from clobbering status messages, and a disposed CTS can no longer spin the error-retry loop - FrameInspectorViewModel: now IDisposable; the DataGrid binds to a new UI-thread mirror collection fed by ModbusFrameLogger.FrameLogged in batches instead of the cross-thread ObservableCollection; ModbusFrameLogger gained a thread-safe Snapshot() for history import; pcap import keeps working through the same pipeline - Custom Watch grid: Address / Write Period / Read Period are now bounded NumericUpDown cells (0-65535 / 1-600000 ms) so invalid input is clamped visibly instead of silently discarded by ConvertBack - ConnectionProfile implements IDataErrorInfo (IP or host name, port, unit ID, server unit ID list incl. ranges, baud rate) and the connection manager validates the edited fields before connecting; 37 new regression tests cover the validators - Navigation list is now a visible-subset ObservableCollection rebuilt on tab-visibility changes (hidden tabs are absent, not invisible) - Compiled Avalonia bindings (x:CompileBindings) are now enabled in all views except the code-behind-only register template import dialog, which has no x:DataType for its row bindings; the removal surfaced and fixed the container-level IsVisible binding in the nav list - Menu: removed the duplicate Frame Inspector entry from Tools; renamed the ambiguous '_Trend' checkbox to '_Trends Tab' - AvaloniaMessageBoxService: returns None immediately in a headless context and activates the fallback window when no owner exists
…estruction - ModbusServerPublishingPortTests: data-path tests now bind 127.0.0.1; exactly one test deliberately exercises the 0.0.0.0 publishing bind (the merged assertions), so a writable Modbus server is no longer exposed on all network interfaces for the duration of every test - ApiServerServiceTests: 15 hard-coded loopback ports (15080-15094) replaced with an OS-assigned free port per test; Server_BindsOnlyToLoopback now also probes a non-loopback interface and asserts the server is unreachable there - ConnectionManager: profiles file path is injectable (default unchanged); ConnectionManagerTests / PerformanceTests / MainViewModel(Integration|Parity)Tests all persist to a unique %TEMP% dir per test instance instead of moving the developer's real %APPDATA%\\ModbusForge\\connection-profiles.json to .bak - ModbusMultiUnitServerTests: fixed 500ms sleep replaced with a deadline-bounded poll for the server's close-of-connection reaction - PollingThroughputTests: the hard 1s wall-clock assert is now a reported baseline with a generous 10s ceiling (CI runners vary) - ModbusServiceTests: rewritten around a new internal ModbusTcpService test-seam constructor (no more reflection that silently no-ops on a renamed field); writes are now positively verified at the mocked NModbus master with the 0-based protocol address, while the sensitive-value logging assertion stays - ModbusTcpServicePerformanceTests: removed the always-true elapsed-time assert - ModbusSerialServiceTests: removed the tautological address-formula test - deleted the empty UnitTest1 placeholder - TFM split: ModbusForge.Tests is now net8.0 (cross-platform, ready for Linux CI); the FlaUI-based Avalonia smoke tests moved to a new net8.0-windows ModbusForge.UITests project - all 6 smoke tests pass against the app
- release.yml: new version-agreement gate - the tag version must match the <Version> element of all three csproj files or the release fails (a mistag used to ship binaries whose assembly version disagrees with the repo); CalVer pattern check is now fatal instead of a warning; job gains timeout-minutes: 90 and a per-ref concurrency group; the Headless test suite now runs before publish; code signing now uses the 64-bit signtool (32-bit from a 64-bit host can fail), an https timestamp URL, verifies each signtool exit code, and signs the headless Windows exe in addition to the Avalonia one; Inno Setup pinned to choco 6.4.1 - all three workflows: actions pinned to commit SHAs instead of mutable major tags (checkout v4.4.0, setup-dotnet v4.3.1, upload-artifact v4.6.2, action-gh-release v2.6.2 - SHAs resolved from the GitHub API) - avalonia.yml: windows + ubuntu legs now build the full solution and run the core, headless and Avalonia suites (the core suite is net8.0 after the TFM split, so it finally runs on Linux CI); the headless smoke test asks the OS for a free port instead of hard-coding 1502; job timeout added - create_release.ps1: token read from GITHUB_TOKEN (no longer a mandatory command-line parameter visible to other processes), git -C \ so the commit sha cannot come from the wrong repo, real failure handling (exit 1, safe response-body read), success banner only on success, and the same three-csproj version-agreement check as the CI gate - build.ps1: anchored at \ (works from any CWD), fails with a clear error when iscc.exe is missing instead of warning and reporting success, checks iscc exit codes, and asserts all three csproj versions agree before doing anything - publish-avalonia.ps1: Inno Setup lookup now falls back to PATH and the alternate install root, like build.ps1 - setup/ModbusForge.iss: AppVersion default updated 2026.8.24 -> 2026.8.27 and MinVersion=10.0.0.0 added (.NET 8 requires Windows 10) - AI_RELEASE_WORKFLOW.md rewritten to match reality: three-csproj versioning policy, the version gate, all six release assets (headless zips were missing), README-based release notes (generate_release_notes is false), choco-installed Inno Setup (the old \ download no longer exists) - ModbusForge.csproj: ImplicitUsings deliberately left disabled, with a comment recording why (Avalonia System.IO.Path ambiguity)
The 4,370-line MainViewModel was a god class: connection, polling, register grid editing, custom watch, project persistence, navigation, and window commands all lived in one file. It is now seven partial files (same class, zero behavior change, all 585 tests green): - MainViewModel.cs (core) - lifecycle, DI fields, unit-configuration state and per-area config plumbing, child view-model properties, status - MainViewModel.Connection.cs - connect/disconnect, auto-reconnect, profile events, unit-id parsing, connection status and failure counters - MainViewModel.Polling.cs - the monitor poll loop, due-queue, area reads - MainViewModel.Registers.cs - register/coil grid read/write, prompts, per-area metadata, legacy dual-state properties - MainViewModel.CustomWatch.cs - custom entries, bulk add, watch loop - MainViewModel.Project.cs - project save/load, unit-id import/export, workspace snapshots - MainViewModel.Navigation.cs - tab visibility, navigation list, console and debug messages, theme - MainViewModel.Windows.cs - menu/dialog commands Decorative #region markers were dropped (they would have been orphaned). The split was produced by a Roslyn member extractor so every member moved verbatim (393 members in == 393 out, verified); the splitter script lives in a scratch project and is not committed.
StartAddress, RegisterCount, GlobalType, SwapBytes, SwapWords, SelectedAreaIndex and IsContinuousRead were a second, selected-area-mirrored copy of the per-area configuration. No view binds to any of them (the UI has used HoldingRegisterStart/Count/RegistersGlobalType/RegistersSwapBytes/... since the per-area rework) and no code reads them - they only existed in a two-way sync cycle with themselves. Removing them eliminates a trap for future bindings that would silently apply to one area only. MainViewModelIntegrationTests now configures the holding-register area directly.
ModbusForge.Tests is net8.0 after the TFM split (FlaUI smoke tests moved to the new ModbusForge.UITests project), and the Headless test project is now listed as well.
- CA2013 (real bug): AutoReconnectLoopAsync compared its CancellationToken to the current source's token with ReferenceEquals. Both operands are structs, so the comparison boxed two fresh instances and was always false - the loop's cleanup therefore never ran and the auto-reconnect CancellationTokenSource was never disposed (each connect/disconnect cycle leaked a CTS plus its registration table). Now a value comparison, which is also the original intent (true only when this loop still owns the CTS). - xUnit1031: the bad-protocol-ID compliance test read a completed task via .Result; it now awaits it (the preceding assert guarantees completion).
![codefactor-io[bot]](https://avatars.githubusercontent.com/in/25603?s=60&v=4){kind=small}
| /// MainViewModel - core members (lifecycle, unit configuration, status, child view models). | ||
| /// </summary> | ||
| public partial class MainViewModel : ObservableObject, IDisposable | ||
|
|
There was a problem hiding this comment.
Opening braces should not be preceded by blank line.
Suggested change
…tants - RetryPolicyService: retryability is now decided by exception TYPE only. The previous fallback retried whenever the exception MESSAGE contained words like 'connection', 'network' or 'timeout' - which silently re-ran non-transient failures (e.g. invalid configuration) up to maxRetries. New regression tests cover: non-retryable types run exactly once, message-sounding failures are not retried, transient IO failures retry until success, and exhausted retries rethrow the original exception. - PollingEngine.Stop: the worker wait was a hard-coded 5 s, exactly the transport I/O timeout - a cancel arriving mid-read could time out the wait while the worker was still alive, with no trace. It now waits 6 s (5 s I/O timeout + margin, documented) and logs a warning if the worker ever fails to exit. - ScriptRuleService: the silent catch in CompareValues now logs the failed comparison at debug level instead of swallowing it. - ModbusMultiUnitServer: removed the four pre-seed Add loops in GetOrCreateDataStore (ModbusDataCollection.Add is a documented no-op on the fixed-size store, so the loops only cost 4*65536 no-op calls per unit) and the now-unused DefaultDataStoreSize constant, which also did not match the actual store size. - TagService: removed the dead private ErrorPreview helper (never called). - ModbusTcpService/ModbusSerialService: diagnostics used bare 5000 literals for connect/transport/port timeouts; they now reference the shared IoTimeoutMs constant.
ModbusServerService (the in-process 'server mode' data source) had a patchwork
of bounds checks: WriteSingleCoil rejected address 0 but WriteSingleRegister
and the multi-write paths accepted it (then blew up deep inside
ModbusDataCollection, which treats index 0 as an unsettable placeholder),
unknown unit IDs threw different exception types per method with no message,
and error messages never said what the valid range was.
Unified rules, all with clear messages:
- Writes: address must be 1..Count-1 (the placeholder at 0 is not settable).
- Reads: address 0..Count-1 (index 0 remains readable as its default value -
the UI's server-mode convention, covered by the existing parity test, and
mirroring client mode where addresses 0 and 1 alias the first register).
- Unknown unit IDs: ArgumentOutOfRangeException('Data store not initialized
for unit N') on every read and write path (no silent fallback to the
primary unit's store).
Regression tests: zero-address writes (single register, single coil, coil
range) throw with the new messages, zero-address reads return the
placeholder default, valid 1-based ranges round-trip, and unknown unit IDs
are rejected on both the read and write paths.
- Bumped version to 2026.8.28 in all three projects (ModbusForge, ModbusForge.Core, ModbusForge.Headless). - README: new changelog entry summarizing the release, title and installer-example version synced. Contents of the branch: bounded TCP transport timeouts (no more frozen app on dead devices), visible poll failures, reliable auto-reconnect, script-rule comparison/reentrancy fixes, spec-compliant server mode with strict unit/address validation, input validation before connect, frame inspector fixes, UI lifecycle polish, headless runtime reconnect, MainViewModel partial split, SHA-pinned CI with a release version-agreement gate, and a 596-test warning-free suite.
nokkies
changed the title
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
2 participants
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.
Summary
Comprehensive housekeeping pass based on three full code reviews (core, UI, infra/CI) plus repo hygiene, built on the branch's original keyboard-shortcuts and trend-monitoring work. Every actionable finding was addressed - fixed at root cause, or deliberately kept with a documented rationale.
User-visible changes
CI / release pipeline
Code
Verification
Notes