[Review] feat: Publishing of DataSetMetaData via mqtt json #5118

wlkrm · 2022-05-09T18:59:03Z

This PR enables the server to publish DataSetMetaData via mqtt and json encoding. If enabled, DataSetMetaData are published, whenever a new PublishedDataSet is added. In addition, it introduces a new function for the server API (UA_Server_updateDataSetField()) which allows the server to inform the pubsub layer that FieldMetaData were changed. The pubsub layer will then publish the updated version of the DataSetMetaData. This enables the user to make multiple changes to the FieldMetaData in an transactional style. The tutorial_pubsub_mqtt_publish was extended to publish the DataSetMetaData via json mqtt, if UA_ENABLE_PUBSUB, UA_ENABLE_PUBSUB_MQTT, UA_ENABLE_PUBSUB_MQTT_METADATA and UA_ENABLE_JSON_ENCODING are set. The binary encoding is not supported. As stated in the draft for UA Specification Part 14 - MQTT Enhancements, a DataSetMetaDataType message is an instance of a DataSetMetaDataType encoded using the JSON encoding defined in OPC 10000-6.

CLAassistant · 2022-05-09T18:59:08Z

All committers have signed the CLA.

andreasebner · 2022-05-18T06:45:15Z

Thank you for the contribution @wlkrm! Currently the appveyor build is failing: 5>Compiling...
6>..\src\pubsub\ua_pubsub_networkmessage_json.c(246) : error C2361: initialization of 'count' is skipped by 'default' label
6> ..\src\pubsub\ua_pubsub_networkmessage_json.c(217) : see declaration of 'count'
6>..\src\pubsub\ua_pubsub_networkmessage_json.c(246) : error C2361: initialization of 's' is skipped by 'default' label
6> ..\src\pubsub\ua_pubsub_networkmessage_json.c(171) : see declaration of 's

It would be great if you could rebase your changes and search for a solution. I’m looking forward to merge your changes, but currently the coverage will decrease significantly because your changes do not contain test cases.
@NoelGraf could you please review this PR?

wlkrm · 2022-05-20T09:27:15Z

I will add the missing test cases.

NoelGraf · 2022-06-13T12:04:27Z

@wlkrm
The changes look good overall. First, I would suggest doing a rebase since some changes have been made to MQTT.

src/server/ua_server.c

NoelGraf · 2022-06-13T12:27:03Z

include/open62541/server_pubsub.h

@@ -287,6 +288,7 @@ typedef struct {
 typedef struct {
 UA_String name;
 UA_PublishedDataSetType publishedDataSetType;
+ UA_Boolean sendViaWriterGroupTopic;


Is the insertion of the field necessary?
And what's the field for?

The field allows for selective batching. DataSetMessages of DataSetWriters can be batched by their common WriterGroup using the UA_BrokerDataSetWriterTransportDataType of the WriterGroup according to Part 14. PublishedDataSets of Writers grouped by a common WriterGroup with this flag set will be sent out batched under the Data-Topic-Queue defined in the transportSettings (UA_BrokerDataSetWriterTransportDataType.queueName) of the WriterGroup. The reason for the implementation is, that my use-case gets simplified, if I receive DataSetMessages of multiple DataSetWriter grouped together through batching. Example JSONs and Topics of this can be found here (https://github.com/umati/Showcase/blob/pubsub/PubSub.md#topic-structure).

…etMessages

…a mqtt json

…tWriter for publishing

…ub_mqtt_publish

… mqtt json messages

wlkrm · 2022-08-01T15:26:42Z

@NoelGraf, thank you for your comments. I rebased the PR and tests for the encoding. I was not really sure how to properly test the MQTT-Part, as the tests would have to spin up a broker? Please review again.

jpfr

Looks really good overall.
Please make this configurable with a configuration flag, not a build flag.

MQTT+JSON is not used in environments where every kilobyte counts.
So we can afford to have this compiled in and keep the complexity of the build options low.

CMakeLists.txt

wlkrm · 2022-08-02T17:08:52Z

@jpfr I removed the compile time switches and replaced them by a per DataSetWriter config field. A DataSetWriter can now be configured (https://github.com/wlkrm/open62541/blob/34156d549bab3e00d9d71b715e99dec81ffa6f81/include/open62541/server_pubsub.h#L599) to publish the DataSetMetaData using the corresponding transport layer (e.g. JSON-MQTT, which is the only metadata transport implemented in this PR).

jpfr · 2022-08-03T07:02:14Z

I removed the compile time switches and replaced them by a per DataSetWriter config field. A DataSetWriter can now be configured (https://github.com/wlkrm/open62541/blob/34156d549bab3e00d9d71b715e99dec81ffa6f81/include/open62541/server_pubsub.h#L599

If this is the place where the feature is enabled, add a comment that the feature requires JSON+MQTT.
Otherwise people will not understand what it does without looking at the code.

We can also log a warning if the feature is enabled without JSON+MQTT.

If there is a canonical section in the PubSub spec, also refer to that in the comment.

examples/pubsub/tutorial_pubsub_mqtt_publish.c

andreasebner · 2022-08-04T08:41:10Z

include/open62541/server_pubsub.h

@@ -314,6 +315,7 @@ typedef struct {
 typedef struct {
 UA_String name;
 UA_PublishedDataSetType publishedDataSetType;
+ UA_Boolean sendViaWriterGroupTopic;


Maybe a more precise name of this flag would help to understand it. In addition add a short inline comment.

Renamed it to batchMessagesViaWriterGroupTopic and added the coment.

src/pubsub/ua_pubsub_datasetmetadata_json.c

src/pubsub/ua_pubsub_writer.c

andreasebner · 2022-08-04T08:56:57Z

src/pubsub/ua_pubsub_writer.c

+void
+UA_Server_updateDataSetField(UA_Server *server, const UA_NodeId dsf, UA_Boolean minor, UA_Boolean major) {
+ UA_DataSetField *currentField = UA_DataSetField_findDSFbyId(server, dsf);
+ if(!currentField) {
+ return;
+ }
+
+ if(currentField->configurationFrozen) {
+ UA_LOG_WARNING(&server->config.logger, UA_LOGCATEGORY_SERVER,
+ "Update DataSetField failed. DataSetField is frozen.");
+ return;
+ }
+
+ UA_PublishedDataSet *pds =
+ UA_PublishedDataSet_findPDSbyId(server, currentField->publishedDataSet);
+ if(!pds) {
+ return;
+ }
+
+ if(pds->configurationFrozen) {
+ UA_LOG_WARNING(&server->config.logger, UA_LOGCATEGORY_SERVER,
+ "Update DataSetField failed. PublishedDataSet is frozen.");
+ return;
+ }
+
+ /* Update minor version of PublishedDataSet */
+ UA_UInt32 timeDifference = UA_PubSubConfigurationVersionTimeDifference();
+ if(minor)
+ pds->dataSetMetaData.configurationVersion.minorVersion = timeDifference;
+ if(major)
+ pds->dataSetMetaData.configurationVersion.majorVersion = timeDifference;
+}
+


This code does not match the logic implied by the function name. In addition the user can change the minor/major version directly. I would preffer to have only implicit changes.

I removed the flags.

andreasebner · 2022-08-04T08:57:58Z

src/pubsub/ua_pubsub_writer.c

+ break;
+ }
+#ifdef UA_ENABLE_JSON_ENCODING
+ res = sendNetworkMessageMetadataJson(connection, &dataSetMetaData, &newDataSetWriter->config.dataSetWriterId, 1, &newDataSetWriter->config.transportSettings);


This means we send out the Metadata during the WG creation. Not sure if this is allowed if the state is not operational.

MetaData are now published for the first time, when the transport is ready.
63ab001

…ational

wlkrm · 2022-08-05T09:40:29Z

I removed the compile time switches and replaced them by a per DataSetWriter config field. A DataSetWriter can now be configured (https://github.com/wlkrm/open62541/blob/34156d549bab3e00d9d71b715e99dec81ffa6f81/include/open62541/server_pubsub.h#L599

If this is the place where the feature is enabled, add a comment that the feature requires JSON+MQTT. Otherwise people will not understand what it does without looking at the code.

We can also log a warning if the feature is enabled without JSON+MQTT.

If there is a canonical section in the PubSub spec, also refer to that in the comment.

I added the comment. And a warning: https://github.com/wlkrm/open62541/blob/63ab001e387136a30925936656b9e60aa3c113fb/src/pubsub/ua_pubsub_writer.c#L891

wlkrm changed the title ~~[WIP] feat: Üublishing of DataSetMetaData via mqtt json~~ [WIP] feat: Publishing of DataSetMetaData via mqtt json May 9, 2022

andreasebner assigned wlkrm and NoelGraf May 10, 2022

andreasebner added the Component: PubSub label May 10, 2022

wlkrm force-pushed the mqtt-metadata-rebase branch 3 times, most recently from 08409a9 to d9afcd5 Compare May 16, 2022 14:42

wlkrm changed the title ~~[WIP] feat: Publishing of DataSetMetaData via mqtt json~~ feat: Publishing of DataSetMetaData via mqtt json May 16, 2022

wlkrm changed the title ~~feat: Publishing of DataSetMetaData via mqtt json~~ [Review] feat: Publishing of DataSetMetaData via mqtt json May 16, 2022

NoelGraf reviewed Jun 13, 2022

View reviewed changes

wlkrm force-pushed the mqtt-metadata-rebase branch from d9afcd5 to 63bb5eb Compare June 20, 2022 17:23

wlkrm added 12 commits August 1, 2022 13:34

feat(server): Add switch to manually enable/disable batching of DataS…

71125c9

…etMessages

feat(server): Enable RawData encoded DataSetFields for mqtt json

806979c

feat(server): Introduce DataSetMetaData, helpers and json encoding

b398efa

feat(server): Compile time switches for DataSetMetaData publishing vi…

c20f78f

…a mqtt json

fix(server): Additionally allow usage of transport settings of DataSe…

e7fc3a0

…tWriter for publishing

feat(server): Publishing of DataSetMetaData via mqtt-json

347322f

fix: Missing compile switches

dcd0020

fix(examples): Segfault if json is used for encoding in tutorial_oubs…

ecf4e06

…ub_mqtt_publish

feat(examples): Extend tutorial_pubsub_mqtt_public by DataSetMetaData…

24f3206

… mqtt json messages

feat: add description for pds

0b0a7d0

fix: only encode DataSetWriterName if present

828e7d7

feat(test): Test for ua-metadata json encoding

87f4832

wlkrm force-pushed the mqtt-metadata-rebase branch 2 times, most recently from 87bf0f8 to d552dd3 Compare August 1, 2022 14:02

fix: Minor and Major updates for DataSetFields

fff3433

wlkrm force-pushed the mqtt-metadata-rebase branch from d552dd3 to fff3433 Compare August 1, 2022 14:18

wlkrm added 2 commits August 1, 2022 16:39

fix: build error

bb1d88a

fix: Send messages via correct transportSettings

2fd6d2c

fix: Update tutorial

8dc74ae

jpfr reviewed Aug 2, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

fix: Configuration option for DataSetWriters to publish DataSetMetaData

34156d5

fix: Deadlocks

5e970a4

andreasebner reviewed Aug 4, 2022

View reviewed changes

wlkrm added 3 commits August 5, 2022 09:05

fix: Tutorial

bfbd272

fix: Address comments

d5ce05c

fix: Publish metadata via json mqtt only when transport layer is oper…

63ab001

…ational

wlkrm added 2 commits August 26, 2022 14:50

fix: Value Rank etc. are not not mandatory for metadata

23c9f4d

feat: retained messages

1a1b070

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Review] feat: Publishing of DataSetMetaData via mqtt json #5118

[Review] feat: Publishing of DataSetMetaData via mqtt json #5118

wlkrm commented May 9, 2022 •

edited

Loading

CLAassistant commented May 9, 2022 •

edited

Loading

andreasebner commented May 18, 2022

wlkrm commented May 20, 2022

NoelGraf commented Jun 13, 2022

NoelGraf Jun 13, 2022

wlkrm Aug 1, 2022

wlkrm commented Aug 1, 2022

jpfr left a comment

wlkrm commented Aug 2, 2022

jpfr commented Aug 3, 2022

andreasebner Aug 4, 2022

wlkrm Aug 5, 2022

andreasebner Aug 4, 2022

wlkrm Aug 5, 2022

andreasebner Aug 4, 2022

wlkrm Aug 5, 2022

wlkrm commented Aug 5, 2022

[Review] feat: Publishing of DataSetMetaData via mqtt json #5118

Are you sure you want to change the base?

[Review] feat: Publishing of DataSetMetaData via mqtt json #5118

Conversation

wlkrm commented May 9, 2022 • edited Loading

CLAassistant commented May 9, 2022 • edited Loading

andreasebner commented May 18, 2022

wlkrm commented May 20, 2022

NoelGraf commented Jun 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlkrm commented Aug 1, 2022

jpfr left a comment

Choose a reason for hiding this comment

wlkrm commented Aug 2, 2022

jpfr commented Aug 3, 2022

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

wlkrm commented Aug 5, 2022

wlkrm commented May 9, 2022 •

edited

Loading

CLAassistant commented May 9, 2022 •

edited

Loading