Included ModelChanged Eventhandling. Fixed update of nodes after reconnection. #368

mdornaus · 2023-03-07T07:34:43Z

Added ModelChanged Eventhandling. Refactored Model Changed Eventhandling. Solved Reconnection Issue.

CLAassistant · 2023-03-07T07:34:49Z

All committers have signed the CLA.

ccvca

There are may additional LOG(INFO), are some of them outdated, or do all provide useful information?

DashboardClient/DashboardClient.cpp

DashboardClient/IDashboardDataClient.hpp

DashboardClient/DashboardClient.cpp

DashboardOpcUaClient.hpp

DashboardClient/IDashboardDataClient.hpp

Kantiran91

@ccvca I´m finish with the review

Kantiran91 · 2023-03-15T15:45:54Z

DashboardClient/DashboardClient.cpp

+					}
+				}
+			}
+			//Hotfix clear subscription cache.


Is this currently a hotfix? What would be a better solution?

Kantiran91 · 2023-03-15T16:00:22Z

DashboardClient/DashboardClient.cpp

-					if(value && value.get()->getNodeId() == pNode.get()->NodeId)
-					return;
+					if(value && value.get()->getNodeId() == pNode.get()->NodeId) {
+						//LOG(INFO) << "Allready Subscribed" << value.get()->getNodeId();


can you remove the comment?

I'll Remove the comment.

MachineObserver/DashboardMachineObserver.cpp

OpcUaClient/OpcUaInterface.hpp

ccvca

@GoetzGoerisch The linter check seems broken?

ccvca · 2023-03-16T08:59:40Z

DashboardClient/DashboardClient.cpp

+				}
+			}
+			//Hotfix clear subscription cache.
+			m_subscribedValues.clear();


Is this intended to be outside the If's?

ccvca · 2023-03-16T09:00:12Z

DashboardClient/DashboardClient.cpp

+					if(placeHolderNode == nullptr) { continue;}
+					std::list<ModelOpcUa::PlaceholderElement> instances = placeHolderNode->getInstances();
+					for(auto& el : instances) {
+						deleteAndUnsubscribeNode(el);


std::foreach

ccvca · 2023-03-16T09:01:15Z

DashboardClient/DashboardClient.cpp

+					if(placeHolderNode != nullptr) {
+						std::list<ModelOpcUa::PlaceholderElement> instances = placeHolderNode->getInstances();
+						for(std::list<ModelOpcUa::PlaceholderElement>::iterator it = instances.begin(); it != instances.end(); it++) {
+							deleteAndUnsubscribeNode(*it);


std::foreach

ccvca · 2023-03-16T09:03:07Z

DashboardClient/DashboardClient.cpp

+						this->updateAddDataSet(sce.refreshNode);
+						this->Publish();
+    				});
+					t.detach();


How is ensured, that the DashboardClient instance is available until this thread is completed. (e.g. when started before a shutdown of the application)

ccvca · 2023-03-16T09:04:12Z

DashboardClient/DashboardClient.cpp

+						}
+						this -> Publish();
+    				});
+					t.detach();


Object lifetime issues? (this)

MachineObserver/DashboardMachineObserver.hpp

OpcUaClient/OpcUaClient.cpp

OpcUaClient/OpcUaInterface.hpp

ccvca

Please run a memory check, There are propably more memory leaks.

ccvca · 2023-04-26T11:09:33Z

DashboardClient/DashboardClient.cpp

+							}
+							this -> Publish();
+    					}
+					}


Indentation looks suscious.
A continue inside should allow to preoces more than 10 Events per seconds (in case of busy OPC UA Servers)

Replaced by Threadsafe queue!

ccvca · 2023-04-26T11:11:45Z

DashboardClient/DashboardClient.cpp

+
+		void DashboardClient::updateDeleteDataSet(ModelOpcUa::NodeId_t refreshNodeId) {
+			auto search = browsedSimpleNodes.find(refreshNodeId);
+			if( search != browsedSimpleNodes.end()) {


Invert + Return

ccvca · 2023-04-26T11:11:53Z

DashboardClient/DashboardClient.cpp

+			if( search != browsedSimpleNodes.end()) {
+				std::shared_ptr<const ModelOpcUa::SimpleNode> simpleNode = search->second;
+				auto childNodes = simpleNode->ChildNodes;
+				if(!childNodes.empty()) {


Invert + Return

ccvca · 2023-04-26T11:17:03Z

DashboardClient/DashboardClient.cpp

+		}
+		void DashboardClient::deleteAndUnsubscribeNode(const ModelOpcUa::SimpleNode simpleNode) {
+			const std::list<std::shared_ptr<const ModelOpcUa::Node>> children = simpleNode.ChildNodes;
+			for(auto node : children) {


Looks identical to the method above? Use a single function for this?

ccvca · 2023-04-26T11:19:24Z

DashboardClient/DashboardClient.cpp

+		}
+		void DashboardClient::subscribeEvents() {
+			auto ecbf = [this](IDashboardDataClient::StructureChangeEvent sce) {
+				this->m_eventqueue.push(sce);


std::queue is not thread safe, mutex required.

ccvca · 2023-04-26T11:43:52Z

OpcUaClient/OpcUaInterface.hpp

+		}
+
+		contentFilter.elementsSize = 1;
+		contentFilter.elements = (UA_ContentFilterElement* ) UA_Array_new(contentFilter.elementsSize, &UA_TYPES[UA_TYPES_CONTENTFILTERELEMENT]);


UA_ContentFilterElement_new?

ccvca · 2023-04-26T11:47:14Z

OpcUaClient/OpcUaInterface.hpp

+		if(contentFilterElement.filterOperands) {
+			contentFilterElement.filterOperands[0] = extensionObject;
+		} else {
+			UA_ContentFilterElement_delete(&contentFilterElement);


Deleting a stack variable?

ccvca · 2023-04-26T11:50:23Z

OpcUaClient/OpcUaInterface.hpp

+		item.requestedParameters.filter.content.decoded.type = &UA_TYPES[UA_TYPES_EVENTFILTER];
+
+		UA_MonitoredItemCreateResult result = UA_Client_MonitoredItems_createEvent(client, subId, UA_TIMESTAMPSTORETURN_BOTH, item, this, handler_events, NULL);
+


Missing cleanup.
See also: https://github.com/open62541/open62541/blob/78e42517be1c5cf823370e45866865398cd682d6/examples/events/client_eventfilter.c#L454-L457

ccvca · 2023-04-26T11:51:55Z

OpcUaClient/OpcUaInterface.hpp

+		}
+		UA_DeleteSubscriptionsRequest_clear(&deleteRequest);
+		UA_DeleteSubscriptionsResponse_clear(&response);
+		UA_Array_delete(pSubscriptionId, 1, &UA_TYPES[UA_TYPES_UINT32]);


Is this already deleted by the previous UA_DeleteSubscriptionsRequest_clear ?

ccvca · 2023-04-26T11:52:38Z

OpcUaClient/OpcUaInterface.hpp

+	Dashboard::IDashboardDataClient::eventCallbackFunction_t eventcallback) {
+		this->eventcallback = eventcallback;
+		UA_CreateSubscriptionRequest request = UA_CreateSubscriptionRequest_default();
+		UA_CreateSubscriptionResponse response = UA_Client_Subscriptions_create(client, request, NULL, NULL, NULL);


Missing _clear

wlkrm · 2023-04-25T15:44:53Z

DashboardClient/DashboardClient.cpp

+		}
+
+		void DashboardClient::startEventThread() {
+			if (m_eventThreadRunning)


m_eventThreadRunning is

a missleading name, more like m_keepEventThreadRunning

is m_eventThreadRunning synchronized and wrapped by a mutex?

wlkrm · 2023-04-25T15:45:39Z

DashboardClient/DashboardClient.cpp

+			m_eventThread = std::thread(func);
+		}
+		void DashboardClient::stopEventThread() {
+			m_eventThreadRunning = false;


m_eventThreadRunning is not protected. Might lead to memory corruption

Replaced by atomic bool.

wlkrm · 2023-04-25T15:46:46Z

DashboardClient/IDashboardDataClient.hpp

@@ -25,7 +25,22 @@ namespace Umati
        class IDashboardDataClient
        {
        public:
+        /// Struct for handling Events
+            struct StructureChangeEvent{


Can there be more than one true, otherwise use ENUM?

wlkrm · 2023-04-25T15:50:46Z

OpcUaClient/OpcUaInterface.hpp

-};
-}  // namespace OpcUa
-}  // namespace Umati
+	void SubscriptionUnsubscribe(UA_Client *client, std::vector<int32_t> monItemIds, std::vector<int32_t> clientHandles){


Consider moving some parts out to reusable functions?

wlkrm · 2023-04-26T13:45:09Z

DashboardClient/DashboardClient.cpp

+			this->startEventThread();
+		}
+
+		DashboardClient::~DashboardClient() {


Best practice is following the rule of tree.

https://en.cppreference.com/w/cpp/language/rule_of_three

wlkrm · 2023-04-26T13:45:42Z

DashboardClient/DashboardClient.cpp

+
+			auto func = [this]() {
+				int cnt = 0;
+				while (this->m_eventThreadRunning) {


Not protected by mutex

Replaced by atomic.

wlkrm · 2023-04-26T13:46:32Z

DashboardClient/DashboardClient.hpp

 			std::recursive_mutex m_dataSetMutex;
 			std::list<std::shared_ptr<DataSetStorage_t>> m_dataSets;
 			std::map<std::string, LastMessage_t> m_latestMessages;

+			bool m_eventThreadRunning;
+			std::thread m_eventThread;
+			std::queue<IDashboardDataClient::StructureChangeEvent> m_eventqueue;


std::queue is not threadsafe! Add mutex/lockfree queue or sth

Replaced by a threadsafe queue.

wlkrm · 2023-04-26T13:52:27Z

DashboardClient/DashboardClient.cpp

-					if(value && value.get()->getNodeId() == pNode.get()->NodeId)
-					return;
+					if(value && value.get()->getNodeId() == pNode.get()->NodeId) {
+						//LOG(INFO) << "Allready Subscribed" << value.get()->getNodeId();


wlkrm · 2023-04-26T13:56:16Z

DashboardClient/DashboardClient.cpp

+				return;
+			}
+			auto child = childNodes.front();
+			std::shared_ptr<const ModelOpcUa::PlaceholderNode> placeholderNode = std::dynamic_pointer_cast<const ModelOpcUa::PlaceholderNode>(child);


Why not make data model non-const?

This involves changeing some classes in the model and i the codebase. I want to do this, but maybe it is better to do this in a separate task.

wlkrm · 2023-04-28T12:26:44Z

LGTM, after the remaining comments are resolved.

GoetzGoerisch · 2023-05-23T15:19:24Z

@mdornaus when could you resolve the remaining comments and rebase?

…ocation.

… to create open62541cpp::UA_NodeId.

…or for Node.

DasbachVDMA · 2024-04-04T07:54:11Z

Don't merge, moce to new gateway.

mdornaus changed the title ~~Flatglas2~~ Included ModelChanged Eventhandling. Fixed update of nodes after reconnection. Mar 7, 2023

This was referenced Mar 7, 2023

Integrate ModelChangeEvent Handling in Dashboard-OPC UA Client #201

Closed

After restart of the server new dynamic nodes not shown #281

Closed

ccvca requested changes Mar 7, 2023

View reviewed changes

This was linked to issues Mar 8, 2023

Integrate ModelChangeEvent Handling in Dashboard-OPC UA Client #201

Closed

After restart of the server new dynamic nodes not shown #281

Closed

ccvca reviewed Mar 14, 2023

View reviewed changes

DashboardClient/IDashboardDataClient.hpp Outdated Show resolved Hide resolved

ccvca reviewed Mar 14, 2023

View reviewed changes

DashboardClient/IDashboardDataClient.hpp Outdated Show resolved Hide resolved

mdornaus force-pushed the flatglas2 branch from 70a3bb7 to 99e4f39 Compare March 15, 2023 12:44

Kantiran91 approved these changes Mar 15, 2023

View reviewed changes

ccvca reviewed Mar 16, 2023

View reviewed changes

Kantiran91 assigned mdornaus Mar 17, 2023

Kantiran91 added this to the HMI 2023 milestone Mar 17, 2023

ccvca reviewed Apr 6, 2023

View reviewed changes

OpcUaClient/OpcUaInterface.hpp Outdated Show resolved Hide resolved

ccvca reviewed Apr 6, 2023

View reviewed changes

OpcUaClient/OpcUaInterface.hpp Outdated Show resolved Hide resolved

ccvca requested changes Apr 26, 2023

View reviewed changes

wlkrm requested changes Apr 26, 2023

View reviewed changes

wlkrm reviewed Apr 26, 2023

View reviewed changes

mdornaus enabled auto-merge November 20, 2023 15:10

mdornaus disabled auto-merge November 20, 2023 15:10

mdornaus added 7 commits November 20, 2023 17:07

Initial commit.

370df35

Decoupled eventhandling from event processing due to threading issues.

16ada09

Changed deletion of Jobs.

3545052

Removed some debug logging. Increased Queue timing (10ms) lookup.

68c9d0f

Added Logging.

d52d8b2

Increased Timing for update loop.

5fe6d56

Added mutex on Adding Dataset.

a77b113

mdornaus added 15 commits November 20, 2023 17:08

Changed back timings removed lock guard.

84747cd

Improved Node Cache update on remove. Hotfix cleared Subscriptioncache.

397d86e

Refactored ModelChangedEventhandling. Preparation for Dev merge.

6e05866

Restored configuration.json.example file.

4d13a60

Removed unused Pointer.

51f8c90

Refactored Comments.

46cd1b0

Changed pseudo constant initialization of static array to dynamic all…

2e3217a

…ocation.

Changed back the updateInterval.

1ac7267

Changed numerical Id of UA_NS0ID_Server with constant.

713e2e9

Several minor refactorings in OpcUaInterface.hpp.

ed6b945

Corrected Indentation.

112ccf8

Removed unused locale in handler_events. Used Conversions Constructor…

0d92ec5

… to create open62541cpp::UA_NodeId.

Removed unused structure and queue for MachineObserver. Corrected err…

af8d443

…or for Node.

Inlined methods for EventSubscription to allow multiple includes.

cc7822e

Fixed Threading issue by implementing Eventqueue.

c4518b5

mdornaus force-pushed the flatglas2 branch from ff30b2a to c4518b5 Compare November 20, 2023 16:14

DasbachVDMA closed this Apr 4, 2024

		item.requestedParameters.filter.content.decoded.type = &UA_TYPES[UA_TYPES_EVENTFILTER];

		UA_MonitoredItemCreateResult result = UA_Client_MonitoredItems_createEvent(client, subId, UA_TIMESTAMPSTORETURN_BOTH, item, this, handler_events, NULL);

Included ModelChanged Eventhandling. Fixed update of nodes after reconnection. #368

Included ModelChanged Eventhandling. Fixed update of nodes after reconnection. #368

Conversation

mdornaus commented Mar 7, 2023

CLAassistant commented Mar 7, 2023 • edited by cla-assistant bot Loading

ccvca left a comment

Choose a reason for hiding this comment

Kantiran91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccvca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccvca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlkrm commented Apr 28, 2023

GoetzGoerisch commented May 23, 2023

DasbachVDMA commented Apr 4, 2024

CLAassistant commented Mar 7, 2023 •

edited by cla-assistant bot

Loading