Consider renaming rclcpp::ok about rclcpp HOT 15 CLOSED

@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);
}

from rclcpp.

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).

from rclcpp.

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

from rclcpp.

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

from rclcpp.

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()

from rclcpp.

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).

from rclcpp.

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.

from rclcpp.

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.

from rclcpp.

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

from rclcpp.

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

+1

from rclcpp.

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.

Since interruption doesn't trigger shutdown, I'm not convinced it makes sense to conflate them so explicitly. That doesn't mean we have to differentiate interruption and shutting down, though: !ok is nice and vague in that sense 😄

As an example of why conflating them is confusing, consider the case where you want to explicitly call shutdown before a main exit, e.g. to deregister our signal handler before exiting, or because you want to call init again (not that it's currently possible). You'd have:

while (!rclcpp::is_shutdown()) {  // or is_shutting_down, either way
  // do stuff
  rclcpp::spin_once(node);
}
rclcpp::shutdown();

which, to me, is confusing to see, even if it's not the default usage.

I'm not aware of any plans in the future to ever have our signal handler explicitly call shutdown (if there are, then that would change my opinion). However, from the docs you linked to @wjwwood, there is a direct relationship between rclcpp::ok and rclcpp::shutdown that is not captured in the current state of the code: "In order to test whether or not :cpp:func:rclcpp::shutdown has been called, the :cpp:func:rclcpp::ok function can be used".

I can appreciate that it's an aspirational document so my point isn't that it doesn't match the current state of the code today, but that I think there's generally a difference of opinion about what rclcpp::ok should represent, which is why there's a difference of opinion about the name it should have.

Today rclcpp::ok() has nothing to do with rclcpp being shutdown, but are there plans for that to change in the future?

from rclcpp.

Since interruption doesn't trigger shutdown, I'm not convinced it makes sense to conflate them so explicitly.

If that's the case then I'd say it's mostly an oversight, since that is what ROS 1 does, e.g. see the recommendation for a custom signal handler:

http://wiki.ros.org/roscpp/Overview/Initialization%20and%20Shutdown#Custom_SIGINT_Handler

Also from that page:

By default roscpp also installs a SIGINT handler which will detect Ctrl-C and automatically shutdown for you.

So I think that's worth emulating, or at least I don't know why would change that (maybe I just implemented it incorrectly in rclcpp).

from rclcpp.

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

Not sure if this is still being looked at, but I personally think rclcpp::ok() is better because it's "positive" as opposed to rclcpp::is_shutdown() which is "negative" for most use cases and requires user to negate it to get the desired effect.

I don't remember where I read it, but a general best practice is to make bools (or functions that return them) "positive" instead of "negative" to simplify use. I've found this extremely helpful, especially when combining them into more complex conditions and not having to work through all the negations in my head. So if you do decide to move away from rclcpp::ok() (which is think is perfectly fine as is) I would suggest something like rclcpp::should_run() or rclcpp::should_spin() or something similar.

On a personal library, I have a thread wrapper class, and have function called request_quit() which sets a bool and associated function is_quit_requested() which the managed thread can query to know if it should shutdown. I hate the is_quit_requested() function name because in every thread that uses it I always have to do the mental gymnastics to remember where to include the negations. I wish I had made it should_run()

Only reason I mention is I think there's a bug in the documentation for rclcpp::ok on the return value. It seems opposite of what it should be.

from rclcpp.

Given that:

1. rclcpp::ok is close to what was in ROS 1 as ros::ok.
2. This bug was opened in 2014
3. The last time we seriously discussed it was in 2018

I think it is safe to say that we don't have any plans to do this. I'm going to close this out for now, but feel free to reopen if you disagree.

Only reason I mention is I think there's a bug in the documentation for rclcpp::ok on the return value. It seems opposite of what it should be.

Yes, you are right. The documentation sense is inverted; I'll open a PR to fix that. Thanks for the heads up.

from rclcpp.

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/what-is-the-expected-behavior-of-rclcpp-in-case-of-an-exception-raised-in-a-user-callback/27527/1

from rclcpp.