rclcpp
rclcpp copied to clipboard
Executor::spin_some, spin_all, spin_once are odd function names
Executor::spin_some, Executor::spin_all and Executor::spin_once are odd function names, as they do not give a good idea of what the functions intention is. Multiple developers in my company ran into this issue, and used the wrong functions for their use case, especially spin_some and spin_all are confusing. Therefore I would vote to deprecate these and replace them by
something like :
void execute_all_ready_events();
void execute_one_ready_event();
void execute_all_ready_events_for();
void execute_all_ready_events_until();
void execute_for();
void execute_until();
@jmachowinski thanks for posting the proposal.
I am not necessarily against changing names, but i would check the doc even with proposed names are provided. lets see more feedbacks on this!
+1 on changing names. People not familiar with ROS are often scaried by the term "spin" which in some context indicates a busy loop where you never sleep and keep polling for events https://en.wikipedia.org/wiki/Busy_waiting
I have a couple of different thoughts about this:
- The
execute_nomenclature isn't really correct either. The functions here don't necessarily execute anything. They wait for events to happen, and then based on what kind of event it is (and which variant is called), react to those events. - A deprecation like this is going to be very painful. These functions are used absolutely everywhere, all of which will have to be updated. That doesn't mean we shouldn't do it, but whoever decides to take it on will have a long road ahead of them.
One other thing that I think we should consider here is whether we can actually remove some of these variants. Things like spin_once have extremely surprising semantics unless you are well-versed in how executors work. We could also consider whether some of the other variants can be removed (or combined). I'm not sure whether that would be possible, but I think that would go a long way towards making this easier to understand.