diff --git a/code-reviews/Driver.FOCAS/findings.md b/code-reviews/Driver.FOCAS/findings.md index 9c81b64..bede10f 100644 --- a/code-reviews/Driver.FOCAS/findings.md +++ b/code-reviews/Driver.FOCAS/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 10 | +| Open findings | 9 | ## Checklist coverage @@ -95,7 +95,7 @@ or op-mode read to be `IsOk` before declaring the capability present. | Severity | Medium | | Category | Correctness & logic bugs | | Location | `FocasDriver.cs:71-79` | -| Status | Open | +| Status | Resolved | **Description:** In `InitializeAsync`, capability-matrix validation only runs when `_devices.TryGetValue(tag.DeviceHostAddress, out var device)` succeeds. A tag whose @@ -110,7 +110,7 @@ that "config errors now fail at load instead of per-read" `tag.DeviceHostAddress`, throw an `InvalidOperationException` naming the tag and the unresolved device host so the operator fixes the typo at startup. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `InitializeAsync` now throws `InvalidOperationException` naming the tag and the unresolved device when `_devices` does not contain `tag.DeviceHostAddress`, preventing silent skip-and-defer to per-read `BadNodeIdUnknown`. ### Driver.FOCAS-004 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index 7899e66..37a2ea2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -25,6 +25,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary _tagsByName = new(StringComparer.OrdinalIgnoreCase); private FocasAlarmProjection? _alarmProjection; + // _health is read/written from multiple threads (ReadAsync, WriteAsync, ProbeLoopAsync). + // Volatile.Read/Write ensures every thread sees the latest reference without a lock — the + // record is immutable so there is no torn-read risk on the object itself. private DriverHealth _health = new(DriverState.Unknown, null, null); public event EventHandler? OnDataChange; @@ -49,7 +52,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, public Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { - _health = new DriverHealth(DriverState.Initializing, null, null); + Volatile.Write(ref _health, new DriverHealth(DriverState.Initializing, null, null)); try { foreach (var device in _options.Devices) @@ -69,8 +72,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, ?? throw new InvalidOperationException( $"FOCAS tag '{tag.Name}' has invalid Address '{tag.Address}'. " + $"Expected forms: R100, R100.3, PARAM:1815/0, MACRO:500."); - if (_devices.TryGetValue(tag.DeviceHostAddress, out var device) - && FocasCapabilityMatrix.Validate(device.Options.Series, parsed) is { } reason) + if (!_devices.TryGetValue(tag.DeviceHostAddress, out var device)) + throw new InvalidOperationException( + $"FOCAS tag '{tag.Name}' references device '{tag.DeviceHostAddress}' " + + $"which is not in the Devices list. Check for a typo (e.g. wrong port or hostname)."); + if (FocasCapabilityMatrix.Validate(device.Options.Series, parsed) is { } reason) { throw new InvalidOperationException( $"FOCAS tag '{tag.Name}' ({tag.Address}) rejected by capability matrix: {reason}"); @@ -111,11 +117,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } } - _health = new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null); + Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); } catch (Exception ex) { - _health = new DriverHealth(DriverState.Faulted, null, ex.Message); + Volatile.Write(ref _health, new DriverHealth(DriverState.Faulted, null, ex.Message)); throw; } return Task.CompletedTask; @@ -150,10 +156,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } _devices.Clear(); _tagsByName.Clear(); - _health = new DriverHealth(DriverState.Unknown, _health.LastSuccessfulRead, null); + Volatile.Write(ref _health, new DriverHealth(DriverState.Unknown, Volatile.Read(ref _health).LastSuccessfulRead, null)); } - public DriverHealth GetHealth() => _health; + public DriverHealth GetHealth() => Volatile.Read(ref _health); public long GetMemoryFootprint() => 0; public Task FlushOptionalCachesAsync(CancellationToken cancellationToken) => Task.CompletedTask; @@ -206,16 +212,18 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, results[i] = new DataValueSnapshot(value, status, now, now); if (status == FocasStatusMapper.Good) - _health = new DriverHealth(DriverState.Healthy, now, null); + Volatile.Write(ref _health, new DriverHealth(DriverState.Healthy, now, null)); else - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, - $"FOCAS status 0x{status:X8} reading {reference}"); + Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, + Volatile.Read(ref _health).LastSuccessfulRead, + $"FOCAS status 0x{status:X8} reading {reference}")); } catch (OperationCanceledException) { throw; } catch (Exception ex) { results[i] = new DataValueSnapshot(null, FocasStatusMapper.BadCommunicationError, null, now); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, + Volatile.Read(ref _health).LastSuccessfulRead, ex.Message)); } } @@ -261,7 +269,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, catch (NotSupportedException nse) { results[i] = new WriteResult(FocasStatusMapper.BadNotSupported); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, nse.Message); + Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, + Volatile.Read(ref _health).LastSuccessfulRead, nse.Message)); } catch (Exception ex) when (ex is FormatException or InvalidCastException) { @@ -274,7 +283,8 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, catch (Exception ex) { results[i] = new WriteResult(FocasStatusMapper.BadCommunicationError); - _health = new DriverHealth(DriverState.Degraded, _health.LastSuccessfulRead, ex.Message); + Volatile.Write(ref _health, new DriverHealth(DriverState.Degraded, + Volatile.Read(ref _health).LastSuccessfulRead, ex.Message)); } } @@ -369,17 +379,20 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, string.Equals(t.DeviceHostAddress, device.HostAddress, StringComparison.OrdinalIgnoreCase)); foreach (var tag in tagsForDevice) { + // The wire backend is read-only by design (WriteAsync returns BadNotWritable + // for every address) so all FOCAS tags are advertised as ViewOnly regardless + // of the Writable config field. Advertising Operate would mislead OPC UA + // clients and the DriverNodeManager ACL layer into granting write permission + // on nodes that can never be written. deviceFolder.Variable(tag.Name, tag.Name, new DriverAttributeInfo( FullName: tag.Name, DriverDataType: tag.DataType.ToDriverDataType(), IsArray: false, ArrayDim: null, - SecurityClass: tag.Writable - ? SecurityClassification.Operate - : SecurityClassification.ViewOnly, + SecurityClass: SecurityClassification.ViewOnly, IsHistorized: false, IsAlarm: false, - WriteIdempotent: tag.WriteIdempotent)); + WriteIdempotent: false)); } } return Task.CompletedTask; @@ -862,7 +875,19 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, private async Task EnsureConnectedAsync(DeviceState device, CancellationToken ct) { if (device.Client is { IsConnected: true } c) return c; - device.Client ??= _clientFactory.Create(); + + // Discard the existing client (if any) before creating a new one. A client that is + // non-null but not connected may have been disposed by a HandleRecycle race or a prior + // teardown — retrying ConnectAsync on a disposed FocasWireClient hits ThrowIfDisposed and + // returns a permanent BadCommunicationError with no recovery. Replacing it unconditionally + // ensures EnsureConnectedAsync always works with a fresh, non-disposed instance. + if (device.Client is not null) + { + device.Client.Dispose(); + device.Client = null; + } + + device.Client = _clientFactory.Create(); try { await device.Client.ConnectAsync(device.ParsedAddress, _options.Timeout, ct).ConfigureAwait(false);