Cannot import nodes if the object node type has a parent variable node type.

[xydan83]

Hi!
I have a problem importing some sequence of nodes from xml. This xml from the UA_Server model looks like this:

I exported this part of the nodes using my xml exporter and then tried to import.

<?xml version="1.0" encoding="UTF-8"?>
<UANodeSet xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:uax="http://opcfoundation.org/UA/2008/02/Types.xsd" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns="http://opcfoundation.org/UA/2011/03/UANodeSet.xsd">
    <NamespaceUris>
        <Uri>urn:OPCUAServ1:UnifiedAutomation:UaServerCpp</Uri>
        <Uri>http://www.unifiedautomation.com/DemoServer/</Uri>
        <Uri>urn:UnifiedAutomation:CppDemoServer:BuildingAutomation</Uri>
        <Uri>http://www.unifiedautomation.com/DemoServer/AccessPermission</Uri>
        <Uri>http://www.unifiedautomation.com/DemoServer/StateMachines</Uri>
        <Uri>urn:UnifiedAutomation:CppDemoServer:UANodeSetXmlImport</Uri>
        <Uri>urn:UnifiedAutomation:PubSubConfiguration</Uri>
    </NamespaceUris>
    <Aliases>
        <Alias Alias="Boolean">i=1</Alias>
        <Alias Alias="Byte">i=3</Alias>
        <Alias Alias="HasComponent">i=47</Alias>
        <Alias Alias="HasHistoricalConfiguration">i=56</Alias>
        <Alias Alias="HasProperty">i=46</Alias>
        <Alias Alias="HasTypeDefinition">i=40</Alias>
        <Alias Alias="Organizes">i=35</Alias>
    </Aliases>
    <!--Value elements are currently not supported in Variable and VariableType nodes.-->
    <!--Definition elements are currently not supported in UADataType.-->
    <UAVariable NodeId="ns=2;s=Demo.History.Historian_1" BrowseName="2:Historian_1" ParentNodeId="i=85" DataType="Byte" AccessLevel="4" UserAccessLevel="4">
        <DisplayName>Historian_1</DisplayName>
        <Description>Holds sample data to test aggregates as described in OPC UA Specification Part 13.</Description>
        <References>
            <Reference ReferenceType="Organizes" IsForward="false">i=85</Reference>
            <Reference ReferenceType="HasTypeDefinition">i=63</Reference>
            <Reference ReferenceType="HasHistoricalConfiguration">ns=2;s=History1.HistoricalDataConfiguration</Reference>
        </References>
    </UAVariable>
    <UAObject NodeId="ns=2;s=History1.HistoricalDataConfiguration" BrowseName="HA Configuration" ParentNodeId="ns=2;s=Demo.History.Historian_1">
        <DisplayName>HA Configuration</DisplayName>
        <References>
            <Reference ReferenceType="HasTypeDefinition">i=2318</Reference>
            <Reference ReferenceType="HasComponent">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration</Reference>
            <Reference ReferenceType="HasProperty">ns=2;s=History1.HistoricalDataConfiguration.Stepped</Reference>
            <Reference ReferenceType="HasHistoricalConfiguration" IsForward="false">ns=2;s=Demo.History.Historian_1</Reference>
        </References>
    </UAObject>
    <UAObject NodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration" BrowseName="AggregateConfiguration" ParentNodeId="ns=2;s=History1.HistoricalDataConfiguration">
        <DisplayName>AggregateConfiguration</DisplayName>
        <References>
            <Reference ReferenceType="HasTypeDefinition">i=11187</Reference>
            <Reference ReferenceType="HasProperty">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.PercentDataBad</Reference>
            <Reference ReferenceType="HasProperty">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.PercentDataGood</Reference>
            <Reference ReferenceType="HasProperty">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.TreatUncertainAsBad</Reference>
            <Reference ReferenceType="HasProperty">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.UseSlopedExtrapolation</Reference>
            <Reference ReferenceType="HasComponent" IsForward="false">ns=2;s=History1.HistoricalDataConfiguration</Reference>
        </References>
    </UAObject>
    <UAVariable NodeId="ns=2;s=History1.HistoricalDataConfiguration.Stepped" BrowseName="Stepped" ParentNodeId="ns=2;s=History1.HistoricalDataConfiguration" DataType="Boolean">
        <DisplayName>Stepped</DisplayName>
        <References>
            <Reference ReferenceType="HasTypeDefinition">i=68</Reference>
            <Reference ReferenceType="HasProperty" IsForward="false">ns=2;s=History1.HistoricalDataConfiguration</Reference>
        </References>
    </UAVariable>
    <UAVariable NodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.PercentDataBad" BrowseName="PercentDataBad" ParentNodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration" DataType="Byte">
        <DisplayName>PercentDataBad</DisplayName>
        <References>
            <Reference ReferenceType="HasTypeDefinition">i=68</Reference>
            <Reference ReferenceType="HasProperty" IsForward="false">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration</Reference>
        </References>
    </UAVariable>
    <UAVariable NodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.PercentDataGood" BrowseName="PercentDataGood" ParentNodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration" DataType="Byte">
        <DisplayName>PercentDataGood</DisplayName>
        <References>
            <Reference ReferenceType="HasTypeDefinition">i=68</Reference>
            <Reference ReferenceType="HasProperty" IsForward="false">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration</Reference>
        </References>
    </UAVariable>
    <UAVariable NodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.TreatUncertainAsBad" BrowseName="TreatUncertainAsBad" ParentNodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration" DataType="Boolean">
        <DisplayName>TreatUncertainAsBad</DisplayName>
        <References>
            <Reference ReferenceType="HasTypeDefinition">i=68</Reference>
            <Reference ReferenceType="HasProperty" IsForward="false">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration</Reference>
        </References>
    </UAVariable>
    <UAVariable NodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration.UseSlopedExtrapolation" BrowseName="UseSlopedExtrapolation" ParentNodeId="ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration" DataType="Boolean">
        <DisplayName>UseSlopedExtrapolation</DisplayName>
        <References>
            <Reference ReferenceType="HasTypeDefinition">i=68</Reference>
            <Reference ReferenceType="HasProperty" IsForward="false">ns=2;s=History1.HistoricalDataConfiguration.AggregateConfiguration</Reference>
        </References>
    </UAVariable>
</UANodeSet>

The main problem is that nodesed-loader sorts the object by type and then adds it to the server. And this order, as far as I understand, is this:

• Don't look at the imported node counts, those are from another import. Here I just noted the order.

First we import the Objects nodes and then the Variable nodes, but what if we have a reference like Objects --ref--> Variable (parent)? Because when object nodes try to be added to the server, the server cannot find the variables because they do not exist.
And we have an error:

Can we do anything about this?

[matkonnerth]

Hi, thanks for the report, you are right, the objects are imported before the variables.

I don't think that an object should be with a hierachical reference below a variable?

[xydan83]

Hi, thanks for the report, you are right, the objects are imported before the variables.

I don't think that an object should be with a hierachical reference below a variable?

Yes, I think the same thing, but the demo model is built on top of a certified UA_Server. And the demo model was created by UA. I think this can be trusted. Moreover, in the world of OPC - Unified Automation is of great importance.
I think we need to look into the standard, maybe there we will find some clarification about reference connections.

[xydan83]

Hmm. This type of reference is something special)
Has a special condition - If a HistoricalDataNode has configuration defined then one instance shall have a BrowseName of ‘HA Configuration’.
https://reference.opcfoundation.org/Core/Part11/v104/docs/5.2.4
Maybe only with these types of references can objects have variables in their parents? But so far I have not found an explanation.

[matkonnerth]

Ok, seems that for that use case it makes sense, but I don't know why this is a hierachical reference. Nevertheless we should think of how to handle this.

First I do a topological sort without considering the nodeclass, but then I fall back to import the objects before the variables.

[matkonnerth]

open62541-nodeset-loader/src/Nodeset.c

Line 261 in fd60349

return Sort_start(nodeset->sortCtx, nodeset, Nodeset_addNode,

here the nodes are inserted in NodeContainers, every nodeclass has its own container. Maybe we can add variables and objects in a container for instances and then later import them. So the ordering should be preserved.

[xydan83]

Ok, seems that for that use case it makes sense, but I don't know why this is a hierachical reference.

So, I found an explonations...I think so)))

https://reference.opcfoundation.org/Core/Part11/v104/docs/5

In this image we can see that the variable has a reference to an object (HA configuration) since it is an object node.

[xydan83]

open62541-nodeset-loader/src/Nodeset.c

Line 261 in fd60349

return Sort_start(nodeset->sortCtx, nodeset, Nodeset_addNode,

here the nodes are inserted in NodeContainers, every nodeclass has its own container. Maybe we can add variables and objects in a container for instances and then later import them. So the ordering should be preserved.

As far as I understand, in this reference type we need to allow the variable to be imported before the object, maybe there could be some workaround, but I think the same situation might be in other weird reference types as well.

What if we do post-processing of nodes whose parents were not found? Let's say, if a node was not added because the parent was not found - before moving on - mark this node or copy its link to another list (deferred list of nodes, second chance :)))
Then, at the very end, run such post-processing, try to add such nodes again (maybe by then the parent will be on the server?)
For example, process sequence:
....
Objects:
Methods:
Variables:
Views:
Trying to import nodes without found parents (or some other description):

What do you think about this solution? :)

[matkonnerth]

ok, this is another solution, we should give that a try.

[xydan83]

ok, this is another solution, we should give that a try.

Of course, maybe my solution for some reason this will not be entirely correct, but I don’t see any problems with this yet, if the only problem is that there is no parent, in any case, even if the node really does not have a parent, the second chance will not work for such a node, otherwise it will be added.

And we don't need to make any changes to the sorting itself.

[xydan83]

So, tomorrow I can try to prepare this solution, but if you do it earlier, please write to me in this issue.

[matkonnerth]

Would be cool if you could prepare a pull request, I will have no time before the weekend.

[xydan83]

Hi!
Well I made some pre-changes to the code and here is the result.

But I'm still thinking about the logic and how best to incorporate these changes into your code.

And I have questions for you about the changes:

1. Can I will do this section of code (second chance) switchable from CMakeLists.txt?
2. Now I have written it in such a way that this code will once again go through the nodes in sorted form in an attempt to add them to the server only once, but there may well be a situation when nodes of the Object type will be connected to Variables every other time (Object-->Variable-->Object-->Variable) and then this code will only work on the first such pair. Is it worth doing a limited cycle of adding attempts? For example, we try to execute an add loop every time if, after executing the previous loop, we had at least one error associated with the parent. Plus, we can make protection against an infinite loop, no more than 100 attempts. It's a stupid mechanism, but easy to implement. If we want to be smarter, I think we need to analyze the entire parent chain, and this will be more difficult and may have to change a lot in the overall algorithm.

There is another simple way, make one only for the node that has type defenition like i=2318 [HistoricalDataConfigurationType]. Because I have not yet noticed such problems with any other types. Perhaps it is worth considering each such case separately.

3. By the way, I noticed that the counter in NodesetLoader_forEachNode simply returns the size.And this information is used in the debug message. But if we have a problem adding a node, we will see that the node is supposedly added, but it will not be true. Maybe we can create a second method Nodeset_forEachNode that can return the nodes that were actually added?

[matkonnerth]

Hi,
regarding 1.)
we should not make it switchable. There are a lot of tests and we should add tests (for example your test nodesets) for the secondChance algorithm, so I don't fear any regression.

2.) lets try a limited amount of cycles (for example 10 times, but lets make it a constant).

[xydan83]

Hi, regarding 1.) we should not make it switchable. There are a lot of tests and we should add tests (for example your test nodesets) for the secondChance algorithm, so I don't fear any regression.

2.) lets try a limited amount of cycles (for example 10 times, but lets make it a constant).

Hi.
Okay, I'll do it, but I'm just afraid that if we do it as hard code, and if someone tries to import 100,000 or more nodes and something at the beginning of the structure has a missing parent, but due to an error in xml, the loop will run 10 times to no avail and it will be a waste of time.
I don’t see a problem if I add a couple of parameters to CMakeLists.txt:

1. Enabling or disabling the second chance algorithm and its tests
2. Add a counter cycle parameter and set it to 10 cycles by default.

But if you think it’s better without it, then I will do so.

[matkonnerth]

Hi, please make it without the compiler switch, it makes it harder to test. If someone has an error in the nodeset.xml, he should notice it (we should also log a warning, if the second chance is needed) and fix the xml.

[xydan83]

Hi!
I have prepared a PR [draft] with a solution. There are no tests yet. Please see if everything is correct regarding your code base?
#247

[xydan83]

Want to ask if you know how to solve a problem using a local usage test (unit test)?
I'm using Ubuntu 22.04 and CLion. I enable ENABLE_TESTING and compile with CTest, but I can't compile because I have problems with this:

undefined reference to

I added the set(CMAKE_C_FLAGS "-lsubunit") flag to CMakeLists.txt, but nothing happened.
I haven't worked with "check" before...

[matkonnerth]

here is the way the CI builds it on ubuntu:
https://github.com/open62541/open62541-nodeset-loader/blob/master/.github/workflows/GccDebugMemcheck.yml

It installs libcheck:
sudo apt-get install check

and configures the build with cmake:

cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_CONAN=OFF -DENABLE_TESTING=ON -DBUILD_SHARED_LIBS=ON -DENABLE_DATATYPEIMPORT_TEST=ON -DCALC_COVERAGE=ON ..

maybe you also need:
sudo apt-get install libsubunit-dev

[xydan83]

here is the way the CI builds it on ubuntu: https://github.com/open62541/open62541-nodeset-loader/blob/master/.github/workflows/GccDebugMemcheck.yml

It installs libcheck: sudo apt-get install check

and configures the build with cmake:

cmake -DCMAKE_BUILD_TYPE=Debug -DENABLE_CONAN=OFF -DENABLE_TESTING=ON -DBUILD_SHARED_LIBS=ON -DENABLE_DATATYPEIMPORT_TEST=ON -DCALC_COVERAGE=ON ..

maybe you also need: sudo apt-get install libsubunit-dev

Thanks!
I tried using some commands in this script, but it still doesn't work, same error. But here's what confused me when working with cmake:

sudo apt-get install check
sudo apt-get install libsubunit-dev

This was the first thing I did.

This is the last version of check:

Maybe I'll compile it and try to install it manually.

[xydan83]

Hi!
I made the changes and uploaded them to git. Could you take a look?
I still haven't solved my problem with building tests. I will try different options.

[xydan83]

I found that using the newest "FindCheck.cmake" I can build the test without the link error. Can we try replacing FindCheck.cmake with a newer one?
https://github.com/babelouest/ulfius/blob/master/cmake-modules/FindCheck.cmake

[matkonnerth]

for sure, just open a PR with that

[xydan83]

FindCheck.cmake

Well, I tried updating FindCheck.cmake, but there was a problem with the GIT build. So I think it's better to keep the old one.
Maybe I'm the only one who has a problem with the assembly with "check" locally)