fix(abcip): unblock the controller-browse gate, and fix the decoder it exposed (#468)
v2-ci / build (push) Successful in 4m17s
v2-ci / unit-tests (push) Failing after 14m43s

The AbCip controller browse could never be verified end to end: libplctag's
ab_server answers CIP service 0x55 (Get Instance Attribute List) on the Symbol
Object class with "unsupported", so the `@tags` walk had no backend. #468's own
definition of done offered a sim that serves `@tags` as an alternative to real
hardware; this is that.

The fixture now applies a vendored patch teaching ab_server to serve the Symbol
Object listing, with partial-transfer paging. Correctness is checked against
libplctag's own `list_tags_logix` rather than against our decoder — our decoder
is the thing under test, so its agreement would prove nothing. The clone is also
pinned to a concrete tag: the Dockerfile called `--branch release` "pinned" while
tracking a branch that moves.

That immediately exposed a real defect. CipSymbolObjectDecoder advanced to an
even byte boundary after each symbol name, but entries are packed with no
padding — libplctag walks them by sizeof(entry) + string_len, and a captured
reply confirms it. Any odd-length name therefore desynchronised every entry
behind it, and odd-length names are ordinary. In the live browse below,
`Motor_Run` is 9 bytes and first on the wire, so the old decoder would have
shown that one tag and silently dropped the other eight.

Every existing multi-entry test passed throughout, because the test builder
padded exactly as the decoder did: the two agreed with each other and neither
agreed with a controller. The builder no longer pads, and the layout is pinned
by a captured reply (CipSymbolObjectWireCaptureTests) instead of by a second
statement of the assumption. Restoring the alignment step reddens exactly the
three new tests and none of the 29 others.

Live gate on docker-dev, all three of #468's criteria:
  - populated tree — all nine seeded tags render as leaves, in wire order
  - refresh re-captures
  - leaf commit — Pump_1_Speed + TestDINT saved with their CIP tag paths

The gate also surfaced two things it was not asked about: browse-commit drops
the discovered data type, so a browsed REAL commits as a DINT and the driver
decodes it as one (filed as #536), and Test Connect fails against this fixture
regardless of the device because it probes `@raw_cpu_type` (documented).

TwinCAT and FOCAS still have no browsable backend; #468's AbCip half is closed,
theirs is not.

Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
Joseph Doherty
2026-08-01 12:25:15 -04:00
parent 1a1dbc3f00
commit c2c3c52274
8 changed files with 485 additions and 24 deletions
@@ -11,8 +11,14 @@ public sealed class CipSymbolObjectDecoderTests
{
/// <summary>
/// Build one Symbol Object entry in the byte layout
/// <c>instance_id(u32) symbol_type(u16) element_length(u16) array_dims(u32×3) name_len(u16) name[len] pad</c>.
/// <c>instance_id(u32) symbol_type(u16) element_length(u16) array_dims(u32×3) name_len(u16) name[len]</c>.
/// </summary>
/// <remarks>
/// There is no trailing pad. This builder used to round the entry up to an even length,
/// which is what let the padding defect survive: the builder and the decoder shared one
/// wrong assumption, so every multi-entry test agreed with itself and none of them agreed
/// with a controller. <see cref="CipSymbolObjectWireCaptureTests"/> pins the real layout.
/// </remarks>
private static byte[] BuildEntry(
uint instanceId,
ushort symbolType,
@@ -23,7 +29,6 @@ public sealed class CipSymbolObjectDecoderTests
var nameBytes = Encoding.ASCII.GetBytes(name);
var nameLen = nameBytes.Length;
var totalLen = 22 + nameLen;
if ((totalLen & 1) != 0) totalLen++; // pad to even
var buf = new byte[totalLen];
BinaryPrimitives.WriteUInt32LittleEndian(buf.AsSpan(0), instanceId);
@@ -169,23 +174,48 @@ public sealed class CipSymbolObjectDecoderTests
tag.Name.ShouldBe("StepIndex");
}
/// <summary>Verifies that multiple entries decode in wire order with proper padding.</summary>
/// <summary>Verifies that multiple entries decode in wire order.</summary>
[Fact]
public void Multiple_entries_decode_in_wire_order_with_even_padding()
public void Multiple_entries_decode_in_wire_order()
{
// Name "Abc" is 3 bytes — triggers the even-pad branch between entries.
var bytes = Concat(
BuildEntry(1, 0xC4, 4, (0, 0, 0), "Abc"), // DINT named "Abc" (3-byte name, pads to 4)
BuildEntry(2, 0xCA, 4, (0, 0, 0), "Pi")); // REAL named "Pi"
BuildEntry(1, 0xC4, 4, (0, 0, 0), "Abcd"),
BuildEntry(2, 0xCA, 4, (0, 0, 0), "Pi"));
var tags = CipSymbolObjectDecoder.Decode(bytes).ToList();
tags.Count.ShouldBe(2);
tags[0].Name.ShouldBe("Abc");
tags[0].Name.ShouldBe("Abcd");
tags[0].DataType.ShouldBe(AbCipDataType.DInt);
tags[1].Name.ShouldBe("Pi");
tags[1].DataType.ShouldBe(AbCipDataType.Real);
}
/// <summary>
/// An odd-length name leaves the following entry on an odd offset, and the decoder must
/// read it there.
/// </summary>
/// <remarks>
/// This is the case Gitea #468 could not reach: every prior multi-entry test used a builder
/// that padded exactly as the decoder did, so the pair agreed with each other and neither
/// agreed with a controller. Odd-length names are not exotic — <c>TestArray</c>,
/// <c>Motor_Run</c> and <c>Line1_Count</c> are all odd, and one of them is enough to lose
/// every tag discovered after it.
/// </remarks>
[Fact]
public void An_entry_after_an_odd_length_name_still_decodes()
{
var bytes = Concat(
BuildEntry(1, 0xC4, 4, (0, 0, 0), "Abc"), // 3 bytes — next entry starts at 25
BuildEntry(2, 0xCA, 4, (0, 0, 0), "Pi"),
BuildEntry(3, 0xC1, 1, (0, 0, 0), "Motor_Run")); // 9 bytes — and again after this one
var tags = CipSymbolObjectDecoder.Decode(bytes).ToList();
tags.Select(t => t.Name).ShouldBe(["Abc", "Pi", "Motor_Run"]);
tags.Select(t => t.DataType)
.ShouldBe([AbCipDataType.DInt, AbCipDataType.Real, AbCipDataType.Bool]);
}
/// <summary>Verifies that truncated buffers stop decoding gracefully.</summary>
[Fact]
public void Truncated_buffer_stops_decoding_gracefully()