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

Browse-commit discards the discovered data type, so a browsed REAL commits as a DINT (AbCip, TwinCAT) #536

New Issue
Closed
opened 2026-08-01 12:23:11 -04:00 by dohertj2 · 1 comment
dohertj2 commented 2026-08-01 12:23:11 -04:00
Owner
Copy Link
Copy Source

Found while running the #468 controller-browse live gate on docker-dev, now that the AbCip fixture serves @tags.

What happens

Browsing an AbCip device and committing two leaves produced:

Name         | DataType | TagConfig
TestDINT     | Int32    | {"tagPath":"TestDINT","dataType":"DInt"}
Pump_1_Speed | Float    | {"tagPath":"Pump_1_Speed","dataType":"DInt"}

Pump_1_Speed is a REAL. The row DataType is right (Float) — the discovered type does reach the commit — but the TagConfig says DInt, because RawBrowseCommitMapper.BuildTagConfig sets only the address field and leaves everything else at the model default, and AbCipTagConfigModel.DataType defaults to AbCipDataType.DInt.

Why it matters

The driver does not read the row's DataType. AbCipTagDefinitionFactory.FromTagConfig reads dataType out of the TagConfig JSON and builds the libplctag tag definition from it, so a browse-committed REAL is read as a DINT — four bytes of IEEE-754 reinterpreted as a signed integer. The OPC UA node is typed Float while the driver decodes Int32, and nothing reconciles the two.

This is silent: the deploy gate's only dataType check warns on a present-but-invalid enum (05/CONV-2), and DInt is perfectly valid. It is also easy to miss on a sim, where every tag reads 0 and a misdecode still yields 0.

Scope

Two drivers are affected — the ones that are both browsable and decode off TagConfig.dataType:

  • AbCip — AbCipTagConfigModel.DataType = AbCipDataType.DInt, consumed by AbCipTagDefinitionFactory. Reproduced above.
  • TwinCAT — TwinCATTagConfigModel.DataType = TwinCATDataType.DInt, consumed by TwinCATTagDefinitionFactory. Same shape by inspection; not reproduced, because the ADS symbol-upload backend is still fixture-blocked (the sibling half of #468).

The other typed models also carry a defaulted DataType (AbLegacy, Focas, Modbus, Mqtt, MTConnect), but they are either not browsable or do not decode off that field — worth re-checking rather than assuming when this is picked up.

Suggested fix

BuildTagConfig already receives everything it needs: the browse side-channel supplies the DriverDataType that RawBrowseCommitMapper.MapDataType turns into the row's built-in type name. Thread the same value into the driver-typed model — mapping DriverDataType to AbCipDataType / TwinCATDataType — instead of leaving the model default.

Note this widens BuildTagConfig's stated contract ("setting ONLY the driver's address field ... leaving every other field at its model default"), so the doc-comment should change with it. Committing no type is not an option — the model has no unset state, so the default lands either way; the choice is between a known-correct type and a guess.

Test note

A fixture where every tag reads 0 cannot tell a correct decode from a wrong one. Any test for this wants a non-zero REAL whose DINT reinterpretation is obviously different — 1.0f reads back as 1065353216.

Found by: #468 live gate, 2026-08-01.

Found while running the #468 controller-browse live gate on docker-dev, now that the AbCip fixture serves `@tags`. ## What happens Browsing an AbCip device and committing two leaves produced: ``` Name | DataType | TagConfig TestDINT | Int32 | {"tagPath":"TestDINT","dataType":"DInt"} Pump_1_Speed | Float | {"tagPath":"Pump_1_Speed","dataType":"DInt"} ``` `Pump_1_Speed` is a REAL. The **row** `DataType` is right (`Float`) — the discovered type does reach the commit — but the **`TagConfig`** says `DInt`, because `RawBrowseCommitMapper.BuildTagConfig` sets only the address field and leaves everything else at the model default, and `AbCipTagConfigModel.DataType` defaults to `AbCipDataType.DInt`. ## Why it matters The driver does not read the row's `DataType`. `AbCipTagDefinitionFactory.FromTagConfig` reads `dataType` out of the `TagConfig` JSON and builds the libplctag tag definition from it, so a browse-committed REAL is read as a DINT — four bytes of IEEE-754 reinterpreted as a signed integer. The OPC UA node is typed `Float` while the driver decodes `Int32`, and nothing reconciles the two. This is silent: the deploy gate's only `dataType` check warns on a *present-but-invalid* enum (05/CONV-2), and `DInt` is perfectly valid. It is also easy to miss on a sim, where every tag reads 0 and a misdecode still yields 0. ## Scope Two drivers are affected — the ones that are both browsable and decode off `TagConfig.dataType`: - **AbCip** — `AbCipTagConfigModel.DataType = AbCipDataType.DInt`, consumed by `AbCipTagDefinitionFactory`. Reproduced above. - **TwinCAT** — `TwinCATTagConfigModel.DataType = TwinCATDataType.DInt`, consumed by `TwinCATTagDefinitionFactory`. Same shape by inspection; not reproduced, because the ADS symbol-upload backend is still fixture-blocked (the sibling half of #468). The other typed models also carry a defaulted `DataType` (`AbLegacy`, `Focas`, `Modbus`, `Mqtt`, `MTConnect`), but they are either not browsable or do not decode off that field — worth re-checking rather than assuming when this is picked up. ## Suggested fix `BuildTagConfig` already receives everything it needs: the browse side-channel supplies the `DriverDataType` that `RawBrowseCommitMapper.MapDataType` turns into the row's built-in type name. Thread the same value into the driver-typed model — mapping `DriverDataType` to `AbCipDataType` / `TwinCATDataType` — instead of leaving the model default. Note this widens `BuildTagConfig`'s stated contract ("setting ONLY the driver's address field ... leaving every other field at its model default"), so the doc-comment should change with it. Committing *no* type is not an option — the model has no unset state, so the default lands either way; the choice is between a known-correct type and a guess. ## Test note A fixture where every tag reads 0 cannot tell a correct decode from a wrong one. Any test for this wants a non-zero REAL whose DINT reinterpretation is obviously different — `1.0f` reads back as `1065353216`. Found by: #468 live gate, 2026-08-01.
dohertj2 commented 2026-08-01 15:20:32 -04:00
Author
Owner
Copy Link
Copy Source

Fixed in 6b3da931.

The scope was wider than filed: FOCAS is affected too. All three drivers whose TagConfig carries their own data-type enum — AbCip, TwinCAT, FOCAS — decode the wire bytes by that field rather than by the raw row's OPC UA DataType, and all three are universal-browser drivers.

The obvious fix would not have worked. Reverse-mapping the leaf's driver-agnostic DriverDataType fixes the REAL and silently keeps getting the integers wrong: SInt/Int/DInt/USInt/UInt all collapse into Int32 on the way out of the driver, so a 16-bit INT would still bind as a 32-bit DINT. The driver now states its own type name (DriverAttributeInfo.NativeDataType → AttributeInfo.AddressFields["dataType"] → RawBrowseCommitMapper); a leaf that states nothing keeps the model default, which is exactly the old behaviour rather than a guess.

A numeric string is refused before parsing — Enum.TryParse accepts "9" for any enum and Enum.IsDefined then confirms it, so an unguarded parse binds a tag to a type nobody named. My first version of the guard had that hole; a test caught it.

Live on docker-dev, browse-committing against the patched ab_server fixture:

Tag Row DataType Stored TagConfig
Pump_1_Speed Float {"dataType":"DInt"} ← committed before the fix
TestString String {"dataType":"String"}
Motor_Run Boolean {"dataType":"Bool"}
TestSINT Int32 {"dataType":"SInt"}

TestSINT is the falsifier: its row DataType is Int32, identical to TestDINT's, so no reverse map from the agnostic type could have produced SInt.

⚠️ Pre-existing browse-committed tags keep their wrong dataType — the fix applies to new commits only. Re-commit or edit them in the typed editor.

A second defect found on the way, fixed in the same commit

While verifying I re-tested the Test Connect limitation this repo had documented as a fixture gap ("ab_server doesn't implement @raw_cpu_type"). Every part of that was wrong, and it was hiding a production defect.

  • @raw_cpu_type is not a libplctag special tag — the CIP dispatch knows only @raw, @tags and @udt/. It is sent as an ordinary symbolic name that no controller defines, and the controller's "tag does not exist" is the successful answer.
  • ab_server answered an unresolvable tag path with CIP 0x03 where a Logix controller answers 0x04 — ErrorBadParam vs ErrorNotFound, which the probe classifies oppositely. Fixed in vendored patch 0002.
  • With the fixture then telling the truth, the real defect surfaced: AbCipDriverProbe.IsReachableException matched the native C spellings (STATUS_NOT_FOUND), but libplctag.NET throws with the managed enum name (ErrorNotFound). Those substrings can never match, so the reachable classification could not fire on the exception path — the path every no-authored-tags probe takes. Test Connect was failing on real hardware too, not only against the sim, and it fails at exactly the moment an operator uses it: on a device with no tags authored yet.

Rejected along the way: switching the fallback to @tags. libplctag builds a tag-listing tag with no network I/O, so it reports OK against any TCP listener — measured green against a Mosquitto broker.

Full solution suite green. Falsification runs confirm exactly the intended tests go red: 9 for the type carry, 3 (the reachable statuses only) for the probe classification.

Fixed in `6b3da931`. **The scope was wider than filed: FOCAS is affected too.** All three drivers whose `TagConfig` carries their own data-type enum — AbCip, TwinCAT, **FOCAS** — decode the wire bytes by that field rather than by the raw row's OPC UA `DataType`, and all three are universal-browser drivers. **The obvious fix would not have worked.** Reverse-mapping the leaf's driver-agnostic `DriverDataType` fixes the REAL and silently keeps getting the integers wrong: `SInt`/`Int`/`DInt`/`USInt`/`UInt` all collapse into `Int32` on the way out of the driver, so a 16-bit INT would still bind as a 32-bit DINT. The driver now *states* its own type name (`DriverAttributeInfo.NativeDataType` → `AttributeInfo.AddressFields["dataType"]` → `RawBrowseCommitMapper`); a leaf that states nothing keeps the model default, which is exactly the old behaviour rather than a guess. A numeric string is refused before parsing — `Enum.TryParse` accepts `"9"` for any enum and `Enum.IsDefined` then confirms it, so an unguarded parse binds a tag to a type nobody named. My first version of the guard had that hole; a test caught it. **Live on docker-dev**, browse-committing against the patched `ab_server` fixture: | Tag | Row DataType | Stored TagConfig | |---|---|---| | `Pump_1_Speed` | Float | `{"dataType":"DInt"}` ← committed before the fix | | `TestString` | String | `{"dataType":"String"}` | | `Motor_Run` | Boolean | `{"dataType":"Bool"}` | | `TestSINT` | Int32 | `{"dataType":"SInt"}` | `TestSINT` is the falsifier: its row `DataType` is `Int32`, identical to `TestDINT`'s, so no reverse map from the agnostic type could have produced `SInt`. ⚠️ **Pre-existing browse-committed tags keep their wrong `dataType`** — the fix applies to new commits only. Re-commit or edit them in the typed editor. --- ### A second defect found on the way, fixed in the same commit While verifying I re-tested the Test Connect limitation this repo had documented as a fixture gap ("`ab_server` doesn't implement `@raw_cpu_type`"). **Every part of that was wrong, and it was hiding a production defect.** - `@raw_cpu_type` is not a libplctag special tag — the CIP dispatch knows only `@raw`, `@tags` and `@udt/`. It is sent as an ordinary symbolic name that no controller defines, and the controller's "tag does not exist" **is** the successful answer. - `ab_server` answered an unresolvable tag path with CIP **0x03** where a Logix controller answers **0x04** — `ErrorBadParam` vs `ErrorNotFound`, which the probe classifies oppositely. Fixed in vendored patch `0002`. - With the fixture then telling the truth, the real defect surfaced: `AbCipDriverProbe.IsReachableException` matched the native C spellings (`STATUS_NOT_FOUND`), but libplctag.NET throws with the **managed enum name** (`ErrorNotFound`). Those substrings can never match, so the reachable classification could not fire on the exception path — the path every no-authored-tags probe takes. **Test Connect was failing on real hardware too, not only against the sim**, and it fails at exactly the moment an operator uses it: on a device with no tags authored yet. Rejected along the way: switching the fallback to `@tags`. libplctag builds a tag-listing tag with **no network I/O**, so it reports OK against any TCP listener — measured green against a Mosquitto broker. Full solution suite green. Falsification runs confirm exactly the intended tests go red: 9 for the type carry, 3 (the reachable statuses only) for the probe classification.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
auto-managed
queue management
 cross-driver
queue management
 driver/abcip
queue management
 driver/ablegacy
queue management
 driver/focas
queue management
 driver/mqtt
MQTT / Sparkplug B broker-subscribe driver
 driver/opcuaclient
queue management
 driver/s7
queue management
 driver/twincat
queue management
 phase/1
queue management
 phase/2
queue management
 phase/3
queue management
 phase/4
queue management
 phase/5
queue management
 phase/6
queue management
 queue/blocked
queue management
 queue/done
queue management
 queue/failed
queue management
 queue/in-progress
queue management
 queue/queued
queue management
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dohertj2 (dohertj2)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#536
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?