diff --git a/archreview/plans/R2-11-tagconfig-consolidation-plan.md.tasks.json b/archreview/plans/R2-11-tagconfig-consolidation-plan.md.tasks.json index baa6b73e..aef572f3 100644 --- a/archreview/plans/R2-11-tagconfig-consolidation-plan.md.tasks.json +++ b/archreview/plans/R2-11-tagconfig-consolidation-plan.md.tasks.json @@ -13,7 +13,7 @@ { "id": "T9", "subject": "EquipmentNodeWalker.ExtractFullName delegates to TagConfigIntent; repoint tree-wide canonical-parser doc cites", "status": "completed", "blockedBy": ["T2", "T3"] }, { "id": "T10", "subject": "Retire OpcUaServer.Tests ExtractTag*Tests (authority moved to Commons.Tests); verify zero 'MUST parse identically' hits", "status": "completed", "blockedBy": ["T3", "T5"] }, { "id": "T11", "subject": "TagConfigJson strict-capable readers in Core.Abstractions (TryReadEnum Absent/Valid/Invalid, ReadEnumOrDefault, ReadWritable, DescribeInvalidEnum) (TDD)", "status": "completed", "blockedBy": [] }, - { "id": "T12", "subject": "Modbus equipment-tag parser: freeze test, shared readers, honour writable key, Inspect warnings", "status": "pending", "blockedBy": ["T11"] }, + { "id": "T12", "subject": "Modbus equipment-tag parser: freeze test, shared readers, honour writable key, Inspect warnings", "status": "completed", "blockedBy": ["T11"] }, { "id": "T13", "subject": "S7 equipment-tag parser: freeze test, shared readers, honour writable key, Inspect warnings (+ amend Contracts banner)", "status": "pending", "blockedBy": ["T11"] }, { "id": "T14", "subject": "AbCip equipment-tag parser: freeze test, shared readers (writable already honoured), Inspect warnings", "status": "pending", "blockedBy": ["T11"] }, { "id": "T15", "subject": "AbLegacy equipment-tag parser: freeze test, shared readers, honour writable key, Inspect warnings", "status": "pending", "blockedBy": ["T11"] }, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs index 88e3b40a..a9ce79a7 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs @@ -1,4 +1,5 @@ using System.Text.Json; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus; @@ -29,9 +30,9 @@ public static class ModbusEquipmentTagParser || !addr.TryGetInt32(out var address) || address < 0 || address > ushort.MaxValue) return false; - var region = ReadEnum(root, "region", ModbusRegion.HoldingRegisters); - var dataType = ReadEnum(root, "dataType", ModbusDataType.Int16); - var byteOrder = ReadEnum(root, "byteOrder", ModbusByteOrder.BigEndian); + var region = TagConfigJson.ReadEnumOrDefault(root, "region", ModbusRegion.HoldingRegisters); + var dataType = TagConfigJson.ReadEnumOrDefault(root, "dataType", ModbusDataType.Int16); + var byteOrder = TagConfigJson.ReadEnumOrDefault(root, "byteOrder", ModbusByteOrder.BigEndian); var bitIndex = (byte)ReadInt(root, "bitIndex"); var stringLength = (ushort)ReadInt(root, "stringLength"); // Guard: String tags require StringLength >= 1. RegisterCount = (StringLength+1)/2, @@ -51,9 +52,12 @@ public static class ModbusEquipmentTagParser var isArray = ReadBool(root, "isArray"); var arrayLength = ReadInt(root, "arrayLength"); int? arrayCount = (isArray && arrayLength >= 1) ? arrayLength : null; + // "writable" defaults to true when absent (today's value) — makes read-only equipment tags + // authorable (UNDER-6). Node-level authz remains the effective write gate. + var writable = TagConfigJson.ReadWritable(root); def = new ModbusTagDefinition( Name: reference, Region: region, Address: (ushort)address, DataType: dataType, - Writable: true, ByteOrder: byteOrder, BitIndex: bitIndex, StringLength: stringLength, + Writable: writable, ByteOrder: byteOrder, BitIndex: bitIndex, StringLength: stringLength, ArrayCount: arrayCount); return true; } @@ -62,9 +66,44 @@ public static class ModbusEquipmentTagParser catch (InvalidOperationException) { return false; } } - private static TEnum ReadEnum(JsonElement o, string name, TEnum fallback) where TEnum : struct, Enum - => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.String - && Enum.TryParse(e.GetString(), ignoreCase: true, out var v) ? v : fallback; + /// + /// Deploy-time inspection of an equipment-tag TagConfig blob (05/CONV-2). Returns + /// human-readable warnings for present-but-invalid enum values (region / dataType / + /// byteOrder — which the lenient runtime silently defaults) and for a structurally + /// unparseable TagConfig (which the runtime turns into a silent BadNodeIdUnknown). Empty when + /// the blob is clean or is not an equipment-tag TagConfig object. Never throws. + /// + /// The equipment tag's TagConfig JSON. + /// The warnings; empty when clean. + public static IReadOnlyList Inspect(string reference) + { + var warnings = new List(); + if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + try + { + using var doc = JsonDocument.Parse(reference); + var root = doc.RootElement; + if (root.ValueKind != JsonValueKind.Object) + { + warnings.Add("Modbus TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; + } + foreach (var w in new[] + { + TagConfigJson.DescribeInvalidEnum(root, "region"), + TagConfigJson.DescribeInvalidEnum(root, "dataType"), + TagConfigJson.DescribeInvalidEnum(root, "byteOrder"), + }) + { + if (w is not null) warnings.Add(w); + } + } + catch (JsonException) + { + warnings.Add("Modbus TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + } + return warnings; + } private static int ReadInt(JsonElement o, string name) => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.Number diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts.csproj b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts.csproj index 98b2207c..0c2ad3ab 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts.csproj +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts.csproj @@ -12,6 +12,9 @@ and was designed for exactly this use — Admin can reference it without transport-layer deps. --> + + diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusEquipmentTagParserStrictnessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusEquipmentTagParserStrictnessTests.cs new file mode 100644 index 00000000..56690441 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusEquipmentTagParserStrictnessTests.cs @@ -0,0 +1,71 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// R2-11 (05/CONV-2) strictness surface for the Modbus equipment-tag parser: the runtime stays lenient +/// (freeze), the new Inspect reports the silently-defaulted typo, and the writable key is +/// now honoured (default true). +/// +public sealed class ModbusEquipmentTagParserStrictnessTests +{ + // ---- Phase-A behaviour freeze: a typo'd enum still silently defaults ---- + + [Fact] + public void Freeze_typo_dataType_still_defaults_to_Int16() + { + ModbusEquipmentTagParser.TryParse( + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Intt16\"}", out var def) + .ShouldBeTrue(); + def.DataType.ShouldBe(ModbusDataType.Int16); + } + + // ---- Inspect reports the typo ---- + + [Fact] + public void Inspect_reports_typo_dataType_with_field_and_valid_values() + { + var warnings = ModbusEquipmentTagParser.Inspect( + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Intt16\"}"); + warnings.ShouldHaveSingleItem(); + warnings[0].ShouldContain("Intt16"); + warnings[0].ShouldContain("dataType"); + warnings[0].ShouldContain("Int16"); + } + + [Fact] + public void Inspect_clean_config_has_no_warnings() + { + ModbusEquipmentTagParser.Inspect( + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Int16\"}") + .ShouldBeEmpty(); + } + + [Fact] + public void Inspect_malformed_json_warns_unresolvable() + { + ModbusEquipmentTagParser.Inspect("{not json").ShouldHaveSingleItem(); + } + + // ---- writable now honoured ---- + + [Fact] + public void Writable_false_is_honoured() + { + ModbusEquipmentTagParser.TryParse( + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Int16\",\"writable\":false}", out var def) + .ShouldBeTrue(); + def.Writable.ShouldBeFalse(); + } + + [Fact] + public void Writable_absent_defaults_to_true() + { + ModbusEquipmentTagParser.TryParse( + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Int16\"}", out var def) + .ShouldBeTrue(); + def.Writable.ShouldBeTrue(); + } +}