BuiltInDataSerializer is incredibly verbose

[danieleades]

the BuiltInDataSerializer is incredibly verbose. It's also quite an unusual construction.

What is this object doing exactly? You've opted for an imperative style- is there a reason why the usual declarative serde approach doesn't work here? It looks like most of the fields are None, most of the time. Maybe the variants should be different objects?

[jhelovuo]

Yes, it is more verbose and confusing than necessary. It should be improved, i.e. rewritten. I do not know of any technical reason to use the imperative style, but I am not the original author for this part. It seems to have been constructed by heavy use of copy, paste, and edit.

Rewriting could certainly improve this, but on the other hand:

• It takes some time and effort. Volunteers are welcome.
• Rewrite may introduce bugs. Now it seems somehow work. "If it is not broken, do not fix it."

Regarding "What is this object doing exactly?":

The purpose of BuiltinDataSerializer and the corresponding deserializer is to just (de)serialize some Discovery data types that the RTPS specification, Sections 8.5 and 9.6, mandates. I have been told that they are specified so that it not possible to use the usual CDR serializer/deserializer, but have not gone through the technical details myself.

This serialization is used to communicate with other DDS implementations, so it is important to follow the specification and check interoperability with others when doing any changes.

[danieleades]

@jhelovuo i hear you. not to be touched without some test coverage. #47 can help with that

[jhelovuo]

If my memory serves, the idea in BuiltinDataDeserializer is that first it produces self by parse_data method. The data on the wire is a tagged list of items, or a "parameter list" representation, and those are used to fill in the various Option fields.

The self object can then be converted to various different types using generate_ -methods, depending on the context (or Topic, really).

The BuiltinDataSerializer does basically the same, but in the other direction.

[danieleades]

I'm just starting to look at this in https://github.com/danieleades/rtps... i'm forming the opinion that serde is not the right solution (neither is speedy!).

• the specification does a good job of separating the platform independent spec from the UDP-specific elements. I think for maximum flexibility, the implementation should do the same. implementing serialisation within the platform independent modules breaks that encapsulation
• using a serde-based approach makes it difficult to add other modalities in future. For example you would bend over backwards to get the UDP serialization wire format correct by creating a very bespoke serializer, or by careful and bespoke implementation of serde::{Serialize, Deserialize}, or some combination of the two. This would then wreak havoc if you later wanted to create an independent platform-dependent implementation

I now the think the most appropriate serialization strategy is to create new traits (ToBytes/FromBytes, or ToCdr/FromCdr, something along those lines). These traits would handle conversion of the various types to and from the appropriate wire format. This also removes the coupling between the structure of the types in the crate and the structure defined in the specification. Additional platform specific constructors and other methods could also be implemented separately, in a platform-dependent module.

pub trait Cdr {
   type DecodeErr: std::error::Error
   fn as_bytes(&self) -> Vec<u8>;
   fn from_bytes(bytes: impl AsRef<&[u8]>) -> Result<Self, Self::DecodeErr>;
}

that doesn't mean to say that types couldn't selectively make use of speedy or serde as helpers for implementing the Cdr trait, if that's useful.

Adding a new platform-dependent modality would involve creating new traits, and would be entirely decoupled from the UDP implementation.

[jhelovuo]

This is a duplicate of issue #32 .