search

ros-tooling / rosbag2_storage_mcap

rosbag2 storage implementation for MCAP file format
https://mcap.dev/
Apache License 2.0
32 stars 5 forks source link

Discussion: what should a storage plugin do if it can't find a message schema? #70

Closed james-rms closed 1 year ago

james-rms commented 1 year ago

currently we log an error to rosout and record schemas with empty schema_encoding and data.

MichaelOrlov commented 1 year ago

Looks like a reasonable behavior. What could be an option? Throw exception and stop recording?

jhurliman commented 1 year ago

I think throwing an exception and stopping the recording process is preferable. Recording unparseable data with a log message to that effect seems like pushing the problem downstream past the point where it can be properly fixed.

MichaelOrlov commented 1 year ago

It might be not a real problem when message definition not found. In some/most cases it's by system design. The recording node might not have full access to the all message definitions.
Storing message definition inside recording is a nice option but not a show stopper for recording/replay IMO.

It will be more frustrations from users when need to quickly record messages on resource constrained HW/SW platform which doesn't have full ROS2 setup and build with all message definitions and typesupports. On some platform it's just not feasible to make a full build on target HW. The only option is cross-platform build and copy only those parts which is really needed. Although it's usually a significant effort and not trivial how to make such cross-platform build and squeeze it to bare minimum.

jhurliman commented 1 year ago

As frustrating as it may be to get ROS 2 environments setup correctly, the MCAP “ros2” profile is a published specification and writing CDR payloads with no preceding schema violates the spec. The resulting recording would be invalid.

MichaelOrlov commented 1 year ago

Then we need to change the spec. :open_hands: I would consider it to be a show stopper to make mcap storage plugin to be a default in ROS2.

amacneil commented 1 year ago

Agree with @MichaelOrlov

I think the current behavior is the best option (log a warning and proceed with blank schema). It is not ideal for downstream consumers, but it is still perfectly usable within ROS itself.

Failing to record if the message schema can’t be found is not acceptable, since it is a regression in functionality from the sqlite plugin.

This will become a non issue (hopefully in Iron) once REP2011 lands and we have native access to schemas at runtime.

james-rms commented 1 year ago

Sounds like consensus has been reached.