From 0a9b5fb5946e59c140ae87ef03a6f49908ef9a4c Mon Sep 17 00:00:00 2001 From: Ross Desmond <44277324+ross-desmond@users.noreply.github.com> Date: Wed, 10 Apr 2019 10:26:12 -0700 Subject: [PATCH] Add rcl public comments Details: * Address comments from PR * Add actual_qos to rcl_publisher_t * Add comments to rcl public functions Signed-off-by: Ross Desmond <44277324+ross-desmond@users.noreply.github.com> --- rcl/include/rcl/event.h | 42 +++++++++++++++++++++ rcl/src/rcl/event.c | 14 +++---- rcl/src/rcl/publisher_impl.h | 2 +- rcl/test/rcl/test_events.cpp | 73 ++++++++++++++++++++---------------- 4 files changed, 90 insertions(+), 41 deletions(-) diff --git a/rcl/include/rcl/event.h b/rcl/include/rcl/event.h index e10012d18..fd88d219d 100644 --- a/rcl/include/rcl/event.h +++ b/rcl/include/rcl/event.h @@ -63,6 +63,17 @@ RCL_WARN_UNUSED rcl_event_t rcl_get_zero_initialized_event(void); +/// Initialize an rcl_event_t with a publisher. +/** + * Fill the rcl_event_t with the publisher and desired event_type. + * + * \param[in,out] event pointer to fill + * \param[in] publisher to get events from + * \param[in] event_type to listen for + * \return `RCL_RET_OK` if the rcl_event_t is filled, or + * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_ERROR` if an unspecified error occurs. + */ RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t @@ -71,6 +82,17 @@ rcl_publisher_event_init( const rcl_publisher_t * publisher, const rcl_publisher_event_type_t event_type); +/// Initialize an rcl_event_t with a subscription. +/** + * Fill the rcl_event_t with the subscription and desired event_type. + * + * \param[in,out] event pointer to fill + * \param[in] subscription to get events from + * \param[in] event_type to listen for + * \return `RCL_RET_OK` if the rcl_event_t is filled, or + * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_ERROR` if an unspecified error occurs. + */ RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t @@ -79,6 +101,17 @@ rcl_subscription_event_init( const rcl_subscription_t * subscription, const rcl_subscription_event_type_t event_type); +// Take event using the event handle. +/** + * Take an event from the event handle. + * + * \param[in] event_handle event object to take from + * \param[in, out] event_info event info object to write taken data into + * \param[in, out] taken boolean flag indicating if an event was taken or not + * \return `RCL_RET_OK` if successful, or + * \return `RCL_RET_BAD_ALLOC` if memory allocation failed, or + * \return `RCL_RET_ERROR` if an unexpected error occurs. + */ RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t @@ -86,6 +119,15 @@ rcl_take_event( const rcl_event_t * event, void * event_info); +// Finalize an event. +/** + * Finalize an event. + * + * \param[in] event to finalize + * \return `RCL_RET_OK` if successful, or + * \return `RCL_RET_EVENT_INVALID` if event is null, or + * \return `RCL_RET_ERROR` if an unexpected error occurs. + */ RCL_PUBLIC RCL_WARN_UNUSED rcl_ret_t diff --git a/rcl/src/rcl/event.c b/rcl/src/rcl/event.c index 0f44638e2..b44b2a31a 100644 --- a/rcl/src/rcl/event.c +++ b/rcl/src/rcl/event.c @@ -33,14 +33,12 @@ extern "C" #include "./publisher_impl.h" #include "./subscription_impl.h" - typedef struct rcl_event_impl_t { rmw_event_t rmw_handle; rcl_allocator_t allocator; } rcl_event_impl_t; - rcl_event_t rcl_get_zero_initialized_event() { @@ -55,7 +53,6 @@ rcl_publisher_event_init( const rcl_publisher_event_type_t event_type) { rcl_ret_t ret = RCL_RET_OK; - // Check publisher and allocator first, so allocator can be used with errors. RCL_CHECK_ARGUMENT_FOR_NULL(publisher, RCL_RET_INVALID_ARGUMENT); rcl_allocator_t * allocator = &publisher->impl->options.allocator; @@ -79,9 +76,9 @@ rcl_publisher_event_init( rmw_event_type = RMW_EVENT_LIVELINESS_LOST; break; default: + RCL_SET_ERROR_MSG("Event type for publisher not supported"); return RCL_RET_INVALID_ARGUMENT; } - return rmw_publisher_event_init( &event->impl->rmw_handle, publisher->impl->rmw_handle, @@ -95,7 +92,6 @@ rcl_subscription_event_init( const rcl_subscription_event_type_t event_type) { rcl_ret_t ret = RCL_RET_OK; - // Check subscription and allocator first, so allocator can be used with errors. RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT); rcl_allocator_t * allocator = &subscription->impl->options.allocator; @@ -119,9 +115,9 @@ rcl_subscription_event_init( rmw_event_type = RMW_EVENT_LIVELINESS_CHANGED; break; default: + RCL_SET_ERROR_MSG("Event type for subscription not supported"); return RCL_RET_INVALID_ARGUMENT; } - return rmw_subscription_event_init( &event->impl->rmw_handle, subscription->impl->rmw_handle, @@ -141,11 +137,13 @@ rcl_take_event( RCL_SET_ERROR_MSG(rmw_get_error_string().str); return rcl_convert_rmw_ret_to_rcl_ret(ret); } - RCUTILS_LOG_DEBUG_NAMED( - ROS_PACKAGE_NAME, "Event take request succeeded: %s", taken ? "true" : "false"); if (!taken) { + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "take_event request complete, unable to take event"); return RCL_RET_EVENT_TAKE_FAILED; } + RCUTILS_LOG_DEBUG_NAMED( + ROS_PACKAGE_NAME, "take_event request success"); return rcl_convert_rmw_ret_to_rcl_ret(ret); } diff --git a/rcl/src/rcl/publisher_impl.h b/rcl/src/rcl/publisher_impl.h index 358a3f965..485a7c5ec 100644 --- a/rcl/src/rcl/publisher_impl.h +++ b/rcl/src/rcl/publisher_impl.h @@ -19,10 +19,10 @@ #include "rcl/publisher.h" - typedef struct rcl_publisher_impl_t { rcl_publisher_options_t options; + rmw_qos_profile_t actual_qos; rcl_context_t * context; rmw_publisher_t * rmw_handle; } rcl_publisher_impl_t; diff --git a/rcl/test/rcl/test_events.cpp b/rcl/test/rcl/test_events.cpp index 78b8cc9d0..c443516d0 100644 --- a/rcl/test/rcl/test_events.cpp +++ b/rcl/test/rcl/test_events.cpp @@ -30,7 +30,6 @@ using std::chrono::milliseconds; using std::chrono::seconds; -constexpr seconds DISCOVERY_WAIT_TIME_IN_S(1); constexpr seconds LIVELINESS_LEASE_DURATION_IN_S(1); constexpr seconds DEADLINE_PERIOD_IN_S(1); @@ -74,7 +73,7 @@ class CLASSNAME (TestEventFixture, RMW_IMPLEMENTATION) : public ::testing::Test ts = ROSIDL_GET_MSG_TYPE_SUPPORT(test_msgs, msg, Primitives); } - rcl_ret_t SetupPublisher( + rcl_ret_t setupPublisher( const rmw_time_t & deadline, const rmw_time_t & lifespan, const rmw_time_t & liveliness_lease_duration, @@ -91,7 +90,7 @@ class CLASSNAME (TestEventFixture, RMW_IMPLEMENTATION) : public ::testing::Test &publisher_options); } - rcl_ret_t SetupSubscriber( + rcl_ret_t setupSubscriber( const rmw_time_t & deadline, const rmw_time_t & lifespan, const rmw_time_t & liveliness_lease_duration, @@ -109,7 +108,7 @@ class CLASSNAME (TestEventFixture, RMW_IMPLEMENTATION) : public ::testing::Test &subscription_options); } - void SetupPublisherAndSubscriber( + void setupPublisherAndSubscriber( const rcl_publisher_event_type_t & pub_event_type, const rcl_subscription_event_type_t & sub_event_type) { @@ -121,7 +120,7 @@ class CLASSNAME (TestEventFixture, RMW_IMPLEMENTATION) : public ::testing::Test rmw_qos_liveliness_policy_t liveliness_policy = RMW_QOS_POLICY_LIVELINESS_AUTOMATIC; // init publisher - ret = SetupPublisher(deadline, lifespan, lease_duration, liveliness_policy); + ret = setupPublisher(deadline, lifespan, lease_duration, liveliness_policy); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; // init publisher events @@ -130,16 +129,32 @@ class CLASSNAME (TestEventFixture, RMW_IMPLEMENTATION) : public ::testing::Test ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; // init subscription - ret = SetupSubscriber(deadline, lifespan, lease_duration, liveliness_policy); + ret = setupSubscriber(deadline, lifespan, lease_duration, liveliness_policy); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; // init subscription event subscription_event = rcl_get_zero_initialized_event(); ret = rcl_subscription_event_init(&subscription_event, &subscription, sub_event_type); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; + + // wait for discovery + // total wait time of 10 seconds, if never ready + size_t max_iterations = 100; + milliseconds wait_period(10); + size_t iteration = 0; + do { + iteration++; + size_t count = 0; + rcl_ret_t ret = rcl_subscription_get_publisher_count(&subscription, &count); + ASSERT_EQ(RCL_RET_OK, ret); + if (count > 0) { + break; + } + std::this_thread::sleep_for(wait_period); + } while (iteration < max_iterations); } - void TearDownPubSub() + void tearDownPubSub() { rcl_ret_t ret; @@ -241,6 +256,7 @@ wait_for_msgs_and_events( for (size_t i = 0; i < wait_set.size_of_subscriptions; ++i) { if (wait_set.subscriptions[i] && wait_set.subscriptions[i] == subscription) { msg_ready = true; + break; } } for (size_t i = 0; i < wait_set.size_of_events; ++i) { @@ -264,18 +280,18 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_unsupported_lifespa rmw_time_t lease_duration {1, 0}; rmw_qos_liveliness_policy_t liveliness_policy = RMW_QOS_POLICY_LIVELINESS_AUTOMATIC; EXPECT_EQ(RMW_RET_ERROR, - SetupSubscriber(deadline, lifespan, lease_duration, + setupSubscriber(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized subscriber lifespan when unsupported"; EXPECT_EQ(RMW_RET_ERROR, - SetupPublisher(deadline, lifespan, lease_duration, + setupPublisher(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized publisher lifespan when unsupported"; lifespan = {0, 1}; EXPECT_EQ(RMW_RET_ERROR, - SetupSubscriber(deadline, lifespan, lease_duration, + setupSubscriber(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized subscriber lifespan when unsupported"; EXPECT_EQ(RMW_RET_ERROR, - SetupPublisher(deadline, lifespan, lease_duration, + setupPublisher(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized publisher lifespan when unsupported"; } } @@ -287,21 +303,21 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_unsupported_livelin rmw_time_t lease_duration {0, 0}; rmw_qos_liveliness_policy_t liveliness_policy = RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_NODE; EXPECT_EQ(RMW_RET_ERROR, - SetupSubscriber(deadline, lifespan, lease_duration, + setupSubscriber(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized subscriber RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_NODE when unsupported"; EXPECT_EQ(RMW_RET_ERROR, - SetupPublisher(deadline, lifespan, lease_duration, + setupPublisher(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized publisher RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_NODE when unsupported"; liveliness_policy = RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC; EXPECT_EQ(RMW_RET_ERROR, - SetupSubscriber(deadline, lifespan, lease_duration, + setupSubscriber(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized subscriber RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC when unsupported"; EXPECT_EQ(RMW_RET_ERROR, - SetupPublisher(deadline, lifespan, lease_duration, + setupPublisher(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized publisher RMW_QOS_POLICY_LIVELINESS_MANUAL_BY_TOPIC when unsupported"; } @@ -309,23 +325,23 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_unsupported_livelin TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_unsupported_unsupported_deadline) { if (is_unsupported) { - rmw_time_t deadline; + rmw_time_t deadline {0, 0}; rmw_time_t lifespan {0, 0}; rmw_time_t lease_duration {0, 0}; rmw_qos_liveliness_policy_t liveliness_policy = RMW_QOS_POLICY_LIVELINESS_AUTOMATIC; EXPECT_EQ(RMW_RET_ERROR, - SetupSubscriber(deadline, lifespan, lease_duration, + setupSubscriber(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized subscriber deadline when unsupported"; EXPECT_EQ(RMW_RET_ERROR, - SetupPublisher(deadline, lifespan, lease_duration, + setupPublisher(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized publisher deadline when unsupported"; deadline = {0, 1}; EXPECT_EQ(RMW_RET_ERROR, - SetupSubscriber(deadline, lifespan, lease_duration, + setupSubscriber(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized subscriber deadline when unsupported"; EXPECT_EQ(RMW_RET_ERROR, - SetupPublisher(deadline, lifespan, lease_duration, + setupPublisher(deadline, lifespan, lease_duration, liveliness_policy)) << "Initialized publisher deadline when unsupported"; } } @@ -339,14 +355,7 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_pubsub_liveliness_k return; } - SetupPublisherAndSubscriber(RCL_PUBLISHER_LIVELINESS_LOST, RCL_SUBSCRIPTION_LIVELINESS_CHANGED); - - // wait for discovery - // TODO(wjwwood): add logic to wait for the connection to be established - // probably using the count_subscriptions busy wait mechanism - // until then we will sleep for a short period of time - std::this_thread::sleep_for(DISCOVERY_WAIT_TIME_IN_S); - + setupPublisherAndSubscriber(RCL_PUBLISHER_LIVELINESS_LOST, RCL_SUBSCRIPTION_LIVELINESS_CHANGED); rcl_ret_t ret; // publish message to topic @@ -418,7 +427,7 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_pubsub_deadline_mis return; } - SetupPublisherAndSubscriber(RCL_PUBLISHER_OFFERED_DEADLINE_MISSED, + setupPublisherAndSubscriber(RCL_PUBLISHER_OFFERED_DEADLINE_MISSED, RCL_SUBSCRIPTION_REQUESTED_DEADLINE_MISSED); // wait for discovery. also adds delay to publishing of message @@ -479,7 +488,7 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_pubsub_deadline_mis } // clean up - TearDownPubSub(); + tearDownPubSub(); } /* @@ -491,7 +500,7 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_pubsub_no_deadline_ return; } - SetupPublisherAndSubscriber(RCL_PUBLISHER_OFFERED_DEADLINE_MISSED, + setupPublisherAndSubscriber(RCL_PUBLISHER_OFFERED_DEADLINE_MISSED, RCL_SUBSCRIPTION_REQUESTED_DEADLINE_MISSED); // wait for discovery. also adds delay to publishing of message @@ -552,5 +561,5 @@ TEST_F(CLASSNAME(TestEventFixture, RMW_IMPLEMENTATION), test_pubsub_no_deadline_ } // clean up - TearDownPubSub(); + tearDownPubSub(); }