convertersystems / opc-ua-client

Visualize and control your enterprise using OPC Unified Architecture (OPC UA) and Visual Studio.
MIT License
397 stars 115 forks source link

Writing back a property when updating (in ViewModel) fails #194

Closed ismdiego closed 3 years ago

ismdiego commented 3 years ago

If you subscribe for changes in one of the ViewModel properties, and then inmediately change it (for instance, reacting to a new value in server and writing back a new one), the write fails because the ViewModel is still in "isPublishing" state.

This is related to this piece of code in SubscriptionBase:

private async void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
{
    if (this.isPublishing || string.IsNullOrEmpty(e.PropertyName))
    {
        return;
    }
...

I added a temporary ConcurrentQueue to add the Properties that are changed in this case, and then I can set them after the main OnPublishResponse method (differentiating them from the ones set in the method with a temporal List which is populated just before the item.Publish which in turn raises OnPropertyChanged).

The solution is half-baked but it works. I did not prepare a Pull request because I feel this is more or less a hack and would like to hear your opinion first. Maybe they are better ways to solve this scenario.

quinmars commented 3 years ago

Good observation! I had similar problems with pure MVVM applications (without UaClient). The main difficulty is that the property change event does not tell you who has changed the property. And there is no way to know that, because the property itself does not know by whom it was changed. For set value I'd personally wouldn't mind to write them back even if the value is the value just coming from the OPC UA server. But there is no kind of "binding mode", so we would also send back all actual values, that aren't intented to be written at all. And that would introduce many subtile bugs, even if we retrofit a "binding mode". Hence, the option I can see, is, as you proposed, to collect all changed values during the publishing phase. There is one cornor case that is not covered by your approach. A property could coerce the value, for example like that:

        [MonitoredItem(nodeId: "....")]
        public double NonNegativeValue
        {
            get => _nonNegativeNumber;
            set => this.SetProperty(ref _nonNegativeNumber, Math.Min(value, 0.0));
        }

In the worst case the value is 0.0 and the OPC server sends the value -1, than there wouldn't even be an change event. I have no idea how to handle that. Let's see what Andrew thinks about that.

awcullen commented 3 years ago

Good troubleshooting! I can think of two ways to write back a value immediately.

Option 1

        /// <summary>
        /// Gets the value of ScalarInt32.
        /// </summary>
        [MonitoredItem(nodeId: "ns=2;s=Demo.Static.Scalar.Int32")]
        public int ScalarInt32
        {
            get { return this._scalarInt32; }
            private set 
            { 
                this.SetProperty(ref _scalarInt32, value);

                if (value != 0)
                {
                    Dispatcher.CurrentDispatcher.InvokeAsync(() => { this.ScalarInt32 = 0; });
                }
            }
        }

        private int _scalarInt32;

option 2

        /// <summary>
        /// Gets the value of ScalarInt32.
        /// </summary>
        [MonitoredItem(nodeId: "ns=2;s=Demo.Static.Scalar.Int32")]
        public int ScalarInt32
        {
            get { return this._scalarInt32; }
            private set 
            { 
                this.SetProperty(ref _scalarInt32, value);

                if (value != 0)
                {
                    Task.Run(async () =>
                    {
                        try
                        {
                            await this.InnerChannel.WriteAsync(new WriteRequest
                            {
                                NodesToWrite = new[]
                                {
                                    new WriteValue
                                    {
                                        NodeId = NodeId.Parse("ns=2;s=Demo.Static.Scalar.Int32"),
                                        AttributeId = AttributeIds.Value,
                                        Value = new DataValue((int)0)
                                    }
                                }
                            });
                        }
                        catch (Exception ex)
                        {
                            Debug.WriteLine("Error writing value : {0}", ex.Message);
                        }
                    });
                }
            }
        }

        private int _scalarInt32;
ismdiego commented 3 years ago

Sorry for writing back too late, in fact you have just closed the issue. The problem is not related to write it back immediately, but not losing write values.

My solution consists on modifying OnPublishResponse and OnPropertyChanged methods in SubscriptionBase class.

When OnPublishResponse starts, it already sets this.isPublishing to true, and then the property being published is saved in an internal temporary list with the changed item and value (tuple of MonitoringItemBase and DataValue). This of course fires the OnPropertyChanged and so the modified property is compared with the ones already in that temporary list. If is contained, then the method returns. If not (change coming from outside) then it saves it in a ConcurrentQueue which is evaluated when OnPublishResponse ends.

I feel that this is more or less a hack, and would like to improve it with publishing the "isPublishing" state to the outside, to be able to implement a SubscriptionBase wrapper, but I did not find a clean way to do it.

If you want I can make a pull request for you to view the changes in context, and improve them. I did not prepare it already because I am not really confident with my implementation.

ismdiego commented 3 years ago

Hi again, I have prepared a patch just in case you want to review my "fix" and/or improve it to be able to include in your repository. Applied over latest 3.1.0 version.

0001-Delay-writing-OPCUA-MonitoredItemBase-while-Publishi.zip

This is currently working, but you probably find a better approach to solve this, like I commented back on 5 Apr.

NOTE: Excuse, I currently don't know how to use the pull request feature (git, in general) because I also created a pull request some time ago and don't know how to create a new one.