Code Smells in ROS Code

Archive Quality Assurance
5

I think it would also be helpful to talk about this kind of thing in the context of ROS2. There are a few patterns which appear in the demos and example packages (and which are therefore implicitly endorsed) that can cause problems down the road. As an example:

ROS2 (Anti-)Pattern: Using spin_until_future_complete (which is used in many places, such as the minimal_client rclcpp example):

  auto request = std::make_shared<AddTwoInts::Request>();
  request->a = 41;
  request->b = 1;
  auto result_future = client->async_send_request(request);
  if (rclcpp::spin_until_future_complete(node, result_future) !=
    rclcpp::FutureReturnCode::SUCCESS)
  {
    RCLCPP_ERROR(node->get_logger(), "service call failed :(");
    return 1;
  }
  auto result = result_future.get();

Reasoning: It is not possible to call spin_until_future_complete from a callback which is being handled by an executor. If you’ve written some portable function that uses it, it may fail depending on the architecture of the node that is using it!

Also, if you’re able to use spin_until_future_complete in your node, then you have intermingled the custom functionality of your node with the scope of the node’s executor. This will make it much more difficult to structure the node so it is composable as a Component.

I’ve been using the future returned by the rclcpp service client’s async_send_request function to help wait until the service has finished, though this requires manual setup of callback groups and a MultithreadedExecutor to let the node get the service result response in a different thread.

7 Likes