-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IR] Implementation of QoS Features #1
Conversation
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
* EDIT: alphabetical order cmakelist and imports Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Fix errors, remove redundant information, and try to make sentences more concise. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
RCL_PUBLIC | ||
RCL_WARN_UNUSED | ||
rcl_ret_t | ||
rcl_publisher_event_init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these functions should have documentation added.
@@ -99,6 +99,13 @@ typedef rmw_ret_t rcl_ret_t; | |||
/// Argument is not a valid log level rule | |||
#define RCL_RET_INVALID_LOG_LEVEL_RULE 1020 | |||
|
|||
// rcl event specific ret codes in 20XX | |||
/// Invalid rcl_event_t given return code. | |||
#define RCL_RET_EVENT_INVALID 2000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just the same as a bad parameter error code?
rcl/src/rcl/event.c
Outdated
rmw_ret_t ret = rmw_take_event(event->impl->rmw_handle, event_status, &taken); | ||
if (RMW_RET_OK != ret) { | ||
RCL_SET_ERROR_MSG(rmw_get_error_string().str); | ||
if (RMW_RET_BAD_ALLOC == ret) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we have a function already for converting rmw errors to rcl errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall our error types still need to be better defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overall our error types still need to be better defined.
What are we missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are far fewer RMW_RET_*
types than RCL_RET_*
types. Getting a rcl_ret_t
from a rmw_ret_t
feels like rcl_ret_t
isn't living up to its potential.
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
(rmw_events_t::events to point to rmw_event_t instead of rmw_event_t::data) Signed-off-by: Miaofei <miaofei@amazon.com>
RCL_PUBLIC | ||
RCL_WARN_UNUSED | ||
rmw_event_t * | ||
rcl_event_get_rmw_handle(const rcl_event_t * event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used? Is there a reason we need to expose the rmw layer above the rcl layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it is not used anywhere. But neither is rcl_subscription_get_rmw_handle()
. However, rcl_publisher_get_rmw_handle()
is used in rclcpp
...
Let's just leave it here for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, this function is needed by the SET_ADD_RMW
macro in wait.c
.
rcl/src/rcl/event.c
Outdated
bool taken; | ||
RCL_CHECK_ARGUMENT_FOR_NULL(event, RCL_RET_INVALID_ARGUMENT); | ||
RCL_CHECK_ARGUMENT_FOR_NULL(event_status, RCL_RET_INVALID_ARGUMENT); | ||
rmw_ret_t ret = rmw_take_event(event->impl->rmw_handle, event_status, &taken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a little odd that we're diverging rcl_take_event
and rmw_take_event
in terms of having the extra parameter to determine if something was actually taken. Why can't they both work the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am for removing the taken
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove the taken
parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, it looks like the other rmw_take()
functions also have this extra taken
boolean. We should probably leave it in there to be consistent.
address PR comments Signed-off-by: Miaofei <miaofei@amazon.com>
Call rcutils_logging_shutdown() in TearDown to avoid leaking logging internal objects. As any change to this file makes test_logging_named fail, this releaxes the EXACT_EQ call on g_last_log_event.location->line_number[1]. Instead of looking for a particular value, we test an invariant of the field (line numbers are positive). Finally, in test_logging_function, initialize g_counter and g_function_called values to make sure the test will not fail even if `--gtest_repeat=2` is passed. Other tests are still failing when this flag is passed: [ FAILED ] TestLoggingMacros.test_logging_once [ FAILED ] TestLoggingMacros.test_logging_skipfirst [ FAILED ] TestLoggingMacros.test_logging_skipfirst_throttle However, fixing those tests would require actually changing the logging implementation which is out of the scope of this change. Before: $ ./test_logging_macros Running main() from gmock_main.cc [==========] Running 8 tests from 1 test case. [----------] Global test environment set-up. [----------] 8 tests from TestLoggingMacros [ RUN ] TestLoggingMacros.test_logging_named [ OK ] TestLoggingMacros.test_logging_named (0 ms) [ RUN ] TestLoggingMacros.test_logging_once [ OK ] TestLoggingMacros.test_logging_once (0 ms) [ RUN ] TestLoggingMacros.test_logging_expression [ OK ] TestLoggingMacros.test_logging_expression (0 ms) [ RUN ] TestLoggingMacros.test_logging_function [ OK ] TestLoggingMacros.test_logging_function (0 ms) [ RUN ] TestLoggingMacros.test_logging_skipfirst [ OK ] TestLoggingMacros.test_logging_skipfirst (0 ms) [ RUN ] TestLoggingMacros.test_logging_throttle [ OK ] TestLoggingMacros.test_logging_throttle (302 ms) [ RUN ] TestLoggingMacros.test_logging_skipfirst_throttle [ OK ] TestLoggingMacros.test_logging_skipfirst_throttle (302 ms) [ RUN ] TestLoggingMacros.test_logger_hierarchy [ OK ] TestLoggingMacros.test_logger_hierarchy (0 ms) [----------] 8 tests from TestLoggingMacros (605 ms total) [----------] Global test environment tear-down [==========] 8 tests from 1 test case ran. (605 ms total) [ PASSED ] 8 tests. ================================================================= ==19903==ERROR: LeakSanitizer: detected memory leaks Direct leak of 504 byte(s) in 7 object(s) allocated from: #0 0x7fd640c1fb50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50) #1 0x7fd64091cbb5 in __default_allocate (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x8bb5) #2 0x7fd64092f1b0 in rcutils_string_map_init (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x1b1b0) #3 0x7fd640926641 in rcutils_logging_initialize_with_allocator (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x12641) #4 0x7fd6409261ab in rcutils_logging_initialize (/home/.../ros2_ws/install-asan/rcutils/lib/librcutils.so+0x121ab) #5 0x564405fc6627 in TestLoggingMacros::SetUp() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x29627) #6 0x564406045999 in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0xa8999) #7 0x564406037b7d in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x9ab7d) #8 0x564405fe4503 in testing::Test::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x47503) #9 0x564405fe59e4 in testing::TestInfo::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x489e4) ros2#10 0x564405fe6588 in testing::TestCase::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x49588) ros2#11 0x564406001699 in testing::internal::UnitTestImpl::RunAllTests() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x64699) ros2#12 0x56440604844c in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0xab44c) ros2#13 0x564406039e46 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x9ce46) ros2#14 0x564405ffe42d in testing::UnitTest::Run() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x6142d) ros2#15 0x564405fd197d in RUN_ALL_TESTS() (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x3497d) ros2#16 0x564405fd18c3 in main (/home/.../ros2_ws/build-asan/rcutils/test_logging_macros+0x348c3) ros2#17 0x7fd63fd84b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) After: $ ./test_logging_macros Running main() from gmock_main.cc [==========] Running 9 tests from 1 test case. [----------] Global test environment set-up. [----------] 9 tests from TestLoggingMacros [ RUN ] TestLoggingMacros.test_empty [ OK ] TestLoggingMacros.test_empty (0 ms) [ RUN ] TestLoggingMacros.test_logging_named [ OK ] TestLoggingMacros.test_logging_named (0 ms) [ RUN ] TestLoggingMacros.test_logging_once [ OK ] TestLoggingMacros.test_logging_once (0 ms) [ RUN ] TestLoggingMacros.test_logging_expression [ OK ] TestLoggingMacros.test_logging_expression (0 ms) [ RUN ] TestLoggingMacros.test_logging_function [ OK ] TestLoggingMacros.test_logging_function (0 ms) [ RUN ] TestLoggingMacros.test_logging_skipfirst [ OK ] TestLoggingMacros.test_logging_skipfirst (0 ms) [ RUN ] TestLoggingMacros.test_logging_throttle [ OK ] TestLoggingMacros.test_logging_throttle (303 ms) [ RUN ] TestLoggingMacros.test_logging_skipfirst_throttle [ OK ] TestLoggingMacros.test_logging_skipfirst_throttle (302 ms) [ RUN ] TestLoggingMacros.test_logger_hierarchy [ OK ] TestLoggingMacros.test_logger_hierarchy (0 ms) [----------] 9 tests from TestLoggingMacros (605 ms total) [----------] Global test environment tear-down [==========] 9 tests from 1 test case ran. (606 ms total) [ PASSED ] 9 tests. [1] https://testing.googleblog.com/2015/01/testing-on-toilet-change-detector-tests.html Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
Add rcutils_logging_shutdown() every time needed in test_logging.cpp. Remove spurious `g_rcutils_logging_initialized = false;` preventing rcutils_logging_shutdown() from freeing memory. Before: $ ./test_logging Running main() from ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc [==========] Running 5 tests from 1 test case. [----------] Global test environment set-up. [----------] 5 tests from TestLogging [ RUN ] TestLogging.test_logging_initialization [ OK ] TestLogging.test_logging_initialization (0 ms) [ RUN ] TestLogging.test_logging [ OK ] TestLogging.test_logging (0 ms) [ RUN ] TestLogging.test_log_severity [ OK ] TestLogging.test_log_severity (0 ms) [ RUN ] TestLogging.test_logger_severities [ OK ] TestLogging.test_logger_severities (0 ms) [ RUN ] TestLogging.test_logger_severity_hierarchy [ OK ] TestLogging.test_logger_severity_hierarchy (0 ms) [----------] 5 tests from TestLogging (0 ms total) [----------] Global test environment tear-down [==========] 5 tests from 1 test case ran. (0 ms total) [ PASSED ] 5 tests. ================================================================= ==1676==ERROR: LeakSanitizer: detected memory leaks Direct leak of 72 byte(s) in 1 object(s) allocated from: #0 0x7f7a7229db50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50) #1 0x7f7a71f9abb5 in __default_allocate (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x8bb5) #2 0x7f7a71fad1b0 in rcutils_string_map_init (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x1b1b0) #3 0x7f7a71fa4641 in rcutils_logging_initialize_with_allocator (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x12641) #4 0x7f7a71fa41ab in rcutils_logging_initialize (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x121ab) #5 0x561e9d22c972 in TestLogging_test_logging_Test::TestBody() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x1a972) #6 0x561e9d2b545d in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa345d) #7 0x561e9d2a71c7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x951c7) #8 0x561e9d2534c9 in testing::Test::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x414c9) #9 0x561e9d2548f4 in testing::TestInfo::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x428f4) ros2#10 0x561e9d255498 in testing::TestCase::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x43498) ros2#11 0x561e9d2705a9 in testing::internal::UnitTestImpl::RunAllTests() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5e5a9) ros2#12 0x561e9d2b7f10 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa5f10) ros2#13 0x561e9d2a9490 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x97490) ros2#14 0x561e9d26d33d in testing::UnitTest::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5b33d) ros2#15 0x561e9d24088c in RUN_ALL_TESTS() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e88c) ros2#16 0x561e9d2407d2 in main (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e7d2) ros2#17 0x7f7a71402b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) Direct leak of 72 byte(s) in 1 object(s) allocated from: #0 0x7f7a7229db50 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xdeb50) #1 0x7f7a71f9abb5 in __default_allocate (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x8bb5) #2 0x7f7a71fad1b0 in rcutils_string_map_init (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x1b1b0) #3 0x7f7a71fa4641 in rcutils_logging_initialize_with_allocator (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x12641) #4 0x7f7a71fa41ab in rcutils_logging_initialize (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/install-asan/rcutils/lib/librcutils.so+0x121ab) #5 0x561e9d22a9b6 in TestLogging_test_logging_initialization_Test::TestBody() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x189b6) #6 0x561e9d2b545d in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa345d) #7 0x561e9d2a71c7 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x951c7) #8 0x561e9d2534c9 in testing::Test::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x414c9) #9 0x561e9d2548f4 in testing::TestInfo::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x428f4) ros2#10 0x561e9d255498 in testing::TestCase::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x43498) ros2#11 0x561e9d2705a9 in testing::internal::UnitTestImpl::RunAllTests() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5e5a9) ros2#12 0x561e9d2b7f10 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0xa5f10) ros2#13 0x561e9d2a9490 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x97490) ros2#14 0x561e9d26d33d in testing::UnitTest::Run() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x5b33d) ros2#15 0x561e9d24088c in RUN_ALL_TESTS() (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e88c) ros2#16 0x561e9d2407d2 in main (/home/ANT.AMAZON.COM/tmoulard/ros2_ws/build-asan/rcutils/test_logging+0x2e7d2) ros2#17 0x7f7a71402b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96) SUMMARY: AddressSanitizer: 144 byte(s) leaked in 2 allocation(s). After: $ ./test_logging Running main() from ../../../install-asan/gtest_vendor/src/gtest_vendor/src/gtest_main.cc [==========] Running 5 tests from 1 test case. [----------] Global test environment set-up. [----------] 5 tests from TestLogging [ RUN ] TestLogging.test_logging_initialization [ OK ] TestLogging.test_logging_initialization (0 ms) [ RUN ] TestLogging.test_logging [ OK ] TestLogging.test_logging (0 ms) [ RUN ] TestLogging.test_log_severity [ OK ] TestLogging.test_log_severity (0 ms) [ RUN ] TestLogging.test_logger_severities [ OK ] TestLogging.test_logger_severities (0 ms) [ RUN ] TestLogging.test_logger_severity_hierarchy [ OK ] TestLogging.test_logger_severity_hierarchy (0 ms) [----------] 5 tests from TestLogging (1 ms total) [----------] Global test environment tear-down [==========] 5 tests from 1 test case ran. (1 ms total) [ PASSED ] 5 tests. Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
Implementation of QoS features in the rcl package.