search

ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
514 stars 410 forks source link

Regression : Executor::spin_some_impl is active waiting #2508

Closed jmachowinski closed 2 months ago

jmachowinski commented 2 months ago

This was introduced in #2142

https://github.com/ros2/rclcpp/blob/3d58f0fb703d86461c2ab2fb39cd2640dc26e219/rclcpp/src/rclcpp/executor.cpp#L383

This condition make the executor active spin until the time is over if used in spin_all.

I guess 

    if (!wait_result_.has_value() || !exhaustive) {
      // In the case of spin some, then we can exit
      // In the case of spin all, then we will allow ourselves to wait again.
      break;
    }

is the correct implementation.

MichaelOrlov commented 2 months ago

@wjwwood Could you please handle this issue? We decided to assign this issue to you at the latest waffle meeting. However, I don't have permission to change the Assignee field.

wjwwood commented 2 months ago

Small update, I've iterated on the original pr to fix this here: https://github.com/ros2/rclcpp/pull/2517

Waiting to get it reviewed before merging it.

fujitatomoya commented 2 months ago

@wjwwood i will try to taka a look in this week. thanks!

wjwwood commented 2 months ago

Thanks @fujitatomoya, as always more reviews (even post merge reviews) are welcome, but @mjcarroll reviewed it already too.

wjwwood commented 2 months ago

Closed with https://github.com/ros2/rclcpp/pull/2517