search

newpavlov / rosbag-rs

Reading rosbag files in pure Rust
Apache License 2.0
9 stars 5 forks source link

Allow missing latching field in connection #2

Closed DomWilliamsEE closed 1 year ago

DomWilliamsEE commented 1 year ago

Hello! I've come across some bags that have an empty latching field, and these should fallback to false rather than failing.

newpavlov commented 1 year ago

I do not mid merging it, but could you link any reference about this behavior (e.g. software which previously or presently generates such files)? The field is specified as optional, but I think software simply omits it, not inserts an empty value.

DomWilliamsEE commented 1 year ago

Ah, good point. I found it's coming from an internal tool actually, so I could patch that instead of this crate?

newpavlov commented 1 year ago

Have you tried feeding the generated files to other ROS software (e.g. rosbag play)? If it gets consumed without issues, then it's better to apply your patch to this crate, otherwise you should update your internal tool to improve compatibility with the spec.

DomWilliamsEE commented 1 year ago

The other ROS tools parse these bags just fine, but rather because of sloppy checking it than intentional supporting.

newpavlov commented 1 year ago

Hm...

According to the connection header docs:

The latching field length is 10 bytes (0a 00 00 00):

It's not a spec of the rosbag format per se, but I will take it as an indication that an empty value should not be supported.

DomWilliamsEE commented 1 year ago

I agree, sounds like an empty value is not valid actually. I'll fix up our tool then. Cheers!