ros2 / design

Design documentation for ROS 2.0 effort
http://design.ros2.org/
Apache License 2.0
218 stars 193 forks source link

Update action goal state diagram #226

Closed jacobperron closed 5 years ago

jacobperron commented 5 years ago

Related to changes happening for ros2/rcl#399.

jacobperron commented 5 years ago

Bummer we can't keep cancel_goal, it was more consistent with send_goal

We could keep it, although, IMO, "request_cancel" is more clear.

dirk-thomas commented 5 years ago

I am not sure the "request" in the name is better. With any service your request is just that - a request - and the response could be "no, not going to do it". Therefore I think cancel_goal was the better name and doesn't need to be changed.

jacobperron commented 5 years ago

I am not sure the "request" in the name is better. With any service your request is just that - a request - and the response could be "no, not going to do it". Therefore I think cancel_goal was the better name and doesn't need to be changed.

We should distinguish the cancel request event from the event where the goal is actually canceled (see https://github.com/ros2/rcl/issues/399#issuecomment-478193156). I think the events "cancel_goal" and "cancel" are more ambiguous than "request_cancel" and "cancel". In any case, it's probably not helping that we're conflating the user request to cancel and the goal state transition.

dirk-thomas commented 5 years ago

We should distinguish the cancel request event from the event where the goal is actually canceled (see ros2/rcl#399 (comment)). I think the events "cancel_goal" and "cancel" are more ambiguous than "request_cancel" and "cancel". In any case, it's probably not helping that we're conflating the user request to cancel and the goal state transition.

From the wording "cancel" alone it is again not clear if that is just a request, it is about to happen or it is already done.

I would argue that the natural language already has a clean way to distinguish these two cases:

jacobperron commented 5 years ago

I've reverted the change to the "cancel_goal" transition and renamed the "cancel" transition to "canceled" as per @dirk-thomas's suggestion. I've updated the state diagram accordingly.

lbegani commented 5 years ago

I've reverted the change to the "cancel_goal" transition and renamed the "cancel" transition to "canceled" as per @dirk-thomas's suggestion. I've updated the state diagram accordingly.

Typo: Should it not be cancelled instead of canceled

clalancette commented 5 years ago

Typo: Should it not be cancelled instead of canceled

It depends on which flavor of English you learned: https://www.grammarly.com/blog/canceled-vs-cancelled/

hidmic commented 5 years ago

Though if I get really picky, I find the use of canceled, an adjective, along with abort or succeed, both verbs, a bit inconsistent. But I can't think of a better naming right now.

dirk-thomas commented 5 years ago

I find the use of canceled, an adjective, along with abort or succeed, both verbs, a bit inconsistent.

Could be changed to succeeded / aborted?

hidmic commented 5 years ago

Could be changed to succeeded / aborted?

I'm somewhat inclined towards verbs, but yeah, that's the alternative. Anyways, it isn't super important.

dirk-thomas commented 5 years ago

I'm somewhat inclined towards verbs

Using verbs to describe a status seems off to me.

hidmic commented 5 years ago

Using verbs to describe a status seems off to me.

Agreed, but IIUC it's not the status or state but the event that we're describing here.

jacobperron commented 5 years ago

Agreed, but IIUC it's not the status or state but the event that we're describing here.

Correct. These are the names of the transitions between states. I believe the word "canceled" can be used as an adjective and as a verb.