Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Complete action server implementation #65

Merged
merged 21 commits into from
May 7, 2021
Merged

Complete action server implementation #65

merged 21 commits into from
May 7, 2021

Conversation

ivanpauno
Copy link
Collaborator

@ivanpauno ivanpauno commented Apr 28, 2021 •
edited
Loading

I have made some progress in the action server implementation.
There are still some things missing, but it would be good to get early feedback on some details:

  • Modified GoalHandle interface so that terminal state transitions take a result message as a parameter (e.g. goalHandle.execute(result)).
  • Added a publishFeedback() method to GoalHandle (not implemented yet).
  • (partially) Handle result requests (not tested yet).
  • Added some interfaces to improve type safety: ResultRequestDefinition, ResultResponseDefinition, FeedbackDefinition, ResultDefinition, GoalDefinition
  • Modified message generation for implement those interfaces when needed.
  • Refactored GoalStatus to be able to get the byte equivalent easily.
  • Created some helper methods to avoid repeating code (createGoalInfo, goalExists, sendResultResponse)

TODO:

@ivanpauno
* GoalHandle transitions should be able to set a result.
d4cb0ef 
* Add interfaces for feedback/goal/result, in order to make the API more type safe.
* Modify rcljava rosidl generator to implement the previous interfaces.
* Refactor GoalStatus enumeration.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Add interface for ResultRequestDefinition
cba6bbc 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Handle result request. Made some progress in how to handle goal handl…
f12d7fc 
…e terminal transitions. Added some helper methods

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the enhancement New feature or request label Apr 28, 2021
@ivanpauno ivanpauno self-assigned this Apr 28, 2021
@ivanpauno
Copy link
Collaborator Author

@jacobperron this is not urgent, but some early feedback would be appreciated.

jacobperron
jacobperron reviewed May 3, 2021
View reviewed changes
Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Outdated Show resolved Hide resolved
rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Show resolved Hide resolved
rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Show resolved Hide resolved
rcljava/src/main/java/org/ros2/rcljava/action/GoalStatus.java Show resolved Hide resolved
@ivanpauno
Return early if goal request take failed
b6f247a 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Don't set goal info stamp
88346c2 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Cleanup goal handle status transition bindings
3c6b8f6 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Take advantage of nested class correctly
1f6f2a3 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Implement publishResult
c27f8f5 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Publish status after terminal state
6e6ee6d 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Add binding for notify_goal_done, fixup publish goal status
60dd0fa 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Copy link
Collaborator Author

I have added a bunch of new commits as well

@ivanpauno
Handle expired goals
861a300 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Improve type safety a bit further
256be60 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
It wasn't that easy 😂
a0ee248 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Delete random debugging log
dc1acd3 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno
Fix fun bug
f324037 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno mentioned this pull request May 4, 2021
Open
@ivanpauno
Copy link
Collaborator Author

This is almost complete now, see example code here (which works!).
Remaining TODOs are tracked in the OP.

@ivanpauno ivanpauno requested a review from jacobperron May 4, 2021 18:54
@ivanpauno
Add support for publishing feedback in action server
614b2a6 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno marked this pull request as ready for review May 5, 2021 21:33
@ivanpauno
Copy link
Collaborator Author

I think this is ready for review now.
I will address the remaining TODOs in new PRs.

jacobperron
jacobperron reviewed May 6, 2021
View reviewed changes
Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

rcljava/include/org_ros2_rcljava_action_ActionServerImpl.h Outdated Show resolved Hide resolved
rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated Show resolved Hide resolved
rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated Show resolved Hide resolved
rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated Show resolved Hide resolved
rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated Show resolved Hide resolved
rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl_GoalHandleImpl.cpp Outdated Show resolved Hide resolved
rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Show resolved Hide resolved
rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Outdated Show resolved Hide resolved
rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Outdated Show resolved Hide resolved
rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Show resolved Hide resolved
ivanpauno and others added 3 commits May 6, 2021 16:49
@ivanpauno @jacobperron
Fix nativePublishFeedbackMessage signature
c9056b5 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno @jacobperron
Delete extra space
2ccc202 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno @jacobperron
Test for event >= GOAL_EVENT_NUM_EVENTS
0ce939c 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@ivanpauno
Address peer review comments
e0cc5e5 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno requested a review from jacobperron May 6, 2021 20:32
jacobperron
jacobperron approved these changes May 6, 2021
View reviewed changes
Copy link

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ivanpauno
please linters
04417eb 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno merged commit 4914242 into galactic-devel May 7, 2021
ivanpauno added a commit that referenced this pull request May 17, 2021
@ivanpauno @jacobperron
Complete action server implementation (#65)
eb83d68 
* GoalHandle transitions should be able to set a result.
* Refactor GoalStatus enumeration.
* Add interface for ResultRequestDefinition, ResultResponseDefinition, FeedbackDefinition, GoalDefinition, ResultDefinition.
* Modify rcljava rosidl generator to implement those interfaces.
* Handle result requests.
* Handle goal handle terminal transitions.
* Cleanup goal handle status transition bindings.
* Take advantage of nested class.
* Publish status after terminal state transition.
* Handle expire goals.
* Add support for publishing feedback.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@jacobperron jacobperron mentioned this pull request Nov 13, 2021
Open
4 tasks
jacobperron added a commit to ros2-java/ros2_java that referenced this pull request May 17, 2022
@ivanpauno @jacobperron
Complete action server implementation (osrf#65)
9577ed8 
* GoalHandle transitions should be able to set a result.
* Refactor GoalStatus enumeration.
* Add interface for ResultRequestDefinition, ResultResponseDefinition, FeedbackDefinition, GoalDefinition, ResultDefinition.
* Modify rcljava rosidl generator to implement those interfaces.
* Handle result requests.
* Handle goal handle terminal transitions.
* Cleanup goal handle status transition bindings.
* Take advantage of nested class.
* Publish status after terminal state transition.
* Handle expire goals.
* Add support for publishing feedback.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobperron jacobperron jacobperron approved these changes

Assignees

@ivanpauno ivanpauno

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivanpauno @jacobperron