Feature/method declaration

[mlgiraud]

Add reference to node declared in methodDeclarationId.

If no callback is set, the call will be delegated to the node specified in the methodDeclarationId. This simplifies use with nodesets that definie individual method nodes for each object instance.

See also #4528

[jpfr]

This lookup at runtime thing could lead to nasty surprises that are detected late.
Particularly we now call the method from a) with the context pointer from b).

Why not copy the method pointer instead of copying the nodeid?
That saves the time for the lookup at runtime.

The prototype method can be looked up from the object type.
No need to take another nodeid argument.

More logging, more documentation.
No late-surprising of the user.

keep up the good work!

[mlgiraud]

I was also not quite happy with that. I thought about calling the method of the parent method in the context of the child, but it might in some weird cases be possible, that the parent method has a different argument specification, which would cause an invalid memory access. I also don't want to copy the method pointer from the parent to the child, because it is possible that the parent callback is updated, an then the child would not get the updated pointer (might be possible by looking up all child but that would require some additional references and sounds really bug-prone). If you like i can change it to just look up the parent and call the callback in the context of the current node instead, risking an invalid memory access.

[jpfr]

It appears there is no perfect solution.
I have a preference for the following:

• When a method node is copied from an ObjectType, then it should retain the original method callback pointer.
  That is imho already the case
• When a method is manually generated as part of an object (e.g. in order to give the method a predefined NodeId) and if the method callback pointer is still NULL after the constructor callback has been called, then the method callback pointer from the prototype method within the ObjectType is copied over. The prototype method can be looked up based on the browseName within the ObjectType. This should then also trigger a warning log message.

This has the following advantages:

1. No API change required.
2. No overhead for lookups once the method is finalized.
3. The user gets to see a warning early on. No late-surprise when the method is called for the first time.

Yeah, it can get problematic when the method pointer in the ObjectType is replaced later on.
But that sounds like a very uncommon thing. And the warning for copying the callback pointer should make the implications clear.

[jpfr]

The warnings could get annoying when they appear during the setup of ns0.

You can add a RuleHandling configuration option for this behavior: Error / Log / Accept
https://github.com/open62541/open62541/blob/master/include/open62541/server.h#L102

This config option can then be set to Accept for the duration of the ns0 setup.

[mlgiraud]

I'm not quite sure how you want to look up the prototype method object via browsename? It is entirely possible, that there is a hierarchy like A<-B<-C , i.e. B is prototype of A and C of B. How are we going to find out this hierarchy?
I would propose keeping the API change, since that is anyways necessary to pass the methodDeclarationId from the xml files into the nodeset. Instead of delegating entirely to the parent however, how about trying to call the parent method callback in the context of the current node, but checking if the arguments of the current node match with the parent? That way the context ist as expected, and we also won't have any crashing errors when the callback does not match.
This has the upside that the method callback can be changed at a later time and also be set after construction.

[jpfr]

I'm not quite sure how you want to look up the prototype method object via browsename? It is entirely possible, that there is a hierarchy like A<-B<-C , i.e. B is prototype of A and C of B. How are we going to find out this hierarchy?

That is done automatically.
Just as with mandatory children that are instantiated for an ObjectType.
You can splice in the method callback copying from the ObjectType at this location:
https://github.com/open62541/open62541/blob/master/src/server/ua_services_nodemanagement.c#L558

I would propose keeping the API change, since that is anyways necessary to pass the methodDeclarationId from the xml files into the nodeset. Instead of delegating entirely to the parent however, how about trying to call the parent method callback in the context of the current node, but checking if the arguments of the current node match with the parent? That way the context ist as expected, and we also won't have any crashing errors when the callback does not match.
This has the upside that the method callback can be changed at a later time and also be set after construction.

That leads exactly to the kind of "late surprise" I was talking about.
Imagine the user sets a new callback on a method.
He can check the references to that method which objects are using it.
But he does not know if that method is registered anywhere else as the MethodDeclarationId. That could lead to breakage.
Checking the arguments does not help either. Because the incompatility might be hidden in the void *context pointer attached to the object.

So let's keep it simple and without too much magic.

I added a PR #4551 that implements UA_Server_getMethodNodeCallback.
In order to make the MethodDeclaration stuff happen, please use that to get the callback pointer at the time when the method is generated.

[mlgiraud]

I still don't understand how the custom method is going to get the node from the methodDeclarationID as specified in the nodeset. Currently, this is not passed into the generated code. And since the method is constructed manually by the generated code, and not via the inheritance code you linked, it is impossible to know the referenced node (method nodes do not have a hasTypeDefinition reference), unless the interface is changed to inject the methodDeclarationId. Also, your proposed solution still carries the same potential for a bug with the context pointer, since now the method callback is simply copied on construction, instead of used during the call.

Another possibility could be to expose a new api function to get the parent method node declared in methodDeclarationId. That way the user can get the corresponding node either during construction in the constructor callback or at a later time and set the callback manually. I would however prefer an internal solution.

My use case originally was like this: I want to set the methodcallback for the file open method for example and then use that callback for all types that inherit from the file type. However, since the nodeset specifies the open function manually for each of these functions, this is not possible. Also, the code you linked is never called as far as i can tell, because if the nodes were not added manually beforehand (my use case for this pr) then the method node in question will simply be added as a reference, instead of being copied, resulting in a behavior that i would expect to begin with.

EDIT: I believe i got what you mean now. I'll take a look at it and push it if it works. However, the problem with the context pointer still exists this way.

[mlgiraud]

Your proposed approach would only work, when the method callback is set before the node is constructed, but after the type was constructed. The only way to do this would be inside a constructor of the corresponding method. However, it is not possible to set the nodetypelifecycle on MethodNodes, since they do not have type definitions. I would have to set the lifecycle for the TrustListType but that is currently impossible, since the constructors are only called on objects and variables afaik.

So the following will not work:

namespace_gds_generated(server);
setupMethodCallbacks();

and neither will this:

setupMethodCallbacks();
namespace_gds_generated(server);

And registering a constructor for the type is also not possible. Hence, i still propose to do a runtime lookup. That also has the nice benefit of not having to jump through a lot of hoops to simply get some callback to work on all subtypes.

[jpfr]

To quote the Zen of Python: Explicit is better than implicit.

I have an idea:
We modify the NodeSet compiler to generate both

• namespace_gds_generated_begin(server);
• namespace_gds_generated_finish(server);

The old namespace_gds_generated(server) remains and just calls both in sequence.
That is only a little change because the internal structure of the generated code is already exactly this.

When this is exposed, you can do a

• namespace_gds_generated_begin(server);
• setupMethodCallbacks();
• namespace_gds_generated_finish(server);

The _finish path can then be generated in a way that UA_Server_getMethodCallback is used to pick up the callback via the MethodDeclarationId.

[mlgiraud]

Ok i managed to get the desired behavior without the use of the methodDeclarationId. The callback is now copied with the node when performing a copyAllChildrin. When an existing node is found for the target to be copied, the callback is overwritten as long as it is null. If it was previously already set by the user, the callback will remain untouched.

You can merge if you are satisfied with the changes.

EDIT: The changes should be fully backwards compatible and not break any existing api, since the separate calls to begin and finish are wrapped in the existing namespace generated call.

[mlgiraud]

@jpfr Can we merge this? Or are there some changes you are still unhappy about?

[jpfr]

I had just added UA_Server_getMethodCallback.

it seems the same result can be achieved by calling that and forwarding the method pointer (if not null) to the addMethodNode_finish.
Is there a case that I overlooked and that absolutely requires the “Ex” version?

We are very conservative when it comes to increasing the surface of the public API. So it would be cool to just combine existing API for the same effect.

At any rate, these are very fine points that need good documentation. The new API is not documented. So I might be mis-interpreting stuff. Please add that.

[mlgiraud]

I am using the getMethodCallback function. But i have to know where to get the callback from, and the only way to get that callback is from the methodDeclarationId, which needs to be passed into the addMethodNode_finish function. This also has the benefit, that the parent node from which the callback is inherited will have to be explicitly specified.

[jpfr]

@mlgiraud What's the status for this?

[jpfr]

@mlgiraud What's the status for this?