dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(driver-focas): resolve High code-review findings (Driver.FOCAS-001, Driver.FOCAS-002)

Browse Source 
Driver.FOCAS-001: FocasDriverConfigDto exposed no FixedTree / AlarmProjection /
HandleRecycle sections, so a deployment that opted into those features per
docs/drivers/FOCAS.md had the sections silently dropped by case-insensitive
JSON parsing and the features stayed at their disabled defaults. Added
FocasFixedTreeDto / FocasAlarmProjectionDto / FocasHandleRecycleDto and Build*
mappers in CreateInstance that populate the matching FocasDriverOptions
properties; a missing section or field keeps its existing default.

Driver.FOCAS-002: the fixed-tree bootstrap probe classified ProgramInfo as
"supported" whenever GetProgramInfoAsync returned non-null, but WireFocasClient
.GetProgramInfoAsync substituted defaults instead of throwing on a FOCAS error
return, so a CNC series answering EW_FUNC/EW_NOOPT for cnc_exeprgname2 /
cnc_rdopmode still got the Program/ and OperationMode/ subtrees. The method now
throws InvalidOperationException when neither the program-name nor the op-mode
read is IsOk, so SafeTryProbe correctly suppresses the capability.

Added FocasFactoryConfigTests covering the three opt-in config sections
round-tripping through CreateInstance and the fixed-tree bootstrap classifying
ProgramInfo as unsupported when the probe throws. Added an internal
FocasDriver.Options test seam.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 06:34:03 -04:00
parent ebc0511c72
commit c9e446387a
5 changed files with 282 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
code-reviews/Driver.FOCAS/findings.md
View File

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

3
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs
View File

@@ -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;
/// <summary>Test seam — the resolved options the factory built this driver from.</summary>
internal FocasDriverOptions Options => _options;
// ---- IReadable ----
public async Task<IReadOnlyList<DataValueSnapshot>> ReadAsync(

82
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs
View File

@@ -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
};
}
/// <summary>
/// Map the optional <c>FixedTree</c> config section onto <see cref="FocasFixedTreeOptions"/>.
/// 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
/// <see cref="TimeSpan"/> strings (e.g. <c>"00:00:00.250"</c>) per docs/drivers/FOCAS.md.
/// </summary>
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,
};
}
/// <summary>
/// Map the optional <c>AlarmProjection</c> config section onto
/// <see cref="FocasAlarmProjectionOptions"/>. Missing section keeps the disabled default.
/// </summary>
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,
};
}
/// <summary>
/// Map the optional <c>HandleRecycle</c> config section onto
/// <see cref="FocasHandleRecycleOptions"/>. Missing section keeps the disabled default.
/// </summary>
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<FocasDeviceDto>? Devices { get; init; }
public List<FocasTagDto>? 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; }
}
/// <summary>
/// Optional <c>FixedTree</c> config section. Interval fields are JSON
/// <see cref="TimeSpan"/> strings (<c>"hh:mm:ss[.fff]"</c>) — System.Text.Json
/// parses these natively.
/// </summary>
internal sealed class FocasFixedTreeDto
{
public bool? Enabled { get; init; }
public TimeSpan? PollInterval { get; init; }
public TimeSpan? ProgramPollInterval { get; init; }
public TimeSpan? TimerPollInterval { get; init; }
}
/// <summary>Optional <c>AlarmProjection</c> config section.</summary>
internal sealed class FocasAlarmProjectionDto
{
public bool? Enabled { get; init; }
public TimeSpan? PollInterval { get; init; }
}
/// <summary>Optional <c>HandleRecycle</c> config section.</summary>
internal sealed class FocasHandleRecycleDto
{
public bool? Enabled { get; init; }
public TimeSpan? Interval { get; init; }
}
}

10
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/WireFocasClient.cs
View File

@@ -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,

182
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasFactoryConfigTests.cs Normal file
View File

@@ -0,0 +1,182 @@
using Shouldly;
using Xunit;
using ZB.MOM.WW.OtOpcUa.Driver.FOCAS;
namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests;
/// <summary>
/// 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).
/// </summary>
[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<bool> condition, TimeSpan timeout)
{
var deadline = DateTime.UtcNow + timeout;
while (!condition() && DateTime.UtcNow < deadline)
await Task.Delay(20);
}
/// <summary>
/// Fake whose <see cref="GetProgramInfoAsync"/> throws — mirrors the fixed
/// <see cref="Wire.WireFocasClient.GetProgramInfoAsync"/> behaviour on a series
/// that answers EW_FUNC / EW_NOOPT for cnc_exeprgname2 / cnc_rdopmode.
/// </summary>
private sealed class ProgramInfoFailingFocasClient : FakeFocasClient
{
public override Task<FocasProgramInfo> GetProgramInfoAsync(CancellationToken ct) =>
throw new InvalidOperationException(
"cnc_exeprgname2 failed EW_6 and cnc_rdopmode failed EW_6.");
}
}