Skip to content

Complete action server implementation#65

Merged
ivanpauno merged 21 commits into
galactic-develfrom
ivanpauno/action-server
May 7, 2021
Merged

Complete action server implementation#65
ivanpauno merged 21 commits into
galactic-develfrom
ivanpauno/action-server

Conversation

@ivanpauno
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

ivanpauno commented Apr 28, 2021

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

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

@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!

Comment thread rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Outdated
Comment thread rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java
Comment thread rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java
Comment thread rcljava/src/main/java/org/ros2/rcljava/action/GoalStatus.java
ivanpauno added 7 commits May 3, 2021 16:59
@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
Copy Markdown
Collaborator Author

ivanpauno commented May 3, 2021

I have added a bunch of new commits as well

ivanpauno added 5 commits May 4, 2021 11:36
@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
Copy Markdown
Collaborator Author

ivanpauno commented May 4, 2021

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
Copy Markdown
Collaborator Author

ivanpauno commented May 5, 2021

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
Copy Markdown

@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!

Comment thread rcljava/include/org_ros2_rcljava_action_ActionServerImpl.h Outdated
Comment thread rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated
Comment thread rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated
Comment thread rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated
Comment thread rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl.cpp Outdated
Comment thread rcljava/src/main/cpp/org_ros2_rcljava_action_ActionServerImpl_GoalHandleImpl.cpp Outdated
Comment thread rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java
Comment thread rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Outdated
Comment thread rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java Outdated
Comment thread rcljava/src/main/java/org/ros2/rcljava/action/ActionServerImpl.java
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
Copy Markdown

@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

1 more reviewer

@jacobperron jacobperron jacobperron approved these changes

Reviewers whose approvals may not affect merge requirements

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