rclcpp icon indicating copy to clipboard operation
rclcpp copied to clipboard

Executor::spin_some, spin_all, spin_once are odd function names

Open jmachowinski opened this issue 2 years ago • 3 comments
trafficstars

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 avatar Sep 14 '23 15:09 jmachowinski

@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!

fujitatomoya avatar Sep 14 '23 16:09 fujitatomoya

+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

alsora avatar Sep 14 '23 16:09 alsora

I have a couple of different thoughts about this:

  1. 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.
  2. 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.

clalancette avatar Sep 15 '23 17:09 clalancette