From 8b10d327281e4e3dfdffb687439b02f71cb7015c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 6 Aug 2026 21:09:05 -0400 Subject: [PATCH] fix(focas): disambiguate duplicate sibling axis/spindle names in the fixed tree (#548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A CNC can report duplicate sibling names — the wonder Makino returns two axes both called AA — which made the Axes/ subtree untrustworthy: the browser offered two identical folders the commit gate had to refuse, and the poll loop's name-keyed snapshot cache let the second axis silently overwrite the first. Spindles and servo loads had the same collision in their own keying, plus a resolve-to-first-match on the spindle read path. Scheme: every member of a collision set gets a 1-based occurrence-ordinal suffix in slot order (AA_1, AA_2); unique names stay bare. Chosen over the alternatives deliberately: - Names, not slot numbers, stay the primary identity — the segment map is re-derived from the live device each session, so a name-keyed path follows the physical axis across config changes, where a slot-qualified path would silently rebind on any axis-list shift and rename every axis on every machine for one pathological CNC. - The WHOLE collision set is suffixed: bare AA ceases to exist when ambiguous, so a tag authored against it reads BadNodeIdUnknown rather than one arbitrary axis's data — the #534/#540 silent-wrongness class. - Occurrence ordinal, not absolute slot: inserting/removing an unrelated axis never renames an existing duplicate pair. If the collision-set membership itself changes, segments change on the next browse and stale authored segments read Bad, never another axis's data. One pure helper (FixedTreeNames.Disambiguate, collision-proof against genuine names by suffix escalation) feeds FocasFixedTreeCache.AxisSegments / SpindleSegments, and the emitter, axis snapshot keys, servo-load keys (cnc_rdsvmeter answers in slot order, so the k-th AA load belongs to the k-th AA axis) and the spindle read resolution all consume those — they can no longer disagree. The nameless-spindle S{n} fallback applies before disambiguation so a fallback name colliding with a real one is handled like any other duplicate. Tests are red-first on the pre-fix driver (all five driver-level tests fail on master's code): browse emits AA_1/AA_2 and no bare AA, each duplicate reads its own position and servo load, duplicate spindles resolve per slot, bare AA reads Bad. The wire half runs live against focas-mock (duplicate names survive the managed wire parse and publish as suffixed segments); the mock's dynamic store is name-keyed so the distinct-values half stays with the offline tests. FOCAS 327/327, integration 11/11, FOCAS.Cli 52, AdminUI 1113, ControlPlane 231. Closes #548. Claude-Session: https://claude.ai/code/session_014WNM4vjoVksyyBraTXSZE1 --- docs/drivers/FOCAS.md | 17 +- .../FixedTreeNames.cs | 95 ++++++++ .../FocasDriver.cs | 82 ++++--- .../Series/WireBackendDuplicateAxisTests.cs | 99 +++++++++ .../FixedTreeNamesTests.cs | 70 ++++++ .../FocasDuplicateSiblingNameTests.cs | 203 ++++++++++++++++++ 6 files changed, 536 insertions(+), 30 deletions(-) create mode 100644 src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FixedTreeNames.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendDuplicateAxisTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FixedTreeNamesTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDuplicateSiblingNameTests.cs diff --git a/docs/drivers/FOCAS.md b/docs/drivers/FOCAS.md index 326c11ae..caa1d29b 100644 --- a/docs/drivers/FOCAS.md +++ b/docs/drivers/FOCAS.md @@ -187,11 +187,18 @@ fixed-tree paths are discovered from the machine, not composed from parts. Two caveats worth knowing before you browse: -- **A real CNC can report duplicate sibling names.** One observed Makino returns - two axis folders both called `AA`. Committing both is refused at commit time - (they would be the same RawPath), and the runtime cache is keyed by the same - name — so the `Axes/` subtree on such a machine cannot currently distinguish - them. Prefer the non-axis subtrees there until this is resolved. +- **A real CNC can report duplicate sibling names, and they are disambiguated + automatically** (#548). One observed Makino returns two axes both called `AA`; + every member of such a collision set gets a 1-based occurrence suffix in slot + order — the tree offers `Axes/AA_1/` and `Axes/AA_2/`, each reading its own + axis — while unique names stay bare (`Axes/Y/`). The bare name of a duplicate + set deliberately does not resolve: a tag authored against `Axes/AA/…` on such + a machine reads `BadNodeIdUnknown` rather than one arbitrary axis's data. The + suffix counts occurrences of the *same* name, so adding an unrelated axis + never renames an existing pair — but if the collision-set membership itself + changes (a third `AA` appears, or one of two is removed), the segments change + on the next browse, and previously committed tags on the old segments read Bad + until re-committed. The same rule covers spindles. - **Values are dated from the last answered poll, not from the read.** If a CNC stops answering, its fixed-tree values go `BadCommunicationError` rather than continuing to serve the last snapshot as `Good`; the budget is eight poll diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FixedTreeNames.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FixedTreeNames.cs new file mode 100644 index 00000000..2666310d --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FixedTreeNames.cs @@ -0,0 +1,95 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +/// +/// Turns a device-reported sibling-name sequence (axes, spindles) into unique fixed-tree +/// path segments (Gitea #548). +/// +/// +/// +/// A real CNC can report duplicate sibling names — one observed Makino returns two axis +/// folders both called AA. Before this existed, the emitter offered two identical +/// browse folders, commit-time collision detection refused to author both (same RawPath), +/// and the poll loop's name-keyed snapshot cache let the second axis silently overwrite the +/// first — so the Axes/ subtree was untrustworthy on such a machine. +/// +/// +/// Scheme: every member of a collision set gets a 1-based occurrence-ordinal suffix in +/// slot order (AA_1, AA_2); unique names stay bare. Three deliberate +/// choices inside that: +/// +/// +/// Names stay the primary identity, not slot numbers. The segment map is re-derived +/// from the live device at every session bootstrap, so a name-keyed path follows the +/// physical axis across configuration changes; a slot-qualified path (3_AA) would +/// silently rebind to a different physical axis whenever the axis list shifts, and would +/// rename every axis path on every machine for the sake of the rare duplicate. +/// +/// +/// The whole collision set is suffixed, not just the second occurrence. When two +/// axes are both called AA, the bare segment AA ceases to exist — a bare name +/// that silently means "one of the two" is the same silent-wrongness class #534 and #540 +/// were about. A tag authored against bare AA on such a machine reads Bad, never one +/// arbitrary axis's data. +/// +/// +/// Occurrence ordinal, not absolute slot number. The ordinal is stable under +/// insertion/removal of unrelated axes (a new B axis ahead of the two AAs +/// renames nothing; their slot numbers would both change). If the collision-set membership +/// itself changes — a third AA appears, or one of two is removed — the segments do +/// change; that surfaces on the next browse + commit, and until then the stale authored +/// segment reads Bad rather than another axis's data (the served tree only changes on a +/// config redeploy). +/// +/// +/// Suffix collisions with genuine names are structurally impossible for axes (FOCAS axis +/// displays are at most two characters — name + suffix — so they cannot contain _), +/// but the generator still checks: a generated segment that would collide with anything +/// else in the set escalates its ordinal until unique, so the output is always +/// collision-free whatever the input. +/// +/// +internal static class FixedTreeNames +{ + /// + /// Maps each name in (in slot order) to a unique sibling path + /// segment. Unique names map to themselves; duplicated names map to + /// {name}_{occurrence}. Comparison is case-insensitive, matching the fixed-tree + /// snapshot cache and read-path lookups. + /// + /// Device-reported sibling names, in device (slot) order. + /// One segment per input name, unique within the returned list. + public static IReadOnlyList Disambiguate(IEnumerable names) + { + var list = names as IReadOnlyList ?? [.. names]; + + var counts = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var name in list) + counts[name] = counts.TryGetValue(name, out var c) ? c + 1 : 1; + + // Bare (unique) names are claimed first so a generated "{name}_{k}" can never shadow one. + var used = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var name in list) + if (counts[name] == 1) + used.Add(name); + + var nextOrdinal = new Dictionary(StringComparer.OrdinalIgnoreCase); + var result = new string[list.Count]; + for (var i = 0; i < list.Count; i++) + { + var name = list[i]; + if (counts[name] == 1) + { + result[i] = name; + continue; + } + + var k = nextOrdinal.TryGetValue(name, out var n) ? n : 1; + string segment; + do { segment = $"{name}_{k}"; k++; } + while (!used.Add(segment)); + nextOrdinal[name] = k; + result[i] = segment; + } + return result; + } +} 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 d06529be..8c9aabfc 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -609,15 +609,19 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, EmitIdentityVariable(identity, "AxisCount", FocasDriverDataType.Int32); var axesFolder = deviceFolder.Folder("Axes", "Axes"); - foreach (var axis in cache.Axes) + for (var a = 0; a < cache.Axes.Count; a++) { - var axisFolder = axesFolder.Folder(axis.Display, axis.Display); - EmitAxisVariable(axisFolder, axis.Display, "AbsolutePosition"); - EmitAxisVariable(axisFolder, axis.Display, "MachinePosition"); - EmitAxisVariable(axisFolder, axis.Display, "RelativePosition"); - EmitAxisVariable(axisFolder, axis.Display, "DistanceToGo"); + // The disambiguated segment, not the raw Display — a CNC reporting two + // axes both named AA gets AA_1 / AA_2 folders instead of two identical + // siblings the commit gate must refuse (#548). + var segment = cache.AxisSegments[a]; + var axisFolder = axesFolder.Folder(segment, segment); + EmitAxisVariable(axisFolder, segment, "AbsolutePosition"); + EmitAxisVariable(axisFolder, segment, "MachinePosition"); + EmitAxisVariable(axisFolder, segment, "RelativePosition"); + EmitAxisVariable(axisFolder, segment, "DistanceToGo"); if (cache.Capabilities.ServoLoad) - EmitAxisVariable(axisFolder, axis.Display, "ServoLoad"); + EmitAxisVariable(axisFolder, segment, "ServoLoad"); } EmitAxisVariable(axesFolder, "FeedRate", "Actual"); EmitAxisVariable(axesFolder, "SpindleSpeed", "Actual"); @@ -630,8 +634,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, var spindleRoot = deviceFolder.Folder("Spindle", "Spindle"); for (var i = 0; i < cache.Spindles.Count; i++) { - var s = cache.Spindles[i]; - var name = string.IsNullOrEmpty(s.Display) ? $"S{i + 1}" : s.Display; + var name = cache.SpindleSegments[i]; var spindleFolder = spindleRoot.Folder(name, name); if (cache.Capabilities.SpindleLoad) EmitFixedVariable(spindleFolder, $"Spindle/{name}", "Load", DriverDataType.Int32); @@ -977,9 +980,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, for (var i = 0; i < cache.Axes.Count; i++) { var axisIndex = i + 1; // FOCAS uses 1-based axis indexing - var axis = cache.Axes[i]; var snap = await client.ReadDynamicAsync(axisIndex, ct).ConfigureAwait(false); - PublishAxisSnapshot(state, axis, snap, i); // i = 0-based axis index → PositionFigures lookup + // Keyed by the disambiguated segment, matching what DiscoverAsync emitted — + // two same-named axes must not share (and overwrite) one cache slot (#548). + PublishAxisSnapshot(state, cache.AxisSegments[i], snap, i); // i = 0-based axis index → PositionFigures lookup if (i == 0) { firstAxisSnap = snap; PublishRateSnapshot(state, snap); } } @@ -1082,7 +1086,7 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// → path, which hits /// and returns these cached values. /// - private static void PublishAxisSnapshot(DeviceState state, FocasAxisName axis, FocasDynamicSnapshot snap, int axisIndex) + private static void PublishAxisSnapshot(DeviceState state, string axisSegment, FocasDynamicSnapshot snap, int axisIndex) { var host = state.Options.HostAddress; // cnc_rddynamic2 returns positions as scaled integers; divide by 10^figures so they @@ -1094,10 +1098,10 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, // via ReadPositionFiguresAsync; the config knob is the per-axis fallback only. FeedRate / // SpindleSpeed (rate snapshot) and ServoLoad are NOT position-scaled (published elsewhere). var factor = AxisFactor(state, axisIndex); - state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axis.Display}/AbsolutePosition")] = snap.AbsolutePosition / factor; - state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axis.Display}/MachinePosition")] = snap.MachinePosition / factor; - state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axis.Display}/RelativePosition")] = snap.RelativePosition / factor; - state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axis.Display}/DistanceToGo")] = snap.DistanceToGo / factor; + state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axisSegment}/AbsolutePosition")] = snap.AbsolutePosition / factor; + state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axisSegment}/MachinePosition")] = snap.MachinePosition / factor; + state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axisSegment}/RelativePosition")] = snap.RelativePosition / factor; + state.LastFixedSnapshots[FixedTreeReference(host, $"Axes/{axisSegment}/DistanceToGo")] = snap.DistanceToGo / factor; } /// @@ -1125,14 +1129,22 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, } /// - /// Cache servo-load percentages keyed by axis name. Stored separately from + /// Cache servo-load percentages keyed by axis path segment. Stored separately from /// LastFixedSnapshots (which is keyed by full-reference) because the - /// servo-load read path resolves by axis name. + /// servo-load read path resolves by axis segment. /// + /// + /// The device-reported load names run through the same disambiguation as the axis list + /// (#548): cnc_rdsvmeter answers per-axis in slot order, so the k-th load named + /// AA belongs to the k-th axis named AA and lands under the same + /// AA_k segment the emitter published — without this, two same-named axes + /// collapsed onto one dictionary entry and the second overwrote the first. + /// private static void PublishServoLoads(DeviceState state, IReadOnlyList loads) { - foreach (var load in loads) - state.LastServoLoads[load.AxisName] = load.LoadPercent; + var segments = FixedTreeNames.Disambiguate(loads.Select(l => l.AxisName)); + for (var i = 0; i < loads.Count; i++) + state.LastServoLoads[segments[i]] = loads[i].LoadPercent; } private static object? TimerValue(DeviceState state, FocasTimerKind kind) => @@ -1235,11 +1247,11 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, var spindleName = tail[..slash]; var field = tail[(slash + 1)..]; var idx = -1; - for (var i = 0; i < spindleCache.Spindles.Count; i++) + for (var i = 0; i < spindleCache.SpindleSegments.Count; i++) { - var s = spindleCache.Spindles[i]; - var display = string.IsNullOrEmpty(s.Display) ? $"S{i + 1}" : s.Display; - if (string.Equals(display, spindleName, StringComparison.OrdinalIgnoreCase)) { idx = i; break; } + // Match on the disambiguated segment the emitter published (#548) — matching + // raw Display would resolve every duplicate name to the first spindle. + if (string.Equals(spindleCache.SpindleSegments[i], spindleName, StringComparison.OrdinalIgnoreCase)) { idx = i; break; } } if (idx >= 0) { @@ -1462,7 +1474,27 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, IReadOnlyList Axes, IReadOnlyList Spindles, IReadOnlyList SpindleMaxRpms, - FocasFixedTreeCapabilities Capabilities); + FocasFixedTreeCapabilities Capabilities) + { + /// + /// Unique Axes/{segment} path segments, one per axis in slot order — bare + /// display names when unique, occurrence-suffixed (AA_1, AA_2) when a + /// CNC reports duplicate sibling axis names (#548). The emitter, the poll loop's + /// snapshot keys and the read path all use THIS, never Display directly, so + /// they cannot disagree. + /// + public IReadOnlyList AxisSegments { get; } = + FixedTreeNames.Disambiguate(Axes.Select(a => a.Display)); + + /// + /// Unique Spindle/{segment} path segments, one per spindle in slot order. The + /// nameless-spindle fallback (S{n}) is applied BEFORE disambiguation so a + /// fallback name colliding with a real one is disambiguated like any other duplicate. + /// + public IReadOnlyList SpindleSegments { get; } = + FixedTreeNames.Disambiguate(Spindles.Select( + (s, i) => string.IsNullOrEmpty(s.Display) ? $"S{i + 1}" : s.Display)); + } /// /// Per-device optional-API capability flags — which of the "this may or may not diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendDuplicateAxisTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendDuplicateAxisTests.cs new file mode 100644 index 00000000..7896d588 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests/Series/WireBackendDuplicateAxisTests.cs @@ -0,0 +1,99 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.IntegrationTests.Series; + +/// +/// Gitea #548, wire half — duplicate sibling axis names must survive the managed wire +/// client's cnc_rdaxisname parse (no dedupe, no error) and come out of the driver's +/// bootstrap as disambiguated segments, exercised against the real focas-mock over TCP. +/// +/// +/// The mock's own dynamic store is keyed by axis NAME, so it cannot serve two duplicate +/// axes distinct position values — that half of #548 is pinned by the offline +/// FocasDuplicateSiblingNameTests, which drive slot-distinct snapshots through the +/// fake client. What only this test proves is the wire path: the duplicate names reach the +/// driver intact and the published snapshot keys are the suffixed segments. +/// +[Collection(FocasSimCollection.Name)] +public sealed class WireBackendDuplicateAxisTests +{ + private readonly FocasSimFixture _fx; + + /// Initializes a new instance of the class. + /// The FOCAS simulator fixture. + public WireBackendDuplicateAxisTests(FocasSimFixture fx) => _fx = fx; + + private string DeviceHost => _fx.DeviceUri; + + /// Duplicate axis names arrive intact off the wire and publish as suffixed segments. + [Fact] + public async Task Duplicate_axis_names_disambiguate_through_the_wire_backend() + { + if (_fx.SkipReason is not null) Assert.Skip(_fx.SkipReason); + var ct = TestContext.Current.CancellationToken; + + await _fx.LoadProfileAsync("FWLIB64", ct); + await _fx.PatchStateAsync(new + { + sysinfo = new + { + addinfo = 0, max_axis = 8, cnc_type = "M ", mt_type = "M ", + series = "30i ", version = "A1.0", axes = "3 ", + }, + // The Makino shape: two axes rendering the same display name, one unique between them. + axis_names = new[] { "AA", "Y", "AA" }, + }, ct); + + var driver = new FocasDriver(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(DeviceHost)], + RawTags = [], + Probe = new FocasProbeOptions { Enabled = false }, + FixedTree = new FocasFixedTreeOptions + { + Enabled = true, + PollInterval = TimeSpan.FromMilliseconds(100), + ProgramPollInterval = TimeSpan.Zero, + TimerPollInterval = TimeSpan.Zero, + }, + }, driverInstanceId: "focas-wire-dup-axes", clientFactory: new WireFocasClientFactory()); + + await using (driver) + { + await driver.InitializeAsync("{}", ct); + + await WaitFor(() => + driver.GetDeviceState(DeviceHost) is { FixedTreeCache: not null }, TimeSpan.FromSeconds(5)); + + var state = driver.GetDeviceState(DeviceHost); + state.ShouldNotBeNull(); + state.FixedTreeCache.ShouldNotBeNull(); + + // The wire client must not collapse or drop the duplicates... + state.FixedTreeCache.Axes.Count.ShouldBe(3); + state.FixedTreeCache.Axes.Select(a => a.Display).ShouldBe(["AA", "Y", "AA"]); + // ...and the driver must disambiguate them. + state.FixedTreeCache.AxisSegments.ShouldBe(["AA_1", "Y", "AA_2"]); + + // Both duplicate axes publish their own snapshot slot; nothing publishes bare AA. + await WaitFor(() => + state.LastFixedSnapshots.ContainsKey($"{DeviceHost}/Axes/AA_2/AbsolutePosition"), + TimeSpan.FromSeconds(3)); + state.LastFixedSnapshots.ContainsKey($"{DeviceHost}/Axes/AA_1/AbsolutePosition").ShouldBeTrue(); + state.LastFixedSnapshots.ContainsKey($"{DeviceHost}/Axes/Y/AbsolutePosition").ShouldBeTrue(); + state.LastFixedSnapshots.ContainsKey($"{DeviceHost}/Axes/AA/AbsolutePosition").ShouldBeFalse(); + } + } + + private static async Task WaitFor(Func pred, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (DateTime.UtcNow < deadline) + { + if (pred()) return; + await Task.Delay(50); + } + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FixedTreeNamesTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FixedTreeNamesTests.cs new file mode 100644 index 00000000..0bd20b71 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FixedTreeNamesTests.cs @@ -0,0 +1,70 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// Gitea #548 — the sibling-name disambiguator that makes a CNC reporting two axes both +/// called AA publishable as AA_1 / AA_2. +/// +[Trait("Category", "Unit")] +public sealed class FixedTreeNamesTests +{ + [Fact] + public void Unique_names_pass_through_unchanged() + { + FixedTreeNames.Disambiguate(["X", "Y", "Z"]).ShouldBe(["X", "Y", "Z"]); + } + + [Fact] + public void Every_member_of_a_collision_set_is_suffixed_in_slot_order() + { + // The whole set, not just the second occurrence: bare "AA" must cease to exist when + // ambiguous, or it would silently mean "one of the two". + FixedTreeNames.Disambiguate(["AA", "Y", "AA"]).ShouldBe(["AA_1", "Y", "AA_2"]); + } + + [Fact] + public void An_unrelated_name_between_duplicates_does_not_shift_their_ordinals() + { + // The ordinal counts occurrences of the SAME name, not positions in the list — adding an + // unrelated axis never renames an existing duplicate pair. + FixedTreeNames.Disambiguate(["B", "AA", "Y", "AA"]).ShouldBe(["B", "AA_1", "Y", "AA_2"]); + } + + [Fact] + public void Duplicate_grouping_is_case_insensitive() + { + // The snapshot cache and the read path both compare OrdinalIgnoreCase, so "Aa" and "AA" + // would collide there — they must be treated as one collision set here too. + FixedTreeNames.Disambiguate(["Aa", "AA"]).ShouldBe(["Aa_1", "AA_2"]); + } + + [Fact] + public void A_generated_segment_never_collides_with_a_genuine_name() + { + // Pathological but must not silently alias: a real sibling already named "S_1" claims + // that segment, so the duplicate set escalates past it. + FixedTreeNames.Disambiguate(["S_1", "S", "S"]).ShouldBe(["S_1", "S_2", "S_3"]); + } + + [Fact] + public void Three_way_collisions_number_sequentially() + { + FixedTreeNames.Disambiguate(["AA", "AA", "AA"]).ShouldBe(["AA_1", "AA_2", "AA_3"]); + } + + [Fact] + public void The_output_is_always_unique() + { + var segments = FixedTreeNames.Disambiguate(["A", "A_1", "A", "A_2", "A"]); + segments.Distinct(StringComparer.OrdinalIgnoreCase).Count().ShouldBe(segments.Count); + } + + [Fact] + public void An_empty_list_yields_an_empty_list() + { + FixedTreeNames.Disambiguate([]).ShouldBeEmpty(); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDuplicateSiblingNameTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDuplicateSiblingNameTests.cs new file mode 100644 index 00000000..87bc65f4 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasDuplicateSiblingNameTests.cs @@ -0,0 +1,203 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// Gitea #548 — a CNC reporting duplicate sibling axis names (the wonder Makino returns two +/// axis folders both called AA) made the Axes/ subtree untrustworthy: the +/// browser offered two identical folders commit-time collision detection had to refuse, and +/// the poll loop's name-keyed snapshot cache let the second axis silently overwrite the +/// first. Duplicated sibling names are now occurrence-suffixed (AA_1, AA_2) +/// everywhere — emitter, snapshot keys, servo loads, spindles — so both axes are +/// authorable and each reads its own axis's data. +/// +[Trait("Category", "Unit")] +public sealed class FocasDuplicateSiblingNameTests +{ + private const string Host = "focas://10.0.0.5:8193"; + + /// A fake modelled on the Makino: axes AA, Y, AA (slots 1–3) with distinct data. + private static FakeFocasClientFactory MakinoFactory() => new() + { + Customise = () => + { + var c = new FakeFocasClient(); + c.AxisNames.Clear(); + c.AxisNames.AddRange([new("A", "A"), new("Y", ""), new("A", "A")]); + c.DynamicByAxis[1] = new FocasDynamicSnapshot(1, 0, 0, 0, 0, 0, 0, 111, 0, 0, 0); + c.DynamicByAxis[2] = new FocasDynamicSnapshot(2, 0, 0, 0, 0, 0, 0, 222, 0, 0, 0); + c.DynamicByAxis[3] = new FocasDynamicSnapshot(3, 0, 0, 0, 0, 0, 0, 333, 0, 0, 0); + c.ServoLoads.AddRange([new("AA", 10), new("Y", 20), new("AA", 30)]); + // Two spindles whose displays also collide, with distinct data per slot. + c.SpindleNames.Clear(); + c.SpindleNames.AddRange([new("S", "1", "", ""), new("S", "1", "", "")]); + c.SpindleLoads.AddRange([5, 7]); + c.SpindleMaxRpms.AddRange([8_000, 12_000]); + return c; + }, + }; + + private static FocasDriver NewDriver(params FocasTagDefinition[] tags) => + new(new FocasDriverOptions + { + Devices = [new FocasDeviceOptions(Host)], + Probe = new FocasProbeOptions { Enabled = false }, + FixedTree = new FocasFixedTreeOptions { Enabled = true }, + RawTags = FocasRawTags.Entries(tags), + }, "drv-1", MakinoFactory()); + + private static async Task StartedDriverAsync(params FocasTagDefinition[] tags) + { + var drv = NewDriver(tags); + await drv.InitializeAsync("{}", CancellationToken.None); + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(5); + while (DateTime.UtcNow < deadline && drv.GetDeviceState(Host)?.LastFixedPollUtc is null) + await Task.Delay(25, TestContext.Current.CancellationToken); + drv.GetDeviceState(Host)!.LastFixedPollUtc.ShouldNotBeNull("the fixed-tree loop never completed a poll"); + return drv; + } + + [Fact] + public async Task Duplicate_axes_browse_as_distinct_suffixed_folders() + { + var drv = await StartedDriverAsync(); + var builder = new RecordingBuilder(); + await drv.DiscoverAsync(builder, CancellationToken.None); + + var folders = builder.Folders.Select(f => f.BrowseName).ToList(); + folders.ShouldContain("AA_1"); + folders.ShouldContain("AA_2"); + folders.ShouldContain("Y"); + folders.ShouldNotContain("AA", customMessage: + "a bare AA folder would silently mean one of the two duplicate axes"); + // Unique names stay bare — the disambiguation must not rename the rest of the machine. + folders.ShouldNotContain("Y_1"); + + // The emitted leaves are distinct, parseable addresses (the #540 invariant, held here + // for the duplicate set specifically). + var axisLeaves = builder.Variables + .Select(v => v.Info.FullName) + .Where(n => n.StartsWith("FIXED:Axes/AA", StringComparison.Ordinal)) + .ToList(); + axisLeaves.ShouldNotBeEmpty(); + axisLeaves.ShouldAllBe(n => FocasAddress.TryParse(n) != null); + axisLeaves.Distinct(StringComparer.OrdinalIgnoreCase).Count().ShouldBe(axisLeaves.Count); + axisLeaves.ShouldContain("FIXED:Axes/AA_1/AbsolutePosition"); + axisLeaves.ShouldContain("FIXED:Axes/AA_2/AbsolutePosition"); + + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task Each_duplicate_axis_reads_its_own_position() + { + // The runtime half of #548: before disambiguation the second AA overwrote the first's + // snapshot slot, so even a hand-authored tag could only ever see one axis. + var drv = await StartedDriverAsync( + new FocasTagDefinition("raw/aa1", Host, "FIXED:Axes/AA_1/AbsolutePosition", FocasDataType.Float64, Writable: false), + new FocasTagDefinition("raw/aa2", Host, "FIXED:Axes/AA_2/AbsolutePosition", FocasDataType.Float64, Writable: false), + new FocasTagDefinition("raw/y", Host, "FIXED:Axes/Y/AbsolutePosition", FocasDataType.Float64, Writable: false)); + + var read = await drv.ReadAsync(["raw/aa1", "raw/aa2", "raw/y"], CancellationToken.None); + + read[0].StatusCode.ShouldBe(FocasStatusMapper.Good); + read[0].Value.ShouldBe(111.0); // slot 1's AA + read[1].StatusCode.ShouldBe(FocasStatusMapper.Good); + read[1].Value.ShouldBe(333.0); // slot 3's AA — NOT slot 1's overwritten value + read[2].StatusCode.ShouldBe(FocasStatusMapper.Good); + read[2].Value.ShouldBe(222.0); // the unique axis, untouched by disambiguation + + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task The_bare_name_of_a_duplicate_set_reads_Bad_rather_than_one_arbitrary_axis() + { + // Whole-set suffixing: when two axes are both AA, bare AA ceases to exist. Resolving it + // to either axis would be the same silent-wrongness class #534/#540 were about. + var drv = await StartedDriverAsync( + new FocasTagDefinition("raw/bare", Host, "FIXED:Axes/AA/AbsolutePosition", FocasDataType.Float64, Writable: false)); + + var read = await drv.ReadAsync(["raw/bare"], CancellationToken.None); + + read[0].StatusCode.ShouldBe(FocasStatusMapper.BadNodeIdUnknown); + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task Servo_loads_resolve_per_duplicate_axis() + { + // cnc_rdsvmeter answers per-axis in slot order, so the k-th AA load belongs to the k-th + // AA axis. Before #548 both landed on one name-keyed dictionary entry, last write wins. + var drv = await StartedDriverAsync( + new FocasTagDefinition("raw/load1", Host, "FIXED:Axes/AA_1/ServoLoad", FocasDataType.Float64, Writable: false), + new FocasTagDefinition("raw/load2", Host, "FIXED:Axes/AA_2/ServoLoad", FocasDataType.Float64, Writable: false)); + + // The servo-load pass runs after the axis pass inside the same tick; poll until both + // distinct keys are populated rather than racing it. + var state = drv.GetDeviceState(Host)!; + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(5); + while (DateTime.UtcNow < deadline && state.LastServoLoads.Count < 3) + await Task.Delay(25, TestContext.Current.CancellationToken); + + var read = await drv.ReadAsync(["raw/load1", "raw/load2"], CancellationToken.None); + + read[0].StatusCode.ShouldBe(FocasStatusMapper.Good); + read[0].Value.ShouldBe(10.0); + read[1].StatusCode.ShouldBe(FocasStatusMapper.Good); + read[1].Value.ShouldBe(30.0); + + await drv.ShutdownAsync(CancellationToken.None); + } + + [Fact] + public async Task Duplicate_spindles_resolve_per_slot() + { + // The spindle read path matched by display name and took the FIRST index that matched, + // so a duplicate spindle name made the second spindle's data unreachable. + var drv = await StartedDriverAsync( + new FocasTagDefinition("raw/s1load", Host, "FIXED:Spindle/S1_1/Load", FocasDataType.Int32, Writable: false), + new FocasTagDefinition("raw/s2load", Host, "FIXED:Spindle/S1_2/Load", FocasDataType.Int32, Writable: false), + new FocasTagDefinition("raw/s1max", Host, "FIXED:Spindle/S1_1/MaxRpm", FocasDataType.Int32, Writable: false), + new FocasTagDefinition("raw/s2max", Host, "FIXED:Spindle/S1_2/MaxRpm", FocasDataType.Int32, Writable: false)); + + var read = await drv.ReadAsync(["raw/s1load", "raw/s2load", "raw/s1max", "raw/s2max"], CancellationToken.None); + + read[0].Value.ShouldBe(5); + read[1].Value.ShouldBe(7); + read[2].Value.ShouldBe(8_000); + read[3].Value.ShouldBe(12_000); + read.ShouldAllBe(r => r.StatusCode == FocasStatusMapper.Good); + + await drv.ShutdownAsync(CancellationToken.None); + } + + /// Records what DiscoverAsync emits, flattening the folder tree. + private sealed class RecordingBuilder : IAddressSpaceBuilder + { + public List<(string BrowseName, string DisplayName)> Folders { get; } = []; + public List<(string BrowseName, DriverAttributeInfo Info)> Variables { get; } = []; + + public IAddressSpaceBuilder Folder(string browseName, string displayName) + { Folders.Add((browseName, displayName)); return this; } + + public IVariableHandle Variable(string browseName, string displayName, DriverAttributeInfo info) + { Variables.Add((browseName, info)); return new Handle(info.FullName); } + + public void AddProperty(string name, DriverDataType type, object? value) { } + + private sealed class Handle(string fullRef) : IVariableHandle + { + public string FullReference => fullRef; + public IAlarmConditionSink MarkAsAlarmCondition(AlarmConditionInfo info) => new NullSink(); + } + + private sealed class NullSink : IAlarmConditionSink + { + public void OnTransition(AlarmEventArgs args) { } + } + } +}