Add Node Options #3
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Closed
ivanpauno
reviewed
Aug 10, 2020
Collaborator
ivanpauno
left a comment
ivanpauno
left a comment
There was a problem hiding this comment.
I haven't reviewed the tests yet.
The overall approach looks good to me, but I would prefer to have a NodeOptions class defined in Java.
That will make the API look nicer.
This should make the code much easier to read. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This makes it a lot nicer to use. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
1. Check return value of malloc. 2. Get rid of unnecessary cast. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Collaborator
Author
|
All right, I ended up doing the minor fixes pointed out by @ivanpauno . I'm going to go ahead and merge this, then rebase #5 on top. Thanks for the reviews! |
ivanpauno
added a commit
to ros2-java/ros2_java
that referenced
this pull request
Nov 16, 2021
* Get the node name and namespace from the native interface. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in node options to nativeCreateNodeHandle. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add in NodeOptions test. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Move argument parsing to its own helper function. This should make the code much easier to read. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Add a NodeOptions class. This makes it a lot nicer to use. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Make sure to DeleteLocalRef after use. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * Fixes from review. 1. Check return value of malloc. 2. Get rid of unnecessary cast. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org> * android compatibility Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com> Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request adds the ability to specify options to the node, much like is done for rclcpp and rclpy. It consists of 3 commits:
getNameandgetNamespaceso that they both get data from the underlying rcl implementation. While this isn't strictly related to this PR, this is more correct and it makes it possible to write tests to ensure that the argument parsing is working as intended.createNode, and ultimately thenativeCreateNodeHandleJNI functions.I will note that this PR changes the
RCLJava::createNodefull signature in non-backwards-compatible ways. I did it that way to more closely mimic the rclcpp API, but I could also be convinced to just append to thecreateNodeparameters, rather than rearranging them.On a separate note, a lot of methods and functions at the JNI layer are being invoked without error checking. This generally leads to a crash of the JVM if an error condition is hit. We should probably have a separate effort to audit those and add error checking as appropriate.
@jacobperron FYI.