*_raw function #125

Karsten1987 · 2017-10-23T18:16:27Z

wjwwood

bike shed: how attached are you to the _raw name? I was thinking rmw_take_serialized and rmw_publish_serialized and rmw_serialized_message_t, but I'm also not tied to that. I just think raw doesn't describe what's going on very well, and we know what it means, but for a non-native speaker it's probably doesn't imply what's actually happening properly.

rmw/include/rmw/types.h

rmw/include/rmw/rmw.h

rmw/include/rmw/raw_message.h

rmw/src/raw_message.c

sloretz · 2018-05-18T17:53:35Z

rmw/src/raw_message.c

+  char * new_buffer = allocator->allocate(new_size * sizeof(char), allocator->state);
+  if (!new_buffer) {
+    RMW_SET_ERROR_MSG("failed to allocate memory for resizing raw message");
+    return RMW_RET_ERROR;


Could be RMW_RET_BAD_ALLOC

sloretz · 2018-05-18T17:53:51Z

rmw/src/raw_message.c

+{
+  msg->buffer = (char *)allocator->allocate(buffer_capacity * sizeof(char), allocator->state);
+  if (!msg->buffer) {
+    return RMW_RET_ERROR;


Could be RMW_RET_BAD_ALLOC

sloretz · 2018-05-18T17:56:11Z

rmw/src/raw_message.c

+    return RMW_RET_ERROR;
+  }
+  if (new_size < msg->buffer_capacity) {
+    memcpy(new_buffer, msg->buffer, new_size);


Do the bytes beyond msg->buffer_length need to be copied?

sloretz · 2018-05-18T17:58:15Z

rmw/src/raw_message.c

+    return RMW_RET_OK;
+  }
+
+  char * new_buffer = allocator->allocate(new_size * sizeof(char), allocator->state);


The documentation for rcutils allocator->reallocate() says it behaves like std::realloc, which I think means it copies the memory too. It could be used in place of this and the memcpy calls below.

sloretz · 2018-05-18T18:06:32Z

rmw/test/test_raw_message.cpp

+  auto allocator = rcutils_get_default_allocator();
+  EXPECT_EQ(RMW_RET_OK, rmw_initialize_raw_message(&raw_msg, 0, &allocator));
+  EXPECT_EQ(0u, raw_msg.buffer_capacity);
+  EXPECT_TRUE(raw_msg.buffer);


It doesn't appear to be safe to assume buffer is not NULL using the default allocator. The documentation for malloc() says

If size is zero, the behavior is implementation defined (null pointer may be returned, or some non-null pointer may be returned that may not be used to access storage, but has to be passed to free).

How about making rmw_initialize_raw_message() not call allocator->allocate() if the size is zero, and instead expect the buffer to be NULL?

sloretz · 2018-05-18T18:08:29Z

rmw/test/test_raw_message.cpp

+TEST(test_raw_message, resize) {
+  auto raw_msg = rmw_get_zero_initialized_raw_message();
+  auto allocator = rcutils_get_default_allocator();
+  rmw_initialize_raw_message(&raw_msg, 5, &allocator);


Recommend ASSERT_EQ(RMW_RET_OK, ...) to prevent a crash if this call fails.

wjwwood · 2018-06-12T22:54:23Z

rmw/include/rmw/raw_message.h

+rmw_ret_t
+rmw_raw_message_init(
+  rmw_message_raw_t * msg,
+  unsigned int buffer_capacity,


Why is this not a size_t?

same discussion here: ros2/rclcpp#388 (comment)

wjwwood · 2018-06-12T23:09:45Z

rmw/include/rmw/types.h

+  unsigned int buffer_length;
+  unsigned int buffer_capacity;
+  rcutils_allocator_t allocator;
+} rmw_message_raw_t;


Did you consider any other names, like rmw_raw_message_t (which is a more natural sounding to me, i.e. adjective before noun), rmw_serialized_message_t which is more technical sounding, or just rmw_message_t?

It's a bit late to change it now, but on the other hand changing it later is going to potentially be painful.

ok. I will address the style comments first, and then rename them.

the type: rmw_serialized_message_t

what do you propose for the function?
rmw_take_serialized_message?

wjwwood · 2018-06-12T23:10:45Z

rmw/src/raw_message.c

+#include "rmw/error_handling.h"
+#include "rmw/raw_message.h"
+
+RMW_PUBLIC


Visibility macros should not be in the .c files.

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

rmw/include/rmw/raw_message.h

+ */
+RMW_PUBLIC
+rmw_ret_t
+rmw_raw_message_resize(rmw_message_raw_t * msg, unsigned int new_size);


Same here, why not size_t?

wjwwood · 2018-06-12T23:21:51Z

rmw/src/raw_message.c

+    return RMW_RET_OK;
+  }
+
+  msg->buffer = allocator->reallocate(msg->buffer, new_size * sizeof(char), allocator->state);


This is dangerous, use the reallocf style function instead or address the potential memory leak directly, see: https://github.com/ros2/rcutils/blob/3595e7c0a1b815d7ff6aa7e3d2978361ab843bc0/include/rcutils/allocator.h#L128-L136

wjwwood · 2018-06-12T23:39:22Z

rmw/include/rmw/raw_message.h

+ * \return rmw_message_raw_t a zero initialized raw message struct
+ */
+RMW_PUBLIC
+rmw_message_raw_t


Use RMW_WARN_UNUSED on all public functions you added here, see:

rmw/rmw/include/rmw/macros.h

Line 22 in 8ea66db

#define RMW_WARN_UNUSED RCUTILS_WARN_UNUSED

3e133b2 afb72ef 0a53658 8d0acff 35ee9e3

wjwwood · 2018-06-14T06:59:57Z

@Karsten1987 I added back my changes from last night in c044e2a, looks like they got force pushed over (not easy to recover since it was all in the GitHub webui), please pull before pushing in the future 🙏.

dirk-thomas · 2018-07-27T22:46:39Z

@ros2/rmw_implementations FYI these PRs added new API to publish / take raw (not deserialized) messagesto the rmw interface. For FastRTPS and Connext the new API has been implemented in the linked PRs. For OpenSplice only an issue requesting further information has been filled upstream (ADLINK-IST/opensplice#50). Other RMW implementations should be updated to implement this new API in order to stay compatible.

If you are maintaining a RMW impl but are not part of the mentioned team please feel free to reach out to be added to the team. That way you can stay in the loop for future changes.

* publish_raw function * add rmw_take_raw * extract raw message * add rmw_serialize functions * linters * address comments * rename to rmw_raw_message_init * documentation for new functions * address comments * raw->serialized * documentation fixups (restored after being force pushed) ros2@3e133b2 ros2@afb72ef ros2@0a53658 ros2@8d0acff ros2@35ee9e3 * fix rmw_take_serialized() -> rmw_take_serialized_message() * use size_t (ros2#139) Signed-off-by: Devin Bonnie <dbbonnie@amazon.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 force-pushed the expose_cdr branch from 17cff09 to ba32172 Compare November 14, 2017 23:37

Karsten1987 force-pushed the expose_cdr branch from ba32172 to 5550e60 Compare February 8, 2018 17:48

wjwwood reviewed Mar 8, 2018

View reviewed changes

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/types.h Outdated Show resolved Hide resolved

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved

Karsten1987 force-pushed the expose_cdr branch from a2c0d76 to a90f57a Compare April 25, 2018 17:44

sloretz reviewed May 18, 2018

View reviewed changes

rmw/include/rmw/raw_message.h Outdated Show resolved Hide resolved

sloretz reviewed May 18, 2018

View reviewed changes

Karsten1987 added 8 commits June 11, 2018 22:48

publish_raw function

85a3872

add rmw_take_raw

bae7c26

extract raw message

cf7d264

add rmw_serialize functions

6900ae1

linters

585f6f4

address comments

4776600

rename to rmw_raw_message_init

4af8005

documentation for new functions

e780a6a

Karsten1987 force-pushed the expose_cdr branch from bee2a24 to e780a6a Compare June 12, 2018 08:09

wjwwood requested changes Jun 12, 2018

View reviewed changes

wjwwood reviewed Jun 12, 2018

View reviewed changes

address comments

fc0eafe

Karsten1987 force-pushed the expose_cdr branch from 35ee9e3 to fc0eafe Compare June 13, 2018 14:20

raw->serialized

27e1089

wjwwood approved these changes Jun 14, 2018

View reviewed changes

documentation fixups (restored after being force pushed)

c044e2a

3e133b2 afb72ef 0a53658 8d0acff 35ee9e3

wjwwood and others added 2 commits June 14, 2018 00:01

fix rmw_take_serialized() -> rmw_take_serialized_message()

c3758de

use size_t (#139)

a76ae49

wjwwood approved these changes Jun 15, 2018

View reviewed changes

Karsten1987 merged commit 229ab5b into master Jun 16, 2018

dirk-thomas removed the in progress Actively being worked on (Kanban column) label Jun 16, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*_raw function #125

*_raw function #125

Karsten1987 commented Oct 23, 2017 •

edited

Loading

wjwwood left a comment

sloretz May 18, 2018

sloretz May 18, 2018

sloretz May 18, 2018

sloretz May 18, 2018

sloretz May 18, 2018

sloretz May 18, 2018

wjwwood Jun 12, 2018

Karsten1987 Jun 13, 2018

wjwwood Jun 12, 2018

Karsten1987 Jun 13, 2018

wjwwood Jun 12, 2018

wjwwood Jun 12, 2018

wjwwood Jun 12, 2018

wjwwood Jun 12, 2018

wjwwood commented Jun 14, 2018

dirk-thomas commented Jul 27, 2018

*_raw function #125

*_raw function #125

Conversation

Karsten1987 commented Oct 23, 2017 • edited Loading

wjwwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjwwood commented Jun 14, 2018

dirk-thomas commented Jul 27, 2018

Karsten1987 commented Oct 23, 2017 •

edited

Loading