locka99 / opcua

A client and server implementation of the OPC UA specification written in Rust
Mozilla Public License 2.0
496 stars 131 forks source link

Question re. Subscriptions #95

Closed morphace closed 3 years ago

morphace commented 3 years ago

Hi,

I'm creating subscriptions with a publishing interval of 1000 ms and a sampling interval of 1ms, along with a queue size of 1000. Revised values are the same.

Therefore I'd expect that my callback would receive 1000 values every second. No?

If I set the same params on another client (ProSys) and connect to the same server, that's exactly what's happening.

Is there an issue, or am I mssing something?

Thanks!

morphace commented 3 years ago

I see, it's either a logic issue or intended (I don't know). See code below.

I can see the data_change_notifications arriving correctly, but only the last value will be used and forwarded to the callback... is that the way it's supposed work? What if we are intertested in all the values?

I can try to make a suggestion, how to modify it, but I need to know, if the "last value only" approach is intended, or not.


pub(crate) fn on_data_change(&mut self, data_change_notifications: &[DataChangeNotification]) {
        let mut monitored_item_ids = HashSet::new();
        for n in data_change_notifications {
            if let Some(ref monitored_items) = n.monitored_items {
                for i in monitored_items {
                    let monitored_item_id = {
                        let monitored_item_id = self.monitored_item_id_from_handle(i.client_handle);
                        if monitored_item_id.is_none() {
                            continue;
                        }
                        *monitored_item_id.as_ref().unwrap()
                    };
                    let monitored_item = self.monitored_items.get_mut(&monitored_item_id).unwrap();
                    monitored_item.value = i.value.clone();
                    monitored_item_ids.insert(monitored_item_id);
                }
            }
        }

        if !monitored_item_ids.is_empty() {
            let data_change_items: Vec<&MonitoredItem> = monitored_item_ids
                .iter()
                .map(|id| self.monitored_items.get(&id).unwrap())
                .collect();

            // Call the call back with the changes we collected
            let mut cb = trace_lock_unwrap!(self.notification_callback);
            cb.on_data_change(data_change_items);
        }
    }`
locka99 commented 3 years ago

It might make more sense to call the callback's on_data_change per DataChangeNotification. instead of afterwards. That way if there are multiple notifications for the same monitored item you'll get them separately.

locka99 commented 3 years ago

I assume that's what's happening here for you, that you're getting a stack of notifications one after the other and it's only telling you about the last one.

locka99 commented 3 years ago

Upon reading the specification further I wonder if its possible for a bunch of notifications to be in the same DataNofication for the same client handle. I'll think about it a little more. I might have to make it so you receive data notifications with an array of data values per monitored item but I don't know if they should be sorted (expensive) or just supplied in the order received.

morphace commented 3 years ago

I believe that calling the callback as many times as there are notifications would be more logical as the fact that notifications have been queued by the server should not be noticed by the client. And I think that the server sends the notifications in order of its sampling anyway, which means that sorting should not be necessary... if you want me to, I'll suggest a code modification...

locka99 commented 3 years ago

I've checked in a change which I think will work but please check. What I've done is renamed value() to last_value() on the monitored items in the on_data_change callback. I've also added a values() function. The last_value() works like before and returns the last value. The values() returns a vector of all the values in the data change notification in the order received and is wiped after the callback. The code should also call the on_data_change callback for each DataChangeNotification it receives multiple calls rather than aggregating them into one.

locka99 commented 3 years ago

Please reopen if this doesn't work for you.

morphace commented 3 years ago

Great - just tested it - works.