-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add feedback sending capability to RealtimeServerGoalHandle. #18
Conversation
+1 looks decent but someone else should review |
@@ -114,6 +125,10 @@ class RealtimeServerGoalHandle | |||
else | |||
gh_.setSucceeded(); | |||
} | |||
if (req_feedback_) | |||
{ | |||
gh_.publishFeedback(*req_feedback_); |
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.
What happens when req_feedback_
is NULL as per the default value of setFeedback(...)
?
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.
According to my test, just nothing gets published.
@miguelprada everything seems to be fine here, but I didn't notice there wasn't a kinetic-devel branch. Could you please rebase it? I'm ready to merge and should do it today, in the worst case I will cherry pick your changes over there. |
I've merged #19 with these changes, closing. |
@bmagyar Sorry, I didn't see your comment until now. Seems you already handled everything, thanks! |
👍 |
These changes try to make it possible to publish action feedback through the RealtimeServerGoalHandle, mimicking the way it's done with the action result, as discussed in ros-controls/ros_controllers#173.
I'm afraid that there might be some concurrency issues if the realtime thread is writing new feedback values and the non-realtime timer calls runNonRealtime at the same time, but I unfortunately don't have the time and/or knowledge to handle this properly. Any suggestions are welcome here.