eclipse / milo

Eclipse Milo™ - an open source implementation of OPC UA (IEC 62541).
http://www.eclipse.org/milo
Eclipse Public License 2.0
1.16k stars 427 forks source link

0.3 NodeFactory TypeDefiniton #350

Closed Molokay closed 5 years ago

Molokay commented 5 years ago

Hello, i am using the 0.3-Snapshot. Nodes created by the NodeFactory have multiple type definiton references, which is not correct.

Example Type Hierarchy: BaseObjectType -> TopologyElementType -> DeviceType -> TemperatureControllerDeviceType

A TemperatureControllerDeviceType instance created by the NodeFactory must only contain a hasTypeDefintion reference to the TemperatureControllerDeviceType, but it has also references to DeviceType and BaseObjectType (strangely TopologyElementType is missing).

kevinherron commented 5 years ago

Interesting, I thought I fixed this issue once during development. Do you have any sample code I can reproduce this with?

kevinherron commented 5 years ago

Are you using the deprecated NodeFactory or the new one in the org.eclipse.milo.opcua.sdk.server.nodes.factories package?

Molokay commented 5 years ago

I am using the new one. The instances have all the elements of the supertypes (resolved former issue with the old factory). I am putting a code sample together right now.

Molokay commented 5 years ago

This is a very early stage of a server protoype i am developing. Mostly i just implemented a part of the OPC UA Device Integration Specification. nodeFactoryExample.zip

kevinherron commented 5 years ago

Ok great. It's strange because I would expect the analog value instance from the ExampleServer to exhibit the same issue but it doesn't.

kevinherron commented 5 years ago

I don't see any places in your example where you are using the NodeFactory to instantiate an instance, it looks like everything is assembled manually.

Molokay commented 5 years ago

The code you need to reproduce this should be available in the namespaces. If you need further clarification i will be glad to help. I used UaExpert to browse the resulting adress space.

Molokay commented 5 years ago

In the AggregatingNamespace.class is a instance created by the node factory.

kevinherron commented 5 years ago

Oops, I see it now. Thanks.

Molokay commented 5 years ago

I must atttend a meeting now, i will get back to you in a few hours if there are more questions. Maybe one solution would be to implement a check in the nodefactory for the first level, i.e. root node (here TemperatureControllerDeviceType) and only add there references of type HasTypeDefintion.

kevinherron commented 5 years ago

Okay, I think I've fixed it. The problem occurred when the type definition hierarchy spanned multiple namespaces. That's why it wasn't happening with the AnalogItemType instance in ExampleServer - that type hierarchy all belongs to the same namespace.

I'll push the change later today.

If you'd like to try out the fix immediately you can apply this patch:

Index: opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/nodes/factories/ReferenceTable.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/nodes/factories/ReferenceTable.java   (revision 379554add93abb0c6247ffd2de7f0e5f0183ea09)
+++ opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/nodes/factories/ReferenceTable.java   (date 1540818041000)
@@ -61,11 +61,14 @@
                 // This logic may need to be extended to include other Reference types for
                 // which there should only be one.

-                boolean noTypeDefinition = mergedTable.references.stream()
-                    .noneMatch(r ->
-                        r.browsePath.equals(browsePath) && r.nodeId.equals(Identifiers.HasTypeDefinition));
+                boolean hasTypeDefinitionReference = mergedTable.references.stream().anyMatch(r -> {
+                    String name1 = r.browsePath.browseName.getName();
+                    String name2 = browsePath.browseName.getName();

-                if (noTypeDefinition) {
+                    return r.nodeId.equals(Identifiers.HasTypeDefinition) && Objects.equals(name1, name2);
+                });
+
+                if (!hasTypeDefinitionReference) {
                     mergedTable.references.add(row);
                 }
             } else if (!mergedTable.references.contains(row)) {
kevinherron commented 5 years ago

There's still another issue. The Tree<UaNode> created by NodeFactory for this node has no children. You're not using it so you haven't noticed, but it's still something I need to look into.

Molokay commented 5 years ago

Thanks for looking into the issue.

kevinherron commented 5 years ago

Fixed in PR #351 if you want to try it out before I merge.

Molokay commented 5 years ago

Now instances have the correct type definition reference. Thank you.