diff --git a/code-reviews/Driver.FOCAS/findings.md b/code-reviews/Driver.FOCAS/findings.md index 9f9d5ba..9c81b64 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 | 12 | +| Open findings | 10 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | High | | Category | Correctness & logic bugs | | Location | `FocasDriverFactoryExtensions.cs:54-86`, `FocasDriverFactoryExtensions.cs:132-140` | -| Status | Open | +| Status | Resolved | **Description:** `FocasDriverConfigDto` exposes only `Backend`, `Series`, `TimeoutMs`, `Devices`, `Tags`, and `Probe`. It has no `FixedTree`, `AlarmProjection`, or @@ -56,7 +56,7 @@ corresponding `FocasDriverOptions` properties in `CreateInstance`. Consider enab strict JSON handling (`UnmappedMemberHandling.Disallow`) so future unknown config sections fail loudly instead of being dropped. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — added `FixedTreeDto`/`AlarmProjectionDto`/`HandleRecycleDto` to `FocasDriverConfigDto` and `Build*` mappers in `CreateInstance` that populate the matching `FocasDriverOptions` properties (missing section / field keeps its default). ### Driver.FOCAS-002 @@ -65,7 +65,7 @@ sections fail loudly instead of being dropped. | Severity | High | | Category | Correctness & logic bugs | | Location | `WireFocasClient.cs:164-179`, `FocasDriver.cs:513`, `FocasDriver.cs:593` | -| Status | Open | +| Status | Resolved | **Description:** The fixed-tree bootstrap probes the `ProgramInfo` capability via `SafeTryProbe(() => client.GetProgramInfoAsync(ct))` and treats a non-null result as @@ -86,7 +86,7 @@ the underlying `cnc_exeprgname2` / `cnc_rdopmode` calls report a non-zero RC, so `SafeTryProbe` can correctly classify the series. At minimum require the program-name or op-mode read to be `IsOk` before declaring the capability present. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `WireFocasClient.GetProgramInfoAsync` now throws `InvalidOperationException` when neither the `cnc_exeprgname2` nor the `cnc_rdopmode` read is `IsOk`, so `SafeTryProbe` records `ProgramInfo` as unsupported on series that answer `EW_FUNC`/`EW_NOOPT`. ### Driver.FOCAS-003 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 3b0100f..7899e66 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -161,6 +161,9 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, internal DeviceState? GetDeviceState(string hostAddress) => _devices.TryGetValue(hostAddress, out var s) ? s : null; + /// Test seam — the resolved options the factory built this driver from. + internal FocasDriverOptions Options => _options; + // ---- IReadable ---- public async Task> ReadAsync( diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs index cd18733..a1cede2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs @@ -79,6 +79,9 @@ public static class FocasDriverFactoryExtensions Timeout = TimeSpan.FromMilliseconds(dto.Probe?.TimeoutMs ?? 2_000), }, Timeout = TimeSpan.FromMilliseconds(dto.TimeoutMs ?? 2_000), + FixedTree = BuildFixedTree(dto.FixedTree), + AlarmProjection = BuildAlarmProjection(dto.AlarmProjection), + HandleRecycle = BuildHandleRecycle(dto.HandleRecycle), }; var clientFactory = BuildClientFactory(dto, driverInstanceId); @@ -101,6 +104,55 @@ public static class FocasDriverFactoryExtensions }; } + /// + /// Map the optional FixedTree config section onto . + /// A missing section keeps the hard-coded defaults (tree disabled); a present section + /// with omitted intervals keeps each interval's default. Interval fields are JSON + /// strings (e.g. "00:00:00.250") per docs/drivers/FOCAS.md. + /// + private static FocasFixedTreeOptions BuildFixedTree(FocasFixedTreeDto? dto) + { + if (dto is null) return new FocasFixedTreeOptions(); + var defaults = new FocasFixedTreeOptions(); + return new FocasFixedTreeOptions + { + Enabled = dto.Enabled ?? defaults.Enabled, + PollInterval = dto.PollInterval ?? defaults.PollInterval, + ProgramPollInterval = dto.ProgramPollInterval ?? defaults.ProgramPollInterval, + TimerPollInterval = dto.TimerPollInterval ?? defaults.TimerPollInterval, + }; + } + + /// + /// Map the optional AlarmProjection config section onto + /// . Missing section keeps the disabled default. + /// + private static FocasAlarmProjectionOptions BuildAlarmProjection(FocasAlarmProjectionDto? dto) + { + if (dto is null) return new FocasAlarmProjectionOptions(); + var defaults = new FocasAlarmProjectionOptions(); + return new FocasAlarmProjectionOptions + { + Enabled = dto.Enabled ?? defaults.Enabled, + PollInterval = dto.PollInterval ?? defaults.PollInterval, + }; + } + + /// + /// Map the optional HandleRecycle config section onto + /// . Missing section keeps the disabled default. + /// + private static FocasHandleRecycleOptions BuildHandleRecycle(FocasHandleRecycleDto? dto) + { + if (dto is null) return new FocasHandleRecycleOptions(); + var defaults = new FocasHandleRecycleOptions(); + return new FocasHandleRecycleOptions + { + Enabled = dto.Enabled ?? defaults.Enabled, + Interval = dto.Interval ?? defaults.Interval, + }; + } + private static FocasCncSeries ParseSeries(string? raw) { if (string.IsNullOrWhiteSpace(raw)) return FocasCncSeries.Unknown; @@ -137,6 +189,9 @@ public static class FocasDriverFactoryExtensions public List? Devices { get; init; } public List? Tags { get; init; } public FocasProbeDto? Probe { get; init; } + public FocasFixedTreeDto? FixedTree { get; init; } + public FocasAlarmProjectionDto? AlarmProjection { get; init; } + public FocasHandleRecycleDto? HandleRecycle { get; init; } } internal sealed class FocasDeviceDto @@ -162,4 +217,31 @@ public static class FocasDriverFactoryExtensions public int? IntervalMs { get; init; } public int? TimeoutMs { get; init; } } + + /// + /// Optional FixedTree config section. Interval fields are JSON + /// strings ("hh:mm:ss[.fff]") — System.Text.Json + /// parses these natively. + /// + internal sealed class FocasFixedTreeDto + { + public bool? Enabled { get; init; } + public TimeSpan? PollInterval { get; init; } + public TimeSpan? ProgramPollInterval { get; init; } + public TimeSpan? TimerPollInterval { get; init; } + } + + /// Optional AlarmProjection config section. + internal sealed class FocasAlarmProjectionDto + { + public bool? Enabled { get; init; } + public TimeSpan? PollInterval { get; init; } + } + + /// Optional HandleRecycle config section. + internal sealed class FocasHandleRecycleDto + { + public bool? Enabled { get; init; } + public TimeSpan? Interval { get; init; } + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs index c7273ee..7d73245 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs @@ -170,6 +170,16 @@ public sealed class WireFocasClient : IFocasClient // managed ToText path in FocasOpMode can map it for display. var modeResult = await _wire.ReadOperationModeCodeAsync(cancellationToken).ConfigureAwait(false); + // The fixed-tree bootstrap probe (FocasDriver.SafeTryProbe) classifies the + // ProgramInfo capability as "supported" iff this method returns non-null. A CNC + // series without cnc_exeprgname2 / cnc_rdopmode answers EW_FUNC / EW_NOOPT, so + // throw when neither the program-name nor the op-mode read succeeded — otherwise + // SafeTryProbe records a false-positive capability and the driver emits Program/ + // OperationMode/ subtrees that only ever return BadDeviceFailure. + if (!nameResult.IsOk && !modeResult.IsOk) + throw new InvalidOperationException( + $"cnc_exeprgname2 failed EW_{nameResult.Rc} and cnc_rdopmode failed EW_{modeResult.Rc}."); + var wireName = nameResult.Value; return new FocasProgramInfo( Name: wireName?.Name ?? string.Empty, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFactoryConfigTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFactoryConfigTests.cs new file mode 100644 index 0000000..5e2dbe5 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFactoryConfigTests.cs @@ -0,0 +1,182 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// Regression coverage for the FOCAS driver factory and fixed-tree capability +/// bootstrap — Driver.FOCAS-001 (config sections dropped) and Driver.FOCAS-002 +/// (false-positive ProgramInfo capability). +/// +[Trait("Category", "Unit")] +public sealed class FocasFactoryConfigTests +{ + // ---- Driver.FOCAS-001: FixedTree / AlarmProjection / HandleRecycle config sections ---- + + [Fact] + public void CreateInstance_maps_FixedTree_section_onto_options() + { + const string json = """ + { + "Backend": "unimplemented", + "FixedTree": { + "Enabled": true, + "PollInterval": "00:00:00.250", + "ProgramPollInterval": "00:00:01", + "TimerPollInterval": "00:00:30" + } + } + """; + + var drv = FocasDriverFactoryExtensions.CreateInstance("drv-1", json); + + drv.Options.FixedTree.Enabled.ShouldBeTrue(); + drv.Options.FixedTree.PollInterval.ShouldBe(TimeSpan.FromMilliseconds(250)); + drv.Options.FixedTree.ProgramPollInterval.ShouldBe(TimeSpan.FromSeconds(1)); + drv.Options.FixedTree.TimerPollInterval.ShouldBe(TimeSpan.FromSeconds(30)); + } + + [Fact] + public void CreateInstance_maps_AlarmProjection_section_onto_options() + { + const string json = """ + { + "Backend": "unimplemented", + "AlarmProjection": { "Enabled": true, "PollInterval": "00:00:02" } + } + """; + + var drv = FocasDriverFactoryExtensions.CreateInstance("drv-1", json); + + drv.Options.AlarmProjection.Enabled.ShouldBeTrue(); + drv.Options.AlarmProjection.PollInterval.ShouldBe(TimeSpan.FromSeconds(2)); + } + + [Fact] + public void CreateInstance_maps_HandleRecycle_section_onto_options() + { + const string json = """ + { + "Backend": "unimplemented", + "HandleRecycle": { "Enabled": true, "Interval": "01:00:00" } + } + """; + + var drv = FocasDriverFactoryExtensions.CreateInstance("drv-1", json); + + drv.Options.HandleRecycle.Enabled.ShouldBeTrue(); + drv.Options.HandleRecycle.Interval.ShouldBe(TimeSpan.FromHours(1)); + } + + [Fact] + public void CreateInstance_round_trips_all_three_opt_in_sections_together() + { + const string json = """ + { + "Backend": "unimplemented", + "FixedTree": { "Enabled": true }, + "AlarmProjection": { "Enabled": true }, + "HandleRecycle": { "Enabled": true } + } + """; + + var drv = FocasDriverFactoryExtensions.CreateInstance("drv-1", json); + + drv.Options.FixedTree.Enabled.ShouldBeTrue(); + drv.Options.AlarmProjection.Enabled.ShouldBeTrue(); + drv.Options.HandleRecycle.Enabled.ShouldBeTrue(); + } + + [Fact] + public void CreateInstance_keeps_disabled_defaults_when_sections_absent() + { + var drv = FocasDriverFactoryExtensions.CreateInstance("drv-1", """{ "Backend": "unimplemented" }"""); + + drv.Options.FixedTree.Enabled.ShouldBeFalse(); + drv.Options.AlarmProjection.Enabled.ShouldBeFalse(); + drv.Options.HandleRecycle.Enabled.ShouldBeFalse(); + } + + [Fact] + public void CreateInstance_keeps_per_field_defaults_when_only_Enabled_supplied() + { + var defaults = new FocasFixedTreeOptions(); + var drv = FocasDriverFactoryExtensions.CreateInstance( + "drv-1", """{ "Backend": "unimplemented", "FixedTree": { "Enabled": true } }"""); + + drv.Options.FixedTree.PollInterval.ShouldBe(defaults.PollInterval); + drv.Options.FixedTree.ProgramPollInterval.ShouldBe(defaults.ProgramPollInterval); + drv.Options.FixedTree.TimerPollInterval.ShouldBe(defaults.TimerPollInterval); + } + + // ---- Driver.FOCAS-002: fixed-tree bootstrap must not declare a false ProgramInfo capability ---- + + [Fact] + public async Task FixedTree_bootstrap_marks_ProgramInfo_unsupported_when_probe_throws() + { + // A CNC series without cnc_exeprgname2 / cnc_rdopmode now surfaces as a thrown + // exception from GetProgramInfoAsync — SafeTryProbe must classify it as unsupported. + var factory = new FakeFocasClientFactory + { + Customise = () => new ProgramInfoFailingFocasClient(), + }; + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")], + Probe = new FocasProbeOptions { Enabled = false }, + FixedTree = new FocasFixedTreeOptions { Enabled = true }, + }, "drv-1", factory); + + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitForAsync( + () => drv.GetDeviceState("focas://10.0.0.5:8193")?.FixedTreeCache is not null, + TimeSpan.FromSeconds(2)); + + var caps = drv.GetDeviceState("focas://10.0.0.5:8193")!.FixedTreeCache!.Capabilities; + caps.ProgramInfo.ShouldBeFalse(); + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task FixedTree_bootstrap_marks_ProgramInfo_supported_when_probe_succeeds() + { + var factory = new FakeFocasClientFactory(); + var drv = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions("focas://10.0.0.5:8193")], + Probe = new FocasProbeOptions { Enabled = false }, + FixedTree = new FocasFixedTreeOptions { Enabled = true }, + }, "drv-1", factory); + + await drv.InitializeAsync("{}", CancellationToken.None); + await WaitForAsync( + () => drv.GetDeviceState("focas://10.0.0.5:8193")?.FixedTreeCache is not null, + TimeSpan.FromSeconds(2)); + + var caps = drv.GetDeviceState("focas://10.0.0.5:8193")!.FixedTreeCache!.Capabilities; + caps.ProgramInfo.ShouldBeTrue(); + await drv.ShutdownAsync(CancellationToken.None); + } + + // ---- helpers ---- + + private static async Task WaitForAsync(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (!condition() && DateTime.UtcNow < deadline) + await Task.Delay(20); + } + + /// + /// Fake whose throws — mirrors the fixed + /// behaviour on a series + /// that answers EW_FUNC / EW_NOOPT for cnc_exeprgname2 / cnc_rdopmode. + /// + private sealed class ProgramInfoFailingFocasClient : FakeFocasClient + { + public override Task GetProgramInfoAsync(CancellationToken ct) => + throw new InvalidOperationException( + "cnc_exeprgname2 failed EW_6 and cnc_rdopmode failed EW_6."); + } +}