From 043b11babb3788f1aae6baf5e91cc0e63844bb90 Mon Sep 17 00:00:00 2001 From: Shoji Morita Date: Thu, 3 Aug 2023 17:08:59 +0900 Subject: [PATCH] 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 --- rcl/include/rcl/context.h | 17 +++---- rcl/src/rcl/arguments.c | 22 ++++++++-- rcl/src/rcl/context.c | 23 +++++----- rcl/test/CMakeLists.txt | 1 + rcl/test/rcl/test_arguments.cpp | 14 +++++- rcl/test/rcl/test_context.cpp | 44 +++++++++++++++++++ .../test_arguments/test_thread_attrs.yaml | 8 ++++ 7 files changed, 100 insertions(+), 29 deletions(-) create mode 100644 rcl/test/resources/test_arguments/test_thread_attrs.yaml diff --git a/rcl/include/rcl/context.h b/rcl/include/rcl/context.h index 805b5b39c..4a40f4466 100644 --- 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 index 64627388f..2ed84b76b 100644 --- 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 index c30497857..cc8477b9e 100644 --- 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 index 1b73ab5e6..ae128aace 100644 --- 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 index 02f8371a7..c3a34897d 100644 --- 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 index 31e4100b3..925cac57b 100644 --- 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 new file mode 100644 index 000000000..e784b8cac --- /dev/null +++ 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