fix(modbus): review r3 - one address authority (ModbusTagAddressResolution)
N3: a keyless blob carrying the addressResolved marker was served from its string by the driver but saved as HR[0] by the editor model / CSV path. The precedence rule now lives in ONE pure resolver in Driver.Modbus.Contracts (ModbusTagAddressResolution.Classify/Resolve): keys when region+address are both present, else a string-only addressString resolved with the driver's family (refused when it cannot parse), else refused. The marker never decides. Consumers that now ask it instead of restating it: the driver's tag reader (ModbusTagDefinitionFactory.FromTagConfig), the deploy gate (Inspect / DescribeRejection), the editor model (AddressShape / IsStringOnly / HasKeyAddress / ResolveWith, ToJson stamping, Validate), the CSV export cells and the editor hint. The model writes keys only when it loaded them or a field was authored - never defaults it did not resolve (also closes two divergences the new matrix found: malformed-string+marker and non-string addressString were saved as HR[0]). One table-driven test (ModbusAddressAuthorityAgreementTests) runs 15 persisted shapes through driver, deploy gate, model load/save/Validate and the real CSV export->import under all four families; breaking the CSV export or reverting the model to the marker rule fails it. Minors: m1 CSV address cells keep the string+marker while it still describes them (lossless unchanged round trip), drop it when contradicted, and the review grid reports the drop; m2 the pre-lane test uses the real export residual; m3 unmarked string-only + typed-cell case added; m4 stale docs/summary and the unused factory parameter removed. Refs #741 Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
+230
@@ -0,0 +1,230 @@
|
||||
using Shouldly;
|
||||
using Xunit;
|
||||
using ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
|
||||
using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors;
|
||||
using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
|
||||
using ZB.MOM.WW.OtOpcUa.ControlPlane.AdminOperations;
|
||||
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
|
||||
using ZB.MOM.WW.OtOpcUa.Driver.Modbus;
|
||||
|
||||
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
|
||||
|
||||
/// <summary>
|
||||
/// Review r3 N3 (#741, fix round 4): <b>one authority for a Modbus tag's address</b>. Every persisted
|
||||
/// <c>TagConfig</c> shape goes through EVERY consumer that decides, displays or rewrites the address. They
|
||||
/// must all agree with the driver, under every family:
|
||||
/// <list type="bullet">
|
||||
/// <item>the driver's tag reader (<c>ModbusTagDefinitionFactory.FromTagConfig</c> with the driver's
|
||||
/// family, the exact call <c>ModbusDriver</c> makes at initialize);</item>
|
||||
/// <item>the deploy gate (<c>EquipmentTagConfigInspector.Inspect</c>);</item>
|
||||
/// <item>the editor model's load (what its fields claim), save (incl. a Writable toggle) and
|
||||
/// <c>Validate</c>;</item>
|
||||
/// <item>the CSV export → import round trip through the real <c>RawTagCsvMapper</c> (the export residual,
|
||||
/// not the stored blob, is the import base).</item>
|
||||
/// </list>
|
||||
/// The expected verdict per family is pinned in the table, so the authority itself cannot drift either.
|
||||
/// Three rounds converged on the rule one consumer at a time; this matrix is what makes a new divergence
|
||||
/// fail in one place.
|
||||
/// </summary>
|
||||
public sealed class ModbusAddressAuthorityAgreementTests
|
||||
{
|
||||
private static readonly (ModbusFamily Family, MelsecFamily Sub, string Label)[] Families =
|
||||
[
|
||||
(ModbusFamily.Generic, MelsecFamily.Q_L_iQR, "Generic"),
|
||||
(ModbusFamily.DL205, MelsecFamily.Q_L_iQR, "DL205"),
|
||||
(ModbusFamily.MELSEC, MelsecFamily.Q_L_iQR, "MELSEC-Q"),
|
||||
(ModbusFamily.MELSEC, MelsecFamily.F_iQF, "MELSEC-F"),
|
||||
];
|
||||
|
||||
/// <summary>
|
||||
/// Shape name, persisted blob, and the expected driver verdict per family in the order of
|
||||
/// <see cref="Families"/> (<c>IR5</c> = InputRegisters[5], <c>-</c> = refused).
|
||||
/// </summary>
|
||||
public static TheoryData<string, string, string> Shapes => new()
|
||||
{
|
||||
{ "keys only", """{"region":"InputRegisters","address":5}""", "IR5|IR5|IR5|IR5" },
|
||||
{ "pre-lane keys + string", """{"region":"InputRegisters","address":5,"dataType":"Int16","addressString":"40011:F"}""", "IR5|IR5|IR5|IR5" },
|
||||
{ "pre-lane stamped HR/0 + string",
|
||||
"""{"region":"HoldingRegisters","address":0,"dataType":"Int16","byteOrder":"BigEndian","bitIndex":0,"stringLength":0,"addressString":"D100"}""",
|
||||
"HR0|HR0|HR0|HR0" },
|
||||
{ "string only", """{"addressString":"40011:F"}""", "HR10|HR10|HR10|HR10" },
|
||||
{ "string only, family-native", """{"addressString":"D100"}""", "-|-|HR100|HR100" },
|
||||
{ "string + marker + keys", """{"addressString":"D100","addressResolved":true,"region":"HoldingRegisters","address":100,"dataType":"Int16"}""", "HR100|HR100|HR100|HR100" },
|
||||
{ "string + marker, no keys", """{"addressString":"40011:F","addressResolved":true}""", "HR10|HR10|HR10|HR10" },
|
||||
{ "string + marker, no keys, family-native", """{"addressString":"D100","addressResolved":true}""", "-|-|HR100|HR100" },
|
||||
{ "malformed string", """{"addressString":"ZZ9"}""", "-|-|-|-" },
|
||||
{ "malformed string + marker", """{"addressString":"ZZ9","addressResolved":true}""", "-|-|-|-" },
|
||||
{ "malformed string + keys", """{"region":"InputRegisters","address":5,"addressString":"ZZ9"}""", "IR5|IR5|IR5|IR5" },
|
||||
{ "non-string addressString", """{"addressString":5}""", "-|-|-|-" },
|
||||
{ "string + one key", """{"addressString":"40011:F","region":"InputRegisters"}""", "HR10|HR10|HR10|HR10" },
|
||||
{ "no address", """{"writable":true}""", "-|-|-|-" },
|
||||
{ "bit region, legacy Int16", """{"region":"Coils","address":3,"dataType":"Int16"}""", "CO3|CO3|CO3|CO3" },
|
||||
};
|
||||
|
||||
[Theory]
|
||||
[MemberData(nameof(Shapes))]
|
||||
public void Every_consumer_agrees_with_the_driver_on_every_persisted_shape(string shape, string blob, string expected)
|
||||
{
|
||||
var want = expected.Split('|');
|
||||
var anyServed = want.Any(v => v != "-");
|
||||
|
||||
// 1. The driver (the authority) — pinned, so the authority cannot drift unnoticed either.
|
||||
for (var i = 0; i < Families.Length; i++)
|
||||
{
|
||||
Verdict(blob, i).ShouldBe(want[i], $"{shape}: driver / {Families[i].Label}");
|
||||
}
|
||||
|
||||
// 2. The deploy gate: clean exactly when some family's driver serves the tag.
|
||||
(EquipmentTagConfigInspector.Inspect(DriverTypeNames.Modbus, blob).Count == 0)
|
||||
.ShouldBe(anyServed, $"{shape}: deploy gate accepts ⇔ some driver serves it");
|
||||
|
||||
// 3. Editor load: when the model presents its fields as the address, they ARE the driver's address.
|
||||
var loaded = ModbusTagConfigModel.FromJson(blob);
|
||||
if (!loaded.IsStringOnly)
|
||||
{
|
||||
for (var i = 0; i < Families.Length; i++)
|
||||
{
|
||||
if (want[i] == "-") continue;
|
||||
FieldsVerdict(loaded).ShouldBe(want[i], $"{shape}: editor fields / {Families[i].Label}");
|
||||
}
|
||||
}
|
||||
|
||||
// 4. Editor save, untouched and after a Writable toggle (the round trip that remapped N1 / N3).
|
||||
var untouched = ModbusTagConfigModel.FromJson(blob).ToJson();
|
||||
var toggled = ModbusTagConfigModel.FromJson(blob);
|
||||
toggled.Writable = false;
|
||||
var toggledJson = toggled.ToJson();
|
||||
for (var i = 0; i < Families.Length; i++)
|
||||
{
|
||||
Verdict(untouched, i).ShouldBe(want[i], $"{shape}: editor save / {Families[i].Label}");
|
||||
Verdict(toggledJson, i).ShouldBe(want[i], $"{shape}: editor save after Writable toggle / {Families[i].Label}");
|
||||
}
|
||||
|
||||
// 5. Validate: accepts exactly what some family's driver serves.
|
||||
(ModbusTagConfigModel.FromJson(blob).Validate() is null).ShouldBe(anyServed, $"{shape}: Validate");
|
||||
|
||||
// 6. CSV export → import through the production mapper (the residual is the import base).
|
||||
var csv = RawTagCsvMapper.Export(DriverTypeNames.Modbus,
|
||||
[new RawCsvExportTag(null, "t", "Int16", TagAccessLevel.ReadWrite, false, null, blob)]);
|
||||
var parsed = RawTagCsvMapper.Parse(DriverTypeNames.Modbus, csv);
|
||||
parsed.FatalError.ShouldBeNull();
|
||||
var row = parsed.Rows.ShouldHaveSingleItem();
|
||||
if (!row.Ok)
|
||||
{
|
||||
anyServed.ShouldBeFalse($"{shape}: CSV import refused a tag the driver serves ({row.Error})");
|
||||
}
|
||||
else
|
||||
{
|
||||
for (var i = 0; i < Families.Length; i++)
|
||||
{
|
||||
Verdict(row.ImportRow!.Tag.TagConfig, i).ShouldBe(want[i], $"{shape}: CSV export → import / {Families[i].Label}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private static string Verdict(string json, int familyIndex)
|
||||
{
|
||||
var (family, sub, _) = Families[familyIndex];
|
||||
return ModbusTagDefinitionFactory.FromTagConfig(json, "T", family, sub, out var def)
|
||||
? Short(def.Region) + def.Address
|
||||
: "-";
|
||||
}
|
||||
|
||||
private static string FieldsVerdict(ModbusTagConfigModel m) => Short(m.Region) + m.Address;
|
||||
|
||||
private static string Short(ModbusRegion r) => r switch
|
||||
{
|
||||
ModbusRegion.HoldingRegisters => "HR",
|
||||
ModbusRegion.InputRegisters => "IR",
|
||||
ModbusRegion.Coils => "CO",
|
||||
ModbusRegion.DiscreteInputs => "DI",
|
||||
_ => r.ToString(),
|
||||
};
|
||||
|
||||
// ------------------------------------------------------------------ CSV metadata (review r3 m1 / m3)
|
||||
|
||||
private static RawCsvReviewRow RoundTrip(string blob, Func<string, string>? editCsv = null)
|
||||
{
|
||||
var csv = RawTagCsvMapper.Export(DriverTypeNames.Modbus,
|
||||
[new RawCsvExportTag(null, "t", "Int16", TagAccessLevel.ReadWrite, false, null, blob)]);
|
||||
if (editCsv is not null) csv = editCsv(csv);
|
||||
return RawTagCsvMapper.Parse(DriverTypeNames.Modbus, csv).Rows.ShouldHaveSingleItem();
|
||||
}
|
||||
|
||||
/// <summary>m1: an UNCHANGED export → import of a pinned row keeps its address string and marker.</summary>
|
||||
[Fact]
|
||||
public void An_unchanged_csv_round_trip_of_a_pinned_row_keeps_its_address_string_and_marker()
|
||||
{
|
||||
const string pinned = """{"addressString":"D100","addressResolved":true,"region":"HoldingRegisters","address":100,"dataType":"Int16","byteOrder":"BigEndian","bitIndex":0,"stringLength":0}""";
|
||||
var row = RoundTrip(pinned);
|
||||
row.Ok.ShouldBeTrue(row.Error);
|
||||
var json = row.ImportRow!.Tag.TagConfig;
|
||||
json.ShouldContain("\"addressString\":\"D100\"");
|
||||
json.ShouldContain("\"addressResolved\":true");
|
||||
row.TypedOverrodeFallback.ShouldBeEmpty("nothing was overridden");
|
||||
ModbusTagConfigModel.FromJson(json).IsPinned.ShouldBeTrue();
|
||||
}
|
||||
|
||||
/// <summary>m1 positive control: an address cell that CONTRADICTS the string drops it, and the grid says so.</summary>
|
||||
[Fact]
|
||||
public void A_csv_address_cell_that_contradicts_the_string_drops_it_and_the_review_grid_reports_it()
|
||||
{
|
||||
const string pinned = """{"addressString":"D100","addressResolved":true,"region":"HoldingRegisters","address":100,"dataType":"Int16","byteOrder":"BigEndian","bitIndex":0,"stringLength":0}""";
|
||||
// Edit the exported Address cell 100 → 101.
|
||||
var row = RoundTrip(pinned, csv => csv.Replace(",HoldingRegisters,100,", ",HoldingRegisters,101,", StringComparison.Ordinal));
|
||||
row.Ok.ShouldBeTrue(row.Error);
|
||||
var json = row.ImportRow!.Tag.TagConfig;
|
||||
json.ShouldNotContain("addressString");
|
||||
json.ShouldNotContain("addressResolved");
|
||||
row.TypedOverrodeFallback.ShouldNotBeEmpty("the dropped string is reported, not silent");
|
||||
ModbusTagDefinitionFactory.FromTagConfig(json, "T", ModbusFamily.MELSEC, MelsecFamily.Q_L_iQR, out var def).ShouldBeTrue();
|
||||
def.Address.ShouldBe((ushort)101);
|
||||
}
|
||||
|
||||
/// <summary>m3: the n5 case on an UNMARKED string-only row — typed address cells are applied and drop the string.</summary>
|
||||
[Fact]
|
||||
public void A_typed_csv_address_cell_on_an_unmarked_string_only_row_is_applied_and_drops_the_string()
|
||||
{
|
||||
var map = CsvColumnMap.Resolve(DriverTypeNames.Modbus).ShouldNotBeNull();
|
||||
var merged = map.ApplyTypedColumns("""{"addressString":"40011:F"}""",
|
||||
new Dictionary<string, string>(StringComparer.Ordinal) { ["Region"] = "InputRegisters", ["Address"] = "7" });
|
||||
|
||||
merged.ShouldNotContain("addressString");
|
||||
foreach (var (family, sub, label) in Families)
|
||||
{
|
||||
ModbusTagDefinitionFactory.FromTagConfig(merged, "T", family, sub, out var def).ShouldBeTrue(label);
|
||||
def.Region.ShouldBe(ModbusRegion.InputRegisters, label);
|
||||
def.Address.ShouldBe((ushort)7, label);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>N3, by name: the marker without keys is string-only for the editor, its save and its CSV export.</summary>
|
||||
[Fact]
|
||||
public void A_marker_without_keys_is_string_only_and_never_saved_as_HR0()
|
||||
{
|
||||
var m = ModbusTagConfigModel.FromJson("""{"addressString":"40011:F","addressResolved":true}""");
|
||||
m.IsStringOnly.ShouldBeTrue();
|
||||
m.IsPinned.ShouldBeFalse("a marker without keys pins nothing");
|
||||
m.Writable = false;
|
||||
var saved = m.ToJson();
|
||||
saved.ShouldNotContain("\"address\":");
|
||||
saved.ShouldNotContain("\"region\":");
|
||||
CsvColumnMap.Resolve(DriverTypeNames.Modbus)!.ReadTypedColumns(saved).ShouldNotContainKey("Address");
|
||||
}
|
||||
|
||||
/// <summary>The editor never writes default keys it did not resolve: a Writable toggle on an address-less blob
|
||||
/// does not author HoldingRegisters[0]; setting a field does.</summary>
|
||||
[Fact]
|
||||
public void An_address_less_blob_gains_keys_only_when_a_field_is_authored()
|
||||
{
|
||||
var m = ModbusTagConfigModel.FromJson("{}");
|
||||
m.Writable = false;
|
||||
m.ToJson().ShouldNotContain("\"region\":");
|
||||
m.HasKeyAddress.ShouldBeFalse();
|
||||
|
||||
m.Address = 0; // an explicit authoring of HR[0] is honoured
|
||||
m.HasKeyAddress.ShouldBeTrue();
|
||||
ModbusTagDefinitionFactory.FromTagConfig(m.ToJson(), "T", out var def).ShouldBeTrue();
|
||||
def.Address.ShouldBe((ushort)0);
|
||||
}
|
||||
}
|
||||
@@ -247,11 +247,18 @@ public sealed class ModbusTagConfigModelTests
|
||||
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);
|
||||
// The production export → import path (review r3 m2): the import base is the export RESIDUAL, not the row.
|
||||
var csv = ZB.MOM.WW.OtOpcUa.AdminUI.Uns.RawTagCsvMapper.Export(ZB.MOM.WW.OtOpcUa.Core.Abstractions.DriverTypeNames.Modbus,
|
||||
[new ZB.MOM.WW.OtOpcUa.AdminUI.Uns.RawCsvExportTag(null, "t", "Int16",
|
||||
ZB.MOM.WW.OtOpcUa.Configuration.Enums.TagAccessLevel.ReadWrite, false, null, PreLaneRow)]);
|
||||
csv.ShouldContain(",InputRegisters,5,");
|
||||
var row = ZB.MOM.WW.OtOpcUa.AdminUI.Uns.RawTagCsvMapper.Parse(ZB.MOM.WW.OtOpcUa.Core.Abstractions.DriverTypeNames.Modbus, csv)
|
||||
.Rows.ShouldHaveSingleItem();
|
||||
row.Ok.ShouldBeTrue(row.Error);
|
||||
var reimported = row.ImportRow!.Tag.TagConfig;
|
||||
// The legacy string contradicts the keys under every family, so the import drops it — reported, not silent.
|
||||
reimported.ShouldNotContain("addressString");
|
||||
row.TypedOverrodeFallback.ShouldNotBeEmpty();
|
||||
|
||||
foreach (var json in new[] { saved, reimported })
|
||||
foreach (var family in new[] { ModbusFamily.Generic, ModbusFamily.DL205, ModbusFamily.MELSEC })
|
||||
@@ -262,10 +269,11 @@ public sealed class ModbusTagConfigModelTests
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>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).</summary>
|
||||
/// <summary>Review r2 n5: a typed CSV address cell on a keyless row carrying a (stale) marker is an authored
|
||||
/// address — it is applied and drops the string it contradicts. The unmarked string-only case is
|
||||
/// <c>ModbusAddressAuthorityAgreementTests.A_typed_csv_address_cell_on_an_unmarked_string_only_row_…</c> (r3 m3).</summary>
|
||||
[Fact]
|
||||
public void A_typed_csv_address_cell_on_a_string_only_row_is_applied_and_drops_the_string()
|
||||
public void A_typed_csv_address_cell_on_a_marker_bearing_keyless_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}""",
|
||||
|
||||
Reference in New Issue
Block a user