From 93236f4624f3062802d34a5e3c1d846076e7b311 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 4 Aug 2020 16:27:21 +0000 Subject: [PATCH 1/7] Get the node name and namespace from the native interface. Signed-off-by: Chris Lalancette --- .../include/org_ros2_rcljava_node_NodeImpl.h | 19 ++++++++++++++ .../cpp/org_ros2_rcljava_node_NodeImpl.cpp | 14 +++++++++++ .../main/java/org/ros2/rcljava/RCLJava.java | 2 +- .../main/java/org/ros2/rcljava/node/Node.java | 13 ++++++++++ .../java/org/ros2/rcljava/node/NodeImpl.java | 25 +++++++++++++++---- 5 files changed, 67 insertions(+), 6 deletions(-) diff --git a/rcljava/include/org_ros2_rcljava_node_NodeImpl.h b/rcljava/include/org_ros2_rcljava_node_NodeImpl.h index 181c0c30..40d40867 100644 --- a/rcljava/include/org_ros2_rcljava_node_NodeImpl.h +++ b/rcljava/include/org_ros2_rcljava_node_NodeImpl.h @@ -20,6 +20,25 @@ #ifdef __cplusplus extern "C" { #endif + +/* + * Class: org_ros2_rcljava_node_NodeImpl + * Method: nativeGetName + * Signature: (J)Ljava/lang/String + */ +JNIEXPORT jstring +JNICALL Java_org_ros2_rcljava_node_NodeImpl_nativeGetName( + JNIEnv * env, jclass, jlong node_handle); + +/* + * Class: org_ros2_rcljava_node_NodeImpl + * Method: nativeGetNamespace + * Signature: (J)Ljava/lang/String + */ +JNIEXPORT jstring +JNICALL Java_org_ros2_rcljava_node_NodeImpl_nativeGetNamespace( + JNIEnv * env, jclass, jlong node_handle); + /* * Class: org_ros2_rcljava_node_NodeImpl * Method: nativeCreatePublisherHandle diff --git a/rcljava/src/main/cpp/org_ros2_rcljava_node_NodeImpl.cpp b/rcljava/src/main/cpp/org_ros2_rcljava_node_NodeImpl.cpp index 09848339..868fc2e4 100644 --- a/rcljava/src/main/cpp/org_ros2_rcljava_node_NodeImpl.cpp +++ b/rcljava/src/main/cpp/org_ros2_rcljava_node_NodeImpl.cpp @@ -31,6 +31,20 @@ using rcljava_common::exceptions::rcljava_throw_rclexception; +JNIEXPORT jstring JNICALL +Java_org_ros2_rcljava_node_NodeImpl_nativeGetName( + JNIEnv * env, jclass, jlong node_handle) +{ + return env->NewStringUTF(rcl_node_get_name(reinterpret_cast(node_handle))); +} + +JNIEXPORT jstring JNICALL +Java_org_ros2_rcljava_node_NodeImpl_nativeGetNamespace( + JNIEnv * env, jclass, jlong node_handle) +{ + return env->NewStringUTF(rcl_node_get_namespace(reinterpret_cast(node_handle))); +} + JNIEXPORT jlong JNICALL Java_org_ros2_rcljava_node_NodeImpl_nativeCreatePublisherHandle( JNIEnv * env, jclass, jlong node_handle, jclass jmessage_class, jstring jtopic, diff --git a/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java b/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java index ac1cb21b..a8c85f31 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java +++ b/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java @@ -251,7 +251,7 @@ public static Node createNode(final String nodeName, final String namespace, fin public static Node createNode(final String nodeName, final String namespace, final Context context, final boolean allowUndeclaredParameters) { long nodeHandle = nativeCreateNodeHandle(nodeName, namespace, context.getHandle()); - Node node = new NodeImpl(nodeHandle, nodeName, context, allowUndeclaredParameters); + Node node = new NodeImpl(nodeHandle, context, allowUndeclaredParameters); nodes.add(node); return node; } diff --git a/rcljava/src/main/java/org/ros2/rcljava/node/Node.java b/rcljava/src/main/java/org/ros2/rcljava/node/Node.java index e5d875cb..f413ab04 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/node/Node.java +++ b/rcljava/src/main/java/org/ros2/rcljava/node/Node.java @@ -175,8 +175,21 @@ Client createClient(final Class serviceType, WallTimer createWallTimer(final long period, final TimeUnit unit, final Callback callback); + /** Get the name of the node. + * + * @return The name of the node. + */ String getName(); + /** Get the namespace of the node. + * + * This namespace is the "node's" namespace, and therefore is not affected + * by any sub-namespace's that may affect entities created with this instance. + * + * @return The namespace of the node. + */ + String getNamespace(); + /** * Declare and initialize a parameter, return the effective value. * diff --git a/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java b/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java index 7a8041c6..0189e302 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java +++ b/rcljava/src/main/java/org/ros2/rcljava/node/NodeImpl.java @@ -121,8 +121,6 @@ public class NodeImpl implements Node { */ private final Collection timers; - private final String name; - private Object parametersMutex; class ParameterAndDescriptor { @@ -142,10 +140,9 @@ class ParameterAndDescriptor { * @param handle A pointer to the underlying ROS2 node structure. Must not * be zero. */ - public NodeImpl(final long handle, final String name, final Context context, final boolean allowUndeclaredParameters) { + public NodeImpl(final long handle, final Context context, final boolean allowUndeclaredParameters) { this.clock = new Clock(ClockType.SYSTEM_TIME); this.handle = handle; - this.name = name; this.context = context; this.publishers = new LinkedBlockingQueue(); this.subscriptions = new LinkedBlockingQueue(); @@ -159,6 +156,20 @@ public NodeImpl(final long handle, final String name, final Context context, fin this.parameterCallbacks = new ArrayList(); } + /** + * Get the ROS 2 node name. + * @param handle A pointer to the underlying ROS2 node structure. + * @return The name of the node. + */ + private static native String nativeGetName(long handle); + + /** + * Get the ROS 2 node namespace. + * @param handle A pointer to the underlying ROS2 node structure. + * @return The namespace of the node. + */ + private static native String nativeGetNamespace(long handle); + /** * Create a ROS2 publisher (rcl_publisher_t) and return a pointer to * it as an integer. @@ -401,7 +412,11 @@ public final Collection getTimers() { * {@inheritDoc} */ public final String getName() { - return this.name; + return nativeGetName(this.handle); + } + + public final String getNamespace() { + return nativeGetNamespace(this.handle); } public ParameterVariant declareParameter(ParameterVariant parameter) { From 350e9a8fd64ba7b88424c449b2911b1a030f1fe5 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Wed, 22 Jul 2020 20:45:50 +0000 Subject: [PATCH 2/7] Add in node options to nativeCreateNodeHandle. Signed-off-by: Chris Lalancette --- rcljava/include/org_ros2_rcljava_RCLJava.h | 2 +- .../src/main/cpp/org_ros2_rcljava_RCLJava.cpp | 66 +++++++++++++++++-- .../main/java/org/ros2/rcljava/RCLJava.java | 11 ++-- .../node/NodeUndeclaredParametersTest.java | 2 +- 4 files changed, 68 insertions(+), 13 deletions(-) diff --git a/rcljava/include/org_ros2_rcljava_RCLJava.h b/rcljava/include/org_ros2_rcljava_RCLJava.h index 5ef6aff1..4d8d1b50 100644 --- a/rcljava/include/org_ros2_rcljava_RCLJava.h +++ b/rcljava/include/org_ros2_rcljava_RCLJava.h @@ -35,7 +35,7 @@ JNICALL Java_org_ros2_rcljava_RCLJava_nativeCreateContextHandle(JNIEnv *, jclass */ JNIEXPORT jlong JNICALL Java_org_ros2_rcljava_RCLJava_nativeCreateNodeHandle( - JNIEnv *, jclass, jstring, jstring, jlong); + JNIEnv *, jclass, jstring, jstring, jlong, jobject, jboolean, jboolean); /* * Class: org_ros2_rcljava_RCLJava diff --git a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp index 848ce610..65b6c10b 100644 --- a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp +++ b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp @@ -46,7 +46,8 @@ Java_org_ros2_rcljava_RCLJava_nativeCreateContextHandle(JNIEnv *, jclass) JNIEXPORT jlong JNICALL Java_org_ros2_rcljava_RCLJava_nativeCreateNodeHandle( - JNIEnv * env, jclass, jstring jnode_name, jstring jnamespace, jlong context_handle) + JNIEnv * env, jclass, jstring jnode_name, jstring jnamespace, jlong context_handle, + jobject cli_args, jboolean use_global_arguments, jboolean enable_rosout) { const char * node_name_tmp = env->GetStringUTFChars(jnode_name, 0); std::string node_name(node_name_tmp); @@ -58,20 +59,73 @@ Java_org_ros2_rcljava_RCLJava_nativeCreateNodeHandle( rcl_context_t * context = reinterpret_cast(context_handle); + // TODO(clalancette): we could probably make this faster by just doing these + // method lookups during some initialization time. But we'd have to add a lot + // more infrastructure for that, and we don't expect to call + // 'nativeCreateNodeHandle' that often, so I think this is OK for now. + jclass java_util_ArrayList = + static_cast(env->NewGlobalRef(env->FindClass("java/util/ArrayList"))); + jmethodID java_util_ArrayList_size = env->GetMethodID(java_util_ArrayList, "size", "()I"); + jmethodID java_util_ArrayList_get = env->GetMethodID( + java_util_ArrayList, "get", "(I)Ljava/lang/Object;"); + + rcl_arguments_t arguments = rcl_get_zero_initialized_arguments(); + char ** argv = NULL; + jint argc = env->CallIntMethod(cli_args, java_util_ArrayList_size); + argv = static_cast(malloc(argc * sizeof(char *))); + for (jint i = 0; i < argc; ++i) { + jstring element = + static_cast(env->CallObjectMethod(cli_args, java_util_ArrayList_get, i)); + argv[i] = const_cast(env->GetStringUTFChars(element, nullptr)); + } + rcl_ret_t ret = rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &arguments); + for (jint i = 0; i < argc; ++i) { + jstring element = + static_cast(env->CallObjectMethod(cli_args, java_util_ArrayList_get, i)); + env->ReleaseStringUTFChars(element, argv[i]); + } + free(argv); + if (ret != RCL_RET_OK) { + std::string msg = "Failed to parse node arguments: " + std::string(rcl_get_error_string().str); + rcl_reset_error(); + rcljava_throw_rclexception(env, ret, msg); + return 0; + } + rcl_node_t * node = static_cast(malloc(sizeof(rcl_node_t))); *node = rcl_get_zero_initialized_node(); - rcl_node_options_t default_options = rcl_node_get_default_options(); - rcl_ret_t ret = rcl_node_init( - node, node_name.c_str(), namespace_.c_str(), context, &default_options); + rcl_node_options_t options = rcl_node_get_default_options(); + options.use_global_arguments = use_global_arguments; + options.arguments = arguments; + options.enable_rosout = enable_rosout; + ret = rcl_node_init(node, node_name.c_str(), namespace_.c_str(), context, &options); if (ret != RCL_RET_OK) { std::string msg = "Failed to create node: " + std::string(rcl_get_error_string().str); rcl_reset_error(); + free(node); + if (rcl_arguments_fini(&arguments) != RCL_RET_OK) { + // We are already throwing an exception, just ignore the return here + } rcljava_throw_rclexception(env, ret, msg); return 0; } - jlong node_handle = reinterpret_cast(node); - return node_handle; + + ret = rcl_arguments_fini(&arguments); + if (ret != RCL_RET_OK) { + // We failed to cleanup + std::string msg = "Failed to cleanup after creating node: " + std::string( + rcl_get_error_string().str); + rcl_reset_error(); + if (rcl_node_fini(node) != RCL_RET_OK) { + // We are already throwing an exception, just ignore the return here + } + free(node); + rcljava_throw_rclexception(env, ret, msg); + return 0; + } + + return reinterpret_cast(node); } JNIEXPORT jstring JNICALL diff --git a/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java b/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java index a8c85f31..e8e83fa3 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java +++ b/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java @@ -18,6 +18,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.ArrayList; import java.util.Collection; import java.util.Map; import java.util.concurrent.ConcurrentSkipListMap; @@ -177,7 +178,7 @@ public static synchronized void rclJavaInit() { * @param contextHandle Pointer to a context (rcl_context_t) with which to associated the node. * @return A pointer to the underlying ROS2 node structure. */ - private static native long nativeCreateNodeHandle(String nodeName, String namespace, long contextHandle); + private static native long nativeCreateNodeHandle(String nodeName, String namespace, long contextHandle, ArrayList arguments, boolean useGlobalArguments, boolean enableRosout); /** * @return The identifier of the currently active RMW implementation via the @@ -234,7 +235,7 @@ public static Context createContext() { * structure. */ public static Node createNode(final String nodeName) { - return createNode(nodeName, "", RCLJava.getDefaultContext(), false); + return createNode(nodeName, "", RCLJava.getDefaultContext(), true, true, new ArrayList(), false); } /** @@ -246,11 +247,11 @@ public static Node createNode(final String nodeName) { * structure. */ public static Node createNode(final String nodeName, final String namespace, final Context context) { - return createNode(nodeName, namespace, context, false); + return createNode(nodeName, namespace, context, true, true, new ArrayList(), false); } - public static Node createNode(final String nodeName, final String namespace, final Context context, final boolean allowUndeclaredParameters) { - long nodeHandle = nativeCreateNodeHandle(nodeName, namespace, context.getHandle()); + public static Node createNode(final String nodeName, final String namespace, final Context context, final boolean useGlobalArguments, final boolean enableRosout, final ArrayList cliArgs, final boolean allowUndeclaredParameters) { + long nodeHandle = nativeCreateNodeHandle(nodeName, namespace, context.getHandle(), cliArgs, useGlobalArguments, enableRosout); Node node = new NodeImpl(nodeHandle, context, allowUndeclaredParameters); nodes.add(node); return node; diff --git a/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java b/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java index d4004873..35ef868f 100644 --- a/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java +++ b/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java @@ -53,7 +53,7 @@ public static void tearDownOnce() { @Before public void setUp() { - node = RCLJava.createNode("test_node", "", RCLJava.getDefaultContext(), true); + node = RCLJava.createNode("test_node", "", RCLJava.getDefaultContext(), true, true, new ArrayList(), true); } @After From 09d6feb5ed43be7515f1ef2a35278c89a3647a0e Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 4 Aug 2020 16:27:55 +0000 Subject: [PATCH 3/7] Add in NodeOptions test. Signed-off-by: Chris Lalancette --- rcljava/CMakeLists.txt | 2 + .../ros2/rcljava/node/NodeOptionsTest.java | 54 +++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java diff --git a/rcljava/CMakeLists.txt b/rcljava/CMakeLists.txt index 684f2275..bb150da7 100644 --- a/rcljava/CMakeLists.txt +++ b/rcljava/CMakeLists.txt @@ -229,6 +229,7 @@ if(BUILD_TESTING) "src/test/java/org/ros2/rcljava/SpinTest.java" "src/test/java/org/ros2/rcljava/TimeTest.java" # "src/test/java/org/ros2/rcljava/client/ClientTest.java" + "src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java" "src/test/java/org/ros2/rcljava/node/NodeParametersTest.java" "src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java" "src/test/java/org/ros2/rcljava/node/NodeTest.java" @@ -244,6 +245,7 @@ if(BUILD_TESTING) "org.ros2.rcljava.SpinTest" "org.ros2.rcljava.TimeTest" # "org.ros2.rcljava.client.ClientTest" + "org.ros2.rcljava.node.NodeOptionsTest" "org.ros2.rcljava.node.NodeParametersTest" "org.ros2.rcljava.node.NodeUndeclaredParametersTest" "org.ros2.rcljava.node.NodeTest" diff --git a/rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java b/rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java new file mode 100644 index 00000000..121adad2 --- /dev/null +++ b/rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java @@ -0,0 +1,54 @@ +/* Copyright 2020 Open Source Robotics Foundation, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.ros2.rcljava.node; + +import static org.junit.Assert.assertEquals; + +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.Arrays; + +import org.ros2.rcljava.RCLJava; +import org.ros2.rcljava.node.Node; + +public class NodeOptionsTest { + @BeforeClass + public static void setupOnce() throws Exception { + // Just to quiet down warnings + org.apache.log4j.BasicConfigurator.configure(); + + RCLJava.rclJavaInit(); + } + + @AfterClass + public static void tearDownOnce() { + RCLJava.shutdown(); + } + + @Test + public final void testCreateNodeWithArgs() { + ArrayList args = new ArrayList(Arrays.asList("--ros-args", "-r", "__ns:=/foo")); + //ArrayList args = new ArrayList(); + Node node = RCLJava.createNode("test_node", "", RCLJava.getDefaultContext(), true, true, args, false); + assertEquals("test_node", node.getName()); + assertEquals("/foo", node.getNamespace()); + + node.dispose(); + } +} From e69305583b1b279bd9438031aa1e3662615164f8 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 10 Aug 2020 18:04:33 +0000 Subject: [PATCH 4/7] Move argument parsing to its own helper function. This should make the code much easier to read. Signed-off-by: Chris Lalancette --- .../src/main/cpp/org_ros2_rcljava_RCLJava.cpp | 55 +++++++++++-------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp index 65b6c10b..492bebc5 100644 --- a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp +++ b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include "rcl/error_handling.h" #include "rcl/node.h" @@ -44,51 +45,59 @@ Java_org_ros2_rcljava_RCLJava_nativeCreateContextHandle(JNIEnv *, jclass) return context_handle; } -JNIEXPORT jlong JNICALL -Java_org_ros2_rcljava_RCLJava_nativeCreateNodeHandle( - JNIEnv * env, jclass, jstring jnode_name, jstring jnamespace, jlong context_handle, - jobject cli_args, jboolean use_global_arguments, jboolean enable_rosout) +bool parse_arguments(JNIEnv * env, jobject cli_args, rcl_arguments_t * arguments) { - const char * node_name_tmp = env->GetStringUTFChars(jnode_name, 0); - std::string node_name(node_name_tmp); - env->ReleaseStringUTFChars(jnode_name, node_name_tmp); - - const char * namespace_tmp = env->GetStringUTFChars(jnamespace, 0); - std::string namespace_(namespace_tmp); - env->ReleaseStringUTFChars(jnamespace, namespace_tmp); - - rcl_context_t * context = reinterpret_cast(context_handle); - // TODO(clalancette): we could probably make this faster by just doing these // method lookups during some initialization time. But we'd have to add a lot // more infrastructure for that, and we don't expect to call // 'nativeCreateNodeHandle' that often, so I think this is OK for now. - jclass java_util_ArrayList = - static_cast(env->NewGlobalRef(env->FindClass("java/util/ArrayList"))); + jclass java_util_ArrayList = static_cast(env->FindClass("java/util/ArrayList")); jmethodID java_util_ArrayList_size = env->GetMethodID(java_util_ArrayList, "size", "()I"); jmethodID java_util_ArrayList_get = env->GetMethodID( java_util_ArrayList, "get", "(I)Ljava/lang/Object;"); - rcl_arguments_t arguments = rcl_get_zero_initialized_arguments(); - char ** argv = NULL; + *arguments = rcl_get_zero_initialized_arguments(); jint argc = env->CallIntMethod(cli_args, java_util_ArrayList_size); - argv = static_cast(malloc(argc * sizeof(char *))); + std::vector argv(argc); for (jint i = 0; i < argc; ++i) { jstring element = static_cast(env->CallObjectMethod(cli_args, java_util_ArrayList_get, i)); - argv[i] = const_cast(env->GetStringUTFChars(element, nullptr)); + argv[i] = env->GetStringUTFChars(element, nullptr); } - rcl_ret_t ret = rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &arguments); + rcl_ret_t ret = rcl_parse_arguments(argc, &argv[0], rcl_get_default_allocator(), arguments); for (jint i = 0; i < argc; ++i) { jstring element = static_cast(env->CallObjectMethod(cli_args, java_util_ArrayList_get, i)); env->ReleaseStringUTFChars(element, argv[i]); } - free(argv); if (ret != RCL_RET_OK) { std::string msg = "Failed to parse node arguments: " + std::string(rcl_get_error_string().str); rcl_reset_error(); rcljava_throw_rclexception(env, ret, msg); + return false; + } + + return true; +} + +JNIEXPORT jlong JNICALL +Java_org_ros2_rcljava_RCLJava_nativeCreateNodeHandle( + JNIEnv * env, jclass, jstring jnode_name, jstring jnamespace, jlong context_handle, + jobject cli_args, jboolean use_global_arguments, jboolean enable_rosout) +{ + const char * node_name_tmp = env->GetStringUTFChars(jnode_name, 0); + std::string node_name(node_name_tmp); + env->ReleaseStringUTFChars(jnode_name, node_name_tmp); + + const char * namespace_tmp = env->GetStringUTFChars(jnamespace, 0); + std::string namespace_(namespace_tmp); + env->ReleaseStringUTFChars(jnamespace, namespace_tmp); + + rcl_context_t * context = reinterpret_cast(context_handle); + + rcl_arguments_t arguments; + if (!parse_arguments(env, cli_args, &arguments)) { + // All of the exception setup was done by parse_arguments, just return here. return 0; } @@ -99,7 +108,7 @@ Java_org_ros2_rcljava_RCLJava_nativeCreateNodeHandle( options.use_global_arguments = use_global_arguments; options.arguments = arguments; options.enable_rosout = enable_rosout; - ret = rcl_node_init(node, node_name.c_str(), namespace_.c_str(), context, &options); + rcl_ret_t ret = rcl_node_init(node, node_name.c_str(), namespace_.c_str(), context, &options); if (ret != RCL_RET_OK) { std::string msg = "Failed to create node: " + std::string(rcl_get_error_string().str); rcl_reset_error(); From 6236aa82f54b9f3df418683322d09db7d52ed025 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 10 Aug 2020 18:52:44 +0000 Subject: [PATCH 5/7] Add a NodeOptions class. This makes it a lot nicer to use. Signed-off-by: Chris Lalancette --- rcljava/CMakeLists.txt | 1 + .../main/java/org/ros2/rcljava/RCLJava.java | 11 +-- .../org/ros2/rcljava/node/NodeOptions.java | 68 +++++++++++++++++++ .../ros2/rcljava/node/NodeOptionsTest.java | 7 +- .../node/NodeUndeclaredParametersTest.java | 4 +- 5 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 rcljava/src/main/java/org/ros2/rcljava/node/NodeOptions.java diff --git a/rcljava/CMakeLists.txt b/rcljava/CMakeLists.txt index bb150da7..dda754a0 100644 --- a/rcljava/CMakeLists.txt +++ b/rcljava/CMakeLists.txt @@ -134,6 +134,7 @@ set(${PROJECT_NAME}_sources "src/main/java/org/ros2/rcljava/node/ComposableNode.java" "src/main/java/org/ros2/rcljava/node/Node.java" "src/main/java/org/ros2/rcljava/node/NodeImpl.java" + "src/main/java/org/ros2/rcljava/node/NodeOptions.java" "src/main/java/org/ros2/rcljava/parameters/client/AsyncParametersClient.java" "src/main/java/org/ros2/rcljava/parameters/client/AsyncParametersClientImpl.java" "src/main/java/org/ros2/rcljava/parameters/client/SyncParametersClient.java" diff --git a/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java b/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java index e8e83fa3..bb0ccd20 100644 --- a/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java +++ b/rcljava/src/main/java/org/ros2/rcljava/RCLJava.java @@ -34,6 +34,7 @@ import org.ros2.rcljava.node.ComposableNode; import org.ros2.rcljava.node.Node; import org.ros2.rcljava.node.NodeImpl; +import org.ros2.rcljava.node.NodeOptions; import org.ros2.rcljava.publisher.Publisher; import org.ros2.rcljava.qos.QoSProfile; import org.ros2.rcljava.service.RMWRequestId; @@ -235,7 +236,7 @@ public static Context createContext() { * structure. */ public static Node createNode(final String nodeName) { - return createNode(nodeName, "", RCLJava.getDefaultContext(), true, true, new ArrayList(), false); + return createNode(nodeName, "", RCLJava.getDefaultContext(), new NodeOptions()); } /** @@ -247,12 +248,12 @@ public static Node createNode(final String nodeName) { * structure. */ public static Node createNode(final String nodeName, final String namespace, final Context context) { - return createNode(nodeName, namespace, context, true, true, new ArrayList(), false); + return createNode(nodeName, namespace, context, new NodeOptions()); } - public static Node createNode(final String nodeName, final String namespace, final Context context, final boolean useGlobalArguments, final boolean enableRosout, final ArrayList cliArgs, final boolean allowUndeclaredParameters) { - long nodeHandle = nativeCreateNodeHandle(nodeName, namespace, context.getHandle(), cliArgs, useGlobalArguments, enableRosout); - Node node = new NodeImpl(nodeHandle, context, allowUndeclaredParameters); + public static Node createNode(final String nodeName, final String namespace, final Context context, final NodeOptions options) { + long nodeHandle = nativeCreateNodeHandle(nodeName, namespace, context.getHandle(), options.getCliArgs(), options.getUseGlobalArguments(), options.getEnableRosout()); + Node node = new NodeImpl(nodeHandle, context, options.getAllowUndeclaredParameters()); nodes.add(node); return node; } diff --git a/rcljava/src/main/java/org/ros2/rcljava/node/NodeOptions.java b/rcljava/src/main/java/org/ros2/rcljava/node/NodeOptions.java new file mode 100644 index 00000000..c64d65e1 --- /dev/null +++ b/rcljava/src/main/java/org/ros2/rcljava/node/NodeOptions.java @@ -0,0 +1,68 @@ +/* Copyright 2020 Open Source Robotics Foundation, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.ros2.rcljava.node; + +import java.util.ArrayList; + +public class NodeOptions { + private boolean useGlobalArguments; + private boolean enableRosout; + private boolean allowUndeclaredParameters; + private ArrayList cliArgs; + + public NodeOptions() { + this.useGlobalArguments = true; + this.enableRosout = true; + this.allowUndeclaredParameters = false; + this.cliArgs = new ArrayList(); + } + + public final boolean getUseGlobalArguments() { + return this.useGlobalArguments; + } + + public NodeOptions setUseGlobalArguments(boolean useGlobal) { + this.useGlobalArguments = useGlobal; + return this; + } + + public final boolean getEnableRosout() { + return this.enableRosout; + } + + public NodeOptions setEnableRosout(boolean enable) { + this.enableRosout = enable; + return this; + } + + public final boolean getAllowUndeclaredParameters() { + return this.allowUndeclaredParameters; + } + + public NodeOptions setAllowUndeclaredParameters(boolean allow) { + this.allowUndeclaredParameters = allow; + return this; + } + + public final ArrayList getCliArgs() { + return this.cliArgs; + } + + public NodeOptions setCliArgs(ArrayList newArgs) { + this.cliArgs = newArgs; + return this; + } +} diff --git a/rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java b/rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java index 121adad2..ce3bca84 100644 --- a/rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java +++ b/rcljava/src/test/java/org/ros2/rcljava/node/NodeOptionsTest.java @@ -26,6 +26,7 @@ import org.ros2.rcljava.RCLJava; import org.ros2.rcljava.node.Node; +import org.ros2.rcljava.node.NodeOptions; public class NodeOptionsTest { @BeforeClass @@ -43,9 +44,9 @@ public static void tearDownOnce() { @Test public final void testCreateNodeWithArgs() { - ArrayList args = new ArrayList(Arrays.asList("--ros-args", "-r", "__ns:=/foo")); - //ArrayList args = new ArrayList(); - Node node = RCLJava.createNode("test_node", "", RCLJava.getDefaultContext(), true, true, args, false); + NodeOptions options = new NodeOptions(); + options.setCliArgs(new ArrayList(Arrays.asList("--ros-args", "-r", "__ns:=/foo"))); + Node node = RCLJava.createNode("test_node", "", RCLJava.getDefaultContext(), options); assertEquals("test_node", node.getName()); assertEquals("/foo", node.getNamespace()); diff --git a/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java b/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java index 35ef868f..7a02e961 100644 --- a/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java +++ b/rcljava/src/test/java/org/ros2/rcljava/node/NodeUndeclaredParametersTest.java @@ -53,7 +53,9 @@ public static void tearDownOnce() { @Before public void setUp() { - node = RCLJava.createNode("test_node", "", RCLJava.getDefaultContext(), true, true, new ArrayList(), true); + NodeOptions options = new NodeOptions(); + options.setAllowUndeclaredParameters(true); + node = RCLJava.createNode("test_node", "", RCLJava.getDefaultContext(), options); } @After From dd844a9462c12a5045bf369f101d462a1438f7ff Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 10 Aug 2020 19:00:44 +0000 Subject: [PATCH 6/7] Make sure to DeleteLocalRef after use. Signed-off-by: Chris Lalancette --- rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp index 492bebc5..3ea1d473 100644 --- a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp +++ b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp @@ -63,12 +63,14 @@ bool parse_arguments(JNIEnv * env, jobject cli_args, rcl_arguments_t * arguments jstring element = static_cast(env->CallObjectMethod(cli_args, java_util_ArrayList_get, i)); argv[i] = env->GetStringUTFChars(element, nullptr); + env->DeleteLocalRef(element); } rcl_ret_t ret = rcl_parse_arguments(argc, &argv[0], rcl_get_default_allocator(), arguments); for (jint i = 0; i < argc; ++i) { jstring element = static_cast(env->CallObjectMethod(cli_args, java_util_ArrayList_get, i)); env->ReleaseStringUTFChars(element, argv[i]); + env->DeleteLocalRef(element); } if (ret != RCL_RET_OK) { std::string msg = "Failed to parse node arguments: " + std::string(rcl_get_error_string().str); From ad331e50eb700745b721b6a024a38135047c5458 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 11 Aug 2020 13:10:45 +0000 Subject: [PATCH 7/7] Fixes from review. 1. Check return value of malloc. 2. Get rid of unnecessary cast. Signed-off-by: Chris Lalancette --- rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp index 3ea1d473..1a3b640d 100644 --- a/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp +++ b/rcljava/src/main/cpp/org_ros2_rcljava_RCLJava.cpp @@ -51,7 +51,7 @@ bool parse_arguments(JNIEnv * env, jobject cli_args, rcl_arguments_t * arguments // method lookups during some initialization time. But we'd have to add a lot // more infrastructure for that, and we don't expect to call // 'nativeCreateNodeHandle' that often, so I think this is OK for now. - jclass java_util_ArrayList = static_cast(env->FindClass("java/util/ArrayList")); + jclass java_util_ArrayList = env->FindClass("java/util/ArrayList"); jmethodID java_util_ArrayList_size = env->GetMethodID(java_util_ArrayList, "size", "()I"); jmethodID java_util_ArrayList_get = env->GetMethodID( java_util_ArrayList, "get", "(I)Ljava/lang/Object;"); @@ -97,15 +97,20 @@ Java_org_ros2_rcljava_RCLJava_nativeCreateNodeHandle( rcl_context_t * context = reinterpret_cast(context_handle); + rcl_node_t * node = static_cast(malloc(sizeof(rcl_node_t))); + if (node == nullptr) { + env->ThrowNew(env->FindClass("java/lang/OutOfMemoryError"), "Failed to allocate node"); + return 0; + } + *node = rcl_get_zero_initialized_node(); + rcl_arguments_t arguments; if (!parse_arguments(env, cli_args, &arguments)) { // All of the exception setup was done by parse_arguments, just return here. + free(node); return 0; } - rcl_node_t * node = static_cast(malloc(sizeof(rcl_node_t))); - *node = rcl_get_zero_initialized_node(); - rcl_node_options_t options = rcl_node_get_default_options(); options.use_global_arguments = use_global_arguments; options.arguments = arguments;