osrf / dynamic_message_introspection

Apache License 2.0
33 stars 19 forks source link

Fails silently for missing fields #28

Open b-adkins opened 1 month ago

b-adkins commented 1 month ago

I tried dynmsg to parse config files for controlling a real robot. There does not appear to be an option for it to raise an exception if a message field is missing.

I found this issue to be a dealbreaker, due to safety concerns, and so I will not be using this library.

Testing

On ROS 2 Humble, here is my parsing code.

geometry_msgs::msg::Pose pose;
yaml_string = 
"position: {'x': 0.979, 'y': -2.161}"
"orientation: {'x': 0., 'y': 0., 'z': 0., 'w': 1.}";
dynmsg::cpp::yaml_and_typeinfo_to_rosmsg(
    dynmsg::cpp::get_type_info({"geometry_msgs", "Pose"}),
    yaml_string,
    reinterpret_cast<void*>(&pose)
  );

Expected Behavior

The YAML would fail to be parsed into a geometry_msgs/msg/Pose, because it is missing a field. It would have to be via an exception, because there is no return code in the function interfaces nor in the return struct RosMessage_Cpp.

Actual behavior

It gets parsed into:

position:
  x: 0.979000
  y: -2.16100
  z: 0.00000
orientation:
  x: 0.00000
  y: 0.00000
  z: 0.00000
  w: 1.00000

using the default value of zero for the missing position z.

I find this to be incredibly dangerous when controlling a real robot. A typo in the config file that omits a key could fail silently during parsing and then send a robot to an unexpected wrong position.

christophebedard commented 3 weeks ago

Feel free to submit a PR, but this repo isn't really being maintained, so it might never get merged.

find this to be incredibly dangerous when controlling a real robot.

I don't think this was ever meant to be used in production, at least not in its current state.