LiquidCAN Rust Implementation from ECUEmulator

[raffael0]

Added the rust liquidCAN protocol implementation. For development details look at the ECUEmulator Repo (commit 48081e45)

[miDeb]

Looks good, just a few questions

[miDeb]

I guess it's irrelevant for this test, but this would not be a valid payload due to 0xaa not being valid ascii and the string not being null terminated. Would that be handled at a higher level? What do we do if we encounter an invalid message?

[raffael0]

Good point. I'd say that invalid messages trigger an Error Message being sent to the server => but nothing else happens .

For the string: I don't really know how to easily enforce this on a type level. We would need to have to restrict the size and the null termination

[raffael0]

I noticed another issue:
Right now we set the maximum size for all of our values. i.e. even if we have a UInt8, we send 61 bytes over the wire.

Is there a way to easily fix this in your macro? Probably not, right? Especially since we don't always know the type when sending data.

Some ideas I have:

• Instead of having a [u8,61] in the messages, we could modify the DataType enum to also hold the data inside them. So a message would take a DataType field, which contains the data. The deserialize code would then call a custom to_bytes. I guess this would complicate the code, since we would have to construct the to_bytes manually again without the enum padding, but I think we could do this with a macro
• The only alternative I see is moving this somehow annotating the Messages with the Datatype Information, which could be used in the macro to only serialize the [u8,61] up to a certain length.

Do you have some time to work on this? @miDeb I'm a bit busy then next few days

[miDeb]

Right now we set the maximum size for all of our values. i.e. even if we have a UInt8, we send 61 bytes over the wire.

So you're saying for e.g. ParameterSetReq we're always using 61 bytes for the value, even if that is not necessary? In that case I guess we would have to update the protocol specification because right now it specifies 61 bytes for parameter and 62 bytes for field values.

From an implementation standpoint I believe I can think of a way to implement this in the macro; basicallly one way or another we'll have to know what data type we are serializing and then only output a certain length depending on that. For deserializing we then don't know the data type (unless it is passed in from outside) so we'd just produce a byte array again.

If you want I can update the protocol spec to make ParameterSetReq, ParameterSetConfirmation and FieldGetRes variable length depending on the data type, and implement that in rust.

I guess similar arguments could be made about the TelemetryGroup* messages and the messages that contain a string - they could also be shortened if possible.

An idea I just had (though not sure if I'm a fan of it, just wanted to mention it here): We could simply specify that trailing null bytes may be omitted when sending, and incoming messages are padded with null bytes if needed. But maybe that would hinder us in detecting faulty messages?

Let me know what you think.

[raffael0]

In my mind I always wanted the parameter/telemetry values to only be a maximum of 61/62 bytes, and not always that long. But you are right that I didn't correctly specify this. My bad! Actually this also goes for the Telemetry* and Status messages...

you want I can update the protocol spec

Yes please that would be great. I agree with you that this would also make sense for the Telemetry* and the String messages. Especially since we'll be sending looots of TelemetryUpdates.

We could simply specify that trailing null bytes may be omitted when sending

That's not a bad Idea. Obviously the other approach is cleaner. It might have a slight impact on faulty messages, but CAN already has a CRC, so I think we would detect faulty messages through that already. Another downside is some overhead on both ends and that we might miss some issue which fucks us later on.
@carofcons what do you think is easier to implement on the firmware side? Do you have a preference?

Thank you so much for the work!