diff --git a/docs/v2/modbus-addressing.md b/docs/v2/modbus-addressing.md index 42f3fab8..602d5170 100644 --- a/docs/v2/modbus-addressing.md +++ b/docs/v2/modbus-addressing.md @@ -243,15 +243,19 @@ than asking users to rewrite spreadsheets. ## Address strings, families and region/type rules (#741) -- A tag's `addressString` is resolved through `ModbusAddressParser` with the - **driver's** `Family` / `MelsecSubFamily`. When the AdminUI editor resolves - it ("Resolve with driver family") it persists the resolved - `region`/`address`/type fields plus `"addressResolved": true`; those pinned - fields are then authoritative, so a later family change never remaps the - tag. A blob with an **unmarked** `addressString` (CSV / API authoring) is - always resolved from the string — any `region`/`address` beside it is - ignored, so it can never collapse onto HR[0]. Editing a resolved field by - hand drops the string. +- **Whenever a tag carries both `region` and `address`, those keys are its + address** — a classic tag, an editor-pinned tag, and a pre-#741 row that + carried an `addressString` beside its keys (the old driver never read the + string) all load from the keys, exactly as before, and a family change never + remaps them. Only a **string-only** blob (an `addressString` with NO + `region`/`address` — CSV / API authoring) is resolved through + `ModbusAddressParser` with the **driver's** `Family` / `MelsecSubFamily`; the + AdminUI never stamps default keys onto it, so it cannot collapse onto HR[0], + and a family that cannot parse it refuses the tag (BadNodeIdUnknown). +- The editor's "Resolve with driver family" persists the resolved + `region`/`address`/type fields plus `"addressResolved": true` (the pinned + state). Editing a resolved field by hand — in the editor or a typed CSV + address cell — drops the string and the marker. - Coils / DiscreteInputs carry only Bool. A bit-region tag persisted with a register `dataType` (the pre-#741 editor saved `Int16`) is coerced to Bool, never refused. A register region with `Bool` is refused (use diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs index 32b4adf6..ff21ef5e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs @@ -35,11 +35,10 @@ public static class ModbusTagDefinitionFactory => FromTagConfig(tagConfig, rawPath, ModbusFamily.Generic, MelsecFamily.Q_L_iQR, out def); /// - /// Family-aware overload. The persisted region + address keys are AUTHORITATIVE when they - /// were actually authored — a classic tag with no addressString, or one the editor resolved and - /// marked () — so a later Family / MelsecSubFamily change can never - /// silently remap such a tag. A blob whose addressString is unmarked (CSV / API authoring, even if - /// a model round-trip stamped default region/address beside it; the Modicon / mnemonic / family-native + /// Family-aware overload. The persisted region + address keys are AUTHORITATIVE whenever + /// both are present (classic, editor-pinned, or a pre-#741 row with a string beside them), so a later + /// Family / MelsecSubFamily change can never silently remap such a tag. A blob carrying an + /// addressString and NO region/address keys (CSV / API string-only authoring; the Modicon / mnemonic / family-native /// grammar of — 40001:F:CDAB, DL205 V2000, MELSEC /// D100) is resolved here with the DRIVER's / /// . Without an addressString this is exactly the 3-argument @@ -177,25 +176,23 @@ public static class ModbusTagDefinitionFactory /// /// The key the tag editor writes beside the resolved region / address when it resolved an - /// addressString with the driver's family ("addressResolved": true). Its presence — not the - /// mere presence of region / address — is what makes the resolved keys authoritative over - /// the string: the AdminUI model stamps DEFAULTED region/address (HoldingRegisters / 0) on - /// every save, so key presence alone turned a string-only CSV/API tag into HR[0] on its first edit. + /// addressString with the driver's family ("addressResolved": true). It records the editor's + /// pinned state (display / CSV) — the DRIVER rule is key presence (see + /// ), because pre-#741 rows carry keys + string with no marker and + /// must keep loading from their keys. /// public const string AddressResolvedKey = "addressResolved"; /// - /// Whether the persisted region / address are the tag's real address: they are present AND - /// either there is no addressString (a classic region + address tag) or the editor marked them as - /// resolved from it (). A blob carrying an addressString without - /// the marker resolves the string through the parser, never the (possibly defaulted) keys. + /// Whether the persisted region / address are the tag's address: whenever BOTH keys are + /// present. Every shape that carries them — a classic tag, an editor-pinned tag, and a pre-#741 row that + /// carried an addressString beside its keys (the old driver never read the string) — loads from + /// the keys, exactly as before the lane. "String-only" means the keys are ABSENT; only that shape is + /// resolved through the parser with the driver's family, and the AdminUI model never stamps keys onto + /// it, so it cannot collapse onto HR[0]. /// private static bool ResolvedKeysAuthoritative(JsonElement root, string? addressText) - { - if (!root.TryGetProperty("region", out _) || !root.TryGetProperty("address", out _)) return false; - if (addressText is null) return true; - return root.TryGetProperty(AddressResolvedKey, out var marker) && marker.ValueKind == JsonValueKind.True; - } + => root.TryGetProperty("region", out _) && root.TryGetProperty("address", out _); /// Reads the optional addressString: null when absent or blank; flags a non-string value. private static string? ReadAddressString(JsonElement root, out bool invalid) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor index 42fb01eb..06b6eb3b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor @@ -18,6 +18,10 @@ {
String-only tag: the driver resolves @_m.AddressString with its own family (here @_family). The fields below are NOT its address — press Resolve to pin them, or edit a field to replace the string.
} + else if (_m.HasLegacyAddressString) + { +
This tag carries a legacy address string @_m.AddressString that the driver does not use: the fields below are its address. Press Resolve to re-pin it from the string with the driver family.
+ } else if (_m.AddressString is not null) {
Resolved from @_m.AddressString (family @_family) and pinned: the fields below are what the driver polls, so a later family change does not remap the tag. Editing a field drops the string.
diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CsvColumnMap.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CsvColumnMap.cs index c3daa298..c923f0f8 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CsvColumnMap.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CsvColumnMap.cs @@ -28,11 +28,18 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; /// and fullName/dataItemId/address) — see for why the CSV /// path has to know the whole set and not just the one it writes. /// +/// +/// Root keys a NON-BLANK cell in this column invalidates: they are neither restored from the base cell nor +/// owned for export. Modbus's address columns use it for addressString / addressResolved — a +/// typed address cell is an authored address, exactly like a hand edit in the editor, so a stale address +/// string (and its pinned marker) must not survive beside it. +/// public sealed record CsvTypedColumn( string Header, string JsonKey, string? ModelProperty, - IReadOnlyList? ReadKeys = null) + IReadOnlyList? ReadKeys = null, + IReadOnlyList? ClearsKeys = null) { /// /// Every root JSON key this column speaks for, in the runtime's read precedence — i.e. @@ -223,6 +230,7 @@ public sealed class ModelBackedCsvDriverTagMap : CsvDriverTagMap col.Set(model, raw.Trim()); fired.Add(col.Meta); foreach (var key in col.Meta.OwnedJsonKeys) { overridden.Add(key); } + foreach (var key in col.Meta.ClearsKeys ?? []) { overridden.Add(key); } } } @@ -466,17 +474,20 @@ public static class CsvColumnMap DriverTypeNames.Modbus, ModbusTagConfigModel.FromJson, m => m.ToJson(), new[] { - ModbusCol("Region", "region", nameof(ModbusTagConfigModel.Region), + // The six address columns: a non-blank cell is an AUTHORED address (like a hand edit in the + // editor), so it drops the address string and its pinned marker (ClearAddressString + + // ClearsKeys) — otherwise a string-only row silently discarded the cell. + ModbusAddrCol("Region", "region", nameof(ModbusTagConfigModel.Region), m => m.IsStringOnly ? null : m.Region.ToString(), (m, s) => m.Region = CsvValues.ParseEnum("Region", s)), - ModbusCol("Address", "address", nameof(ModbusTagConfigModel.Address), + ModbusAddrCol("Address", "address", nameof(ModbusTagConfigModel.Address), m => m.IsStringOnly ? null : m.Address.ToString(CultureInfo.InvariantCulture), (m, s) => m.Address = CsvValues.ParseInt("Address", s)), - ModbusCol("ModbusDataType", "dataType", nameof(ModbusTagConfigModel.DataType), + ModbusAddrCol("ModbusDataType", "dataType", nameof(ModbusTagConfigModel.DataType), m => m.IsStringOnly ? null : m.DataType.ToString(), (m, s) => m.DataType = CsvValues.ParseEnum("ModbusDataType", s)), - ModbusCol("ByteOrder", "byteOrder", nameof(ModbusTagConfigModel.ByteOrder), + ModbusAddrCol("ByteOrder", "byteOrder", nameof(ModbusTagConfigModel.ByteOrder), m => m.IsStringOnly ? null : m.ByteOrder.ToString(), (m, s) => m.ByteOrder = CsvValues.ParseEnum("ByteOrder", s)), - ModbusCol("BitIndex", "bitIndex", nameof(ModbusTagConfigModel.BitIndex), + ModbusAddrCol("BitIndex", "bitIndex", nameof(ModbusTagConfigModel.BitIndex), m => m.IsStringOnly ? null : m.BitIndex.ToString(CultureInfo.InvariantCulture), (m, s) => m.BitIndex = CsvValues.ParseInt("BitIndex", s)), - ModbusCol("StringLength", "stringLength", nameof(ModbusTagConfigModel.StringLength), + ModbusAddrCol("StringLength", "stringLength", nameof(ModbusTagConfigModel.StringLength), m => m.IsStringOnly ? null : m.StringLength.ToString(CultureInfo.InvariantCulture), (m, s) => m.StringLength = CsvValues.ParseInt("StringLength", s)), ModbusCol("Writable", "writable", nameof(ModbusTagConfigModel.Writable), m => CsvValues.FormatNullableBool(m.Writable), (m, s) => m.Writable = CsvValues.ParseBool("Writable", s)), @@ -620,6 +631,11 @@ public static class CsvColumnMap string h, string k, string p, Func get, Action set) => new(new CsvTypedColumn(h, k, p), get, set); + private static CsvModelColumn ModbusAddrCol( + string h, string k, string p, Func get, Action set) + => new(new CsvTypedColumn(h, k, p, ClearsKeys: ["addressString", ModbusTagDefinitionFactory.AddressResolvedKey]), + get, (m, s) => { m.ClearAddressString(); set(m, s); }); + private static CsvModelColumn S7Col( string h, string k, string p, Func get, Action set) => new(new CsvTypedColumn(h, k, p), get, set); diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs index ac237ae8..516eb3c6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs @@ -51,7 +51,17 @@ public sealed class ModbusTagConfigModel /// and its region / offset / type fields are NOT the tag's address — never stamps /// them, so a round-trip (CSV import, a Writable toggle) cannot turn the tag into HoldingRegisters[0]. /// - public bool IsStringOnly => AddressString is not null && !AddressResolved; + public bool IsStringOnly => AddressString is not null && !AddressResolved && !_loadedWithAddressKeys; + + /// + /// A row that carries an BESIDE region/address keys without the pinned marker + /// — a pre-#741 shape (the old driver never read the string). Its keys are the address (the driver loads + /// from them, as it always did); the string is informational only. + /// + public bool HasLegacyAddressString => AddressString is not null && !AddressResolved && _loadedWithAddressKeys; + + // Whether the blob this model was loaded from carried BOTH region and address keys. + private bool _loadedWithAddressKeys; /// /// Resolves an address string through with the owning driver's @@ -163,6 +173,7 @@ public sealed class ModbusTagConfigModel AddressString = string.IsNullOrWhiteSpace(TagConfigJson.GetString(o, "addressString")) ? null : TagConfigJson.GetString(o, "addressString"), AddressResolved = TagConfigJson.GetBoolNullable(o, ModbusTagDefinitionFactory.AddressResolvedKey) == true, + _loadedWithAddressKeys = o.ContainsKey("region") && o.ContainsKey("address"), _bag = o, }; } @@ -219,9 +230,11 @@ public sealed class ModbusTagConfigModel /// /// An error message describing the validation failure, or null when the model is valid. public string? Validate() - => TagConfigJson.DescribeInvalidEnum(_bag, "region") - ?? TagConfigJson.DescribeInvalidEnum(_bag, "dataType") - ?? TagConfigJson.DescribeInvalidEnum(_bag, "byteOrder") + // A string-only tag's region / dataType / byteOrder keys are not read by the driver (the string is the + // address), so judging them would make authoring stricter than the runtime (review r2 n6). + => (IsStringOnly ? null : TagConfigJson.DescribeInvalidEnum(_bag, "region")) + ?? (IsStringOnly ? null : TagConfigJson.DescribeInvalidEnum(_bag, "dataType")) + ?? (IsStringOnly ? null : TagConfigJson.DescribeInvalidEnum(_bag, "byteOrder")) ?? TagConfigJson.DescribeInvalidEnum(_bag, "stringByteOrder") ?? TagConfigParseGate.Describe( _source, diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs index 07747c1b..00e9ed68 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusFamilyWiringTests.cs @@ -74,14 +74,39 @@ public sealed class ModbusFamilyWiringTests /// what a model round-trip of a string-only tag used to produce) never win: the string is resolved with /// the driver's family, so the tag cannot collapse onto HoldingRegisters[0]. /// - [Fact] - public void Defaulted_region_and_address_beside_an_unmarked_string_do_not_win() + /// + /// Fix round 3 (review r2 N2): the persisted-shape matrix. Every shape that carries region + address loads + /// from them under every family — including the pre-#741 "keys + addressString, no marker" row, which the + /// old driver served from its keys and must keep serving so. Only a string-only blob (no keys) resolves + /// the string. + /// + [Theory] + [InlineData("keys only", """{"region":"InputRegisters","address":5}""", ModbusRegion.InputRegisters, 5)] + [InlineData("pre-lane keys + string", """{"region":"InputRegisters","address":5,"dataType":"Int16","addressString":"40011:F"}""", ModbusRegion.InputRegisters, 5)] + [InlineData("pre-lane stamped HR/0 + string", """{"region":"HoldingRegisters","address":0,"dataType":"Int16","addressString":"D100"}""", ModbusRegion.HoldingRegisters, 0)] + [InlineData("string only", """{"addressString":"40011:F"}""", ModbusRegion.HoldingRegisters, 10)] + [InlineData("string + marker + keys (editor-pinned)", """{"addressString":"D100","addressResolved":true,"region":"HoldingRegisters","address":100}""", ModbusRegion.HoldingRegisters, 100)] + [InlineData("string + marker, no keys", """{"addressString":"40011:F","addressResolved":true}""", ModbusRegion.HoldingRegisters, 10)] + public void Every_persisted_shape_resolves_to_its_address_under_every_family( + string shape, string json, ModbusRegion region, int address) { - const string json = """{"addressString":"D100","region":"HoldingRegisters","address":0,"dataType":"Int16"}"""; - ModbusTagDefinitionFactory.FromTagConfig(json, "T", ModbusFamily.MELSEC, MelsecFamily.Q_L_iQR, out var def).ShouldBeTrue(); - def.Address.ShouldBe((ushort)100); - // Under a family that cannot parse it the tag is refused (BadNodeIdUnknown), never served as HR[0]. - ModbusTagDefinitionFactory.FromTagConfig(json, "T", ModbusFamily.Generic, MelsecFamily.Q_L_iQR, out _).ShouldBeFalse(); + foreach (var (family, sub) in new[] + { + (ModbusFamily.Generic, MelsecFamily.Q_L_iQR), (ModbusFamily.DL205, MelsecFamily.Q_L_iQR), + (ModbusFamily.MELSEC, MelsecFamily.Q_L_iQR), (ModbusFamily.MELSEC, MelsecFamily.F_iQF), + }) + { + ModbusTagDefinitionFactory.FromTagConfig(json, "T", family, sub, out var def).ShouldBeTrue($"{shape} / {family}"); + def.Region.ShouldBe(region, $"{shape} / {family}"); + def.Address.ShouldBe((ushort)address, $"{shape} / {family}"); + } + } + + [Fact] + public void A_string_only_blob_no_family_parses_is_refused_not_served_as_HR0() + { + ModbusTagDefinitionFactory.FromTagConfig("""{"addressString":"D100"}""", "T", ModbusFamily.Generic, MelsecFamily.Q_L_iQR, out _) + .ShouldBeFalse(); } [Fact] diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs index c850a645..d211ed37 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs @@ -234,6 +234,59 @@ public sealed class ModbusTagConfigModelTests } } + private const string PreLaneRow = """{"region":"InputRegisters","address":5,"dataType":"Int16","addressString":"40011:F"}"""; + + /// Fix round 3 (review r2 N2): a pre-#741 row (keys + string, no marker) keeps resolving from its + /// keys through a model save and a CSV export → import, under every family. + [Fact] + public void A_pre_lane_keys_plus_string_row_keeps_its_keys_through_save_and_csv_round_trip() + { + var m = ModbusTagConfigModel.FromJson(PreLaneRow); + m.IsStringOnly.ShouldBeFalse(); + m.HasLegacyAddressString.ShouldBeTrue(); + m.Writable = false; + var saved = m.ToJson(); + + var map = CsvColumnMap.Resolve(ZB.MOM.WW.OtOpcUa.Core.Abstractions.DriverTypeNames.Modbus).ShouldNotBeNull(); + var cells = map.ReadTypedColumns(PreLaneRow); + cells["Region"].ShouldBe("InputRegisters"); + cells["Address"].ShouldBe("5"); + var reimported = map.ApplyTypedColumns(PreLaneRow, cells); + + foreach (var json in new[] { saved, reimported }) + foreach (var family in new[] { ModbusFamily.Generic, ModbusFamily.DL205, ModbusFamily.MELSEC }) + { + ModbusTagDefinitionFactory.FromTagConfig(json, "T", family, MelsecFamily.Q_L_iQR, out var def).ShouldBeTrue(); + def.Region.ShouldBe(ModbusRegion.InputRegisters, family.ToString()); + def.Address.ShouldBe((ushort)5, family.ToString()); + } + } + + /// Review r2 n5: a typed CSV address cell on a string-only row is an authored address — it is applied + /// and drops the string, mirroring a hand edit in the editor (it used to be silently discarded). + [Fact] + public void A_typed_csv_address_cell_on_a_string_only_row_is_applied_and_drops_the_string() + { + var map = CsvColumnMap.Resolve(ZB.MOM.WW.OtOpcUa.Core.Abstractions.DriverTypeNames.Modbus).ShouldNotBeNull(); + var merged = map.ApplyTypedColumns("""{"addressString":"40011:F","addressResolved":true}""", + new Dictionary(StringComparer.Ordinal) { ["Region"] = "InputRegisters", ["Address"] = "7" }); + + merged.ShouldNotContain("addressString"); + merged.ShouldNotContain("addressResolved"); + ModbusTagDefinitionFactory.FromTagConfig(merged, "T", out var def).ShouldBeTrue(); + def.Region.ShouldBe(ModbusRegion.InputRegisters); + def.Address.ShouldBe((ushort)7); + } + + /// Review r2 n6: authoring is never stricter than the driver — a string-only tag's ignored dataType / + /// byteOrder keys are not judged (positive control: the same junk on a classic tag is refused). + [Fact] + public void Validate_does_not_judge_keys_the_driver_ignores_on_a_string_only_tag() + { + ModbusTagConfigModel.FromJson("""{"addressString":"40011:F","dataType":"Intt16","byteOrder":"Nope"}""").Validate().ShouldBeNull(); + ModbusTagConfigModel.FromJson("""{"region":"HoldingRegisters","address":1,"dataType":"Intt16"}""").Validate().ShouldNotBeNull(); + } + [Fact] public void A_hand_edit_after_resolving_drops_the_address_string() {