Added tests and modified the interface to the upper language binding.

The modification of the interface is to go along with the specification described in the draft REP shared in the thread below. https://discourse.ros.org/t/adding-thread-attributes-configuration-in-ros-2-framework/30701/5 Signed-off-by: Shoji Morita <[email protected]>
esol-community · Aug 3, 2023 · 043b11b · 043b11b
1 parent d868104
commit 043b11b
Show file tree

Hide file tree

Showing 7 changed files with 100 additions and 29 deletions.
diff --git a/rcl/include/rcl/context.h b/rcl/include/rcl/context.h
@@ -314,22 +314,15 @@ RCL_WARN_UNUSED
 rmw_context_t *
 rcl_context_get_rmw_context(rcl_context_t * context);
 
-/// Returns the thread attribute context.
+/// Returns pointer to the thread attribute list.
 /**
- * \param[in] context from which the thread attribute should be retrieved.
- * \param[out] thread_attrs output variable where the thread attribute will be returned.
- * \return #RCL_RET_INVALID_ARGUMENT if `context` is invalid (see rcl_context_is_valid()), or
- * \return #RCL_RET_INVALID_ARGUMENT if `context->impl` is `NULL`, or
- * \return #RCL_RET_INVALID_ARGUMENT if `*thread_attrs` is not `NULL`, or
- * \return #RCL_RET_INVALID_ARGUMENT if `context->impl->thread_context.thread_attrs` is `NULL`, or
- * \return #RCL_RET_OK if the thread attribute was correctly retrieved.
+ * \param[in] context object from which the thread attribute list should be retrieved.
+ * \return pointer to thread attribute list if valid, otherwise `NULL`
  */
 RCL_PUBLIC
 RCL_WARN_UNUSED
-rcl_ret_t
-rcl_context_get_thread_attrs(
-  const rcl_context_t * context,
-  rcutils_thread_attrs_t ** thread_attrs);
+rcutils_thread_attrs_t *
+rcl_context_get_thread_attrs(const rcl_context_t * context);
 
 #ifdef __cplusplus
 }

diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c
@@ -581,18 +581,18 @@ rcl_parse_arguments(
             rcl_parse_yaml_thread_attrs_value(argv[i + 1], &args_impl->thread_attrs))
           {
             RCUTILS_LOG_DEBUG_NAMED(
-              ROS_PACKAGE_NAME, "Got thread attribute rule : %s\n", argv[i + 1]);
+              ROS_PACKAGE_NAME, "Got thread attributes value : %s\n", argv[i + 1]);
             ++i;  // Skip flag here, for loop will skip rule.
             continue;
           }
           rcl_error_string_t prev_error_string = rcl_get_error_string();
           rcl_reset_error();
           RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
-            "Couldn't parse thread attribute rule: '%s %s'. Error: %s", argv[i], argv[i + 1],
+            "Couldn't parse thread attributes value: '%s %s'. Error: %s", argv[i], argv[i + 1],
             prev_error_string.str);
         } else {
           RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
-            "Couldn't parse trailing %s flag. No thread attribute rule found.", argv[i]);
+            "Couldn't parse trailing %s flag. No thread attributes value found.", argv[i]);
         }
         ret = RCL_RET_INVALID_ROS_ARGS;
         goto fail;
@@ -615,13 +615,15 @@ rcl_parse_arguments(
             RCL_RET_OK == rcl_parse_yaml_thread_attrs_file(
               argv[i + 1], &args_impl->thread_attrs))
           {
+            RCUTILS_LOG_DEBUG_NAMED(
+              ROS_PACKAGE_NAME, "Got thread attributes file : %s\n", argv[i + 1]);
             ++i;  // Skip flag here, for loop will skip rule.
             continue;
           }
           rcl_error_string_t prev_error_string = rcl_get_error_string();
           rcl_reset_error();
           RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
-            "Couldn't parse thread attr file: '%s %s'. Error: %s", argv[i], argv[i + 1],
+            "Couldn't parse thread attributes file: '%s %s'. Error: %s", argv[i], argv[i + 1],
             prev_error_string.str);
         } else {
           RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
@@ -1000,6 +1002,18 @@ rcl_arguments_copy(
     return RCL_RET_BAD_ALLOC;
   }
   args_out->impl->enclave = enclave_copy;
+
+  // Copy thread attributes
+  if (0 < args->impl->thread_attrs.num_attributes) {
+    rcutils_ret_t thread_attrs_ret =
+      rcutils_thread_attrs_copy(&args->impl->thread_attrs, &args_out->impl->thread_attrs);
+    if (RCUTILS_RET_OK != thread_attrs_ret) {
+      if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
+        RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
+      }
+      return RCL_RET_BAD_ALLOC;
+    }
+  }
   return RCL_RET_OK;
 }
 

diff --git a/rcl/src/rcl/context.c b/rcl/src/rcl/context.c
@@ -106,21 +106,20 @@ rcl_context_get_rmw_context(rcl_context_t * context)
   return &(context->impl->rmw_context);
 }
 
-rcl_ret_t
-rcl_context_get_thread_attrs(
-  const rcl_context_t * context,
-  rcutils_thread_attrs_t ** thread_attrs)
+rcutils_thread_attrs_t *
+rcl_context_get_thread_attrs(const rcl_context_t * context)
 {
-  RCL_CHECK_ARGUMENT_FOR_NULL(context, RCL_RET_INVALID_ARGUMENT);
-  RCL_CHECK_ARGUMENT_FOR_NULL(context->impl, RCL_RET_INVALID_ARGUMENT);
-  RCL_CHECK_ARGUMENT_FOR_NULL(thread_attrs, RCL_RET_INVALID_ARGUMENT);
+  RCL_CHECK_ARGUMENT_FOR_NULL(context, NULL);
+  RCL_CHECK_FOR_NULL_WITH_MSG(context->impl, "context is zero-initialized", return NULL);
 
-  if (0 < context->impl->thread_attrs.num_attributes) {
-    *thread_attrs = &context->impl->thread_attrs;
-    return RCL_RET_OK;
-  } else {
-    return RCL_RET_ERROR;
+  rcutils_thread_attrs_t * attrs = NULL;
+  rcl_ret_t ret = rcl_arguments_get_thread_attrs(&context->global_arguments, &attrs);
+  if (RCL_RET_OK != ret) {
+    if (0 < context->impl->thread_attrs.num_attributes) {
+      attrs = &context->impl->thread_attrs;
+    }
   }
+  return attrs;
 }
 
 rcl_ret_t

diff --git a/rcl/test/CMakeLists.txt b/rcl/test/CMakeLists.txt
@@ -81,6 +81,7 @@ function(test_target_function)
     SRCS rcl/test_context.cpp
     ENV ${rmw_implementation_env_var} ${memory_tools_ld_preload_env_var}
     APPEND_LIBRARY_DIRS ${extra_lib_dirs}
+    INCLUDE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/../src/rcl/
     LIBRARIES ${PROJECT_NAME} mimick osrf_testing_tools_cpp::memory_tools
     AMENT_DEPENDENCIES ${rmw_implementation}
   )

diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp
@@ -201,6 +201,16 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), check_known_vs_unkno
   EXPECT_FALSE(are_known_ros_args({"--ros-args", "stdout-logs"}));
   EXPECT_FALSE(are_known_ros_args({"--ros-args", "external-lib-logs"}));
   EXPECT_FALSE(are_known_ros_args({"--ros-args", "external-lib-logs"}));
+
+  // Thread attributes
+  EXPECT_TRUE(
+    are_known_ros_args(
+      {"--ros-args", "--thread-attrs-file",
+        (test_path / "test_thread_attrs.yaml").string().c_str()}));
+  EXPECT_TRUE(
+    are_known_ros_args(
+      {"--ros-args", "--thread-attrs-value",
+        "[{priority: 10, scheduling_policy: FIFO, name: thread-1, core_affinity: 1}]"}));
 }
 
 bool
@@ -226,7 +236,9 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), check_valid_vs_inval
   {
     "--ros-args", "-p", "foo:=bar", "-r", "__node:=node_name",
     "--params-file", parameters_filepath.c_str(), "--log-level", "INFO",
-    "--log-config-file", "file.config"
+    "--log-config-file", "file.config",
+    "--thread-attrs-value",
+    "[{priority: 10, scheduling_policy: IDLE, name: thread-1, core_affinity: 1}]"
   }));
 
   // ROS args unknown to rcl are not (necessarily) invalid

diff --git a/rcl/test/rcl/test_context.cpp b/rcl/test/rcl/test_context.cpp
@@ -24,6 +24,11 @@
 
 #include "../mocking_utils/patch.hpp"
 
+#include "rcutils/thread_attr.h"
+
+#include "./arguments_impl.h"
+#include "./context_impl.h"
+
 #ifdef RMW_IMPLEMENTATION
 # define CLASSNAME_(NAME, SUFFIX) NAME ## __ ## SUFFIX
 # define CLASSNAME(NAME, SUFFIX) CLASSNAME_(NAME, SUFFIX)
@@ -147,6 +152,45 @@ TEST_F(CLASSNAME(TestContextFixture, RMW_IMPLEMENTATION), nominal) {
   EXPECT_NE(rmw_context_ptr, nullptr) << rcl_get_error_string().str;
   rcl_reset_error();
 
+  // test rcl_context_get_thread_attrs
+  rcutils_thread_attrs_t * thread_attrs;
+  EXPECT_NO_MEMORY_OPERATIONS(
+  {
+    thread_attrs = rcl_context_get_thread_attrs(nullptr);
+  });
+  EXPECT_EQ(nullptr, thread_attrs);
+  EXPECT_TRUE(rcl_error_is_set());
+  rcl_reset_error();
+
+  EXPECT_NO_MEMORY_OPERATIONS(
+  {
+    EXPECT_EQ(nullptr, rcl_context_get_thread_attrs(&context));
+  });
+
+  {
+    rcutils_thread_attrs_t * arg_attrs = &context.global_arguments.impl->thread_attrs;
+    rcutils_thread_attrs_t * ctx_attrs = &context.impl->thread_attrs;
+    rcutils_ret_t ret;
+
+    ret = rcutils_thread_attrs_init(ctx_attrs, rcl_get_default_allocator());
+    EXPECT_EQ(RCUTILS_RET_OK, ret);
+    ret = rcutils_thread_attrs_add_attr(
+      ctx_attrs, RCUTILS_THREAD_SCHEDULING_POLICY_FIFO, 1, 10, "arg");
+    EXPECT_EQ(RCUTILS_RET_OK, ret);
+
+    EXPECT_EQ(ctx_attrs, rcl_context_get_thread_attrs(&context));
+
+    ret = rcutils_thread_attrs_copy(ctx_attrs, arg_attrs);
+    EXPECT_EQ(RCUTILS_RET_OK, ret);
+
+    EXPECT_EQ(arg_attrs, rcl_context_get_thread_attrs(&context));
+
+    ret = rcutils_thread_attrs_fini(ctx_attrs);
+    EXPECT_EQ(RCUTILS_RET_OK, ret);
+    ret = rcutils_thread_attrs_fini(arg_attrs);
+    EXPECT_EQ(RCUTILS_RET_OK, ret);
+  }
+
   ret = rcl_init_options_fini(&init_options);
   EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
 }

diff --git a/rcl/test/resources/test_arguments/test_thread_attrs.yaml b/rcl/test/resources/test_arguments/test_thread_attrs.yaml
@@ -0,0 +1,8 @@
+- name: thread-1
+  priority: 10
+  scheduling_policy: FIFO
+  core_affinity: 1
+- name: thread-2
+  priority: 20
+  scheduling_policy: RR
+  core_affinity: 2