C# protobuf construction errors

[asthomas]

I am using SparkplugBCSharp.cs downloaded from this repo.

When constructing a Sparkplug message, the C# implementation explicitly excludes fields Seq and Timestamp if their values are zero. Seq is required to be zero by the specification for NBIRTH messages. Timestamp is required on all metrics, and there is no reason why the Timestamp cannot be zero.

The result is that code that uses SparkplugBCSharp.cs does not produce specification-compliant messages.

[asthomas]

This is further complicated by the requirement that an NDEATH message cannot include a sequence number. The consequence seems to be that it is impossible to build both a valid NBIRTH and valid NDEATH message from the same SparkplugBCSharp.cs implementation. Fixing this for NBIRTH will break it for NDEATH and vice versa.

[MRIIOT]

@asthomas Is this still the case if you run against protobuf 2 and not 3?

[asthomas]

[Edited - original post confused the the .proto file with the behaviour of protoc]

Before I can answer that question, I need to know which .proto file I should be using. I can see there are both "sparkplug_b.proto" and "sparkplug_b_c_sharp.proto" in the source. sparkplug_b.proto is somewhat newer, and defines "extensions" where sparkplug_b_c_sharp.proto defines "details". sparkplug_b.proto uses Protobuf 2, while sparkplug_b_c_sharp.proto uses Protobuf 3.

Protobuf 2 uses a bit field to track whether a member has been explicitly set. It looks like that would resolve the above issue. Protobuf 3 generates code that suffers from the either/or problem between NBIRTH and NDEATH, and doesn't look like it could be made to work at all without manual hacking.

The pre-built .cs file in c_sharp/core/src is built against sparkplug_b_c_sharp.proto, which uses Protobuf 3. Is it still relevant? What is the intended definition for C# code? Why does sparkplug_b_c_sharp.proto exist? Why does it use Protobuf 3 when other targets use Protobuf 2?

[asthomas]

I decided to take a look at the Google Protobuf spec. The Protobuf 3 spec contains this entry:

Note that for scalar message fields, once a message is parsed there's no way of telling whether a field was explicitly set to the default value (for example whether a boolean was set to false) or just not set at all: you should bear this in mind when defining your message types.

As I read this, the Sparkplug 3 specification cannot be met by a Protobuf 3 implementation. I think this is a pretty good case for a minor adjustment to the spec, to make the Seq field in an NDEATH message optional and ignored by the receiver.

[MRIIOT]

@asthomas thoughts on what needs to change ?

[asthomas]

@MRIIOT - Spec section 4.2.1 says this:

[tck-id-topics-ndeath-payload] The NDEATH message contains a very simple payload that
MUST only include a single metric, the bdSeq number, so that the NDEATH event can be
associated with the NBIRTH. Since this is typically published by the MQTT Server on behalf of the
Edge Node, information about the current state of the Edge Node and its devices is not and cannot
be known. As a result, [tck-id-topics-ndeath-seq] The NDEATH message MUST NOT include a
sequence number.

The justification for the requirement "MUST NOT include a sequence number" is weak. There is no practical reason why NDEATH could not contain a sequence number. The receiver can simply ignore it. If it seemed important, the spec could require that the sequence number, if present, must be zero.

The spec could instead say something like this:

[tck-id-topics-ndeath-payload] The NDEATH message contains a very simple payload that
MUST only include a single metric, the bdSeq number, so that the NDEATH event can be
associated with the NBIRTH. Since this is typically published by the MQTT Server on behalf of the
Edge Node, information about the current state of the Edge Node and its devices is not and cannot
be known. As a result, [tck-id-topics-ndeath-seq] The NDEATH message MAY include a
sequence number. Host Applications SHALL ignore the sequence number, if present.

Spec section 6.4.25 - NDEATH says this:

[tck-id-payloads-ndeath-seq] Every NDEATH message MUST NOT include a sequence number.

It could be changed to this:

[tck-id-payloads-ndeath-seq] An NDEATH message MAY include a sequence number.

Similarly, there are normative statements for NCMD and DCMD messages:

Section 6.4.23:

[tck-id-payloads-ncmd-seq] Every NCMD message MUST NOT include a sequence number.

Section 6.4.24:

[tck-id-payloads-dcmd-seq] Every DCMD message MUST NOT include a sequence number.

I didn't see a justification for these requirements, though it is presumably the same as for NDEATH. The sequence number could be optional, possibly stipulated to be zero, and ignored by the receiver.

I know that sequence number has no meaning in NDEATH, NCMD and DCMD messages, and that there is no reason for them to be there. This suggestion is simply a matter of practicality. Sparkplug is explicitly tied to Google Protobuf, and the Sparkplug 3.0 spec as written ties it to a language-dependent implementation detail of Protobuf version 2. Although Protobuf version 2 is unclear on the point, its spec implies that an absent field is indistinguishable from a default value. The Protobuf version 3 spec makes this explicit. Correctness in Sparkplug 3.0 requires the opposite.

[MRIIOT]

Spec section 4.2.1 says this:

[tck-id-topics-ndeath-payload] The NDEATH message contains a very simple payload that
MUST only include a single metric, the bdSeq number, so that the NDEATH event can be
associated with the NBIRTH. Since this is typically published by the MQTT Server on behalf of the
Edge Node, information about the current state of the Edge Node and its devices is not and cannot
be known. As a result, [tck-id-topics-ndeath-seq] The NDEATH message MUST NOT include a
sequence number.

The justification for the requirement "MUST NOT include a sequence number" is weak. There is no practical reason why NDEATH could not contain a sequence number. The receiver can simply ignore it. If it seemed important, the spec could require that the sequence number, if present, must be zero.

The spec could instead say something like this:

[tck-id-topics-ndeath-payload] The NDEATH message contains a very simple payload that
MUST only include a single metric, the bdSeq number, so that the NDEATH event can be
associated with the NBIRTH. Since this is typically published by the MQTT Server on behalf of the
Edge Node, information about the current state of the Edge Node and its devices is not and cannot
be known. As a result, [tck-id-topics-ndeath-seq] The NDEATH message MAY include a
sequence number. Host Applications SHALL ignore the sequence number, if present.

Spec section 6.4.25 - NDEATH says this:

[tck-id-payloads-ndeath-seq] Every NDEATH message MUST NOT include a sequence number.

It could be changed to this:

[tck-id-payloads-ndeath-seq] An NDEATH message MAY include a sequence number.

NDEATH/DDEATH uses MQTT LWT mechanism to convey node or device disconnect status, so sequence is not known as the broker issues LWT upon disconnect. Which is where bdSeq makes sense as session context for messages.

[MRIIOT]

Similarly, there are normative statements for NCMD and DCMD messages:

Section 6.4.23:

[tck-id-payloads-ncmd-seq] Every NCMD message MUST NOT include a sequence number.

Section 6.4.24:

[tck-id-payloads-dcmd-seq] Every DCMD message MUST NOT include a sequence number.

I didn't see a justification for these requirements, though it is presumably the same as for NDEATH. The sequence number could be optional, possibly stipulated to be zero, and ignored by the receiver.

Only reason I can think of the node or device keeping track of incoming sequence, is delivery order of commands, but then SCADA Hosts would need to synchronize sequence number for commands.

[MRIIOT]

Although Protobuf version 2 is unclear on the point, its spec implies that an absent field is indistinguishable from a default value. The Protobuf version 3 spec makes this explicit. Correctness in Sparkplug 3.0 requires the opposite.

protocolbuffers/protobuf#1606

[asthomas]

Good link. That could be the winner. I found the optional field description in the proto3 docs now that I know what to look for, so it looks like just the paragraph I quoted above needs updating.

I tried making "seq" optional in Payload, and that caused protoc to add the bit field. I'll see how far through the TCK I can get with that change. If that works, then sparkplug_b_c_sharp.proto needs to be modified to make timestamp and seq optional.

Frankly, I still think the Sparkplug spec is unnecessarily strict. There really isn't any reason why it should stipulate that a field must not be present. Specifying those fields as optional and ignored would be a simplification.

[MRIIOT]

@asthomas Found the cause inside https://github.com/SeppPenner/SparkplugNet project.

SeppPenner/SparkplugNet#5 (comment)

[asthomas]

I can confirm that making Seq and both Timestamps optional in sparkplug_b_c_sharp.proto has resolved the issue for me with the latest protoc and Protobuf version 3.

[MRIIOT]

Is this the reason: https://stackoverflow.com/questions/42622015/how-to-define-an-optional-field-in-protobuf-3

[asthomas]

It is. Essentially the optional flag in proto3 reinstates the behaviour of proto2, where in addition to having a value a member may also be unset. Unset values are excluded from the wire packet, and set default values are included. That makes it possible to conform to the Sparkplug requirements on Seq in NBIRTH and NDEATH.

I also had to make Timestamps optional because proto3 does not send a set default value unless the field is optional, and the Sparkplug spec requires timestamps in all cases. I want to send a zero timestamp - the optional flags allows that.

[NiclasLindgren]

The problem is that the sparkplug spec makess layer breaking assumptions of what is sent on the wire instead of just caring about the value is receives from the encoding layer.

this means the for instance profobuf cannot optimize optional values (as they wanted in protobuf 3 before 3.15) to not include presence bits and instead assume the default values. That can save a lot of data in large lists of structures.

All sparkplug should care about is if it gets a value back from seqno or not and verify it.

Part from the fact that requiring that hasSeqno has to return true breaks any edge nodes using this specified behavior in protobf (2 or 3, i.e. not sending presence bits) for no real gain.

[bryce-nakatani]

To get the payload sequence ID to work correctly, build a C# protobuf from the proto file sparkplug_b/sparkplug_b.proto. If you diff the two proto definition files supplied, there are optional fields missing in the sparkplug_b_c_sharp.proto. This is key to allowing the payload sequence ID to be included when it is set to zero.