eclipse / tahu

Eclipse Tahu addresses the existence of legacy SCADA/DCS/ICS protocols and infrastructures and provides a much-needed definition of how best to apply MQTT into these existing industrial operational environments.
https://eclipse.org/tahu
Eclipse Public License 2.0
224 stars 129 forks source link

Clarification on the proto definition in alignment with SparkplugB specs #339

Open mkashwin opened 10 months ago

mkashwin commented 10 months ago

While studying the sparkplugB specification and the protofile created I had a couple of doubts

  1. The SparkplugB spect allows for Array types to be defined

    ◦ [tck-id-payloads-metric-datatype-value] The datatype MUST be one of the enumerated values as shown in the valid Sparkplug Data Types.

    But the proto spec specifies the value as oneof.

    oneof value {
            uint32   int_value                      = 10;
            uint64   long_value                     = 11;
            float    float_value                    = 12;
            double   double_value                   = 13;
            bool     boolean_value                  = 14;
            string   string_value                   = 15;
            bytes    bytes_value                    = 16;       // Bytes, File
            DataSet  dataset_value                  = 17;
            Template template_value                 = 18;
            MetricValueExtension extension_value    = 19;
        }

    should there not be additional entries which repeated phrase against the value? Probably something like

    // Array value fields
    repeated uint32   int_values      = 20;
    repeated uint64   long_values     = 21;
    repeated float    float_values    = 22;
    repeated double   double_values   = 23;
    repeated bool     bool_values     = 24;
    repeated string   string_values   = 25;
    repeated bytes    bytes_values    = 26;
    repeated DataSet  dataset_values  = 27;
    repeated Template template_values = 28;

    I am still trying to learn the protobuf programming so not yet sure on the exact code

  2. do we have to give different names to the value i.e. int_value, string_value etc. can we not have just one field name "value" like it is specified in the sparkplug_b.json

  3. Are there any plans to upgrade from proto2 to proto3?

rosstitmarsh commented 10 months ago

Not a maintainer of this repo but I can answer the first two.

  1. In section 6.4.17 "Datatype Details" (page 80) of the Sparkplug B specification, it explains that the array types are all to be encoded as an array of bytes. They are stored as a single entry in the bytes_value field. This is stored alongside a type indicating what type of array it is so you can know how to decode the bytes.
  2. Protobuf fields must have one of the types from this table https://protobuf.dev/programming-guides/proto2/#scalar, think of it like programming in a strongly typed language like C. Therefore the way to implement a value field with multiple types is to make it oneof field, with all the potential types as subfields.
mkashwin commented 10 months ago

Thanks for the clarification, This opens a second set of questions regarding the arrays

  1. for the Int8Arrays , Int16Array, Int32Array, Int64Array do we need to convert the signed integers into it's unsigned values? Currently it looks like from the spec this doesn't need to be done.
  2. In the specs for the example given for Int8Array [-23, 123] document mentioned the boolean array to be [0xEF, 0x7B] which appears wrong, should it not be [0xE9, 0x7B]
rosstitmarsh commented 10 months ago
  1. Yes, you can see that being done in the python example here.
  2. Yes that is wrong, there a few errors in that part of the spec. Documented in the Sparkplug repo and will be fixed in the next release.
mkashwin commented 9 months ago

Thanks I went that and found some limitations as well as a lack of unit tests Hence In created an alternative implementation https://github.com/mkashwin/unifiednamespace/blob/main/02_mqtt-cluster/src/uns_sparkplugb/uns_spb_helper.py WIth the associated test cases in https://github.com/mkashwin/unifiednamespace/tree/main/02_mqtt-cluster/test

Key updates

If the expert group is in the phase of upgrading the specs I would recommend the following enhancements

  1. Use the field datatype consistently instead of type and types(for datasets perhaps datatypes wouls be good)
  2. update the specs for the test data including details of converting bytes from little to big endian for Float and Double Arrays
  3. A few more elaborating and clarifications on the propertyset usage in the specs will be helpful.
wes-johnson commented 8 months ago

We're always open to contributions/PRs ;)

mkashwin commented 8 months ago

Will try to do that by next weekend. Is it ok for the code to be python 3.12 compliant? The current code did not seem to have any limitation ( other than python 3)?