fix(ablegacy): refuse every libplctag write the #659 translation changes (fix round 2)
Review M1: round 1 refused only the rewritten status bits, so S:/O:/I: writes - which never worked on libplctag because tag creation failed - would have started reaching the PLC (outputs, status-file words) on the default transport. WriteRefusal now keys on LibplctagLegacyName.IsTranslated: file number supplied (S/O/I, incl. bit writes like O:0/3, refused ahead of the parent-word RMW) or status bit rewritten. Reads unaffected. Docs/comments now true; gate doc notes R bit-number wire evidence is non-discriminating (L1). 12 new cases fail with the round-1 predicate. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
This commit is contained in:
Joseph Doherty
@@ -1094,8 +1094,9 @@ re-emit a concrete default. The defaults differ per driver (final review, 2026-1
|
||||
used to fail `S:`/`O:`/`I:` and `R` status bits there (incl. the default probe `S:0`) — its `pccc.c` needs explicit
|
||||
file numbers and has no R status-bit mnemonics; since #659 `LibplctagLegacyName` translates (`S2:`/`O0:`/`I1:`,
|
||||
`R6:0.CON/13`) and both transports read identically on that press. The authored grammar is unchanged. Read-side
|
||||
only: a libplctag WRITE to a rewritten status bit (R `EN…FD`, `C…UA`) is refused `BadNotSupported` before any runtime
|
||||
is created (it could never reach the PLC before); libplctag T/C status-bit writes it parses itself are unchanged.
|
||||
only: a libplctag WRITE to any address the translation changes (`S:`/`O:`/`I:` incl. bit writes, R `EN…FD`, `C…UA`)
|
||||
is refused `BadNotSupported` before any runtime is created (none could reach the PLC before — no write-surface
|
||||
widening on the default transport); libplctag T/C status-bit writes it parses itself are unchanged.
|
||||
L/A files, function files, SLC, PLC-5 and `logixpccc` remain unverified natively. Explicit Native on PLC-5/`LogixPccc` logs a once-per-instance Warning.
|
||||
AbLegacy native refuses status-bit sub-element writes (`T4:0.DN`…, `BadNotSupported`).
|
||||
|
||||
|
||||
@@ -243,13 +243,15 @@ status file when a digit follows it), and its sub-element table has only `con`/`
|
||||
bit libplctag lacks (`R` `EN/EU/DN/EM/ER/UL/IN/FD`, `C` `UA`) → `R6:0.CON/<bit>` (control word + bit, bit numbers
|
||||
from the native table). The authored grammar is unchanged; `S2:0`/`O0:0`/`I1:0` stay rejected by the parser (one
|
||||
spelling per address). Re-run on the same idle press 2026-10-09: every address above reads **identical** on both
|
||||
transports (`S:4` is a free-running clock and differs read to read on each). **The translation is read-side only —
|
||||
it does not widen the write surface:** before it, a libplctag write to an `R` status bit (or `C…UA`) could never reach
|
||||
the PLC (tag creation failed), and `R6:0.CON/13` would otherwise turn it into a masked write to the control word. So
|
||||
on libplctag a write to any status bit the translation rewrites (`R` `EN/EU/DN/EM/ER/UL/IN/FD`, `C` `UA`) is refused
|
||||
**`BadNotSupported` before a runtime is created** — nothing sent, health unchanged
|
||||
(`LibplctagLegacyTagRuntime.WriteRefusal`, asked through `IAbLegacyTagFactory.WriteRefusal`, the same plumbing as the
|
||||
native refusal). T/C status bits libplctag parses itself keep their pre-existing write behaviour (note above).
|
||||
transports (`S:4` is a free-running clock and differs read to read on each). **The translation is read-side only — it does not widen the write surface:** before it, libplctag could not create a
|
||||
tag for `S:`/`O:`/`I:` or for an `R`/`C…UA` status bit, so no write to one ever reached the PLC. Now that the
|
||||
translated names parse, a write would drive a physical output, a status-file word or (masked) a control word. So on
|
||||
libplctag **every write whose address the translation changes** — `S:`, `O:`, `I:` (word and bit writes; a bit write
|
||||
such as `O:0/3` is refused ahead of the parent-word read-modify-write) and the rewritten status bits (`R`
|
||||
`EN/EU/DN/EM/ER/UL/IN/FD`, `C` `UA`) — is refused **`BadNotSupported` before a runtime is created**: nothing read or
|
||||
sent, health unchanged (`LibplctagLegacyTagRuntime.WriteRefusal`, asked through `IAbLegacyTagFactory.WriteRefusal`, the
|
||||
same plumbing as the native refusal). Those writes never worked on libplctag, so nothing is lost; the native
|
||||
transport (opt-in) does write `S`/`O`/`I`. T/C status bits libplctag parses itself keep their pre-existing write behaviour (note above).
|
||||
Not tested on hardware: L and A files (no press has them), function files, PLC-5, SLC, `logixpccc`.
|
||||
|
||||
Simulator evidence:
|
||||
|
||||
@@ -700,3 +700,5 @@ ST14:10 String Native value="Z-33323" status=0x00000000 (
|
||||
```
|
||||
|
||||
Every address reads identically on both transports. `S:4` is the free-running clock, so its value differs read to read on each transport (both Good). `I:0` = -29376 = 0x8D40, and bits `/6` and `/15` read true on both while `/0`, `/1`, `/7` read false — the bit is extracted from the word libplctag read, not defaulted. ab_server has no S/O/I files, so this real-controller read is the only wire evidence; the translation is unit-pinned against a model of the `pccc.c` parser (`LibplctagLegacyNameTests`).
|
||||
|
||||
Caveats: `R6:0.DN` and `R6:0.EN` read `false` on both transports, so this read does not discriminate the control-word bit number (a wrong bit would very likely also read false) — the R bit mapping rests on the native table and the `pccc.c` parser model, not on this wire evidence. Writes: on libplctag every address the translation changes (`S:`/`O:`/`I:`, R/UA status bits) is refused `BadNotSupported` (#659 review), so this fix adds no libplctag write path.
|
||||
|
||||
@@ -46,17 +46,18 @@ internal static class LibplctagLegacyName
|
||||
public static string Translate(string canonical) => TranslateCore(canonical).Name;
|
||||
|
||||
/// <summary>
|
||||
/// True when <paramref name="canonical"/> is a status bit this translator rewrites to control word + bit
|
||||
/// (<c>R6:0.DN</c> → <c>R6:0.CON/13</c>; R <c>EN/EU/DN/EM/ER/UL/IN/FD</c>, counter <c>UA</c>). A WRITE to one is
|
||||
/// refused (<see cref="LibplctagLegacyTagRuntime.WriteRefusal"/>): before the translation libplctag could not
|
||||
/// even create such a tag, so a write never reached the PLC, and the rewrite must not turn it into a new masked
|
||||
/// write to a production controller's control word (the native transport refuses status-bit writes too).
|
||||
/// True when <see cref="Translate"/> changes <paramref name="canonical"/>: a file number supplied for
|
||||
/// <c>S:</c>/<c>O:</c>/<c>I:</c> (incl. bit forms such as <c>O:0/3</c>) or a status bit rewritten to control word +
|
||||
/// bit (<c>R6:0.DN</c> → <c>R6:0.CON/13</c>; R <c>EN/EU/DN/EM/ER/UL/IN/FD</c>, counter <c>UA</c>). A WRITE to such an
|
||||
/// address is refused (<see cref="LibplctagLegacyTagRuntime.WriteRefusal"/>): before #659 libplctag could not
|
||||
/// even create these tags, so no write ever reached the PLC, and the read fix must not turn libplctag — the
|
||||
/// default transport — into a new way to drive outputs, status-file words or control words.
|
||||
/// </summary>
|
||||
/// <param name="canonical">The canonical address.</param>
|
||||
/// <returns><see langword="true"/> for a rewritten status bit.</returns>
|
||||
public static bool IsRewrittenStatusBit(string canonical) => TranslateCore(canonical).RewrittenStatusBit;
|
||||
/// <returns><see langword="true"/> when the libplctag name differs from the canonical one.</returns>
|
||||
public static bool IsTranslated(string canonical) => TranslateCore(canonical).Changed;
|
||||
|
||||
private static (string Name, bool RewrittenStatusBit) TranslateCore(string canonical)
|
||||
private static (string Name, bool Changed) TranslateCore(string canonical)
|
||||
{
|
||||
// Family-neutral parse is correct: the driver hands the runtime AbLegacyAddress.ToLibplctagName(), already
|
||||
// decimal-normalised (PLC-5 octal I/O included) — the same reasoning as NativeAbLegacyTagRuntime.ToPcccAddress.
|
||||
@@ -65,6 +66,7 @@ internal static class LibplctagLegacyName
|
||||
var fileNumber = a.FileNumber ?? PcccFileType.ImplicitFileNumber(a.FileLetter);
|
||||
if (fileNumber is null) return (canonical, false);
|
||||
|
||||
var fileNumberSupplied = a.FileNumber is null; // S:/O:/I: — libplctag needs S2/O0/I1
|
||||
var name = $"{a.FileLetter}{fileNumber}:{a.WordNumber}";
|
||||
if (a.SubElement is { } sub)
|
||||
{
|
||||
@@ -91,6 +93,6 @@ internal static class LibplctagLegacyName
|
||||
}
|
||||
|
||||
if (a.BitIndex is int b) name += $"/{b}";
|
||||
return (name, false);
|
||||
return (name, fileNumberSupplied);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -71,18 +71,20 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime
|
||||
: RunReleasingOrphanAsync(() => _tag.WriteAsync(cancellationToken));
|
||||
|
||||
/// <summary>
|
||||
/// The writes the libplctag runtime refuses before the wire, or <see langword="null"/>: a status bit that
|
||||
/// <see cref="LibplctagLegacyName"/> rewrites to control word + bit (<c>R6:0.DN</c> → <c>R6:0.CON/13</c>, R
|
||||
/// <c>EN/EU/DN/EM/ER/UL/IN/FD</c>, counter <c>UA</c>). Before #659 libplctag could not create those tags at all,
|
||||
/// so a write to one never reached the controller; the read-side translation must not open a new masked write
|
||||
/// to a control word. T/C status bits libplctag itself parses (<c>T4:0.DN</c>, <c>C5:0.CU</c>…) keep their
|
||||
/// pre-existing behaviour — a masked bit write (documented hazard in <c>docs/drivers/AbLegacy.md</c>).
|
||||
/// The writes the libplctag runtime refuses before the wire, or <see langword="null"/>: any address whose
|
||||
/// libplctag name <see cref="LibplctagLegacyName"/> had to change (<see cref="LibplctagLegacyName.IsTranslated"/>)
|
||||
/// — file-number-less <c>S:</c>/<c>O:</c>/<c>I:</c> (incl. bit writes such as <c>O:0/3</c>, refused before the
|
||||
/// parent-word read-modify-write) and the status bits rewritten to control word + bit (<c>R6:0.DN</c>, counter
|
||||
/// <c>UA</c>). Before #659 libplctag could not create those tags at all, so no write to one ever reached the
|
||||
/// controller; the read fix must not widen the write surface of the default transport. T/C status bits
|
||||
/// libplctag parses itself (<c>T4:0.DN</c>, <c>C5:0.CU</c>…) keep their pre-existing behaviour — a masked bit
|
||||
/// write (documented hazard in <c>docs/drivers/AbLegacy.md</c>).
|
||||
/// </summary>
|
||||
/// <param name="p">The tag's create parameters.</param>
|
||||
/// <returns>The refusal reason, or <see langword="null"/>.</returns>
|
||||
internal static string? WriteRefusal(AbLegacyTagCreateParams p) =>
|
||||
LibplctagLegacyName.IsRewrittenStatusBit(p.TagName)
|
||||
? $"Writes to the status bit '{p.TagName}' are not supported on the libplctag transport (libplctag has no mnemonic for it; it is read as control word + bit)."
|
||||
LibplctagLegacyName.IsTranslated(p.TagName)
|
||||
? $"Writes to '{p.TagName}' are not supported on the libplctag transport (its name is translated for libplctag; reads only)."
|
||||
: null;
|
||||
|
||||
/// <summary>
|
||||
|
||||
+53
-19
@@ -7,14 +7,15 @@ using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.PlcFamilies;
|
||||
namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// #659 review ruling: the libplctag read translation of a status bit libplctag has no mnemonic for
|
||||
/// (<c>R6:0.DN</c> → <c>R6:0.CON/13</c>) must not widen the WRITE surface. Before the translation libplctag could
|
||||
/// not create such a tag, so a write never reached the controller; now that the name parses, a write would go out
|
||||
/// as a masked write to the control word. Such writes are refused <c>BadNotSupported</c> before any runtime is
|
||||
/// created, health unchanged. T/C status bits libplctag itself parses keep their pre-existing (unrefused) behaviour.
|
||||
/// #659 review rulings: the libplctag read translation must not widen the WRITE surface of the default transport.
|
||||
/// Before it, libplctag could not create a tag for <c>S:</c>/<c>O:</c>/<c>I:</c> (no file number) or for an R/UA
|
||||
/// status bit, so no write to one ever reached the controller; now that the translated names parse, a write would
|
||||
/// drive an output, a status-file word or (masked) a control word. Every write whose address the translation
|
||||
/// changes is refused <c>BadNotSupported</c> before any runtime is created, health unchanged. T/C status bits
|
||||
/// libplctag itself parses keep their pre-existing (unrefused) behaviour.
|
||||
/// </summary>
|
||||
[Trait("Category", "Unit")]
|
||||
public sealed class LibplctagStatusBitWriteRefusalTests
|
||||
public sealed class LibplctagTranslatedWriteRefusalTests
|
||||
{
|
||||
[Theory]
|
||||
[InlineData("R6:0.EN", true)]
|
||||
@@ -31,19 +32,27 @@ public sealed class LibplctagStatusBitWriteRefusalTests
|
||||
[InlineData("C5:0.DN", false)]
|
||||
[InlineData("R6:0.LEN", false)]
|
||||
[InlineData("R6:0.POS", false)]
|
||||
[InlineData("S:0", false)] // translated, but not a status bit
|
||||
[InlineData("O:0/3", false)]
|
||||
[InlineData("I:0/0", false)]
|
||||
[InlineData("S:0", true)] // file number supplied by the translation (S2:0)
|
||||
[InlineData("S:1/5", true)]
|
||||
[InlineData("S:57", true)]
|
||||
[InlineData("O:0", true)]
|
||||
[InlineData("O:0/3", true)]
|
||||
[InlineData("I:0", true)]
|
||||
[InlineData("I:0/0", true)]
|
||||
[InlineData("N7:0", false)]
|
||||
[InlineData("N7:0/3", false)]
|
||||
[InlineData("F8:0", false)]
|
||||
[InlineData("ST14:10", false)]
|
||||
[InlineData("T4:0.PRE", false)]
|
||||
[InlineData("B3:0/4", false)]
|
||||
[InlineData("R6:0.FOO", false)]
|
||||
[InlineData("garbage", false)]
|
||||
public void The_libplctag_refusal_covers_exactly_the_rewritten_status_bits(string tag, bool refused)
|
||||
public void The_libplctag_refusal_covers_exactly_the_translated_addresses(string tag, bool refused)
|
||||
{
|
||||
var p = new AbLegacyTagCreateParams("h", 44818, "", "micrologix", tag, TimeSpan.FromSeconds(1));
|
||||
(LibplctagLegacyTagRuntime.WriteRefusal(p) is not null).ShouldBe(refused);
|
||||
(new LibplctagLegacyTagFactory().WriteRefusal(p) is not null).ShouldBe(refused);
|
||||
LibplctagLegacyName.IsRewrittenStatusBit(tag).ShouldBe(refused);
|
||||
LibplctagLegacyName.IsTranslated(tag).ShouldBe(refused);
|
||||
}
|
||||
|
||||
/// <summary>Defence in depth: a runtime for a rewritten status bit refuses the write itself, without touching the
|
||||
@@ -69,14 +78,15 @@ public sealed class LibplctagStatusBitWriteRefusalTests
|
||||
}
|
||||
|
||||
#pragma warning disable OTOPCUA0001 // direct driver calls pin the driver's own status mapping (no resilience wrapper)
|
||||
private static async Task<(AbLegacyDriver Driver, CountingLibplctagRefusalFactory Factory)> DriverAsync(string address)
|
||||
private static async Task<(AbLegacyDriver Driver, CountingLibplctagRefusalFactory Factory)> DriverAsync(
|
||||
string address, AbLegacyDataType type = AbLegacyDataType.Bit)
|
||||
{
|
||||
const string host = "ab://10.0.0.5/";
|
||||
var factory = new CountingLibplctagRefusalFactory();
|
||||
var driver = new AbLegacyDriver(new AbLegacyDriverOptions
|
||||
{
|
||||
Devices = [new AbLegacyDeviceOptions(host, AbLegacyPlcFamily.MicroLogix)],
|
||||
RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("Bit", host, address, AbLegacyDataType.Bit, Writable: true)]),
|
||||
RawTags = AbLegacyRawTags.Entries([new AbLegacyTagDefinition("Bit", host, address, type, Writable: true)]),
|
||||
Timeout = TimeSpan.FromSeconds(1),
|
||||
Probe = new AbLegacyProbeOptions { Enabled = false },
|
||||
}, "drv-libplctag-statusbit", factory);
|
||||
@@ -84,17 +94,25 @@ public sealed class LibplctagStatusBitWriteRefusalTests
|
||||
return (driver, factory);
|
||||
}
|
||||
|
||||
/// <summary>Includes bit writes to O/I/S (<c>O:0/3</c>): the refusal runs ahead of the bit read-modify-write, so
|
||||
/// not even the parent-word runtime (<c>O:0</c>) is created — nothing is read or written.</summary>
|
||||
[Theory]
|
||||
[InlineData("R6:0.DN")]
|
||||
[InlineData("R6:0.EN")]
|
||||
[InlineData("C5:0.UA")]
|
||||
public async Task A_rewritten_status_bit_write_is_BadNotSupported_with_no_runtime_and_health_unchanged(string address)
|
||||
[InlineData("R6:0.DN", AbLegacyDataType.Bit, true)]
|
||||
[InlineData("R6:0.EN", AbLegacyDataType.Bit, true)]
|
||||
[InlineData("C5:0.UA", AbLegacyDataType.Bit, true)]
|
||||
[InlineData("O:0/3", AbLegacyDataType.Bit, true)]
|
||||
[InlineData("I:0/0", AbLegacyDataType.Bit, true)]
|
||||
[InlineData("S:1/5", AbLegacyDataType.Bit, true)]
|
||||
[InlineData("O:0", AbLegacyDataType.Int, (short)1)]
|
||||
[InlineData("S:1", AbLegacyDataType.Int, (short)1)]
|
||||
public async Task A_translated_address_write_is_BadNotSupported_with_no_runtime_and_health_unchanged(
|
||||
string address, AbLegacyDataType type, object value)
|
||||
{
|
||||
var ct = TestContext.Current.CancellationToken;
|
||||
var (driver, factory) = await DriverAsync(address);
|
||||
var (driver, factory) = await DriverAsync(address, type);
|
||||
var health = driver.GetHealth();
|
||||
|
||||
(await driver.WriteAsync([new WriteRequest("Bit", true)], ct)).Single().StatusCode
|
||||
(await driver.WriteAsync([new WriteRequest("Bit", value)], ct)).Single().StatusCode
|
||||
.ShouldBe(AbLegacyStatusMapper.BadNotSupported);
|
||||
|
||||
factory.Inner.Tags.ShouldBeEmpty();
|
||||
@@ -120,5 +138,21 @@ public sealed class LibplctagStatusBitWriteRefusalTests
|
||||
factory.Inner.Tags["T4:0.DN"].WriteCount.ShouldBe(1);
|
||||
await driver.ShutdownAsync(ct);
|
||||
}
|
||||
|
||||
/// <summary>Positive control: an untranslated bit write (<c>B3:0/4</c>) still runs the parent-word
|
||||
/// read-modify-write, so the refusal is not swallowing bit writes in general.</summary>
|
||||
[Fact]
|
||||
public async Task An_untranslated_bit_write_still_runs_the_parent_word_read_modify_write()
|
||||
{
|
||||
var ct = TestContext.Current.CancellationToken;
|
||||
var (driver, factory) = await DriverAsync("B3:0/4");
|
||||
factory.Inner.Customise = p => new FakeAbLegacyTag(p) { Value = (short)0 };
|
||||
|
||||
(await driver.WriteAsync([new WriteRequest("Bit", true)], ct)).Single().StatusCode
|
||||
.ShouldBe(AbLegacyStatusMapper.Good);
|
||||
|
||||
factory.Inner.Tags["B3:0"].WriteCount.ShouldBe(1);
|
||||
await driver.ShutdownAsync(ct);
|
||||
}
|
||||
#pragma warning restore OTOPCUA0001
|
||||
}
|
||||
Reference in New Issue
Block a user