Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access to node reference in application too difficult #59

Closed
huebl opened this issue Dec 16, 2018 · 9 comments

Comments

@huebl
Copy link
Contributor

commented Dec 16, 2018

Determining a node reference in an application is too cumbersome. Most of the code should be hidden from the user.

@huebl huebl added the bug label Dec 16, 2018

@huebl huebl added this to todo in Release3-Bugs Dec 16, 2018

@huebl huebl moved this from todo to in progress in Release3-Bugs Dec 16, 2018

@huebl huebl self-assigned this Dec 16, 2018

@huebl huebl moved this from in progress to todo in Release3-Bugs Dec 16, 2018

@huebl huebl moved this from todo to in progress in Release3-Bugs Dec 16, 2018

@huebl huebl moved this from in progress to todo in Release3-Bugs Dec 16, 2018

@huebl huebl removed this from todo in Release3-Bugs Dec 17, 2018

@flipback

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

Currently I write into the OPC UA variable node in this way:

ServiceTransactionGetNodeReference::SPtr trx = constructSPtr<ServiceTransactionGetNodeReference>();
GetNodeReferenceRequest::SPtr req = trx->request();
GetNodeReferenceResponse::SPtr res = trx->response();

OpcUaNodeIdArray::SPtr nodeIds = constructSPtr<OpcUaNodeIdArray>();
nodeIds->resize(1);
OpcUaNodeId::SPtr greetingStringNodeId = constructSPtr<OpcUaNodeId>();
greetingStringNodeId->set(222, 1);

nodeIds->push_back(greetingStringNodeId);
req->nodes(nodeIds);

this->service().sendSync(trx);
if (trx->statusCode() != Success) {
	Log(Error, "response error");
	return false;
}

NodeReference::SPtr greetingStringRef;
trx->response()->nodeReferenceArray()->get(0, greetingStringRef);
if (greetingStringRef->statusCode() != Success) {
	Log(Error, "reference error");
	return false;
}

auto greetingStringApplicationRef = boost::static_pointer_cast<NodeReferenceApplication>(greetingStringRef);
BaseNodeClass::WPtr greetingStringNode = greetingStringApplicationRef->baseNodeClass();

VariableNodeClass::SPtr greetingStringVar = boost::static_pointer_cast<VariableNodeClass>(greetingStringNode.lock());
if (!greetingStringVar) {
	Log(Error, "pointer error");
	return false;
}

Looking at the code above I can split it into some steps:

  1. Creation a transaction
  2. Preparing the request
  3. Checking the references
  4. Getting the pointer on the node

I find the transaction model is very powerful and we don't need to get rid of this part. It gives us more flexibility.

The second part (preparing the request) can be simplified a bit .My ideas:

  1. In this case we don't need use pointers for array of NodeIds. This is a local object-value and we aren't going to share it between other objects or use polymorphic behavior. Why don't we use just a std::vector.
std::vector<OpcUaNodeId> nodeIds = {OpcUaNodeId(222,1), ...};
req->nodes(nodeIds);
  1. Add transaction to get a single node.
GetNodeSingleReferenceRequest::SPtr req = trx->request();
req->node(OpcUaNodeId(222,1))

The next part (checking references) isn't necessary. I think it would be better to return the weak pointers at once and the statuses in the response. So let's just get the pointer

if (trx->response()->statuses()[0] == Success) {
  auto nodePtr = trx->nodes()[0].lock();
}

At the end we have the following code:

ServiceTransactionGetNodeReference::SPtr trx = constructSPtr<ServiceTransactionGetNodeReference>();
GetNodeReferenceRequest::SPtr req = trx->request();
GetNodeReferenceResponse::SPtr res = trx->response();

std::vector<OpcUaNodeId> nodeIds = {OpcUaNodeId(222,1), ...};
req->nodes(nodeIds);

this->service().sendSync(trx);
if (trx->statusCode() != Success) {
	Log(Error, "response error");
	return false;
}

if (trx->response()->statuses()[0] == Success) {
        auto nodePtr = trx->nodes()[0].lock();
}

Of course it is just my suggestions.

Also I think we should avoid to use OpcUaArray on the application level. I understand that it is created to decode\encode OPC UA data and uses the memory more efficient , but it is an internal class of the core of the stack. It'd be better if the users can use the standard containers how they are used to doing.

Maybe we need the new transactions for this goal and mark ServiceTransactionGetNodeReference as deprecated?

@huebl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

The above suggestion helps to make the source code easier and more understandable. That's why we should do it that way.

When using the Weak pointer, however, you have to pay attention to a few things. Two different scenarios must be supported.

  1. In the first case, the content of the information model is static during execution of the application. In this case, a smart pointer can be made directly after the request from the weak pointer. The description above shows exactly this case.

  2. In the second case, the content of the information model is dynamic during execution of the application. In this case, the smart pointer from the Weak pointer should not be created directly after receiving the response. In this case, the reference must be created and checked with every access. Otherwise you would not recognize a deleted variable. In this case, a simpler interface still needs to be designed for data access.

@flipback flipback added enhancement and removed bug labels Dec 18, 2018

@flipback

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

I see... we could use a mutex to prevent the concurrency between the application thread and the stack. But actually, we can't because to lock the mutex we need to get the node before:

auto nodePtr = trx->nodes()[0].lock();
boost::lock_guard<boost::mutex> nodePtr->mutex();

Between these two lines, the node can be already removed from the information model.
But if we aren't so strict about the concurrency and let the application use already removed nodes, what will happen seriously bad?

I have an application whose data can be removed dynamically, if they are removed and the app doesn't know it, then we have a design problem. If it has a notification about changes in the information model, the app could look after its references itself.

What do you think about these notifications, @huebl?

@huebl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2018

I think there are two approaches:

  1. In the current implementation, we can indirectly determine if a node has been deleted. Before accessing a variable, a smart pointer is generated in the application from the weakpointer. If the node was deleted in the intermediate time, the generated smart pointer is zero. So we only know when accessing the variable that the associated node has been deleted. This can be too late in different cases.

  2. There are currently quite a number of notifications that can be registered at the node. Here we could introduce the two new features.
    a) Notification when the node is deleted.
    b) Notification when a new node is created.
    In this case, the question arises whether the weak pointer is needed at all.

@flipback - What do you think about it?

@huebl huebl added this to In progress in Release-3.6.0 Dec 18, 2018

@flipback

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Hi, @huebl!

I don't see any contradictions in the both approaches.

I think we can get rid of NodeReference and NodeReferenceApplication but keep the weak pointers as a result of transactions. Then we will add the notifications. If users need more control, they can subscribe on these notification, if not, they use just the weak pointers.

Are you sure that you need it in version 3.6.0? We have done a lot of new features already. Let me fix #45 and we can make the next release before Christmas.

@huebl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

In version 3.6 we do not need this feature yet.

There are a number of other transactions that are likely to be overhauled as well. We can implement these features together in version 3.7.

@huebl huebl moved this from In progress to Done in Release-3.6.0 Dec 19, 2018

@huebl huebl removed this from Done in Release-3.6.0 Dec 19, 2018

@huebl huebl added this to To do in Release-3.7.0 Dec 19, 2018

@huebl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

I thought again about the topic. I do not want to change the current implementation. This makes the application backward compatible. But it should also a simple solution. For this reason, there is the new class GetNodeReference. For the hello world example that should be enough.

Example Source Code:

			GetNodeReference getNodeReference(OpcUaNodeId(222,1));
			if (!getNodeReference.query(applicationServiceIf_)) {
		  		Log(Error, "response error");
		  		return false;
			}

			if (getNodeReference.statuses()[0] != Success) {
		  		Log(Error, "node reference error");
		  		return false;
			}

			auto ptr = getNodeReference.nodeReferences()[0].lock();
			if (!ptr) {
		  		Log(Error, "node no longer exist");
		  		return false;
			}

			OpcUaDataValue dataValue(OpcUaString("Hello, world!"));
			ptr->setValueSync(dataValue);

The transaction can be reworked in a later release.

@huebl

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

The implementation of the class GetNodeReference is already finished. Therefore, the feature should already be integrated in Release 3.6.

@huebl huebl removed this from To do in Release-3.7.0 Dec 20, 2018

@huebl huebl added this to In progress in Release-3.6.0 Dec 20, 2018

@huebl huebl moved this from In progress to Done in Release-3.6.0 Dec 20, 2018

@flipback

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

Ok. I see. I'll update documentation.

@flipback flipback closed this Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.