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

Node monitor (extended subscription) #252

Merged
merged 14 commits into from
Aug 13, 2019
Merged

Conversation

kung-foo
Copy link
Member

This PR creates a new NodeMonitor struct for creating managed subscriptions. It attempts to hide implementation details from the user, and provide a simple, high-level interface for interacting with data change events.

see related discussion: #240

The example code shows the two different methods for a subscription: callback-based, and channel-based.

@kung-foo
Copy link
Member Author

There are some naming/structure issues I'd still like to resolve. Possibly merge NodeMonitor and Subscription into a single struct and move Subscribe(...) and ChanSubscribe(...) to package level functions (both returning the new, unified struct).

@magiconair
Copy link
Member

This looks like a good start. Maybe call it monitor.Subscription instead of monitor.New. We might have other monitors. I'll need something that reads a list of nodes on a regular basis and that would fit in this quite nicely (e.g. monitor.Nodes).

Do we have to support both channel and callback mechanisms? Wouldn't channel be enough?

Also, you can add nodes as strings and NodeIDs but when you set up the monitor you can use only strings. Don't have a good idea on how to solve that right now but might be something to keep in mind. With Channel and Callback you potentially get four different combinations.

You probably don't want to panic when the subscription id doesn't match ... :)

@kung-foo
Copy link
Member Author

Thanks for the feedback. Regarding chan v callback, my favorite APIs are the ones that usually provide both. From this somewhat hyperbolic article: https://www.jtolio.com/2016/03/go-channels-are-bad-and-you-should-feel-bad/

[...] your API can always be more general, always more flexible, and take drastically less resources if you use callbacks instead of channels.

@kung-foo
Copy link
Member Author

I should point out that the current callback implementation is actually less performant the the channel method because the callback code uses a channel behind the scenes. However, that could be removed with a little bit of work.

@@ -196,6 +198,29 @@ func (s *Subscription) sendNotification(ctx context.Context, data *PublishNotifi

}

// Stats returns a diagnostic struct with metadata about the current subscription
func (s *Subscription) Stats() (*ua.SubscriptionDiagnosticsDataType, error) {
// TODO(kung-foo): once browsing feature is merged, attempt to get direct access to the
Copy link
Member

Choose a reason for hiding this comment

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

does this refer to the more browsing branch or something else?

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. though I haven't tried it on this exact use case. So it is possible that I can't get access to the single diag node. Also, I noticed that with the FreeOPCUA server, no diag nodes are generated. Is there a standard way to indicate that that feature is not supported?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know.

@magiconair
Copy link
Member

@kung-foo are you using this code right now or are you planning on using it?

@kung-foo
Copy link
Member Author

kung-foo commented Aug 2, 2019

Yes, we are using it, though not in production.

@magiconair
Copy link
Member

How did you find the timer leak?

@kung-foo
Copy link
Member Author

kung-foo commented Aug 6, 2019

re the timer leak: manual inspection. I just happen to remember that "idiosyncrasy" of the timer API.

@magiconair
Copy link
Member

@kung-foo How far are we from merging this? I'd like to start using it.

@kung-foo
Copy link
Member Author

I think we are good to merge. We can re-visit features like reconnecting to a subscription in a future PR.

@kung-foo kung-foo changed the title WIP: node monitor (extended subscription) Node monitor (extended subscription) Aug 13, 2019
@magiconair
Copy link
Member

Then lets do it.

@magiconair magiconair merged commit d3a92a2 into gopcua:master Aug 13, 2019
@fdamador
Copy link

fdamador commented Mar 16, 2020

Thank you for creating the monitor code which i've been testing.

I have a need to pass the msg.value.value() values data back to my own custom struct array I have in the main() routine for a series of NodeIDs. Can I do this by interrogating the subscription server? Or do I have to send the NodeIds data (time, value, quality, etc) via channel?

@kung-foo kung-foo deleted the node-monitor branch March 16, 2020 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants