fix(modbus): review r2 - region+address keys always win; string-only means no keys

N2: pre-#741 rows carry region/address beside an addressString with no marker
(the old driver never read the string, the old model preserved it as an
unknown key). The r1 rule resolved those from the string - a silent remap or
BadNodeIdUnknown on upgrade. The driver now loads from region+address whenever
both are present (classic, editor-pinned, pre-lane); only a blob with NO keys
is string-only and resolved through the parser. This is safe because the model
no longer stamps keys onto a string-only tag. The model distinguishes
string-only / legacy-string-beside-keys / pinned, and the editor hint says so.
Test matrix over every persisted shape (keys only, pre-lane keys+string,
pre-lane stamped HR/0+string, string only, marker+keys, marker no keys) under
every family; the pre-lane row also through a model save and a CSV
export->import.
n5: a typed CSV address cell is an authored address - it drops addressString
and the marker (new CsvTypedColumn.ClearsKeys) instead of being discarded.
n6: Validate skips the region/dataType/byteOrder enum checks on a string-only
tag (the driver ignores them). n7: docs/v2/modbus-addressing.md restates the
key-presence rule.

Refs #741
Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
2026-10-22 16:23:37 -04:00
parent 1f82ecfc0d
commit 0200619322
7 changed files with 157 additions and 45 deletions
+13 -9
View File
@@ -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
@@ -35,11 +35,10 @@ public static class ModbusTagDefinitionFactory
=> FromTagConfig(tagConfig, rawPath, ModbusFamily.Generic, MelsecFamily.Q_L_iQR, out def);
/// <summary>
/// Family-aware overload. The persisted <c>region</c> + <c>address</c> keys are AUTHORITATIVE when they
/// were actually authored — a classic tag with no <c>addressString</c>, or one the editor resolved and
/// marked (<see cref="AddressResolvedKey"/>) — so a later Family / MelsecSubFamily change can never
/// silently remap such a tag. A blob whose <c>addressString</c> 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 <c>region</c> + <c>address</c> 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
/// <c>addressString</c> and NO region/address keys (CSV / API string-only authoring; the Modicon / mnemonic / family-native
/// grammar of <see cref="ModbusAddressParser"/> — <c>40001:F:CDAB</c>, DL205 <c>V2000</c>, MELSEC
/// <c>D100</c>) is resolved here with the DRIVER's <paramref name="family"/> /
/// <paramref name="melsecSubFamily"/>. Without an <c>addressString</c> this is exactly the 3-argument
@@ -177,25 +176,23 @@ public static class ModbusTagDefinitionFactory
/// <summary>
/// The key the tag editor writes beside the resolved <c>region</c> / <c>address</c> when it resolved an
/// <c>addressString</c> with the driver's family (<c>"addressResolved": true</c>). Its presence — not the
/// mere presence of <c>region</c> / <c>address</c> — is what makes the resolved keys authoritative over
/// the string: the AdminUI model stamps DEFAULTED <c>region</c>/<c>address</c> (HoldingRegisters / 0) on
/// every save, so key presence alone turned a string-only CSV/API tag into HR[0] on its first edit.
/// <c>addressString</c> with the driver's family (<c>"addressResolved": true</c>). It records the editor's
/// pinned state (display / CSV) — the DRIVER rule is key presence (see
/// <see cref="ResolvedKeysAuthoritative"/>), because pre-#741 rows carry keys + string with no marker and
/// must keep loading from their keys.
/// </summary>
public const string AddressResolvedKey = "addressResolved";
/// <summary>
/// Whether the persisted <c>region</c> / <c>address</c> are the tag's real address: they are present AND
/// either there is no <c>addressString</c> (a classic region + address tag) or the editor marked them as
/// resolved from it (<see cref="AddressResolvedKey"/>). A blob carrying an <c>addressString</c> without
/// the marker resolves the string through the parser, never the (possibly defaulted) keys.
/// Whether the persisted <c>region</c> / <c>address</c> 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 <c>addressString</c> 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].
/// </summary>
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 _);
/// <summary>Reads the optional <c>addressString</c>: null when absent or blank; flags a non-string value.</summary>
private static string? ReadAddressString(JsonElement root, out bool invalid)
@@ -18,6 +18,10 @@
{
<div class="col-12"><div class="form-text text-warning">String-only tag: the driver resolves <code>@_m.AddressString</code> 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.</div></div>
}
else if (_m.HasLegacyAddressString)
{
<div class="col-12"><div class="form-text">This tag carries a legacy address string <code>@_m.AddressString</code> 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.</div></div>
}
else if (_m.AddressString is not null)
{
<div class="col-12"><div class="form-text">Resolved from <code>@_m.AddressString</code> (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.</div></div>
@@ -28,11 +28,18 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors;
/// and <c>fullName</c>/<c>dataItemId</c>/<c>address</c>) — see <see cref="OwnedJsonKeys"/> for why the CSV
/// path has to know the whole set and not just the one it writes.
/// </param>
/// <param name="ClearsKeys">
/// 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 <c>addressString</c> / <c>addressResolved</c> — 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.
/// </param>
public sealed record CsvTypedColumn(
string Header,
string JsonKey,
string? ModelProperty,
IReadOnlyList<string>? ReadKeys = null)
IReadOnlyList<string>? ReadKeys = null,
IReadOnlyList<string>? ClearsKeys = null)
{
/// <summary>
/// Every root JSON key this column speaks for, in the <b>runtime's</b> read precedence — i.e.
@@ -223,6 +230,7 @@ public sealed class ModelBackedCsvDriverTagMap<TModel> : 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<ModbusRegion>("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>("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<ModbusByteOrder>("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<ModbusTagConfigModel, string?> get, Action<ModbusTagConfigModel, string> set)
=> new(new CsvTypedColumn(h, k, p), get, set);
private static CsvModelColumn<ModbusTagConfigModel> ModbusAddrCol(
string h, string k, string p, Func<ModbusTagConfigModel, string?> get, Action<ModbusTagConfigModel, string> set)
=> new(new CsvTypedColumn(h, k, p, ClearsKeys: ["addressString", ModbusTagDefinitionFactory.AddressResolvedKey]),
get, (m, s) => { m.ClearAddressString(); set(m, s); });
private static CsvModelColumn<S7TagConfigModel> S7Col(
string h, string k, string p, Func<S7TagConfigModel, string?> get, Action<S7TagConfigModel, string> set)
=> new(new CsvTypedColumn(h, k, p), get, set);
@@ -51,7 +51,17 @@ public sealed class ModbusTagConfigModel
/// and its region / offset / type fields are NOT the tag's address — <see cref="ToJson"/> never stamps
/// them, so a round-trip (CSV import, a Writable toggle) cannot turn the tag into HoldingRegisters[0].
/// </summary>
public bool IsStringOnly => AddressString is not null && !AddressResolved;
public bool IsStringOnly => AddressString is not null && !AddressResolved && !_loadedWithAddressKeys;
/// <summary>
/// A row that carries an <see cref="AddressString"/> 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.
/// </summary>
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;
/// <summary>
/// Resolves an address string through <see cref="ModbusAddressParser"/> 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
/// </remarks>
/// <returns>An error message describing the validation failure, or <c>null</c> when the model is valid.</returns>
public string? Validate()
=> TagConfigJson.DescribeInvalidEnum<ModbusRegion>(_bag, "region")
?? TagConfigJson.DescribeInvalidEnum<ModbusDataType>(_bag, "dataType")
?? TagConfigJson.DescribeInvalidEnum<ModbusByteOrder>(_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<ModbusRegion>(_bag, "region"))
?? (IsStringOnly ? null : TagConfigJson.DescribeInvalidEnum<ModbusDataType>(_bag, "dataType"))
?? (IsStringOnly ? null : TagConfigJson.DescribeInvalidEnum<ModbusByteOrder>(_bag, "byteOrder"))
?? TagConfigJson.DescribeInvalidEnum<ModbusStringByteOrder>(_bag, "stringByteOrder")
?? TagConfigParseGate.Describe(
_source,
@@ -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].
/// </summary>
[Fact]
public void Defaulted_region_and_address_beside_an_unmarked_string_do_not_win()
/// <summary>
/// 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.
/// </summary>
[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]
@@ -234,6 +234,59 @@ public sealed class ModbusTagConfigModelTests
}
}
private const string PreLaneRow = """{"region":"InputRegisters","address":5,"dataType":"Int16","addressString":"40011:F"}""";
/// <summary>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.</summary>
[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());
}
}
/// <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>
[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<string, string>(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);
}
/// <summary>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).</summary>
[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()
{