-
Notifications
You must be signed in to change notification settings - Fork 259
Allow action servers without execute callback #1219
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
base: rolling
Are you sure you want to change the base?
Conversation
d6f14c9 to
0dcf078
Compare
ac385b9 to
d6163dc
Compare
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.
@Timple thanks for the contribution.
i think this is reasonable and aligns with rclcpp, but i would like to have another review from maintainers.
|
Alright, we'll wait for those 🙂 |
|
Subtle ping 🙂 |
|
Sorry for subtle bump again, but Jade is coming up. And I'm afraid breaking changes won't be accepted anymore soon. |
|
@clalancette @sloretz @ahcorde either of you, can you take a look at this? i think this is reasonable and more flexibility for rclpy client. |
|
@Timple can you rebase this to rolling? almost 100 commits are behind. |
d6163dc to
0cf2d64
Compare
Done! |
Signed-off-by: Tim Clephas <[email protected]>
Signed-off-by: Tim Clephas <[email protected]>
Signed-off-by: Tim Clephas <[email protected]>
0cf2d64 to
26a7fdc
Compare
Signed-off-by: Tim Clephas <[email protected]>
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: |
|
So I missed the But this PR had some proper traction. Can we still get this in rolling? If everybody is very busy doing release stuff for jazzy, I also fully understand. Then I'll apply some sort of exponential backoff to my pings 🙂 |
|
It's green! 😄 |
|
@sloretz @clalancette friendly ping. |
|
Joehoe ❤️ |
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 know the status of this PR or if this is still relevant, but I can see some conflicts
|
Well... Only if I can get some guarantee that it will be merged any time soon after fixing it :D |
Sure, I will review it |
|
@Mergifyio rebase |
☑️ Nothing to do, the required conditions are not met
|
|
That would have been great if that fixed it :). The rebase should be fairly trivial but I won't get to this until next week. So if someone wants to give it a go.. |
Just like rclcpp and ros1, allow action servers without execute callback.
Example like:
are typical cases where an action is succeeded based on information outside of the execute_cb.
It makes more sense to succeed these from outside the execute_cb in this case. And this would be in line with ros1 and rclcpp where this is possible.
I kept the
execute_callbackinargsfor backwards compatibility.