Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Actions proposal #193
Actions proposal #193
Changes from 63 commits
104df38
cc48341
b3edf0a
ded103a
29e0280
cfe4e66
c00f875
68320a4
85dba7d
d3dc0be
da638a9
45a7d0b
b263fd3
508247f
98d2ba5
7782276
23e942b
4efd85e
0d83eb8
8b97ce8
2c7e9e3
2fb606c
95f6ad3
8d9ccbf
d70f38f
1a5d958
bde9d09
224f081
d634a61
4c856b2
244c3f4
63530b5
08c106b
d92fcce
8076d96
5a9ccd0
9ed5359
0172831
ad1dd1f
70b5f8e
8abbb23
69e36b7
1cce6b6
2c104af
d71f0d1
53c0e3c
0f587c0
e12ab46
72894ea
5e6af9c
3404684
9ecd148
d6f7333
e24800a
d7323ec
206f6be
86b2e30
94b9306
9706472
8187434
1e7e6bb
daa529e
5de40c7
887ad87
afaf5f3
7b5144e
77216b1
a6144c7
ad1b326
a439740
1ff406d
b7ac32a
0baa626
d72d100
1c98b9b
f4b59fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps change "progress updates" to "progress feedback" to be more consistent with the naming of things below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after "cancel the request" there probably shouldn't be an "it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, we may consider using the term "goal" here (and elsewhere in the doc) when referring to requests from action clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*and MoveIt!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*"Navigation Stack"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"implementation"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using a UUID may mitigate this, and still allow the client to 'know' the ID of a goal without ever talking to a server. This can be helpful when correlating 'actions' with other libraries/systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable. The original reason for having the server generate the ID is to avoid a race condition where the client may try to use the ID to cancel a goal before it is accepted. But in retrospect, by including the feature to "cancel all goals" or "cancel all goals before timestamp" we can still have a scenario where the cancel request arrives before a goal request. I guess this scenario can be resolved by processing requests as they arrive at the server; if a cancel request arrives first, then any subsequent goal requests are not retroactively canceled. Clients will still be informed that the goal is accepted via the goal response.
In the event that a client tries to use the explicit ID to cancel a goal before it is accepted, the server can reject the request if there are no goals with that ID in a valid state (PENDING or EXECUTING).
Therefore, I see no issue with allowing clients to generate a UUID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You used the name "PENDING/EXECUTING" here -- I would suggest we actually use "ACCEPTED" and "EXECUTING" as the two state names -- it really tells you what stage the goal is in (accepted seems to have a better semantic meaning than pending, pending could mean that handle_goal() has yet to be called or something else. accepted really does state the the action server has acknowledged the goal, but not starting executing it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't expect a goal to be cancellable until it has been accepted. Even if the goal is cancelled between the goal acceptance being transmitted from server to client then the client would still receive a response to its goal request indicating the goal was accepted, followed by a response to its result request indicating the goal was cancelled.
I guess the real problem in ROS 1 isn't the goal id generation, but that
actionlib
doesn't notify the client when a cancel request fails. It seems like the race condition is actually solved by cancel becoming a service.A UUID generated by a client would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the term
actionlib
appears to be deprecated as you only use it twice in this document. please make this explicit, and be consistent with that you call this ROS2 featureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this particular paragraph is describing the race condition in ROS1 -- so actionlib is the correct name -- but the wording should be updated to be more clear. Either break the last 3 sentences of this paragraph into their own paragraph (so they are logically collected), or add ROS1 somewhere in that last sentence so it is clear that it is describing the ROS1 error that is known, and which this change fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"They"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't like this example if its going to be the example, because the solo
dishwasher_id
may be confused with the action ID. how about add another goal input e.g.bool heavy_duty # spend extra time cleaning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove duplicate documentation here from Visibility of Action Services and Topics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes you capitalize "Server" and sometimes you do not, be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem as feedback (https://github.com/ros2/design/pull/193/files#r225264321) with clients receiving data they don't care about. Additionally, there are issues where a 'lost on the wire' status message could cause a failure to transition properly client side.
I think it's really important to consider how this respin of actions is going to work on unreliable networks, as that's one of the major applications (and goals?) of ROS2/DDS.
Reading over this and the 'get_result' section above, would it make sense to have the action server 'push' results and statuses to clients via a service? It would be up to the client to specify a callback where to process the results for any submitted goals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a client will subscribe to this topic. It is meant for introspection only.
Agreed. The QoS settings definitely need to be exposed to the user.
Since ROS 2 services are asynchronous (in
rcl
andrclpy
, exposing this inrclcpp
is tracked in ros2/rclcpp#491) I think the result is the same. Callingget_result
is like asking the server to push the result to the action client in the future. It just happens the action result is communicated as the response to a service call.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying that, I assumed too much carried over from ROS1 actionlib. This picture should be updated (https://github.com/ros2/design/blob/5de40c7802fb39d71ea3489cc6a6ac65e17c61aa/img/actions/interaction_overview.png).
If the result is the same, then unless I'm missing something, the point of having the client call a service to retrieve the result, is to avoid having the result proliferated to every client via topic, otherwise the 'request' portion of
get_result
seems redundant.If that assumption is correct, then there exists a similar problem for the
feedback
comms (proliferation to all connected clients). It seems like it would make sense to use the same approach for both - i.e. the client repeatedly calling aget_feedback
service.Based on https://github.com/ros2/design/pull/193/files#r225325429, it may then make sense to migrate to a 'keyed' topic based on goal id for both
feedback
andresult
once the mechanism is available.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That case wasn't considered. The assumption is that there are usually only a handful of clients (in most cases just one) per server. Are you currently using many action clients per server in ROS 1? If so, how well does it perform?
Services make some parts of the client implementation simpler. The client doesn't need to track the server's state machine.
wait_for_action_server
could reusewait_for_service
. It does have a drawback that for clients who never fetch the result, the server will have to decide how long to hold onto it before discarding it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting to get our 'results' mixed up :)
I've seen low-to-mid double digits clients per server. To some degree that's probably because of how inflexible 'services' were in ROS1, but regardless...
I don't want to advise for over-optimization, and I appreciate that with a central
rcl
implementation of action comms, you have the option to restructure 'later', but it may be worthwhile to make certain scaling-friendly decisions up front. Transmitting N results/feedbacks to M clients over a potentially limited comms channel feels like one of those decisions. Ifget_result
is an acceptable strategy, then maybeget_feedback
is as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the internal workings of an action client doesn't need to know the status of a goal, I can see it being convenient for application developers to have access to the status via the action client API. This means the client would subscribe to the status topic.
Otherwise, assuming the topic is in some hidden namespace, it seems awkward for a developer to create a subscriber to
_action/fibonacci/status
to get the status.If bandwidth/processing is a concern, we could make the status subscriber optional at the C API level. The same could be done for the feedback topic. But I'm leaning towards waiting for the DDS "keyed data" feature that was mentioned in #193 (comment) to be exposed and make use of that to address resource usage concerns, which seems like a more elegant solution IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that "accepted" / "executing" are different between goal states above, and here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add one or two more example sequence diagrams. I wasn't sure if it would be too cluttered to be useful; feedback is welcome on any scenarios that might benefit from a sequence diagram for clarity.