fix(browse): commit the browsed data type, and fix the probe classification the fixture was hiding (#536)
A browse-committed AB CIP / TwinCAT / FOCAS tag bound with its model's DEFAULT
data type, not the one the device reported. A browsed REAL committed as
`{"tagPath":"Pump_1_Speed","dataType":"DInt"}` — and those three drivers decode
the wire bytes by that field, not by the raw row's OPC UA DataType — so the tag
read four bytes of a float through an integer decoder and published a plausible
wrong number with no error raised anywhere. FOCAS was missed when #536 was filed.
The type is CARRIED, not derived. Reverse-mapping the leaf's driver-agnostic
DriverDataType looks like the smaller fix and is wrong: SInt/Int/DInt/USInt/UInt
all collapse into Int32 on the way out of the driver, so that version would bind
a 16-bit INT as a 32-bit DINT while appearing to fix the REAL. Drivers now state
their own type name on DriverAttributeInfo.NativeDataType, which reaches the
picker as AttributeInfo.AddressFields["dataType"] and the commit mapper from
there. A leaf that states nothing keeps the model default — the pre-#536
behaviour, kept deliberately as the fallback rather than a guess.
Numeric strings are refused before parsing: Enum.TryParse accepts "9" for any
enum and Enum.IsDefined then confirms it, so an unguarded parse would bind a tag
to a type nobody named. A test caught this in the first version of the guard.
--- and the other half, which the fixture was hiding ---
Test Connect on a device with NO authored tags — every AB CIP device at the
moment an operator first presses the button — reported "CIP handshake failed"
against a healthy controller. That was written down as a fixture limitation
("ab_server doesn't implement @raw_cpu_type"). Both halves of that were wrong:
* @raw_cpu_type is not a libplctag special tag at all (only @raw, @tags and
@udt/ are), so it goes out as an ordinary symbolic name that no controller
defines, and the controller's "tag does not exist" IS the successful answer.
* ab_server answered an unresolvable tag path with CIP 0x03 where a Logix
controller answers 0x04 — ErrorBadParam vs ErrorNotFound, which the probe
classifies oppositely. Fixed in vendored patch 0002.
* With the fixture then telling the truth, the real defect surfaced:
AbCipDriverProbe.IsReachableException matched the native C status spellings
("STATUS_NOT_FOUND"), but libplctag.NET throws with the MANAGED enum name
("ErrorNotFound"). Those substrings can never match, so the reachable
classification could not fire on the exception path — which is the path every
no-authored-tags probe takes. This was failing on real hardware too, not just
against the sim. It now recovers the Status and defers to IsReachableStatus,
so there is one classification rather than two that can drift.
Rejected: switching the fallback to @tags. libplctag builds a tag-listing tag
with NO network I/O, so it reports OK against any TCP listener — measured green
against a Mosquitto broker.
Verification: full solution suite green. Falsification runs confirm exactly the
intended tests go red — 9 for the type carry, 3 (the reachable statuses only) for
the probe classification. Live on docker-dev: browse-committed TestString/
Motor_Run/TestSINT now store String/Bool/SInt, alongside last session's
Pump_1_Speed still holding the wrong DInt. TestSINT is the falsifier — its row
DataType is Int32, identical to TestDINT's, so no reverse map could have produced
"SInt".
Known consequence: pre-existing browse-committed tags keep their wrong dataType;
the fix applies to new commits only.
Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
@@ -6,6 +6,7 @@ using ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
|
||||
using ZB.MOM.WW.OtOpcUa.Commons.Types;
|
||||
using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.AbCip;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.Mqtt;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
|
||||
@@ -98,6 +99,126 @@ public sealed class RawBrowseCommitMapperTests
|
||||
JsonNode.Parse(json)!.AsObject()["address"]!.GetValue<string>().ShouldBe("40001");
|
||||
}
|
||||
|
||||
// ---- BuildTagConfig: the browsed data type (Gitea #536) --------------------------------------
|
||||
|
||||
/// <summary>The address fields a captured browse leaf states for its driver-native type.</summary>
|
||||
/// <param name="nativeType">The driver's own data-type enum name, as the driver stamped it.</param>
|
||||
/// <returns>The stated address fields.</returns>
|
||||
private static Dictionary<string, string> NativeTypeFields(string nativeType) =>
|
||||
new() { ["dataType"] = nativeType };
|
||||
|
||||
/// <summary>
|
||||
/// The committed <c>dataType</c> is the type the CONTROLLER reported, not the model default.
|
||||
/// AB CIP / TwinCAT / FOCAS decode the wire bytes by this field, so a browsed REAL left at the
|
||||
/// <c>DInt</c> default reads four bytes of a float as an integer and publishes a plausible
|
||||
/// wrong number with no error anywhere.
|
||||
/// </summary>
|
||||
/// <param name="driverType">The owning device's driver type.</param>
|
||||
/// <param name="nativeType">The native type the browse leaf stated.</param>
|
||||
/// <param name="reference">The leaf's driver-side reference.</param>
|
||||
[Theory]
|
||||
[InlineData("AbCip", "Real", "Pump_1_Speed")]
|
||||
[InlineData("AbCip", "Bool", "Motor_Run")]
|
||||
[InlineData("AbCip", "String", "Recipe_Name")]
|
||||
[InlineData("TwinCAT", "Bool", "MAIN.bStart")]
|
||||
[InlineData("FOCAS", "Float64", "MACRO100")]
|
||||
public void BuildTagConfig_stamps_the_browsed_data_type(
|
||||
string driverType, string nativeType, string reference)
|
||||
{
|
||||
var json = RawBrowseCommitMapper.BuildTagConfig(driverType, reference, NativeTypeFields(nativeType));
|
||||
|
||||
JsonNode.Parse(json)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe(nativeType);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The narrow integer widths are the reason the native type is CARRIED rather than derived from
|
||||
/// the leaf's driver-agnostic <c>DriverDataType</c>: <c>SInt</c>, <c>Int</c> and <c>DInt</c> all
|
||||
/// leave the driver as <c>Int32</c>, so a reverse map would read a 16-bit INT through a 32-bit
|
||||
/// decoder — two bytes of the next tag's memory included — and still look like a fix.
|
||||
/// </summary>
|
||||
/// <param name="nativeType">The Logix integer width the controller reported.</param>
|
||||
[Theory]
|
||||
[InlineData("SInt")]
|
||||
[InlineData("Int")]
|
||||
[InlineData("DInt")]
|
||||
[InlineData("UDInt")]
|
||||
public void BuildTagConfig_keeps_integer_widths_that_the_agnostic_type_collapses(string nativeType)
|
||||
{
|
||||
// That these widths really do collapse on the way out of the driver is pinned next to the map
|
||||
// itself — AbCipDataTypeExtensionsTests.The_integer_widths_collapse_which_is_why_the_native_type_is_carried.
|
||||
var json = RawBrowseCommitMapper.BuildTagConfig("AbCip", "Line1_Count", NativeTypeFields(nativeType));
|
||||
|
||||
JsonNode.Parse(json)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe(nativeType);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A leaf that states no type — a browser that reports none, or a session captured before the
|
||||
/// driver stamped it — keeps the model default rather than guessing. That is the pre-#536
|
||||
/// behaviour, deliberately retained as the fallback.
|
||||
/// </summary>
|
||||
/// <param name="driverType">The owning device's driver type.</param>
|
||||
/// <param name="expectedDefault">The model default the commit must fall back to.</param>
|
||||
[Theory]
|
||||
[InlineData("AbCip", "DInt")]
|
||||
[InlineData("TwinCAT", "DInt")]
|
||||
[InlineData("FOCAS", "Int32")]
|
||||
public void BuildTagConfig_falls_back_to_the_model_default_when_no_type_is_stated(
|
||||
string driverType, string expectedDefault)
|
||||
{
|
||||
var noFields = RawBrowseCommitMapper.BuildTagConfig(driverType, "X");
|
||||
var unparseable = RawBrowseCommitMapper.BuildTagConfig(
|
||||
driverType, "X", NativeTypeFields("NotATypeThisDriverKnows"));
|
||||
|
||||
JsonNode.Parse(noFields)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe(expectedDefault);
|
||||
JsonNode.Parse(unparseable)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe(expectedDefault);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A numeric string is NOT a type name. <c>Enum.TryParse</c> accepts "3" for any enum and yields
|
||||
/// whatever member happens to sit at 3, so an unguarded parse would silently bind a tag to a type
|
||||
/// nobody named — the same trap <c>MTConnectTagConfigModel</c> documents for its own dataType.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void BuildTagConfig_rejects_a_numeric_type_rather_than_indexing_the_enum()
|
||||
{
|
||||
// AbCipDataType member 9 is Real; a numeric parse would hand back exactly that.
|
||||
((AbCipDataType)9).ShouldBe(AbCipDataType.Real);
|
||||
|
||||
var json = RawBrowseCommitMapper.BuildTagConfig("AbCip", "X", NativeTypeFields("9"));
|
||||
|
||||
JsonNode.Parse(json)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe("DInt");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The stated type never displaces the address — the two travel in different fields and a commit
|
||||
/// that got the type right by losing the binding would be no better than the defect.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void BuildTagConfig_stamps_the_type_without_disturbing_the_address()
|
||||
{
|
||||
var json = RawBrowseCommitMapper.BuildTagConfig(
|
||||
"AbCip", "Program:Main.Motor.Speed", NativeTypeFields("Real"));
|
||||
|
||||
var o = JsonNode.Parse(json)!.AsObject();
|
||||
o["tagPath"]!.GetValue<string>().ShouldBe("Program:Main.Motor.Speed");
|
||||
o["dataType"]!.GetValue<string>().ShouldBe("Real");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A driver whose tag config carries no data type is unaffected — the key must not appear, or a
|
||||
/// typed editor would round-trip a field its model does not own.
|
||||
/// </summary>
|
||||
/// <param name="driverType">The owning device's driver type.</param>
|
||||
[Theory]
|
||||
[InlineData("OpcUaClient")]
|
||||
[InlineData("GalaxyMxGateway")]
|
||||
public void BuildTagConfig_does_not_invent_a_data_type_for_a_driver_that_has_none(string driverType)
|
||||
{
|
||||
var json = RawBrowseCommitMapper.BuildTagConfig(driverType, "X", NativeTypeFields("Real"));
|
||||
|
||||
JsonNode.Parse(json)!.AsObject().ContainsKey("dataType").ShouldBeFalse();
|
||||
}
|
||||
|
||||
// ---- BuildTagConfig: MQTT (the address is a DESCRIPTOR, not a single reference string) --------
|
||||
|
||||
/// <summary>The Sparkplug address a browse session states for a metric under a device.</summary>
|
||||
|
||||
+31
-5
@@ -173,11 +173,7 @@ public sealed class DriverProbeHandshakeE2eTests
|
||||
{
|
||||
SkipUnless(DockerHost, AbCipPort);
|
||||
// Probe an explicit tag that the ab_server ControlLogix sim actually defines
|
||||
// (`TestDINT:DINT[1]`). The no-tags fallback (`@raw_cpu_type`) is NOT exercised here:
|
||||
// ab_server answers an unknown/unsupported tag with libplctag ErrorBadParam (a REAL
|
||||
// ControlLogix instead returns ErrorNotFound, which the probe classifies as
|
||||
// reachable). Whether the `@raw_cpu_type` system-tag fallback is valid on a real
|
||||
// ControlLogix is a hardware-gated follow-up (AbCipDriverOptions.cs flags it deferred).
|
||||
// (`TestDINT:DINT[1]`). The no-tags fallback is exercised separately, below.
|
||||
// ⚠️ The config must use the v3 `RawTags` shape — `[{RawPath, TagConfig, WriteIdempotent,
|
||||
// DeviceName}]`, where the Logix tag path lives inside the TagConfig blob as `tagPath`. This
|
||||
// test passed the retired v2 `Tags: [{DeviceHostAddress, TagPath}]` shape until 2026-08-01, so
|
||||
@@ -200,4 +196,34 @@ public sealed class DriverProbeHandshakeE2eTests
|
||||
// ever resolved. Reading the authored tag is the entire point of this test.
|
||||
result.Message.ShouldBe("CIP session OK");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Test Connect on a device with NO authored tags — which is every AB CIP device at the moment
|
||||
/// an operator first presses the button — must report the controller as reachable.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The probe falls back to a name no controller defines, so the green answer arrives as a CIP
|
||||
/// "tag does not exist" (0x04 → libplctag <c>ErrorNotFound</c>), which
|
||||
/// <c>AbCipDriverProbe.IsReachableStatus</c> classifies as reachable. Until 2026-08-01 that
|
||||
/// classification had never been exercised against any backend: <c>ab_server</c> answered an
|
||||
/// unresolvable tag path with 0x03 (<c>ErrorBadParam</c>) where a Logix controller answers 0x04,
|
||||
/// so the fixture reported a healthy sim as a failed handshake and the discrepancy was written
|
||||
/// down as a fixture limitation rather than fixed. It is now fixed in the vendored patch —
|
||||
/// <c>Docker/patches/0002-ab_server-answer-tag-not-found-with-cip-0x04.patch</c> — so this test
|
||||
/// needs the fixture rebuilt from that patch, not merely running.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task AbCip_WithNoAuthoredTags_ReportsTheControllerReachable()
|
||||
{
|
||||
SkipUnless(DockerHost, AbCipPort);
|
||||
var result = await new AbCipDriverProbe().ProbeAsync(
|
||||
$"{{\"Devices\":[{{\"HostAddress\":\"ab://{DockerHost}:{AbCipPort}/1,0\"}}],\"RawTags\":[]}}",
|
||||
Timeout, Ct);
|
||||
|
||||
result.Ok.ShouldBeTrue($"Probe message: {result.Message}");
|
||||
|
||||
// EXACT for the same reason as the test above, and here it carries the extra weight of proving
|
||||
// WHICH green this is: the fallback's "probe tag not found", not a tag that was actually read.
|
||||
result.Message.ShouldBe("CIP session OK (controller reachable; probe tag not found)");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user