*_raw function by Karsten1987 · Pull Request #388 · ros2/rclcpp

Karsten1987 · 2017-10-23T18:15:23Z

wjwwood · 2018-03-08T02:28:11Z

rclcpp/include/rclcpp/node_impl.hpp

  const std::string & topic_name,
-  size_t qos_history_depth,
  CallbackT && callback,
+  size_t qos_history_depth,


Can you speak a little about why you had to change the order of the arguments to this function?

i can't really explain why, but with the modifications done in subscription_traits, I get template deduction errors without this change. Somehow it can't really differentiate between the two function overloads. With the change done here, it works though.
I am happy to discuss/debug that further and try to resolve this. I just don't know how :)

wjwwood · 2018-03-08T02:32:31Z

rclcpp/src/rclcpp/executor.cpp

+      subscription->get_subscription_handle(),
+      reinterpret_cast<rcl_message_raw_t *>(message.get()), &message_info);
+    auto rcl_ptr = message.get();
+    (void) rcl_ptr;


What is this for?

wjwwood · 2018-03-08T02:34:14Z

rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp

    }

+    // TODO(karsten1987): Set this back to true whenever we have c-raw publisher
+    constexpr bool publish_update = false;


rcl has a raw publish right? Or do you mean for C types (as opposed to C++ types)?

Shouldn't we support C types now before merging this? How much more work is that?

wjwwood · 2018-03-08T02:41:08Z

rclcpp/src/rclcpp/executor.cpp

+  if (subscription->is_raw()) {
+    ret = rcl_take_raw(
+      subscription->get_subscription_handle(),
+      reinterpret_cast<rcl_message_raw_t *>(message.get()), &message_info);


I don't think the memory allocated by rcl_take_raw (rmw_take_raw indirectly) is freed here, since the smart pointer will only free the memory for the rcl_message_raw_t, which doesn't have a destructor that functions properly (it's a C struct). It should call rmw_free (as long as the rmw_take_raw starts using rmw_allocate as I suggested in the other pr), so it probably needs a custom destructor that calls the right functions before freeing the rcl_message_raw_t itself.

You might need to override the message strategy's borrow_message and return_message functions for the rmw_message_raw_t.

Karsten1987 · 2018-05-07T15:53:19Z

rclcpp/src/rclcpp/executor.cpp

+  message_info.from_intra_process = false;
+
+  if (subscription->is_raw()) {
+    // TODO(karsten1987): Raise if opensplice !?


Right now, this function contains a lot of code duplication, given that big if - else.

Ideally, we also call take_raw to get the still serialized data from the wire and only convert it to a ros message if it's a raw callback.
The reason I didn't do it this way is that Opensplice doesn't support to get the serialized stream yet and thus would stay incompatible.

I think until we make take_raw required it has to be this way.

sloretz · 2018-05-11T22:51:51Z

rclcpp/CMakeLists.txt

  endif()
+  ament_add_gtest(test_raw_message_allocator test/test_raw_message_allocator.cpp
+    ENV RCL_ASSERT_RMW_ID_MATCHES=${rmw_implementation})
+  if(TARGET test_raw_message_allocator)


Just curious, why if(TARGET ...)? Is it not guaranteed that ament_add_gtest() will create a target with that name?

fair point. I simply adhered to the rest of the file. We can change that in a follow up PR

sloretz · 2018-05-11T22:54:50Z

rclcpp/include/rclcpp/message_memory_strategy.hpp

+    auto raw_msg = std::make_shared<rcl_message_raw_t>();
+    raw_msg->buffer_length = 0;
+    raw_msg->buffer_capacity = 0;
+    raw_msg->buffer = NULL;


*raw_msg = rmw_get_zero_initialized_raw_message() instead?

sloretz · 2018-05-11T22:56:36Z

rclcpp/include/rclcpp/message_memory_strategy.hpp

+    raw_msg->buffer_capacity = 0;
+    raw_msg->buffer = NULL;
+    rmw_initialize_raw_message(raw_msg.get(), 0, &rcutils_allocator_);
+    rmw_raw_message_resize(raw_msg.get(), capacity);


How about using capacity on the line above instead of resizing?

rmw_initialize_raw_message(raw_msg.get, capacity, &rcutils_allocator_)

sloretz · 2018-05-11T23:00:31Z

rclcpp/include/rclcpp/message_memory_strategy.hpp


+  virtual std::shared_ptr<rcl_message_raw_t> borrow_raw_message(unsigned int capacity)
+  {
+    auto raw_msg = std::make_shared<rcl_message_raw_t>();


Recommend using the 4th std::shared_ptr constructor on this page to store a deleter that calls rmw_raw_message_fini(). That makes sure it will be cleaned up even if a bug causes return_raw_message() to not be called.

sloretz · 2018-05-18T21:09:22Z

rclcpp/include/rclcpp/publisher.hpp

+    if (status != RCL_RET_OK) {
+      // *INDENT-OFF* (prevent uncrustify from making unnecessary indents here)
+      throw std::runtime_error(
+        std::string("failed to publish raw message: ") + rcl_get_error_string_safe());


Could use rclcpp::exceptions::throw_from_rcl_error(status, "failed to publish raw message");

sloretz · 2018-05-18T21:17:56Z

rclcpp/src/rclcpp/executor.cpp

+      rcl_reset_error();
+    }
+    auto void_raw_msg = std::static_pointer_cast<void>(raw_msg);
+    subscription->handle_message(void_raw_msg, message_info);


It looks like this calls handle_message() even when rcl_take_raw() fails. Is this intentional? The non-raw case doesn't do that.

sloretz · 2018-05-18T21:18:32Z

rclcpp/src/rclcpp/executor.cpp

-      "take failed for subscription on topic '%s': %s",
-      subscription->get_topic_name(), rcl_get_error_string_safe());
-    rcl_reset_error();
+    if (ret == RCL_RET_OK) {


Switch order RCL_RET_OK == ret for misra?

wjwwood · 2018-06-12T23:36:58Z

rclcpp/include/rclcpp/message_memory_strategy.hpp

    return std::allocate_shared<MessageT, MessageAlloc>(*message_allocator_.get());
  }

+  virtual std::shared_ptr<rcl_message_raw_t> borrow_raw_message(unsigned int capacity)


size_t (obviously depends on changing that down the API chain too, but I think it should be)

I chose unsigned int because RTI only serializes this.
https://community.rti.com/static/documentation/connext-dds/5.2.3/doc/api/connext_dds/api_cpp/structDDS__DynamicData.html#ae31195afe3fe6f567ddce38e7f99980e

I can change to size_t and cast it to unsigned for connext, but this might lead to data loss. Do you prefer this? Not sure what the best approach here is.

size_t is unsigned, and usually larger than unsigned int. Do you mean the user could ask for something larger and that would be an issue? You could check that it isn't too big at the point where you pass the size_t to RTI's API.

wjwwood · 2018-06-12T23:38:15Z

rclcpp/include/rclcpp/message_memory_strategy.hpp

+          delete msg;
+        });
+    *raw_msg = rmw_get_zero_initialized_raw_message();
+    rmw_raw_message_init(raw_msg.get(), capacity, &rcutils_allocator_);


Check the return code, and do not setup the destructor for the shared_ptr until you know this succeeds. Make sure to throw based on the rcl ret code.

wjwwood · 2018-06-12T23:41:11Z

rclcpp/include/rclcpp/message_memory_strategy.hpp

+  {
+    auto raw_msg = std::shared_ptr<rcl_message_raw_t>(new rcl_message_raw_t,
+        [](rmw_message_raw_t * msg) {
+          rmw_raw_message_fini(msg);


Check return code.

wjwwood · 2018-06-12T23:41:29Z

rclcpp/include/rclcpp/message_memory_strategy.hpp


+  virtual void return_raw_message(std::shared_ptr<rcl_message_raw_t> & raw_msg)
+  {
+    rmw_raw_message_fini(raw_msg.get());


Isn't this already being done in the custom destructor?

wjwwood · 2018-06-12T23:42:47Z

rclcpp/include/rclcpp/publisher.hpp

+  {
+    if (store_intra_process_message_) {
+      // not supported atm
+      throw std::runtime_error("storing raw messages in intra process is not supported yet.");


Make sure to make an issue about this after merging, and consider putting a TODO in here.

wjwwood · 2018-06-12T23:44:33Z

rclcpp/include/rclcpp/subscription.hpp

    const rmw_message_info_t & message_info) = 0;

+  rosidl_message_type_support_t
+  get_message_type_support_handle() const;


Should this return a const reference instead? Not sure it matters, but worth considering.

wjwwood · 2018-06-12T23:49:25Z

rclcpp/include/rclcpp/subscription_traits.hpp

+ * Cheers!
+ */
+template<typename T>
+struct is_raw_subscription_argument : std::false_type


Maybe if you used the public keyword before the inheritance uncrustify would do the right thing?

template<typename T> struct is_raw_subscription_argument: public std::false_type {};

Unfortunately, it still complains:

31: +++ include/rclcpp/subscription_traits.hpp.uncrustify 31: @@ -40 +40 @@ 31: -struct is_raw_subscription_argument<rcl_message_raw_t> : public std::true_type 31: +struct is_raw_subscription_argument<rcl_message_raw_t>: public std::true_type

Good idea though!

wjwwood · 2018-06-12T23:52:05Z

rclcpp/src/rclcpp/executor.cpp

+  message_info.from_intra_process = false;
+
+  if (subscription->is_raw()) {
+    // TODO(karsten1987): Raise if opensplice !?


I think until we make take_raw required it has to be this way.

wjwwood · 2018-06-12T23:55:43Z

rclcpp/src/rclcpp/time_source.cpp


-  clock_subscription_ = rclcpp::create_subscription<MessageT, decltype(cb), Alloc, SubscriptionT>(
+  clock_subscription_ = rclcpp::create_subscription<
+    MessageT, decltype(cb), Alloc, MessageT, SubscriptionT>(


This is bad style in my opinion because the template arguments line up with the function arguments. Instead do:

clock_subscription_ = rclcpp::create_subscription< MessageT, decltype(cb), Alloc, MessageT, SubscriptionT >( node_topics_.get(), topic_name, std::move(cb), ... );

Or:

clock_subscription_ = rclcpp::create_subscription<MessageT, decltype(cb), Alloc, MessageT, SubscriptionT>( node_topics_.get(), topic_name, std::move(cb), ... );

Both suggest versions don't work, but I'll go for this one then to separate the template arguments from the function arguments:

clock_subscription_ = rclcpp::create_subscription< MessageT, decltype(cb), Alloc, MessageT, SubscriptionT >( node_topics_.get(), topic_name, std::move(cb), rmw_qos_profile_default, group, false, false, msg_mem_strat, allocator);

wjwwood · 2018-06-14T03:14:36Z

I'm going to take a crack at changing the unsigned int to a size_t, so that's what's blocking my approval atm.

dirk-thomas · 2018-06-16T15:34:43Z

The nightly packaging jobs on Linux failed in the tlsf_cpp package due to a signature change in this PR.

Karsten1987 · 2018-06-16T20:34:12Z

I forgot to link the realtime_tools PR into this one. That's why the build farm didn't complain.
please see:
ros2/realtime_support#64

* Enforce non-null argv values on rcl_init(). Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com> * Adds test case for null argv values on rcl_init(). Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

Signed-off-by: ketatam <mohamed.amine.ketata@tngtech.com>

Karsten1987 added the in progress Actively being worked on (Kanban column) label Oct 23, 2017

Karsten1987 self-assigned this Oct 23, 2017

Karsten1987 changed the title ~~publish_raw function~~ *_raw function Oct 23, 2017

Karsten1987 mentioned this pull request Dec 14, 2017

Deduce MessageT from the first argument of the callback #286

Closed

Karsten1987 force-pushed the expose_cdr branch 2 times, most recently from 24b4382 to da27a04 Compare February 13, 2018 20:43

Karsten1987 force-pushed the expose_cdr branch from d1867d6 to 6dab5a1 Compare March 6, 2018 17:38

wjwwood reviewed Mar 8, 2018

View reviewed changes

dhood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 8, 2018

wjwwood mentioned this pull request Apr 16, 2018

Ability to serialize messages without publishing #462

Closed

Karsten1987 force-pushed the expose_cdr branch 2 times, most recently from c86fd2c to c7ee1f9 Compare May 1, 2018 20:22

Karsten1987 commented May 7, 2018

View reviewed changes

Karsten1987 mentioned this pull request May 12, 2018

Demo nodes for raw publish and subscribe ros2/demos#185

Merged

sloretz reviewed May 18, 2018

View reviewed changes

Karsten1987 force-pushed the expose_cdr branch from 606f4b7 to 12ab910 Compare June 12, 2018 08:07

wjwwood requested changes Jun 12, 2018

View reviewed changes

Karsten1987 mentioned this pull request Jun 13, 2018

*_raw function ros2/rmw#125

Merged

Karsten1987 added 11 commits June 13, 2018 16:12

publish_raw function

9ecb2c3

subscription traits

0e1e7ab

listener raw

84e6177

rebased

3cd94a5

cleanup and linters

50a4fc9

explicit test for deleter in unique_ptr

2723826

add missing copyright

ffb6313

cleanup

9d4c07a

add rmw_serialize functions

e1bf3d1

linters

b8e9d28

explicit differentiation between take and take_raw

9eb0a82

Karsten1987 added 4 commits June 13, 2018 16:12

cleanup debug messages

c7e0702

rename to rmw_message_init`

7a95736

address comments

44e1951

address review comments

7666a1a

Karsten1987 force-pushed the expose_cdr branch from 12ab910 to 7666a1a Compare June 13, 2018 14:14

raw->serialized

08d4f2b

use size_t (#497)

36d2b9f

wjwwood approved these changes Jun 15, 2018

View reviewed changes

Karsten1987 merged commit ec17d68 into master Jun 16, 2018

dirk-thomas removed the in review Waiting for review (Kanban column) label Jun 16, 2018

Karsten1987 deleted the expose_cdr branch June 16, 2018 08:37

dirk-thomas mentioned this pull request Jun 16, 2018

2018-06-14 ci.ros2.org failures ros2/build_farmer#125

Closed

17 tasks

esteve mentioned this pull request Jun 20, 2018

Switch order of arguments back to roscpp-like style #503

Closed

DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022

add topic remapping option to rosbag2 play (ros2#388)

33cf70b

Signed-off-by: ketatam <mohamed.amine.ketata@tngtech.com>

Conversation

Karsten1987 commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Karsten1987 Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented Jun 14, 2018

Uh oh!

dirk-thomas commented Jun 16, 2018

Uh oh!

Karsten1987 commented Jun 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Karsten1987 commented Oct 23, 2017 •

edited

Loading

Karsten1987 Jun 13, 2018 •

edited

Loading