mavlink / rust-mavlink

MAVLink library for Rust.
https://mavlink.github.io/rust-mavlink/mavlink/
Apache License 2.0
156 stars 78 forks source link

Project is not truncating trailing zeros for payload #188

Open patrickelectric opened 1 year ago

patrickelectric commented 1 year ago

This is a must for MAVLink2 https://mavlink.io/en/guide/serialization.html#payload_truncation

reference: https://github.com/bluerobotics/BlueOS/issues/1740 reference: https://github.com/mavlink/mavlink2rest/issues/80

hamishwillee commented 1 year ago

This is a must for MAVLink2

Yes. FWIW this came up in a real implementation, and we decided on "must". But you'll note that receivers are supposed to "cope" with messages that don't truncate if they are otherwise valid.

patrickelectric commented 1 year ago

Thanks for pointing that out @hamishwillee

clydemcqueen commented 1 year ago

This is a bit complicated because QGC receives the messages just fine, but writes them to tlog files incorrectly: truncated, but with the CRC computed from the non-truncated payload. This is caused by a bug in mavlink_helpers.h: https://github.com/ArduPilot/pymavlink/issues/237

The result is quite a few messages sent by rust-mavlink and logged by QGC will be treated as BAD_DATA by pymavlink.

@hamishwillee do you have an opinion for how to fix this? Perhaps the best way is to submit a PR to fix the mavlink_helpers.h bug?

hamishwillee commented 1 year ago

@clydemcqueen The original PR submitted to fix this was https://github.com/ArduPilot/pymavlink/pull/241/files which as I understand it recalculates the checksum. Does this do what you need?

That stalled, for reasons given in https://github.com/ArduPilot/pymavlink/pull/241#issuecomment-463593404 . I could try re-energise?

If that does not do what you want, then yes, a new PR. However this would likely stall in the same way unless a test case can be created along with the patch. Its all a bit frustrating since however you look at it this is a bug. Unfortunately ArduPilot/Pymavlink moves at its own schedule.

Happy to try get this pushed again, as long as we're all clear that the fix we push is the right one.

amsmith-pro commented 2 months ago

This is a problem when receiving as well. Made an issue at the PX4 forum about a missing byte from a message, at the time wasn't aware of this mavlink v2 truncation.

As an example, when receiving a truncated packet for OPEN_DRONE_ID_LOCATION (missing the final timestamp_accuracy field if it's set to zero), MavFrame::deser fails to deserialize it because it expects that enum and instead gets whatever value the first checksum byte is. To get around this, I currently do a check on the ID and payload length, and manually insert an extra byte before the checksum before passing it to MavFrame::deser... maybe there's a way I'm missing?