Overwrite of push with length parameter not called.

[naegerob]

Dear developer

We are using this library for our communication in medical industry to communicate with some peer's using Serial Pair Ethernet over QUIC with protobuf files. We are really happy with this choice, because protobuf are really simple to use and provide efficient package sizes and simple encoding mechanism.

However, we are facing some problems which we need some help.
We have a sensor board with a STM32 MCU which reads data from a TOF-sensor and streams these values to the peer. We designed following protobuf files. The SensingCommandSet acts as a main interface for the peer. We list all possible commands in the oneOf structure and describe them in separate files.

SensingCommandSet {
  int32 messageType = 1;
  oneof CommandType {
    clld.Data data = 43;
    // more commands to add...
  }
}

Here is the message Type of the data to be streamed:

message Data {
  repeated fixed32 tof1 = 1 [packed = true];
  repeated fixed32 tof2 = 2 [packed = true];
  optional StatusCode result = 3;
}

Basically, we have some nested messages, but most bytes would be in the array of tof1 and tof2. The amount of data to be transmitted is 100 values of each, which results in 400 bytes. We acquire every 400us two tof-values which are 8 bytes together. We implemented it like this, that we add tof-values to the array, until the buffer is full with 100 samples of each tof-value. Then, we let them be serialized and pushed on the bus to transmit. This is required to be happend within this 400us window, because the data acquiring of 400us can not be stopped or postponed.
We implemented the Top level message SensingCommandSet like this:

template<size_t BufferSize = 256>
class SerializableNetworkStreamer : public EmbeddedProto::WriteBufferInterface {
public:
  explicit SerializableNetworkStreamer(NetworkStreamer<BufferSize> & streamer) : m_networkStreamer(streamer) {};

  ~SerializableNetworkStreamer() override = default;

  void clear() override {m_networkStreamer.reset();};

  // some more methods here

  bool push(const uint8_t byte) override {
    return pushAllOrTimeout(&byte, 1);
  };

  bool push(const uint8_t * bytes, const uint32_t length) override {
    return pushAllOrTimeout(bytes, length);
  };

private:

  static constexpr systime_t kPushTotalTimeoutMs = 1000;

  static constexpr systime_t kPushRetrySliceMs   = 50;

  bool pushAllOrTimeout(const uint8_t * bytes, uint32_t length) noexcept {
    if (length == 0) {
      return true;
    }
    if (length > BufferSize) {
      // bytes would never fit no matter how long we wait
      return false;
    }

    const systime_t deadline = osGetSystemTime() + kPushTotalTimeoutMs;

    while (get_available_size() < length) {
      const systime_t now = osGetSystemTime();
      if (now >= deadline) {
        return false;
      }
      const systime_t leftMs    = deadline - now;
      const systime_t waitSlice = (leftMs < kPushRetrySliceMs) ? leftMs : kPushRetrySliceMs;
      m_networkStreamer.waitForNotify(waitSlice);
    }

    auto span = std::span<const uint8_t>(bytes, length);
    return m_networkStreamer.writeBytes(span) == length;
  }

  NetworkStreamer<BufferSize> & m_networkStreamer;
};

We discovered, that every byte in the stream of 400 is called with push(&) separately. That is quite inefficient, because this results in many function calls and nested loops, even it it declared as packed and a push method with a length parameter is provided. Why is the embedded proto code not generated, that it serializes all 400 bytes in one? I have not seen push(&, length) is called in embedded proto files such as field.h or similar.
Are our protobufs correct? We tried alternatives like using uint32 instead of fixed32 to avoid compressing to speed up the calculation time. Also we used following protobuf, which are not supported by packaging, so we dropped it:

// Datapair are not packed, because it is not a primitive type
message Datapair {
  uint32 tof1 = 1;
  uint32 tof2 = 2;
}
message Data {
  Datapair datapair = 1;
  optional StatusCode result = 2;
}

The bus usage is not a problem, we have 10MBit/s which we do not use all.
We also thought, about shifting the packeting into another thread, and do the acquisition work of the sensor data in a more important task, but that is to be avoided, since we need to change architecture.
Do we use embedded proto correctly? What improvements can we expect to gain?
Best regards
Robert

[BartHertog]

Hello Robert,

Thanks for the detailed write-up! Your analysis is correct.

The current state:
The way packed repeated fixed32 is serialized today is not ideal: each value is pushed to the buffer one byte at a time rather than in bulk, so your 400-byte array turns into 400 individual push() calls.

There's actually a TODO in the code about this (in WireFormatter.h, around SerializeFixedNoTag), noting that on little-endian targets the data could be memcpy'd into the buffer in one go.

The future:
Proper support for this is in the v4 backlog and is currently being worked on, but it doesn't have a release date yet. Please note that v4 will contain breaking changes.

Solution for now:
As a semi-permanent workaround, you can switch the TOF data to a bytes field: bytes serializes in a single bulk copy (push(ptr, length)) instead of byte-by-byte, which is exactly the behaviour you're after.

The trade-off is that a bytes field is opaque, so you have to pack/unpack the uint32 values into the array yourself, and you must mind the endianness on both platforms, as long as both ends agree (STM32 is little-endian, matching the fixed32 wire format), the raw bytes line up.

I'll keep this ticket open and post an update here once v4 hits beta.

Best regards,

Bart