From d758dd1e168261a3c0fcd6a6e7751a250229fd99 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 22 Oct 2026 15:20:10 -0400 Subject: [PATCH] fix(ablegacy): encode array writes at the data file's element width Review r0 I3. The array encoder used the AUTHORED dataType's width while both runtimes size their buffer from the FILE: an Int array on F8 wrote four shorts into a 16-byte float buffer and sent it all (garbage neighbours, answered Good); Long/Float on N overran it. - The write path resolves the element width from the data file the address names (N/B/A/I/O/S 16-bit word, L 32-bit, F float) and encodes at THAT width. - An authored type that does not match its file is refused BadTypeMismatch before any runtime exists (warned once, no health write). - The same rule (AbLegacyTagDefinitionFactory.ArrayFileMismatch) backs an Inspect deploy warning and the AdminUI model's Validate error; FromTagConfig still accepts the tag so persisted configs keep loading. - M6: the skipped F1 parity test gains the shapes the write path refuses (/bit, .sub, indirect, type/file mismatch, PLC-5 native). Tests: one case per file type, a mismatch per pairing with nothing created, and a positive control. Refs #738 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT --- .../AbLegacyTagDefinitionFactory.cs | 51 +++++++++++++++ .../AbLegacyDriver.cs | 25 ++++++-- .../Uns/TagEditors/AbLegacyTagConfigModel.cs | 5 ++ .../AbLegacyArrayWriteSupportParityTests.cs | 19 +++++- .../AbLegacyArrayWriteTests.cs | 62 +++++++++++++++++++ .../Uns/AbLegacyTagConfigModelTests.cs | 18 ++++++ 6 files changed, 175 insertions(+), 5 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs index c6ab0a97..ea87f079 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs @@ -177,6 +177,9 @@ public static class AbLegacyTagDefinitionFactory { if (AbLegacyAddress.Validate(accepted.Address, family) is { } addressError) warnings.Add($"AbLegacy tag address: {addressError} The driver reads this tag as BadNodeIdUnknown."); + else if (accepted.ArrayLength is not null + && ArrayFileMismatch(accepted.DataType, accepted.Address) is { } widthError) + warnings.Add(widthError); return warnings; } @@ -252,6 +255,54 @@ public static class AbLegacyTagDefinitionFactory $"AbLegacy {type} is not supported as an array element (String, Timer, Counter and Control elements are " + "structured); author a scalar tag per element instead. The driver skips this tag and it reads BadNodeIdUnknown."; + /// + /// The element width an array on 's data file has, as the data type that decodes / + /// encodes it: N, B, A and the I/O/S word files are 16-bit words (), L is + /// 32-bit (), F is IEEE float (). + /// for a file with no flat array layout (ST, T, C, R, function files) or an address that + /// does not parse. + /// + /// The authored address. + /// The file's element type, or . + public static AbLegacyDataType? ArrayFileElementType(string address) => + AbLegacyAddress.TryParse(address) is { } a ? ArrayFileElementType(a) : null; + + /// + public static AbLegacyDataType? ArrayFileElementType(AbLegacyAddress a) => a.FileLetter switch + { + "N" or "B" or "A" or "I" or "O" or "S" => AbLegacyDataType.Int, + "L" => AbLegacyDataType.Long, + "F" => AbLegacyDataType.Float, + _ => null, + }; + + /// + /// Why an ARRAY authored as on does not match its data + /// file's element width (review 07 r0 I3), or when it does. Int / AnalogInt need a 16-bit + /// word file (N/B/A/I/O/S), Long an L file, Float an F file; Bit arrays are bit-packed words and fit any 16-bit + /// word file. A mismatch reads the file at the wrong width (garbage) and the driver refuses to WRITE it + /// (BadTypeMismatch), because encoding at the authored width would corrupt the neighbouring elements. + /// + /// The authored element data type. + /// The authored address. + /// The mismatch message, or . + public static string? ArrayFileMismatch(AbLegacyDataType type, string address) + { + if (!IsArrayElementTypeSupported(type)) return null; // structured arrays have their own refusal + if (AbLegacyAddress.TryParse(address) is not { } a || a.IsIndirect) return null; + var file = ArrayFileElementType(a); + var ok = type switch + { + AbLegacyDataType.Int or AbLegacyDataType.AnalogInt or AbLegacyDataType.Bit => file == AbLegacyDataType.Int, + _ => file == type, + }; + return ok + ? null + : $"AbLegacy {type} array on '{address}' does not match the {a.FileLetter} file's element width " + + "(Int/AnalogInt/Bit need an N/B/A word file, Long an L file, Float an F file): reads decode the wrong width " + + "and writes are refused BadTypeMismatch."; + } + private static int ReadInt(JsonElement o, string name) => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.Number && e.TryGetInt32(out var v) ? v : 0; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs index 899e2c38..ffc96f46 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyDriver.cs @@ -885,12 +885,29 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag // Array writes (review 07 U10): convert and length-check BEFORE any runtime exists, then encode every // element. A bit / sub-element array has no flat encoding. + // The encode width is the DATA FILE's element width (review 07 r0 I3), never the authored type's: an + // authored type that does not match its file is refused BadTypeMismatch before anything is encoded. Array? arrayValue = null; + AbLegacyDataType arrayFileType = default; if (IsArrayTag(def)) { - var conversion = parsed.BitIndex is not null || parsed.SubElement is not null - ? AbLegacyStatusMapper.BadNotSupported - : AbLegacyArrayWriteEncoder.TryConvert(def.DataType, w.Value, EffectiveArrayLength(def), out arrayValue); + var fileType = AbLegacyTagDefinitionFactory.ArrayFileElementType(parsed); + var widthRefusal = AbLegacyTagDefinitionFactory.ArrayFileMismatch(def.DataType, def.Address) + ?? (fileType is null ? $"{def.Address} has no flat array layout." : null); + uint conversion; + if (parsed.BitIndex is not null || parsed.SubElement is not null || def.DataType == AbLegacyDataType.Bit) + conversion = AbLegacyStatusMapper.BadNotSupported; + else if (widthRefusal is not null || fileType is null) + { + WarnRefusalOnce(device, def, "write", widthRefusal ?? "no flat array layout", AbLegacyStatusMapper.BadTypeMismatch); + results[i] = new WriteResult(AbLegacyStatusMapper.BadTypeMismatch); + continue; + } + else + { + arrayFileType = fileType.Value; + conversion = AbLegacyArrayWriteEncoder.TryConvert(arrayFileType, w.Value, EffectiveArrayLength(def), out arrayValue); + } if (conversion != AbLegacyStatusMapper.Good) { if (conversion == AbLegacyStatusMapper.BadNotSupported) @@ -925,7 +942,7 @@ public sealed partial class AbLegacyDriver : IDriver, IReadable, IWritable, ITag await opLock.WaitAsync(cancellationToken).ConfigureAwait(false); try { - if (arrayValue is not null) runtime.EncodeArray(def.DataType, arrayValue); + if (arrayValue is not null) runtime.EncodeArray(arrayFileType, arrayValue); else runtime.EncodeValue(def.DataType, parsed.BitIndex, w.Value); await runtime.WriteAsync(cancellationToken).ConfigureAwait(false); status = runtime.GetStatus(); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs index dcb67114..4d5a88ee 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs @@ -92,6 +92,11 @@ public sealed class AbLegacyTagConfigModel if (isArray && arrayLength >= 1 && !AbLegacyTagDefinitionFactory.IsArrayElementTypeSupported(DataType)) return AbLegacyTagDefinitionFactory.ArrayElementTypeError(DataType); + // An array must match its data file's element width (review 07 r0 I3): the driver refuses to write a + // mismatched one, and reads it at the wrong width. + if (isArray && arrayLength >= 1 && AbLegacyTagDefinitionFactory.ArrayFileMismatch(DataType, Address.Trim()) is { } widthError) + return widthError; + // Parse-gate backstop: the checks above mirror the reader's whole rejection surface today, so this is // expected to be silent for AbLegacy. It is here so a future reader rule cannot become a gap. return TagConfigParseGate.Describe( diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteSupportParityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteSupportParityTests.cs index acae0cdd..3f5a50f6 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteSupportParityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteSupportParityTests.cs @@ -43,7 +43,24 @@ public sealed class AbLegacyArrayWriteSupportParityTests DriverArrayWriteSupport.Supports(DriverTypeNames.AbLegacy, "Int16", json).ShouldBe(encoderAccepts, dataType.ToString()); } - /// What the encoder accepts today — the row F1 must add (Int, AnalogInt, Long, Float). + /// + /// The shapes the WRITE PATH refuses although their element type alone looks writable (review 07 r0 M6): a + /// /bit or .sub address, an indirect address, a type that does not match its data file (I3), and + /// a native PLC-5 device. The F1 row must answer false for every one — keyed on dataType alone it would + /// advertise them writable. Skipped with the parity theory until F1 lands. + /// + [Theory(Skip = "F1: DriverArrayWriteSupport has no AbLegacy row yet (Core.Abstractions is outside this lane)")] + [InlineData("""{"address":"N7:0/3","dataType":"Int","isArray":true,"arrayLength":2}""", "{}")] + [InlineData("""{"address":"T4:0.ACC","dataType":"Int","isArray":true,"arrayLength":2}""", "{}")] + [InlineData("""{"address":"N7:[N7:0]","dataType":"Int","isArray":true,"arrayLength":2}""", "{}")] + [InlineData("""{"address":"F8:0","dataType":"Int","isArray":true,"arrayLength":2}""", "{}")] + [InlineData("""{"address":"N7:0","dataType":"Long","isArray":true,"arrayLength":2}""", "{}")] + [InlineData("""{"address":"N7:0","dataType":"Int","isArray":true,"arrayLength":2}""", + """{"transport":"Native","devices":[{"hostAddress":"ab://10.0.0.5/1,0","plcFamily":"Plc5"}]}""")] + public void Table_refuses_the_shapes_the_write_path_refuses(string tagConfig, string driverConfig) => + DriverArrayWriteSupport.Supports(DriverTypeNames.AbLegacy, "Int16", tagConfig, driverConfig).ShouldBeFalse(); + + /// What the encoder accepts today — the row F1 must add (Int, AnalogInt, Long, Float on a matching file). [Theory] [InlineData(AbLegacyDataType.Int, true)] [InlineData(AbLegacyDataType.AnalogInt, true)] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteTests.cs index 0ef846a0..20d9f203 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyArrayWriteTests.cs @@ -72,6 +72,68 @@ public sealed class AbLegacyArrayWriteTests drv.GetHealth().State.ShouldBe(DriverState.Healthy); } + /// Review 07 r0 I3: every data file type encodes at ITS element width (one case per file). + [Theory] + [InlineData("N7:0", AbLegacyDataType.Int, AbLegacyDataType.Int)] + [InlineData("N7:0", AbLegacyDataType.AnalogInt, AbLegacyDataType.Int)] + [InlineData("B3:0", AbLegacyDataType.Int, AbLegacyDataType.Int)] + [InlineData("A9:0", AbLegacyDataType.Int, AbLegacyDataType.Int)] + [InlineData("L9:0", AbLegacyDataType.Long, AbLegacyDataType.Long)] + [InlineData("F8:0", AbLegacyDataType.Float, AbLegacyDataType.Float)] + public async Task Each_file_type_encodes_at_the_file_element_width(string address, AbLegacyDataType authored, AbLegacyDataType fileWidth) + { + var (drv, f) = await NewAsync(authored, address, 2); + + var r = await drv.WriteAsync([new WriteRequest("Arr", new[] { 1, 2 })], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + var tag = (RecordingTag)f.Tags[address]; + tag.EncodedType.ShouldBe(fileWidth); + tag.Encoded!.GetType().ShouldBe(fileWidth switch + { + AbLegacyDataType.Long => typeof(int[]), + AbLegacyDataType.Float => typeof(float[]), + _ => typeof(short[]), + }); + tag.WriteCount.ShouldBe(1); + } + + /// + /// Review 07 r0 I3: an array whose authored type does not match its data file (Int on F8, Long on N7, Float on + /// L9) used to be encoded at the AUTHORED width into the file-typed buffer and sent — garbage in the + /// neighbouring elements, answered Good. It is now refused BadTypeMismatch before any runtime exists. + /// + [Theory] + [InlineData("F8:0", AbLegacyDataType.Int)] + [InlineData("N7:0", AbLegacyDataType.Long)] + [InlineData("N7:0", AbLegacyDataType.Float)] + [InlineData("L9:0", AbLegacyDataType.Float)] + [InlineData("L9:0", AbLegacyDataType.Int)] + public async Task A_type_that_does_not_match_its_file_is_refused_and_never_written(string address, AbLegacyDataType authored) + { + var (drv, f) = await NewAsync(authored, address, 4); + + var r = await drv.WriteAsync([new WriteRequest("Arr", new[] { 1, 2, 3, 4 })], CancellationToken.None); + + r[0].StatusCode.ShouldBe(AbLegacyStatusMapper.BadTypeMismatch); + f.Tags.ShouldBeEmpty(); // nothing created, nothing on the wire + drv.GetHealth().State.ShouldBe(DriverState.Healthy); + AbLegacyTagDefinitionFactory.Inspect( + $$"""{"address":"{{address}}","dataType":"{{authored}}","isArray":true,"arrayLength":4}""") + .ShouldContain(w => w.Contains("element width")); // and the deploy gate names it + } + + [Fact] + public async Task Positive_control_a_Float_array_on_an_F_file_is_written() + { + var (drv, f) = await NewAsync(AbLegacyDataType.Float, "F8:0", 4); + (await drv.WriteAsync([new WriteRequest("Arr", new[] { 1, 2, 3, 4 })], CancellationToken.None))[0] + .StatusCode.ShouldBe(AbLegacyStatusMapper.Good); + ((RecordingTag)f.Tags["F8:0"]).Encoded.ShouldBe(new[] { 1f, 2f, 3f, 4f }); + AbLegacyTagDefinitionFactory.Inspect("""{"address":"F8:0","dataType":"Float","isArray":true,"arrayLength":4}""") + .ShouldBeEmpty(); + } + [Fact] public async Task Float_and_Long_arrays_encode_at_their_element_width() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs index 2bf866bb..5d8ebfe1 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/AbLegacyTagConfigModelTests.cs @@ -136,6 +136,24 @@ public sealed class AbLegacyTagConfigModelTests AbLegacyTagConfigModel.FromJson($$"""{"address":"{{address}}","dataType":"{{dataType}}"}""").Validate().ShouldBeNull(); } + /// Review 07 r0 I3: an array must match its data file's element width. + [Theory] + [InlineData("F8:0", "Int", false)] + [InlineData("N7:0", "Long", false)] + [InlineData("N7:0", "Int", true)] + [InlineData("L9:0", "Long", true)] + [InlineData("F8:0", "Float", true)] + public void Validate_refuses_an_array_whose_type_does_not_match_its_file(string address, string dataType, bool valid) + { + var m = AbLegacyTagConfigModel.FromJson( + $$"""{"address":"{{address}}","dataType":"{{dataType}}","isArray":true,"arrayLength":4}"""); + + if (valid) m.Validate().ShouldBeNull(); + else m.Validate().ShouldNotBeNull().ShouldContain("element width"); + // A SCALAR of the same shape is not this rule's business. + AbLegacyTagConfigModel.FromJson($$"""{"address":"{{address}}","dataType":"{{dataType}}"}""").Validate().ShouldBeNull(); + } + [Fact] public void Validate_returns_null_for_scalar_tag_regardless_of_no_array_length() {