Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mkashwin opened this issue Dec 21, 2023 · 6 comments
Open

Comments

@mkashwin
Copy link

mkashwin commented Dec 21, 2023

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
Copy link

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
Copy link
Author

mkashwin commented Jan 5, 2024

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
Copy link

  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
Copy link
Author

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

  • comprehensive unit tests based on the specs
  • simplified the encoding and decoding of values by encapsulating the logic in the associated enum for the data type
  • works with Python 3.12
  • support for PropertySets, PropertySetLists, DataSets, Templates as well as Arrays and metadata for the metrics
    If that adds value to this project please feel free to use it or if you deem it good enough to raise a PR let me know.

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
Copy link
Contributor

We're always open to contributions/PRs ;)

@mkashwin
Copy link
Author

mkashwin commented Mar 5, 2024

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)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants