fix(abcip): #611 review round 1 — native keeps BOOL type distinction, WriteWidthRefusal tests, doc caveats
- L2: WriteWidthRefusal leaves a BOOL over a wider value to a runtime that sees the CIP type (native): mistyped integer member -> BadTypeMismatch (as its read), 0xD3 BOOL-array DWORD -> BadNotSupported; libplctag keeps BadNotSupported for both (size only). - L4: direct theory over WriteWidthRefusal (1/2/4/8 widths incl LINT/ULINT/LREAL, String/Dt, unknown size) + array/bit-selector exemptions with a non-vacuous control. - L1: CLAUDE.md member/BIT-member writes are ab_server-proven only. - L3: patch 0003 blind spots documented (Docker README, AbServer-Test-Fixture.md); nits. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
@@ -1102,7 +1102,7 @@ once per tag — only genuine transport failures evict and degrade (a runtime `N
|
||||
it used to latch `Degraded`). AbCip width-checks every scalar **write** against the controller's bytes before
|
||||
sending, on both transports (#611; libplctag used to patch part of its buffer and send it all). Controller-discovered
|
||||
UDT members are writable once browse-committed and authored `ReadWrite` (BIT members included — the controller
|
||||
applies the bit); whole-UDT writes are refused `BadNotSupported` by decision (`docs/drivers/AbCip.md` §Member writes). `DeviceSessionPool.Shared` logs through the
|
||||
applies the bit) — ab_server-proven only, member and BIT-member writes are unverified on real hardware; whole-UDT writes are refused `BadNotSupported` by decision (`docs/drivers/AbCip.md` §Member writes). `DeviceSessionPool.Shared` logs through the
|
||||
host logger factory (wired in `DriverFactoryBootstrap`; the FOCAS and TwinCAT factories' `Register(registry, ILoggerFactory?)` take one too, #602 CONV-3). The AbCip `browsePageDelayMs` knob has an AdminUI field (`AbCipDriverForm` "Browse page delay (ms)"). libplctag is confined to `Libplctag*.cs` adapters by
|
||||
`LibplctagConfinementGuardTests`; do not reference it elsewhere. See `docs/drivers/AbCip.md` §Transport,
|
||||
`docs/drivers/AbLegacy.md` §Transport + §Wire evidence, `docs/Configuration.md` §`Drivers:AbCip`, and
|
||||
|
||||
@@ -405,8 +405,10 @@ when it is browse-committed in `/raw`, and that tag writes like any other atomic
|
||||
with the bytes the controller holds at the path (the runtime's last read) and refuses a mismatch without
|
||||
writing: a narrower/wider atomic type is `BadTypeMismatch` (libplctag would otherwise patch part of its read
|
||||
buffer and send the whole buffer back — e.g. an `Int` over a DINT member wrote four bytes with a stale high
|
||||
word); a `Bool` over a wider value (a packed BOOL-array DWORD, a mistyped integer) is `BadNotSupported`,
|
||||
because writing it would rewrite every neighbouring bit. Native additionally refuses a same-width
|
||||
word); a `Bool` over a wider value is never written, because writing it would rewrite every neighbouring
|
||||
bit. On **native** the runtime judges it from the CIP type, exactly as its read does: a packed BOOL-array
|
||||
DWORD (`0xD3`) is `BadNotSupported`, a BOOL authored over a mistyped integer member is `BadTypeMismatch`.
|
||||
**libplctag** exposes only the size, cannot tell the two apart, and refuses both `BadNotSupported`. Native additionally refuses a same-width
|
||||
mismatch (REAL vs DINT) from the CIP type code — see [Native transport behaviour](#native-transport-behaviour).
|
||||
A refusal keeps the handle, warns once per tag and **does not touch driver health**.
|
||||
- **Not verified on a real controller.** Production presses are read-only, so member writes are proven on
|
||||
@@ -414,7 +416,8 @@ when it is browse-committed in `/raw`, and that tag writes like any other atomic
|
||||
Z-35162 controller tag `_EmptyTemperatureSegment` (`UDT_TemperatureProfile_Segment`), which no logic
|
||||
references — its members (`ID`, `Enable`, `RampTime`, …) are all zero and resolve as described above.
|
||||
|
||||
Pre-declared `Members` with `Writable: true` keep working exactly as before (same write path).
|
||||
Pre-declared `Members` with `Writable: true` keep working as before — same write path, now width-checked
|
||||
(a mistyped declared member is refused where libplctag used to write it).
|
||||
|
||||
### Whole-UDT writes — refused, by decision
|
||||
|
||||
|
||||
@@ -55,7 +55,9 @@ smoke test passes on both transports. Evidence:
|
||||
- ab_server serves no UDTs, so template parity is vacuous there. Docker patch 0003 serves a UDT **member
|
||||
path** from a flat tag named by that dotted path (`UdtSeg.ID`, `UdtSeg.Sub.Code`, `UdtSeg.Points[2]`) — the
|
||||
wire of a member read/write, not a structure — and accepts the pad byte libplctag adds to a 1-byte write,
|
||||
which stock ab_server refused (`0x03`) for every libplctag BOOL/SINT write. Native writes remain
|
||||
which stock ab_server refused (`0x03`) for every libplctag BOOL/SINT write. Two blind spots follow (see
|
||||
the Docker README §0003): a +1-byte over-write of an odd-sized tag is no longer caught, and a malformed
|
||||
single segment `"UdtSeg.ID"` resolves where a real Logix would refuse it. Native writes remain
|
||||
simulator-only.
|
||||
- **Golden-frame capture:** `scripts/ab-capture/capture.sh` (capture client in `scripts/ab-capture/client`);
|
||||
fixtures live in `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests/Captures/` with a README.
|
||||
|
||||
@@ -1197,7 +1197,7 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
|
||||
// BOOL-array DWORD wrote all 32 bits. Discovered UDT members are where a mis-authored type
|
||||
// is most likely (browse-commit fills dataType from the template, an operator may retype).
|
||||
size = runtime.GetSize();
|
||||
refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size);
|
||||
refusal = WriteWidthRefusal(def, parsedPath?.BitIndex, size, runtimeTypesBool: runtime is NativeAbCipTagRuntime);
|
||||
if (refusal is null)
|
||||
{
|
||||
runtime.EncodeValue(def.DataType, parsedPath?.BitIndex, w.Value);
|
||||
@@ -1710,15 +1710,24 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery,
|
||||
/// would clobber the neighbouring bits, the same refusal the native runtime raises; any other width
|
||||
/// mismatch is <c>BadTypeMismatch</c>. Arrays, bit selectors (<c>Flags.3</c>, served by the
|
||||
/// read-modify-write path) and an unknown size (<c><= 0</c>) are not judged here.
|
||||
/// <para>
|
||||
/// A BOOL over a wider value is left to the runtime when it can see the CIP type
|
||||
/// (<paramref name="runtimeTypesBool"/> — the native transport): it tells a packed BOOL-array DWORD
|
||||
/// (<c>0xD3</c>, <c>BadNotSupported</c>) from a mistyped integer member (<c>BadTypeMismatch</c>, the
|
||||
/// same verdict as its read), and refuses both before sending. libplctag exposes only the size, so the
|
||||
/// two are indistinguishable there and both get the safe <c>BadNotSupported</c>.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="def">The tag being written.</param>
|
||||
/// <param name="bitIndex">The parsed bit selector, if any.</param>
|
||||
/// <param name="size">The runtime's read-buffer size in bytes.</param>
|
||||
/// <param name="runtimeTypesBool">The runtime checks a BOOL write against the controller's CIP type itself.</param>
|
||||
/// <returns>The refusal status, or <c>null</c>.</returns>
|
||||
internal static uint? WriteWidthRefusal(AbCipTagDefinition def, int? bitIndex, int size)
|
||||
internal static uint? WriteWidthRefusal(AbCipTagDefinition def, int? bitIndex, int size, bool runtimeTypesBool = false)
|
||||
{
|
||||
if (IsArrayTag(def) || bitIndex is not null || size <= 0) return null;
|
||||
if (ExpectedScalarWriteWidth(def.DataType) is not { } width || size == width) return null;
|
||||
if (def.DataType == AbCipDataType.Bool && runtimeTypesBool) return null;
|
||||
return def.DataType == AbCipDataType.Bool
|
||||
? AbCipStatusMapper.BadNotSupported
|
||||
: AbCipStatusMapper.BadTypeMismatch;
|
||||
|
||||
@@ -90,6 +90,14 @@ things for the discovered-UDT-member write suite (`AbCipUdtMemberWriteTests`, Gi
|
||||
`0x03` (libplctag `ErrorBadParam`), so no libplctag BOOL/SINT write could pass here. The
|
||||
patch drops exactly one trailing pad byte when it alone would overrun an odd-sized target.
|
||||
|
||||
**Blind spots this patch introduces (accepted for a fixture):** (a) the pad trim also accepts a genuinely
|
||||
wrong 2-byte payload aimed at a 1-byte tag, so ab_server can no longer catch a client over-writing an
|
||||
odd-sized target by exactly one byte — the driver's own pre-write width check (`AbCipDriver.WriteWidthRefusal`)
|
||||
covers our paths; (b) because `.` is legal in a `--tag` name, a malformed *single* symbolic segment
|
||||
`"UdtSeg.ID"` (which a real Logix rejects) also resolves, via the original exact-match lookup —
|
||||
`NativeAbCipTagRuntimeTests.A_member_path_write_names_the_member_and_sends_only_its_own_bytes` pins that our
|
||||
encoder splits members into separate segments.
|
||||
|
||||
**Bumping `LIBPLCTAG_TAG` means re-checking every patch against the new tag.** It
|
||||
is a concrete tag rather than the `release` branch precisely so that this stays
|
||||
a deliberate act — the branch moves, and the Dockerfile previously called it
|
||||
@@ -159,7 +167,7 @@ place means updating both.
|
||||
|
||||
| Family | Seeded tags | Notes |
|
||||
|---|---|---|
|
||||
| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` `UdtSeg.*` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server; the `UdtSeg.*` member tags (patch 0003) serve member-path writes only. The last three serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it |
|
||||
| ControlLogix | `TestDINT` `TestREAL` `TestBOOL` `TestSINT` `TestString` `TestArray` `Pump_1_Speed` `Line1_Count` `Motor_Run` `UdtSeg.*` | Widest-coverage; PR 9 baseline. UDT emulation missing from ab_server; the `UdtSeg.*` member tags (patch 0003) serve member-path writes only. `Pump_1_Speed`, `Line1_Count` and `Motor_Run` serve the controller-browse gate and are additive — the read/write suites key off the `Test*` tags. `Motor_Run` (9), `Line1_Count` (11) and `TestArray` (9) have **odd-length names on purpose**: entries are unpadded, so an odd name puts every following entry on an odd offset, and a decoder that assumes even alignment loses everything behind it |
|
||||
| CompactLogix | `TestDINT` `TestREAL` `TestBOOL` | Narrow ConnectionSize cap enforced driver-side; ab_server accepts any size |
|
||||
| Micro800 | `TestDINT` `TestREAL` | ab_server has no `micro800` mode; falls back to `controllogix` emulation |
|
||||
| GuardLogix | `TestDINT` `SafetyDINT_S` | ab_server has no safety subsystem; `_S` suffix triggers driver-side classification only |
|
||||
|
||||
+1
-1
@@ -20,7 +20,7 @@ services:
|
||||
restart: "no"
|
||||
ports:
|
||||
- "44818:44818"
|
||||
# The last three are here for the controller-browse gate (Gitea #468) and
|
||||
# Pump_1_Speed, Line1_Count and Motor_Run are here for the controller-browse gate (Gitea #468) and
|
||||
# are additive — the read/write suites key off the Test* tags above.
|
||||
# The UdtSeg.* tags (Gitea #611) stand in for the members of a UDT tag `UdtSeg`
|
||||
# (modelled on the Z-35162 press's UDT_TemperatureProfile_Segment): ab_server has
|
||||
|
||||
+115
@@ -2,6 +2,7 @@ using System.Runtime.CompilerServices;
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.Ab.Transport.Tests.Fakes;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests;
|
||||
|
||||
@@ -153,6 +154,120 @@ public sealed class AbCipDiscoveredMemberWriteTests
|
||||
factory.Tags["Seg.ID"].WriteCount.ShouldBe(0);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Review L4 — <see cref="AbCipDriver.WriteWidthRefusal"/> directly, with a real size so the check runs:
|
||||
/// every atomic width (1/2/4/8, incl. LINT/ULINT/LREAL) passes at its own size and is refused at another;
|
||||
/// arrays, <c>Tag.N</c> bit selectors, STRING/DT and an unknown size (<c><= 0</c>) are never judged.
|
||||
/// </summary>
|
||||
[Theory]
|
||||
// matching widths write
|
||||
[InlineData(AbCipDataType.Bool, 1, null)]
|
||||
[InlineData(AbCipDataType.SInt, 1, null)]
|
||||
[InlineData(AbCipDataType.USInt, 1, null)]
|
||||
[InlineData(AbCipDataType.Int, 2, null)]
|
||||
[InlineData(AbCipDataType.UInt, 2, null)]
|
||||
[InlineData(AbCipDataType.DInt, 4, null)]
|
||||
[InlineData(AbCipDataType.UDInt, 4, null)]
|
||||
[InlineData(AbCipDataType.Real, 4, null)]
|
||||
[InlineData(AbCipDataType.LInt, 8, null)]
|
||||
[InlineData(AbCipDataType.ULInt, 8, null)]
|
||||
[InlineData(AbCipDataType.LReal, 8, null)]
|
||||
// mismatches are refused
|
||||
[InlineData(AbCipDataType.LInt, 4, AbCipStatusMapper.BadTypeMismatch)]
|
||||
[InlineData(AbCipDataType.LReal, 4, AbCipStatusMapper.BadTypeMismatch)]
|
||||
[InlineData(AbCipDataType.ULInt, 2, AbCipStatusMapper.BadTypeMismatch)]
|
||||
[InlineData(AbCipDataType.Real, 8, AbCipStatusMapper.BadTypeMismatch)]
|
||||
[InlineData(AbCipDataType.Int, 4, AbCipStatusMapper.BadTypeMismatch)]
|
||||
[InlineData(AbCipDataType.SInt, 2, AbCipStatusMapper.BadTypeMismatch)]
|
||||
[InlineData(AbCipDataType.Bool, 4, AbCipStatusMapper.BadNotSupported)]
|
||||
// types with no fixed encoded width are never judged
|
||||
[InlineData(AbCipDataType.String, 88, null)]
|
||||
[InlineData(AbCipDataType.String, 4, null)]
|
||||
[InlineData(AbCipDataType.Dt, 8, null)]
|
||||
// unknown size is never judged
|
||||
[InlineData(AbCipDataType.DInt, 0, null)]
|
||||
[InlineData(AbCipDataType.DInt, -1, null)]
|
||||
public void WriteWidthRefusal_judges_scalar_widths_only(AbCipDataType type, int size, uint? expected)
|
||||
{
|
||||
var def = new AbCipTagDefinition("T", Device, "Seg.X", type);
|
||||
AbCipDriver.WriteWidthRefusal(def, bitIndex: null, size).ShouldBe(expected);
|
||||
}
|
||||
|
||||
/// <summary>Review L4 — array tags and bit selectors are exempt even at a size that would otherwise refuse.</summary>
|
||||
[Fact]
|
||||
public void WriteWidthRefusal_exempts_arrays_and_bit_selectors()
|
||||
{
|
||||
// Control: the same scalar at the same size IS refused, so the exemptions below are not vacuous.
|
||||
AbCipDriver.WriteWidthRefusal(new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int), null, 4)
|
||||
.ShouldBe(AbCipStatusMapper.BadTypeMismatch);
|
||||
|
||||
AbCipDriver.WriteWidthRefusal(
|
||||
new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int, ElementCount: 4, IsArray: true), null, 4)
|
||||
.ShouldBeNull();
|
||||
AbCipDriver.WriteWidthRefusal(
|
||||
new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int, ElementCount: 2), null, 4)
|
||||
.ShouldBeNull();
|
||||
AbCipDriver.WriteWidthRefusal(
|
||||
new AbCipTagDefinition("T", Device, "Flags.3", AbCipDataType.Bool), bitIndex: 3, size: 4)
|
||||
.ShouldBeNull();
|
||||
AbCipDriver.WriteWidthRefusal(
|
||||
new AbCipTagDefinition("T", Device, "Word.3", AbCipDataType.Int), bitIndex: 3, size: 4)
|
||||
.ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Review L2 — a runtime that types BOOL writes itself (native) is left to judge a BOOL over a wider value;
|
||||
/// only libplctag, which sees a size and nothing else, gets the pre-check's BadNotSupported.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void WriteWidthRefusal_leaves_a_wide_BOOL_to_a_runtime_that_sees_the_CIP_type()
|
||||
{
|
||||
var def = new AbCipTagDefinition("T", Device, "Seg.Flag", AbCipDataType.Bool);
|
||||
AbCipDriver.WriteWidthRefusal(def, null, 4, runtimeTypesBool: false).ShouldBe(AbCipStatusMapper.BadNotSupported);
|
||||
AbCipDriver.WriteWidthRefusal(def, null, 4, runtimeTypesBool: true).ShouldBeNull();
|
||||
// Non-BOOL mismatches are still refused up front on every runtime.
|
||||
AbCipDriver.WriteWidthRefusal(new AbCipTagDefinition("T", Device, "Seg.X", AbCipDataType.Int), null, 4,
|
||||
runtimeTypesBool: true).ShouldBe(AbCipStatusMapper.BadTypeMismatch);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Review L2, end to end on the native runtime: a BOOL authored over a mistyped DINT member writes
|
||||
/// BadTypeMismatch (the same verdict as its read); a BOOL over a packed BOOL-array DWORD writes
|
||||
/// BadNotSupported. Neither sends a write; health is untouched.
|
||||
/// </summary>
|
||||
[Theory]
|
||||
[InlineData("C400", AbCipStatusMapper.BadTypeMismatch)] // DINT member mistyped as Bool
|
||||
[InlineData("D300", AbCipStatusMapper.BadNotSupported)] // packed BOOL-array DWORD
|
||||
public async Task A_native_BOOL_over_a_wider_value_keeps_its_type_distinction(string typeHex, uint expected)
|
||||
{
|
||||
var channels = new List<FakeCipChannel>();
|
||||
var factory = new NativeFactory(p =>
|
||||
{
|
||||
var ch = new FakeCipChannel { Respond = _ => Task.FromResult(Convert.FromHexString("D2000000" + typeHex + "00000000")) };
|
||||
lock (channels) channels.Add(ch);
|
||||
return NativeAbCipTagRuntime.ForTest(ch, p);
|
||||
});
|
||||
var drv = new AbCipDriver(new AbCipDriverOptions
|
||||
{
|
||||
Devices = [new AbCipDeviceOptions(Device)],
|
||||
RawTags = [Committed("Seg.Flag", "Bool")],
|
||||
Probe = new AbCipProbeOptions { Enabled = false },
|
||||
}, "drv-native", factory);
|
||||
await drv.InitializeAsync("{}", CancellationToken.None);
|
||||
var health = drv.GetHealth();
|
||||
|
||||
(await drv.WriteAsync([new WriteRequest("Seg.Flag", true)], CancellationToken.None))
|
||||
.Single().StatusCode.ShouldBe(expected);
|
||||
|
||||
channels.ShouldHaveSingleItem().Sent.ShouldAllBe(req => req[0] != 0x4D); // no Write Tag request
|
||||
drv.GetHealth().ShouldBe(health);
|
||||
}
|
||||
|
||||
private sealed class NativeFactory(Func<AbCipTagCreateParams, IAbCipTagRuntime> create) : IAbCipTagFactory
|
||||
{
|
||||
public IAbCipTagRuntime Create(AbCipTagCreateParams createParams) => create(createParams);
|
||||
}
|
||||
|
||||
// ---- helpers ---------------------------------------------------------------------------------------
|
||||
|
||||
/// <summary>Browses a UDT shaped like the Z-35162 press's UDT_TemperatureProfile_Segment (+ a nested struct).</summary>
|
||||
|
||||
Reference in New Issue
Block a user