dohertj2/lmxopcua
1
0
Fork 0
Code Issues 43 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(modbus): review r1 - resolved address keys win only when the editor pinned them

Browse Source 
N1: the m2 precedence trusted region/address whenever the keys were present,
but the AdminUI model stamps defaulted HoldingRegisters/0 on every save, so a
string-only tag (CSV import, or any editor toggle) silently became HR[0] -
reads AND writes. The resolved keys are now authoritative only when the editor
marked them ("addressResolved": true, written by Resolve); an unmarked
addressString is always resolved through the parser with the driver's family.
The model no longer stamps address fields for a string-only tag, CSV export
leaves its address cells blank, and the editor hint distinguishes string-only
from pinned tags. Tests: CSV import and editor save of a string-only tag, a
stamped-default blob, and the pinned positive control across families.
n1: docs/v2/modbus-addressing.md updated (FC23 opt-in, maxReadGap semantics,
coalescing scope, addressString/family rules, coil coercion, refusals).
n2: form round-trip tests for UseFC23ForMultiRegisterWrites /
DisableReadCoalescing. n4: the editor keeps unresolved address text when the
parent echoes back its own emit. n3: noted, no change.

Refs #741
Claude-Session: https://claude.ai/code/session_01Ub9hGmYLQvn9XETzw7CMsT
This commit is contained in:
Joseph Doherty
2026-10-22 16:06:02 -04:00
parent fd6ecf8293
commit 3531834cce
9 changed files with 219 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs
+29 -6
View File
@@ -35,10 +35,11 @@ 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 whenever
/// present — the editor resolves an <c>addressString</c> with the driver's family and persists the
/// result, so a later Family / MelsecSubFamily change can never silently remap a live tag. A blob that
/// carries ONLY an <c>addressString</c> (CSV / API authoring, the Modicon / mnemonic / family-native
/// 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
/// 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
@@ -69,7 +70,7 @@ public static class ModbusTagDefinitionFactory
ushort stringLength;
int? arrayCount;
var addressText = ReadAddressString(root, out var addressStringInvalid);
var hasResolvedKeys = root.TryGetProperty("region", out _) && root.TryGetProperty("address", out _);
var hasResolvedKeys = ResolvedKeysAuthoritative(root, addressText);
if (!hasResolvedKeys && addressStringInvalid) return false;
if (!hasResolvedKeys && addressText is not null)
{
@@ -174,6 +175,28 @@ public static class ModbusTagDefinitionFactory
(ModbusFamily.MELSEC, MelsecFamily.F_iQF),
];
/// <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.
/// </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.
/// </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;
}
/// <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)
{
@@ -320,7 +343,7 @@ public static class ModbusTagDefinitionFactory
private static void DescribeRejection(JsonElement root, List<string> warnings)
{
var addressText = ReadAddressString(root, out var addressStringInvalid);
var hasResolvedKeys = root.TryGetProperty("region", out _) && root.TryGetProperty("address", out _);
var hasResolvedKeys = ResolvedKeysAuthoritative(root, addressText);
if (!hasResolvedKeys && addressStringInvalid)
{
warnings.Add("Modbus TagConfig 'addressString' is not a string — the tag will not resolve (BadNodeIdUnknown).");
src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/ModbusTagConfigEditor.razor
+23 -7
View File
@@ -14,9 +14,13 @@
{
<div class="col-12"><div class="form-text text-danger">@_addressError</div></div>
}
else if (_m.IsStringOnly)
{
<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.AddressString is not null)
{
<div class="col-12"><div class="form-text">Resolved from <code>@_m.AddressString</code> (family @_family) — the driver re-resolves it with its own family. Editing a field below drops it.</div></div>
<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>
}
<div class="col-md-4"><label class="form-label">Region</label>
<select class="form-select form-select-sm" value="@_m.Region" @onchange="@(e => UpdateAddress(() => _m.SetRegion(ParseEnum(e.Value, ModbusRegion.HoldingRegisters))))">
@@ -80,10 +84,14 @@
if (ConfigJson == _lastConfigJson) { return; }
_lastConfigJson = ConfigJson;
_m = ModbusTagConfigModel.FromJson(ConfigJson);
// Reset on EVERY config change: a reused editor instance switched to another tag must not show the
// previous tag's address text.
_addressText = _m.AddressString;
_addressError = null;
// Reset the address box only when a DIFFERENT config arrives (another tag in a reused instance) — not
// when the parent echoes back what this editor just emitted, which would wipe text the operator typed
// but has not resolved yet.
if (ConfigJson != _lastEmitted)
{
_addressText = _m.AddressString;
_addressError = null;
}
_family = ModbusTagConfigModel.ReadFamily(GetDriverConfigJson()).Family;
}
@@ -93,10 +101,18 @@
private static TEnum ParseEnum<TEnum>(object? v, TEnum fallback) where TEnum : struct, Enum
=> Enum.TryParse<TEnum>(v?.ToString(), out var r) ? r : fallback;
private string? _lastEmitted;
private async Task Update(Action apply)
{
apply();
await ConfigJsonChanged.InvokeAsync(_m.ToJson());
await EmitAsync();
}
private async Task EmitAsync()
{
_lastEmitted = _m.ToJson();
await ConfigJsonChanged.InvokeAsync(_lastEmitted);
}
private string? _addressText;
@@ -115,7 +131,7 @@
_family = family;
var error = _m.ResolveAddress(_addressText, family, sub);
_addressError = error;
if (error is null) await ConfigJsonChanged.InvokeAsync(_m.ToJson());
if (error is null) await EmitAsync();
}
// Parse the builder string (e.g. "4x00001-1") back into Region + Address fields.
src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CsvColumnMap.cs
+9 -6
View File
@@ -459,22 +459,25 @@ public static class CsvColumnMap
{
var maps = new CsvDriverTagMap[]
{
// A string-only Modbus tag (addressString, no addressResolved marker) exports BLANK address cells:
// its model fields are load-time defaults, and exporting them as HoldingRegisters / 0 invited a
// re-import that looked like an authored HR[0] address. The driver resolves the string.
new ModelBackedCsvDriverTagMap<ModbusTagConfigModel>(
DriverTypeNames.Modbus, ModbusTagConfigModel.FromJson, m => m.ToJson(),
new[]
{
ModbusCol("Region", "region", nameof(ModbusTagConfigModel.Region),
m => m.Region.ToString(), (m, s) => m.Region = CsvValues.ParseEnum<ModbusRegion>("Region", s)),
m => m.IsStringOnly ? null : m.Region.ToString(), (m, s) => m.Region = CsvValues.ParseEnum<ModbusRegion>("Region", s)),
ModbusCol("Address", "address", nameof(ModbusTagConfigModel.Address),
m => m.Address.ToString(CultureInfo.InvariantCulture), (m, s) => m.Address = CsvValues.ParseInt("Address", s)),
m => m.IsStringOnly ? null : m.Address.ToString(CultureInfo.InvariantCulture), (m, s) => m.Address = CsvValues.ParseInt("Address", s)),
ModbusCol("ModbusDataType", "dataType", nameof(ModbusTagConfigModel.DataType),
m => m.DataType.ToString(), (m, s) => m.DataType = CsvValues.ParseEnum<ModbusDataType>("ModbusDataType", s)),
m => m.IsStringOnly ? null : m.DataType.ToString(), (m, s) => m.DataType = CsvValues.ParseEnum<ModbusDataType>("ModbusDataType", s)),
ModbusCol("ByteOrder", "byteOrder", nameof(ModbusTagConfigModel.ByteOrder),
m => m.ByteOrder.ToString(), (m, s) => m.ByteOrder = CsvValues.ParseEnum<ModbusByteOrder>("ByteOrder", s)),
m => m.IsStringOnly ? null : m.ByteOrder.ToString(), (m, s) => m.ByteOrder = CsvValues.ParseEnum<ModbusByteOrder>("ByteOrder", s)),
ModbusCol("BitIndex", "bitIndex", nameof(ModbusTagConfigModel.BitIndex),
m => m.BitIndex.ToString(CultureInfo.InvariantCulture), (m, s) => m.BitIndex = CsvValues.ParseInt("BitIndex", s)),
m => m.IsStringOnly ? null : m.BitIndex.ToString(CultureInfo.InvariantCulture), (m, s) => m.BitIndex = CsvValues.ParseInt("BitIndex", s)),
ModbusCol("StringLength", "stringLength", nameof(ModbusTagConfigModel.StringLength),
m => m.StringLength.ToString(CultureInfo.InvariantCulture), (m, s) => m.StringLength = CsvValues.ParseInt("StringLength", s)),
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)),
}),
src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs
+37 -11
View File
@@ -32,13 +32,27 @@ public sealed class ModbusTagConfigModel
/// <summary>
/// The address in the parser grammar (<c>40001:F:CDAB</c>, DL205 <c>V2000</c>, MELSEC <c>D100</c>) when
/// the tag was authored that way; <c>null</c> for a tag authored by region + numeric address. The
/// persisted resolved fields are what the driver reads (the string is only resolved by the driver for a
/// string-only blob), so editing any resolved field by hand clears it (<see cref="ClearAddressString"/>)
/// to keep the two from disagreeing.
/// the tag was authored that way; <c>null</c> for a tag authored by region + numeric address. When
/// <see cref="AddressResolved"/> the persisted resolved fields are what the driver reads; otherwise the
/// driver resolves the string with its own family (see <see cref="IsStringOnly"/>). Editing any resolved
/// field by hand clears it (<see cref="ClearAddressString"/>) so the fields become the authored address.
/// </summary>
public string? AddressString { get; set; }
/// <summary>
/// True when the region / offset / type fields were resolved FROM <see cref="AddressString"/> by
/// <see cref="ResolveAddress"/> (persisted as <c>"addressResolved": true</c>). Only then are the persisted
/// fields authoritative over the string; an unmarked string is resolved by the driver with its own family.
/// </summary>
public bool AddressResolved { get; set; }
/// <summary>
/// A string-only tag (CSV / API authored): it carries an <see cref="AddressString"/> the driver resolves,
/// 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;
/// <summary>
/// Resolves an address string through <see cref="ModbusAddressParser"/> with the owning driver's
/// family and copies the result into the region / offset / type / bit / string-length / byte-order /
@@ -54,6 +68,7 @@ public sealed class ModbusTagConfigModel
if (!ModbusAddressParser.TryParse(text, family, melsecSubFamily, out var parsed, out var error) || parsed is null)
return error ?? "Address did not parse.";
AddressString = text!.Trim();
AddressResolved = true;
Region = parsed.Region;
Address = parsed.Offset;
DataType = parsed.DataType;
@@ -74,7 +89,11 @@ public sealed class ModbusTagConfigModel
}
/// <summary>Drops the address string so the hand-edited region / address fields are what the driver reads.</summary>
public void ClearAddressString() => AddressString = null;
public void ClearAddressString()
{
AddressString = null;
AddressResolved = false;
}
/// <summary>
/// Reads the owning driver's <c>family</c> / <c>melsecSubFamily</c> from its DriverConfig JSON (keys
@@ -143,6 +162,7 @@ public sealed class ModbusTagConfigModel
Writable = TagConfigJson.GetBoolNullable(o, "writable"),
AddressString = string.IsNullOrWhiteSpace(TagConfigJson.GetString(o, "addressString"))
? null : TagConfigJson.GetString(o, "addressString"),
AddressResolved = TagConfigJson.GetBoolNullable(o, ModbusTagDefinitionFactory.AddressResolvedKey) == true,
_bag = o,
};
}
@@ -152,14 +172,20 @@ public sealed class ModbusTagConfigModel
/// <returns>The serialised TagConfig JSON string.</returns>
public string ToJson()
{
TagConfigJson.Set(_bag, "region", Region);
TagConfigJson.Set(_bag, "address", Address);
TagConfigJson.Set(_bag, "dataType", DataType);
TagConfigJson.Set(_bag, "byteOrder", ByteOrder);
TagConfigJson.Set(_bag, "bitIndex", BitIndex);
TagConfigJson.Set(_bag, "stringLength", StringLength);
// A string-only tag's address fields are not its address (they are load-time defaults): never stamp
// them, or the defaulted region/address would sit beside the string looking authored.
if (!IsStringOnly)
{
TagConfigJson.Set(_bag, "region", Region);
TagConfigJson.Set(_bag, "address", Address);
TagConfigJson.Set(_bag, "dataType", DataType);
TagConfigJson.Set(_bag, "byteOrder", ByteOrder);
TagConfigJson.Set(_bag, "bitIndex", BitIndex);
TagConfigJson.Set(_bag, "stringLength", StringLength);
}
TagConfigJson.Set(_bag, "writable", Writable);
TagConfigJson.Set(_bag, "addressString", AddressString);
TagConfigJson.Set(_bag, ModbusTagDefinitionFactory.AddressResolvedKey, AddressString is not null && AddressResolved ? true : null);
return TagConfigJson.Serialize(_bag);
}