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 67 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.
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.
?
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.