-
Notifications
You must be signed in to change notification settings - Fork 574
Fix action cancellation by passing status from JSON #953
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
Conversation
98730e2 to
6d738f8
Compare
|
Meh, I guess CI doesn't set the Since Iron is EOL in like 2 months, I'm kind of okay working around this problem by removing the Iron CI job or continuing to have this test skipped for a bit longer. As people prefer. |
EzraBrooks
left a comment
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.
Do we squash in this repo? if so, we probably should separate the pre-commit changes from the functional changes for commit traceability
Yeah, good idea. Will do! |
d76782c to
5ed480a
Compare
|
Offloaded pre-commit stuff to #954 -- that should be merged before this one to keep history clean. |
0d9209a to
fea1eaa
Compare
EzraBrooks
left a comment
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.
LGTM
Public API Changes
This slightly changes the protocol, specifically that the
action_resultmessage now contains an additionalstatusfield which contains an integer corresponding to the enum in https://docs.ros2.org/latest/api/action_msgs/msg/GoalStatus.html.This does mean that the client libraries (namely,
roslibjswhich has the only working implementation) need to be updated as well.Description
As described above, this PR adds a new
statusfield to theaction_resultmessage to handle action cancellation properly.Closes #920 ... maybe?