Update the driver after real world experience - #1253
Conversation
The changes in this commit is the result of observations made within real world applications. It substantially addresses bugs and performance issues. Unfortunately the change set got quite big because many changes have been made in parallel or caused each other. The two biggest changes are: * The async API has already been introduced with MORYX 10 and got implemented properly * NodeBrowsing got abandoned: It took too much time to read all nodes on the OPC UA server (depending on servers ~40s to serveral minutes). While most of the time, only a small subset is used in an application, there was no reason to browse them all (and keep them in memory) compared to lazy load them when needed.
dbeuchler
left a comment
dbeuchler
left a comment
There was a problem hiding this comment.
I know it's draft... but I have scrolled through the changes and found some breaking changes for dev. Just keep in mind.
|
|
||
| /// <inheritdoc /> | ||
| public OpcUaNode(IOpcUaDriver driver, IModuleLogger logger, string identifier) | ||
| public OpcUaNode(IOpcUaDriver driver, IModuleLogger logger, string identifier, NodeClass nodeClass) |
There was a problem hiding this comment.
Should be easy to make minor conform, just add a commit after this PR is ready which can then be easily reverted on future 🙂
| Task AddSubscriptionAsync(OpcUaNode node, CancellationToken cancellationToken = default); | ||
| Task AddSubscriptionAsync(string nodeId, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
[...] and I also want to gather opinions on some of the decisions being made so far (there is a breaking change for example which I considered fine as the driver should be beta).
There was a problem hiding this comment.
there is a breaking change for example which I considered fine as the driver should be beta
What is beta? The driver was released several times and we should not break the API
| /// <param name="nodeId"></param> | ||
| /// <param name="namespaceTable"></param> | ||
| /// <param name="session"></param> | ||
| /// <param name="cancellationToken"></param> | ||
| /// <returns></returns> |
1nf0rmagician
left a comment
1nf0rmagician
left a comment
There was a problem hiding this comment.
All test I saw removed concerned the browse feature, or have I overlooked anything that I should have paid closer attention to?
| public static Category ParentCategory(this Resource resource) | ||
| { | ||
| var parentType = resource.Parent?.GetType(); | ||
| if (parentType == null) |
There was a problem hiding this comment.
Looks like a fix related to #1239, did it slip into this PR?
| @@ -62,7 +62,7 @@ public class OpcUaNode : IMessageChannel | |||
| /// <summary> | |||
| /// List of all Subnodes. This property is null, when the node is no object node | |||
|
|
||
| /// <inheritdoc /> | ||
| public OpcUaNode(IOpcUaDriver driver, IModuleLogger logger, string identifier) | ||
| public OpcUaNode(IOpcUaDriver driver, IModuleLogger logger, string identifier, NodeClass nodeClass) |
There was a problem hiding this comment.
Should be easy to make minor conform, just add a commit after this PR is ready which can then be easily reverted on future 🙂
| Context.ReadDeviceSet(); | ||
| return Task.CompletedTask; | ||
| await base.OnEnterAsync(cancellationToken); | ||
| Context.RemoveSubscription(); |
There was a problem hiding this comment.
Haven't we done this on the disconnect?
There was a problem hiding this comment.
Will investigate and test that
| } | ||
|
|
||
| internal override void AddSubscription(OpcUaNode node) | ||
| internal override OpcUaNode GetNode(string identifier) |
There was a problem hiding this comment.
Any reason you didn't make this async as well?
There was a problem hiding this comment.
Now that I browsed through some of the changes, I think, back then I decided to not update it because the name didn't fit the *Async convention (which is the same for AddSubscription). I'm open to change this, though.
| <ItemGroup> | ||
| <ProjectReference Include="..\..\Moryx.Drivers.OpcUa\Moryx.Drivers.OpcUa.csproj" /> | ||
| </ItemGroup> | ||
| <ItemGroup> |
Yes, they were mainly related to that. |
3 participants
The changes in this commit is the result of observations made within real world applications. It substantially addresses bugs and performance issues. Unfortunately the change set got quite big because many changes have been made in parallel or caused each other.
The two biggest changes are: