From 29b566d51b435d383a47566839706f8a0c85abe4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 14:24:05 -0400 Subject: [PATCH] fix(focas): disambiguate the cnc_rdalmmsg2 record layout at 1,520-byte multiples (R07:H20) ParseAlarms took the 76-byte vendor layout whenever Length % 76 == 0, so a legacy 80-byte reply of exactly 19 records (1,520 B = 20 x 76) decoded as 20 garbage vendor records. The response block carries no record count (its 16-byte header is length/class/path/ command/rc/payload length), so the layout is chosen from what is known: the record count the request asked for (a reply never holds more), then each layout's field invariants on every record. For legacy records, the int32 type/axis/length must fit a short and the length must be at most 64. For vendor records, the i16 length must be at most 64. If both still fit, the layout whose declared lengths match the decoded text wins. Vendor stays the default when both readings are equally plausible, and unambiguous lengths are unchanged. Refs #740 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../Wire/FocasWireClient.cs | 77 ++++++++++++++- .../FocasAlarmLayoutTests.cs | 96 +++++++++++++++++++ 2 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs index 53a23d2c..fd76bf66 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/Wire/FocasWireClient.cs @@ -1137,15 +1137,88 @@ public sealed class FocasWireClient : IAsyncDisposable, IDisposable /// The requested record count. /// The decoded alarms. /// The payload length matches neither record layout. + /// + /// Choosing the layout. The response block carries no record count, so a length that is a + /// multiple of BOTH strides (1,520 B = 20 × 76 = 19 × 80, and its multiples) is ambiguous; picking the + /// vendor stride on Length % 76 == 0 alone decoded 19 legacy records as 20 garbage ones. The choice + /// now uses what IS known: (1) the record count the REQUEST asked for — a reply never holds more, so a + /// layout implying more records than is ruled out; (2) each layout's own field + /// invariants, checked on every record ( / ). + /// Only when both readings remain plausible does the vendor layout win, as before. + /// internal static IReadOnlyList ParseAlarms(byte[] payload, short count) { - if (payload.Length % 76 == 0) return ParseVendorAlarms(payload, count); - if (payload.Length % 80 == 0) return ParseLegacyWireAlarms(payload, count); + if (payload.Length == 0) return []; + var vendor = payload.Length % VendorAlarmStride == 0 && payload.Length / VendorAlarmStride <= Math.Max((int)count, 1); + var legacy = payload.Length % LegacyAlarmStride == 0 && payload.Length / LegacyAlarmStride <= Math.Max((int)count, 1); + if (vendor && legacy) + { + // Ambiguous length: a legacy record's int32 type/axis/length fields must each fit the short they are + // read into and its length must fit the 64-byte message — a vendor payload read at the legacy stride + // fails that on its first non-empty message. Prefer the reading whose every record is self-consistent. + var legacyOk = LegacyRecordsValid(payload); + var vendorOk = VendorRecordsValid(payload); + if (legacyOk && !vendorOk) vendor = false; + else if (legacyOk && vendorOk && LengthsMatchText(payload, LegacyAlarmStride, 12, 4, 16) + && !LengthsMatchText(payload, VendorAlarmStride, 10, 2, 12)) vendor = false; + } + if (vendor) return ParseVendorAlarms(payload, count); + if (legacy) return ParseLegacyWireAlarms(payload, count); + if (payload.Length % VendorAlarmStride == 0 || payload.Length % LegacyAlarmStride == 0) + { + // Fits a stride but holds more records than were requested: fall back to the stride it fits, capped + // at the count, exactly as before this check existed. + return payload.Length % VendorAlarmStride == 0 + ? ParseVendorAlarms(payload, count) + : ParseLegacyWireAlarms(payload, count); + } throw new FocasWireException( $"cnc_rdalmmsg2 returned a {payload.Length}-byte payload, which fits neither the 76- nor the 80-byte " + "record layout; refusing to read it as an empty alarm list.", rc: null); } + private const int VendorAlarmStride = 76; + private const int LegacyAlarmStride = 80; + private const int AlarmMessageBytes = 64; + + /// Every 80-byte record's int32 type / axis / length fit a short, and the length fits the message. + private static bool LegacyRecordsValid(byte[] payload) + { + for (var o = 0; o + LegacyAlarmStride <= payload.Length; o += LegacyAlarmStride) + { + var type = ReadInt32(payload, o + 4); + var axis = ReadInt32(payload, o + 8); + var length = ReadInt32(payload, o + 12); + if (type is < short.MinValue or > short.MaxValue) return false; + if (axis is < short.MinValue or > short.MaxValue) return false; + if (length is < 0 or > AlarmMessageBytes) return false; + } + return true; + } + + /// Every 76-byte record's i16 message length fits the 64-byte message. + private static bool VendorRecordsValid(byte[] payload) + { + for (var o = 0; o + VendorAlarmStride <= payload.Length; o += VendorAlarmStride) + { + var length = ReadInt16(payload, o + 10); + if (length is < 0 or > AlarmMessageBytes) return false; + } + return true; + } + + /// Every record's declared message length equals the length of the text actually in its message field. + private static bool LengthsMatchText(byte[] payload, int stride, int lengthOffset, int lengthWidth, int messageOffset) + { + for (var o = 0; o + stride <= payload.Length; o += stride) + { + var length = lengthWidth == 4 ? ReadInt32(payload, o + lengthOffset) : ReadInt16(payload, o + lengthOffset); + var text = FocasWireProtocol.ReadAscii(payload.AsSpan(o + messageOffset, AlarmMessageBytes)); + if (length != text.Length) return false; + } + return true; + } + private static IReadOnlyList ParseVendorAlarms(byte[] payload, short count) { var alarms = new List(); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs new file mode 100644 index 00000000..bc1bc1b2 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasAlarmLayoutTests.cs @@ -0,0 +1,96 @@ +using System.Buffers.Binary; +using System.Text; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Wire; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// #740 / R07:H20 — ParseAlarms picked the 76-byte vendor layout whenever Length % 76 == 0, so a +/// legacy 80-byte reply of exactly 19 records (1,520 B = 20 × 76) decoded as 20 garbage vendor records. The +/// response block carries no record count, so the layout is now chosen from the two things that are known: the +/// count the REQUEST asked for (a reply never holds more) and each layout's own field invariants. +/// +[Trait("Category", "Unit")] +public sealed class FocasAlarmLayoutTests +{ + private static byte[] Legacy(params (int no, int type, int axis, string msg)[] records) + { + var buf = new byte[records.Length * 80]; + for (var i = 0; i < records.Length; i++) + { + var o = i * 80; + var (no, type, axis, msg) = records[i]; + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o, 4), no); + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o + 4, 4), type); + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o + 8, 4), axis); + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o + 12, 4), msg.Length); + Encoding.ASCII.GetBytes(msg).CopyTo(buf.AsSpan(o + 16)); + } + return buf; + } + + private static byte[] Vendor(params (int no, short type, short axis, string msg)[] records) + { + var buf = new byte[records.Length * 76]; + for (var i = 0; i < records.Length; i++) + { + var o = i * 76; + var (no, type, axis, msg) = records[i]; + BinaryPrimitives.WriteInt32BigEndian(buf.AsSpan(o, 4), no); + BinaryPrimitives.WriteInt16BigEndian(buf.AsSpan(o + 4, 2), type); + BinaryPrimitives.WriteInt16BigEndian(buf.AsSpan(o + 6, 2), axis); + BinaryPrimitives.WriteInt16BigEndian(buf.AsSpan(o + 10, 2), (short)msg.Length); + Encoding.ASCII.GetBytes(msg).CopyTo(buf.AsSpan(o + 12)); + } + return buf; + } + + private static (int, int, int, string)[] LegacySet(int n) => + [.. Enumerable.Range(0, n).Select(i => (400 + i, 2 + (i % 3), 1 + (i % 4), $"SERVO ALARM {400 + i}"))]; + + private static (int, short, short, string)[] VendorSet(int n) => + [.. Enumerable.Range(0, n).Select(i => (1000 + i, (short)(4 + (i % 3)), (short)(i % 2), $"OVERTRAVEL +{i}"))]; + + /// The ambiguous length: 19 legacy records decode as 19 legacy records, not 20 vendor ones. + [Fact] + public void Nineteen_legacy_records_decode_as_legacy() + { + var payload = Legacy(LegacySet(19)); + payload.Length.ShouldBe(1520); + (payload.Length % 76).ShouldBe(0, "the length really is ambiguous"); + + var alarms = FocasWireClient.ParseAlarms(payload, 32); + + alarms.Count.ShouldBe(19); + alarms[0].AlarmNumber.ShouldBe(400); + alarms[0].Message.ShouldBe("SERVO ALARM 400"); + alarms[18].AlarmNumber.ShouldBe(418); + alarms[18].Axis.ShouldBe((short)(1 + (18 % 4))); + } + + /// Positive control at the same length: 20 vendor records still decode as vendor. + [Fact] + public void Twenty_vendor_records_decode_as_vendor() + { + var payload = Vendor(VendorSet(20)); + payload.Length.ShouldBe(1520); + + var alarms = FocasWireClient.ParseAlarms(payload, 32); + + alarms.Count.ShouldBe(20); + alarms[0].AlarmNumber.ShouldBe(1000); + alarms[19].Message.ShouldBe("OVERTRAVEL +19"); + } + + /// The unambiguous lengths are unchanged. + [Fact] + public void Unambiguous_lengths_are_unchanged() + { + FocasWireClient.ParseAlarms(Vendor(VendorSet(3)), 32).Count.ShouldBe(3); + FocasWireClient.ParseAlarms(Legacy(LegacySet(3)), 32).Count.ShouldBe(3); + FocasWireClient.ParseAlarms([], 32).ShouldBeEmpty(); + Should.Throw(() => FocasWireClient.ParseAlarms(new byte[77], 32)); + } +}