Support Metadata frames in PubSub #1518

mrsuciu · 2021-09-17T13:02:36Z

Implemented changes:

Refactored WriterGroupPublishState out of IPubSubConnection file
JSON Discovery messages for DataSetMetaData
UADP Discovery messages for DataSetMetaData (Request + response)
Refactored the UaPublisher, extracted IntervalRunner and reused it in UdpDiscoveryPublisher & UdpDiscoverySubscriber
Proposal: automatic setting of ConfigurationVersion based on the rules in 6.2.2.1.5 (part 14)in new class Opc.Ua.PubSub.Configuration.ConfigurationVersionUtils
New events added to UaPubSubApplication

        /// <summary>
        /// Event that is triggered when the <see cref="UaPubSubApplication"/> receives and decodes subscribed DataSet MetaData
        /// </summary>
        public event EventHandler<SubscribedDataEventArgs> MetaDataReceived;

        /// <summary>
        /// Event that is triggered before the configuration is updated with a new MetaData 
        /// The configuration will not be updated if <see cref="ConfigurationUpdatingEventArgs.Cancel"/> flag is set on true.
        /// </summary>
        public event EventHandler<ConfigurationUpdatingEventArgs> ConfigurationUpdating;

Unit tests were updated to cover the new functionality

…a is decoded - first draft

Update sample apps to be able to start suing MQTT & UADP

… messages

…s in 6.2.2.1.5 (part 14)

Fix tests because they fail after creating also the dataset metadata messages together with dataset messages

…essage header is not correct

…dard into pubSubDev

…tandard into pubSubDev

…n publisherId value is provided as positive signed type

…iate between detected Metadata version changes

…n publisherId value is provided as positive signed type

…iate between detected Metadata version changes

…into pubSubDev

mregen · 2021-09-20T16:28:10Z

Please check new lgtm alerts and remove launchsettings.json from list of added files

…t value

…thod

lgtm-com · 2021-09-21T06:19:20Z

This pull request introduces 22 alerts and fixes 5 when merging 1d1d832 into 7d310d5 - view on LGTM.com

new alerts:

11 for Dereferenced variable may be null
8 for Useless assignment to local variable
1 for Invalid string formatting
1 for Futile conditional
1 for Poor error handling: empty catch block

fixed alerts:

4 for Dereferenced variable may be null
1 for Poor error handling: empty catch block

mregen · 2021-09-21T10:12:51Z

Applications/ConsoleReferencePublisher/Program.cs

+            writerGroup1.DataSetWriters.Add(dataSetWriter1);
+
+            // Define DataSetWriter 'AllTypes'
+            DataSetWriterDataType dataSetWriter2 = new DataSetWriterDataType();


There are a number of IEncodeable datatypes which are defined in Core as partial class. Imho they are all candidates for simplifying the use by adding partial class helpers in core.

mregen · 2021-09-21T10:13:17Z

Applications/ConsoleReferencePublisher/Program.cs

+            dataSetWriter2.DataSetName = "AllTypes";
+            dataSetWriter2.KeyFrameCount = 1;
+            uadpDataSetWriterMessage = new UadpDataSetWriterMessageDataType() {
+                ConfiguredSize = 32,


how is this calculated?

I agree to this complain, since one has to have the specification and has to calculate these values.

mregen · 2021-09-21T10:13:32Z

Applications/ConsoleReferencePublisher/Program.cs

+            dataSetWriter2.KeyFrameCount = 1;
+            uadpDataSetWriterMessage = new UadpDataSetWriterMessageDataType() {
+                ConfiguredSize = 32,
+                DataSetOffset = 47,


Yes, I totally agree.

mregen

I see a couple of architectural improvements that need to happen to make this implementation better usable across platforms.. but not necessarily need to happen for this PR

specify more interface abstractions where necessary, e.g. IJsonEncoder/IJsonDecoder to be able to decouple from the default implementation.
refactor some of the partial PubSub base types to have a complement in core to make them easier to use
refactor the core encoding/decoding NetworkMessage/DataSetMessage classes to be based on interface classes, not abstract classes.
Seperate implementation from application code by moving some classes in core, e.g. JsonNetworkMessage/UadpNetWorkmessage etc
there is a hack to use GlobalContext for namespaceUri
is the discoveryResponse part of the 1.04 spec or ist it ported from the starter kit code?

mregen · 2021-09-21T10:14:26Z

Applications/ConsoleReferencePublisher/Program.cs

            publishedDataSetSimple.DataSetMetaData.ConfigurationVersion = new ConfigurationVersionDataType() {
-                MinorVersion = 1,
-                MajorVersion = 1
+                MinorVersion = ConfigurationVersionUtils.CalculateVersionTime(kTimeOfConfiguration),


e.g. such a default init could be part of a partial class in core, extending ConfigurationVersionDataType

mregen · 2021-09-21T10:22:55Z

Applications/ConsoleReferenceSubscriber/Program.cs

+        // constant DateTime that represents the initial time when the metadata for the configuration was created
+        private static DateTime kTimeOfConfiguration = new DateTime(2021, 5, 1, 0, 0, 0, DateTimeKind.Utc);
+        private const string kDisplaySeparator = "------------------------------------------------";
+
        private static object m_lock = new object();

        public static void Main(string[] args)


general note to the sample code to improve for FUTURE: The construction of the PubSub configuration looks complicated and error prone, it would be nice to have 'fluent API' or helper extension functions to build the structure content. Its unclear to me how some of the offset are calculated, so if this could be done by such an API would make the job a lot easier.

Totally agree with your above observations and we are thinking of implementing such a "fluent API".

mregen · 2021-09-21T10:41:37Z

Libraries/Opc.Ua.PubSub/Encoding/JsonNetworkMessage.cs

@@ -179,10 +178,7 @@ public void SetNetworkMessageContentMask(JsonNetworkMessageContentMask networkMe
        /// <returns></returns>
        public override byte[] Encode()
        {
-            IServiceMessageContext messageContext = new ServiceMessageContext {
-                NamespaceUris = ServiceMessageContext.GlobalContext.NamespaceUris,


to directly use the GlobalContext here seems like a hack

Ok, we will have to refactor this method and pass the messageContext as a parameter.
We recall that you requested that an Encode method should exist without any parameters, is it ok in your opinion that we refactor it and add the IServiceMessageContext parameter?

yes, that would make sense.

mregen · 2021-09-21T11:01:04Z

half way through -- will continue review later

…e.Count()

lgtm-com · 2021-09-21T13:13:14Z

This pull request introduces 8 alerts and fixes 6 when merging 510c34c into 7d310d5 - view on LGTM.com

new alerts:

6 for Dereferenced variable may be null
2 for Useless assignment to local variable

fixed alerts:

5 for Dereferenced variable may be null
1 for Poor error handling: empty catch block

mregen

Hi @mrsuciu , looks good for this release since we are still in preview state. For the long run some architectural changes should be considered, mainly around refactories some dtaatypes and classes. Please see my previous comment.

cristipogacean · 2021-09-22T08:29:04Z

Applications/ConsoleReferenceSubscriber/Program.cs

@@ -206,9 +247,10 @@ private static void UaPubSubApplication_MetaDataDataReceived(object sender, Subs
        {
            lock (m_lock)
            {
+                Console.WriteLine("MetaDataDataReceived event:");
                if (e.NetworkMessage is JsonNetworkMessage)


Would it make sense to handle the UADP MetaData Message as well, like in the UaPubSubApplication_DataReceived()?

@cristipogacean Yes, treating the UADP Metadata Message similar to the handling in UaPubSubApplication_DataReceived() is a good idea.

cristipogacean · 2021-09-22T09:24:07Z

Libraries/Opc.Ua.PubSub/Encoding/JsonNetworkMessage.cs

-                DecodeSubscribedDataSets(decoder, dataSetReaders);
-            }
-        }
+            string json = System.Text.Encoding.ASCII.GetString(message);  


Is the ASCII encoding the right choice here? We might consider using UTF8 since the default encoding for JSON is Unicode.

cristipogacean · 2021-09-22T10:56:55Z

Libraries/Opc.Ua.PubSub/Encoding/UadpNetworkMessage.cs

-                    default:
-                        // Reserved - no type provided
-                        break;
+                    Utils.Trace(TraceMasks.Error, "NetworkMessageHeader cannot be encoded. PublisherId is null but it is expected to be encoded.");


Maybe we should handle this situation with an exception and not generate the inconsistent message at all. Or reset the UADPFlags to reflect the fact that the PublisherId is not part of the encoded message.

cristipogacean · 2021-09-22T11:18:14Z

Libraries/Opc.Ua.PubSub/Transport/MqttClientCreator.cs

@@ -101,7 +101,7 @@ internal class MqttClientCreator

            while (mqttClient.IsConnected == false)
            {
-                Connect(reconnectInterval, mqttClientOptions, mqttClient);
+                await Connect(reconnectInterval, mqttClientOptions, mqttClient);


can this lead to an infinite loop in case of a bad configuration of mqtt client?

@cristipogacean Yes it can, but the problem here is that the the mqttClient.ConnectAsync called in the Connect method does not set the correct/expected MqttClientConnectResultCode code in case a configuration error is present thus distinguishing between it and a non configuration error such as the server( MQTT broker) not being available becomes impossible. For ex: trying to connect to a broker using an unsupported MQTT protocol version simply throws an exception and does not return as one would expect MqttClientConnectResultCode.ProtocolError. The desired effect of this loop is to allow a connection to connect if the settings are correct and the broker becomes available even after the connection is initiated, of course the undesired effect is that it will loop infinitely if the broker is unreachable or the configuration contains an error.

…n for publisher/subscriber are initialized in the same process

…eContext as parameter

7.2.2.3.5 Data Key Frame DataSetMessage 7.2.2.3.6 Data Delta Frame DataSetMessage

lgtm-com · 2021-09-28T06:48:14Z

This pull request introduces 9 alerts and fixes 6 when merging 0d301d9 into 33ae8bb - view on LGTM.com

new alerts:

6 for Dereferenced variable may be null
3 for Useless assignment to local variable

fixed alerts:

5 for Dereferenced variable may be null
1 for Poor error handling: empty catch block

AlinMoldovean and others added 30 commits July 12, 2021 12:25

PubSub test fixes.

c4d441c

Automatically update the dataSetMetadata and raise event when metadat…

62429b7

…a is decoded - first draft

Refactor WriterGroupPublishState out of IPubSubConnection file

7f6cfa4

Add first version of MQTT & UADP rnacoding/Decoding for metadata

13b819c

Update sample apps to be able to start suing MQTT & UADP

partial implementation of udp handling of discovery messages

68b4d77

First version of: Refactor UdpConnection class for handling discovery…

6c251d0

… messages

some method renamings

01081d5

Proposal: automatic setting of ConfigurationVersion based on the rule…

139c2c8

…s in 6.2.2.1.5 (part 14)

remanes and refactor

ccff80e

Arrange code in WriteGroupPublishState

7249cbc

Fix tests because they fail after creating also the dataset metadata messages together with dataset messages

REwork Tests for : Handlinng of the DataSetClassId from JsonNetwork m…

a66bd3c

…essage header is not correct

Raise event with raw bytes from UDP connection & MQTT connection

70b6358

Merge branch 'master' of https://github.com/AlinMoldovean/UA-.NETStan…

34d0dcc

…dard into pubSubDev

Ignore not operational interfaces in GetUdpClients()

2a23b2e

Implement Traffic Reduction for Discovery Request & Response messages

998492d

Merge branch 'pubSubDev' of https://github.com/AlinMoldovean/UA-.NETS…

e474525

…tandard into pubSubDev

test commit

d5ae361

Added MetaDataVersion flag to the DataSetMessageContentMask

4afc720

Handle MetadataVersion during decode V1

8d927bd

V2 for handling MetaDataVersion contents during Decode

3d7ffd5

Small fix on decoding publisherId and added cast to unsigned type whe…

16cbcc2

…n publisherId value is provided as positive signed type

Enable DataSet decoding if MetadataMinorVersion changed and different…

bb9351c

…iate between detected Metadata version changes

Added necessary tests for detecting MetadataVersion changes

5a606d3

Implement specified MetaDataUpdateTime from 6.4.2.3.6

906aad8

Handle MetadataVersion during decode V1

88c25ee

V2 for handling MetaDataVersion contents during Decode

102da1a

Small fix on decoding publisherId and added cast to unsigned type whe…

43735b0

…n publisherId value is provided as positive signed type

Enable DataSet decoding if MetadataMinorVersion changed and different…

833603c

…iate between detected Metadata version changes

Added necessary tests for detecting MetadataVersion changes

de2ccdb

Merge branch 'pubSubDev' of https://github.com/mrsuciu/UA-.NETStandard …

64f1665

…into pubSubDev

mregen requested review from cristipogacean and randy-armstrong September 20, 2021 06:13

mrsuciu and others added 2 commits September 20, 2021 12:32

Merge branch 'master' into pubSubDev-PR

289a1f3

Fix for Unicast testing issue.

9652b8d

mregen linked an issue Sep 20, 2021 that may be closed by this pull request

Support MetaData frames in PubSub #1511

Closed

1 task

mrsuciu added 3 commits September 21, 2021 08:23

Removed launchSettings.json files

0918029

Increment DataSet sequence number independent of write's KeyFrameCoun…

158c33f

…t value

Removed unused deltaFrame parameter from the ReadPublishedDataItem me…

1d1d832

…thod

mregen reviewed Sep 21, 2021

View reviewed changes

mrsuciu added 2 commits September 21, 2021 15:56

Fixed Warning CA1829: Use the 'Length\' property instead of Enumerabl…

08ec4c5

…e.Count()

Fixed valid lgtm warnings

510c34c

mregen approved these changes Sep 22, 2021

View reviewed changes

cristipogacean reviewed Sep 22, 2021

View reviewed changes

cristipogacean approved these changes Sep 22, 2021

View reviewed changes

neaflorin and others added 6 commits September 28, 2021 09:20

Small correction on metadata callback

bd35732

Changed JsonNetworkMessage Encoding from Encoding.ASCII to Encoding.UTF8

a046f19

Small transport protocol correction

1856718

defect 2126; MQTTnet.Client seems not to work properly if a connectio…

798c64c

…n for publisher/subscriber are initialized in the same process

Refactor UaNetworkMessage.Encode method to receive the IServiceMessag…

16b9f82

…eContext as parameter

implement UADP shall create Data Key Frame DataSetMessages

0d301d9

7.2.2.3.5 Data Key Frame DataSetMessage 7.2.2.3.6 Data Delta Frame DataSetMessage

mrsuciu merged commit 75744e2 into OPCFoundation:master Sep 28, 2021

mrsuciu deleted the pubSubDev-PR branch September 28, 2021 08:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Metadata frames in PubSub #1518

Support Metadata frames in PubSub #1518

mrsuciu commented Sep 17, 2021

mregen commented Sep 20, 2021

lgtm-com bot commented Sep 21, 2021

mregen Sep 21, 2021

mregen Sep 21, 2021

mrsuciu Sep 22, 2021

mregen Sep 21, 2021

mrsuciu Sep 22, 2021

mregen left a comment

mregen Sep 21, 2021

mregen Sep 21, 2021

mrsuciu Sep 22, 2021

mregen Sep 21, 2021

mrsuciu Sep 23, 2021

mregen Sep 23, 2021

mregen commented Sep 21, 2021

lgtm-com bot commented Sep 21, 2021

mregen left a comment

cristipogacean Sep 22, 2021

mrsuciu Sep 22, 2021

cristipogacean Sep 22, 2021

cristipogacean Sep 22, 2021

cristipogacean Sep 22, 2021

mrsuciu Sep 23, 2021

lgtm-com bot commented Sep 28, 2021

Support Metadata frames in PubSub #1518

Support Metadata frames in PubSub #1518

Conversation

mrsuciu commented Sep 17, 2021

mregen commented Sep 20, 2021

lgtm-com bot commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mregen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mregen commented Sep 21, 2021

lgtm-com bot commented Sep 21, 2021

mregen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 28, 2021