Register requested consumers with opc ua subscriptions

[JonasH-munters]

This handles issue 2238.

Currently, when requesting an opc ua subscription any consumers in the request will never be called. This PR registers any consumers for the whole subscription with the subscription handle. It also adds support for registering consumers for individual tags (as can be added to the request already) and registers those with the subscription handle as well.

Ps. The link to the contributing guidelines is broken so I hope I'm not breaking any :D

[sruehl]

Thanks for your PR

Ps. The link to the contributing guidelines is broken so I hope I'm not breaking any :D

You likely forgot to praise Toddy so I will search the Attic for a proper punishment.
In the meantime maybe @splatch or @hutcheb can take a look at the PR.

[sruehl]

also not sure, but I think @chrisdutz did something with subscriptions but my memories are a bit foggy

[splatch]

Have to ask for a friend too. Did we have any changes in subscription handling logic? I do not recall a tag consumer from before.

[JonasH-munters]

I assumed that the intention was there to support a consumer per tag since PlcSubscriptionRequest defines, for each method adding a tag/tagAddress, a method for adding them together with a consumer as well. And since it was so easy for me to implement fully that strengthened that reasoning about the intention here.

If you know some reason to not allow implementing consumers per tag that would be unfortunate since I'd have to solve my use case in some other way, but I'd just have to adapt :D In that case I'm assuming that at least the consumer for the whole request is intended to be registered with the Subscription.

In any case, if any of the types of consumers that can be added to the request shouldn't be used in the subscription might I suggest logging a message or something to the developer so that they can see that they are trying to do something that won't have an effect? That'd have saved me time at least when I was troubleshooting why all the consumers I added were never called. (getting a bit ahead of things since this last paragraph is irrelevant if this PR is accepted as is).

[splatch]

Per tag subscriptions in case of OPC-UA are not very helpful because cyclic subscription receives all the tags each time, thus plc event you receive will always be complete. This is one of miss-conceptions when it comes to UA, that it will be based on value change, but actually it is not. In essence plc4x (subscriber) send a publish request and device answers with all values of subscribed tags.
Registration of subscribed tag consumers makes a ton of sense for typical device side push notifications (I believe possible with Beckhoff ADS), where tags can be sent independently of each other.

I am afraid of confusing users (I believe you may be one of confused ones), by supporting both modes. From code point of view I do not mind any of these changes, but from consistency point of view we should clarify expected behavior at PLC4X API level.

[JonasH-munters]

Thank you for the response and I definitely think I might be one of the confused ones so any clarifications that can be done on the PLC4X API level as a result of this PR will be great I think :D Just gotta figure out the best clarifications to do!

What you say makes sense for cyclic subscriptions so maybe removing the addCyclicTag and addCyclicTagAddress methods with a consumer parameter from the DefualtPlcSubscriptionRequest.Builder class would make sense? Or some other way of reducing user confusion compared to the current state where the consumers are just ignored. But I was under the impression that for the PlcSubscriptionType.CHANGE_OF_STATE ones that I'm using the plc event only contains the ones that actually had their values change. When I debug with a breakpoint in the OpcuaSubscriptionHandle class I can see that the DataChangeNotification object that I receive most of the time contains only 18 values of 74 tags I subscribed to in my test case. Is that not how OPC UA change of state subscriptions should work? If not, maybe it could be relevant to support anyway since it seems at least the server I'm using (Kepware) behaves that way?

[chrisdutz]

Currently traveling at community over code.. So limited brain capacity.

Initially subscriptions were only tag based. But it was super annoying and a performance nightmare.

I added support for global handlers for all tags. Possibly I didn't finish the opcua integration correctly.

[JonasH-munters]

Thank you for your response and no worries! Happy to continue this when you're done traveling and hope you have a good time at community over code.

That makes a lot of sense explaining why the code looks like it does right now. Do you think it makes more sense to keep allowing for both tag based consumers and a global consumer when subscribing like in this PR right now? Or should I adjust the implementation to only support a global consumer?

[chrisdutz]

I do think that both options make sense. I'm personally more a fan of the global consumer pattern, but I do understand if some people more like the other.

We just need to ensure both options are implemented by our drivers.

[JonasH-munters]

Great! Then it sounds like I leave the code as-is, implementing support for both options for opc ua, until the PR can be reviewed.

[splatch]

@JonasH-munters With above I believe we are good to go. Can you please squash and sign-off result commit so I can merge this with your email/name?

[JonasH-munters]

Great! Yes I can do that. Just to double check, I should squash the three commits into one and sign that using GPG? Sorry for the potentially stupid question, a bit new to contributing to open source.

[splatch]

No need for GPG, just sign-off with your real name.

I'd strongly encourage you to sign Contributor License Agreements described here: https://www.apache.org/licenses/contributor-agreements.html. It clarifies the legal / copyright / license side for you and others who would like to use library later on.

[JonasH-munters]

I have now squashed the commits into one, but I don't know if it's signed off. I have my real name set in my git config and on my github profile. Don't know if there's some more place it needs to be set?

[splatch]

If you use a command line just do git commit [--amend] -s to add sign off line. Most of IDE will have something in GUI to trigger same.

[JonasH-munters]

Ah, that feature was new to me, I've signed off the commit now. Thanks for explaining! That should be it for this PR to be merged, right?

[JonasH-munters]

Hello! Sorry to bother you again, but it seems like while the checks needed to merge this ran yesterday one of them just got stuck and timed out after 6h. Should this just be re-ran or is there something I need to do?

[chrisdutz]

Unfortunately that's quite usual. In the opcua test and the go build this happens in a few percent of the job runs. Given we do a lot of concurrent runs, I usually simply rerun failed jobs.

[JonasH-munters]

Great, thanks for the response and for rerunning! Look like everything should be ready to merge then :D

[JonasH-munters]

Hello! Excuse the ping again, I just wanted to double check if anything else is required of me since this seems to have been approved, but not merged yet. I'm absolutely fine with waiting, just wanted to be sure that's what I'm supposed to do so I'm not wasting time when I should be doing something :D

[chrisdutz]

Oh ...I hope you're not waiting for me to merge the PR .... I usually stay clear of the OPC-UA driver ... so @splatch @hutcheb ?

[JonasH-munters]

Thank you for the response! I was not waiting for anyone in particular, just wanted to clarify so that you guys weren't waiting for me and I didn't know about it :D

[splatch]

Merged manually in 34a9db2.

[JonasH-munters]

Thanks a lot!

[splatch]

Thank you for contributing to the project. :-)