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

Fix calling stop monitored item callback. #132

Merged
merged 8 commits into from
Mar 10, 2019

Conversation

atimin
Copy link
Member

@atimin atimin commented Mar 8, 2019

If several monitored items are created for a node in the same subscription, a user application becomes no callbacks when all of them will be deleted.

@atimin
Copy link
Member Author

atimin commented Mar 8, 2019

@huebl , the PR is not ready to be merged. I'd like to write some unit tests before and discuss some questions. See my comments to the code.

@@ -582,8 +582,9 @@ namespace OpcUaStackServer
}

// check monitored item list
monitoredItemIds_.erase(it1);
if (newMonitoredItemIds.size() > 0) {
Copy link
Member Author

@atimin atimin Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure that this functional used to work correctly. But judging the history of file it could never work at all.

Copy link
Member Author

@atimin atimin Mar 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've understood. The problem is only for duplication monitored items inside of a single subscription. I tested my application with different clients.

Copy link
Member Author

@atimin atimin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L563-562: If there is a callback for starting monitored item but no for stopping. The start callback will be called only once. I'm not sure that it's correct.

For L517-518 is the same situation.

@atimin
Copy link
Member Author

atimin commented Mar 8, 2019

The algorithm seems too complicated. Why we need to know IDs of monitored items to count them? We could just increment and decrement a monitored item counter.

@atimin atimin requested a review from huebl March 8, 2019 17:20
@atimin atimin added bug Something isn't working wip Work In Pogress labels Mar 8, 2019
@atimin atimin changed the base branch from master to Release3_Bugs March 8, 2019 17:25
@@ -582,8 +582,9 @@ namespace OpcUaStackServer
}

// check monitored item list
monitoredItemIds_.erase(it1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, The ID is not deleted if only one ID exists in the list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if there are other IDs, `newMonitoredItemIds' is not empty and I insert the rest IDs in L587.

if (newMonitoredItemIds.size() > 0) {
monitoredItemIds_.insert(std::make_pair(baseNodeClass->nodeId().data(), monitoredItemIds));
monitoredItemIds_.insert(std::make_pair(baseNodeClass->nodeId().data(), newMonitoredItemIds));
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh , the same data will be inserted again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not the same. newMonitoredItemIds has the rest of ID's and I re-insert them deleting old ID's before. See L585.

@huebl
Copy link
Contributor

huebl commented Mar 9, 2019

L563-562: If there is a callback for starting monitored item but no for stopping. The start callback will be called only once. I'm not sure that it's correct.

For L517-518 is the same situation.

That is an error. The ID must always be removed from the list. If you take it exactly, the bookkeeping of Start and Stop would have to be independent of the callbacks. The following combinations are possible.

  1. None
  2. Start, Stop
  3. Start
  4. Stop

We should open a bug for this bug.

@huebl huebl closed this Mar 9, 2019
@huebl
Copy link
Contributor

huebl commented Mar 9, 2019

The algorithm seems too complicated. Why we need to know IDs of monitored items to count them? We could just increment and decrement a monitored item counter.

The IDs are needed for the unique identification of parameter sets (sampling interval, queue size, ... ). Several Monitored Items for the same Node ID can have different parameter sets. The parameter sets are transferred to the application with the ID (Not yet in the current implementation). In the Monitored Stop, the ID is used to identify the Monitored Item inside the application.

@huebl huebl reopened this Mar 9, 2019
@huebl
Copy link
Contributor

huebl commented Mar 9, 2019

I accidentally executed the pull. Sorry.

@atimin atimin changed the title [WIP] Fix calling stop monitored item callback. Fix calling stop monitored item callback. Mar 9, 2019
@atimin atimin removed the wip Work In Pogress label Mar 9, 2019
@huebl huebl merged commit 749ec94 into Release3_Bugs Mar 10, 2019
@atimin atimin deleted the fix_monitor_item_callbacks branch March 11, 2019 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants