Consider renaming rclcpp::ok

[wjwwood]

Discussed names were:

• bool rclcpp::is_shutdown and bool rclcpp::is_not_shutdown
• bool rclcpp::is_shutting_down
• State rclcpp::state and compare to predefined states
  • e.g. while (rclcpp::state() == rclcpp::RUNNING)
  • or while (rclcpp::state() != rclcpp::SHUTDOWN)

[dirk-thomas]

I would only provide a single function (not a second one with inverted return value).

As long as we only need a boolean return value I would favor a function returning a bool (rather then en enum). We can always revisit the API decision when that becomes the case.

Regarding the name is_shutdown sounds best to me (avoiding shutting which implies some ongoing thing).

[Karsten1987]

@wjwwood @dirk-thomas Do you think it's worth iterating over this ticket?

[dirk-thomas]

We should make a decision and then just implement it that way.

[dhood]

To summarise the relationship between rclcpp::ok(), signals, and rclcpp::shutdown() in the current state of the code:

• rclcpp::ok() returning true suggests that a signal has been received (and not cleared).
• The rclcpp signal handler will call the user's signal handler, then call trigger_interrupt_guard_condition.
• trigger_interrupt_guard_condition will wake up waitsets and set the signal received.
• rclcpp::shutdown will call trigger_interrupt_guard_condition and will call whatever on_shutdown callbacks have been registered.

Today it looks that rclcpp::ok() is not directly related to any shutdown processes, it just is usually followed by a shutdown. But not always, as pointed out by @dirk-thomas in this thread:

init()
...
spin()  // waiting for sigint to return from wait
// do some else
spin()  // a second SIGINT while waiting here should wake up the wait again
...
shutdown()

[dhood]

To conclude (wanted to get that post out before the meeting), my opinion is that if we want to rename rclcpp::ok() it should not be to something that implies shutdown, but interruption.

I'd vote for keeping it as rclcpp::ok, or returning a state enum.

I considered rclcpp::signal_received() or (not entirely accurate) rclcpp::interrupted() but both introduce concepts that users are not necessarily familiar with (and don't need to be for the most part).

[dirk-thomas]

I don't think ok is a good name since there is not clear what "ok" / "not ok" stands for. Interrupted seems to describe best what the boolean value currently expresses.

[Karsten1987]

I personally also vote for keeping it as ok. I believe that the snippet like

while rclcpp::ok() {
  // do awesome stuff
  rclcpp::spin();
}

is well established in the ROS world and users know how to handle it. I am not saying, we couldn't convince them to use interrupted instead, I just don't see a strong argument for why we should do it. Just my 2 cents.

[wjwwood]

@Karsten1987 I also think ok is fine to keep, but I don't think your example would ever exist, you'd do this maybe though:

while rclcpp::ok() {
  // do awesome stuff
  rclcpp::spin_once(node);
}

[wjwwood]

I actually don't think that using SIGINT to interrupt spin, only to go back into it is a good pattern to encourage. The idea is that when SIGINT comes, you should be shutting down. I understand the point that this is not strictly the case, but I wouldn't differentiate between the two. If the user wants to handle the signals themselves then that's fine, but I'm speaking about the default behavior.

As @dirk-thomas originally expressed (#3 (comment)), if we change it, I'd prefer is_shutdown, which would be used like this:

while (!rclcpp::is_shutdown()) {
  // do stuff
  rclcpp::spin_once(node);
}

I'll also point out my "aspirational" rclcpp documentation that I try to deal with the "quick shutdown-reinit" case too:

https://github.com/ros2/ros_core_documentation/blob/master/source/rclcpp_cpp_client_library_overview.rst#initialization-and-shutdown

[allenh1]

As @dirk-thomas originally expressed (#3 (comment)), if we change it, I'd prefer is_shutdown

+1