Skip to content
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

Update serialization/deserialization API documentation. #258

Merged
merged 7 commits into from
Aug 14, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions rmw/include/rmw/rmw.h
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,14 @@ rmw_publisher_assert_liveliness(const rmw_publisher_t * publisher);
/**
* The ROS message is serialized into a byte stream contained within the
* rmw_serialized_message_t structure.
* The serialization format depends on the underlying middleware.
* The serialization format depends on the underlying implementation.
*
* \pre Given ROS message must be a valid non-null instance, initialized
* by the caller and matching the provided typesupport.
* \pre Given typesupport must be a valid non-null instance, as provided
* by `rosidl` APIs.
* \pre Given serialized message must be a valid non-null instance, initialized
* by the caller.
*
* \param[in] ros_message the typed ROS message
* \param[in] type_support the typesupport for the ROS message
Expand All @@ -577,10 +584,16 @@ rmw_serialize(
/**
* The given rmw_serialized_message_t's internal byte stream buffer is deserialized
* into the given ROS message.
* The ROS message must already be allocated and initialized, and must match
* the given typesupport structure.
* The serialization format expected in the rmw_serialized_message_t depends on the
* underlying middleware.
* underlying implementation.
*
* \pre Given serialized message must be a valid non-null instance, such
* as that returned by `rmw_serialize()`, matching provided typesupport
* and ROS message.
* \pre Given typesupport must be a valid non-null instance, as provided
* by `rosidl` APIs.
* \pre Given ROS message must be a valid non-null instance, initialized
* by the caller.
*
* \param[in] serialized_message the serialized message holding the byte stream
* \param[in] type_support the typesupport for the typed ros message
Expand Down
40 changes: 40 additions & 0 deletions rmw/include/rmw/serialized_message.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,49 @@ extern "C"
* more complex and is therefore aliased.
*/
typedef rcutils_uint8_array_t rmw_serialized_message_t;

/// Return a zero initialized serialized message struct.
#define rmw_get_zero_initialized_serialized_message rcutils_get_zero_initialized_uint8_array

/// Initialize a serialized message, zero initializing its contents.
/**
* \param[inout] serialized_message a pointer to the serialized message to initialize
* \param[in] message_capacity the size of the memory to allocate
* \param[in] allocator the allocator to use for the memory allocation
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if any arguments are invalid, or
* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly
* \return 'RMW_RET_BAD_ALLOC` if no memory could be allocated correctly, or

* \return `RMW_RET_ERROR` if an unexpected error occurs
*/
#define rmw_serialized_message_init rcutils_uint8_array_init
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought here. I would find this documentation easier to read if rmw_serialized_message_init had arguments. That is, can we change the macro to:

#define rmw_serialized_message_init(serialized_message, message_capacity, allocator) rcutils_uint8_array_init(serialized_message, message_capacity, allocator)

The same goes for all of the other rmw_serialized* macros in this file.


/// Finalize a serialized message.
/**
* Passing an uninitialized instance to this function leads to undefined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better to make this a checked pre-condition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, it's probably a good idea to add a \pre notice like we did above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an update. Passing uninitialized arguments will always be UB, and known states (zero-initialized or initialized) will be handled correctly in runtime.

* behavior.
*
* \param[in] serialized_message pointer to the serialized message to be cleaned up
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if the serialized_message argument is invalid
* \return `RMW_RET_ERROR` if an unexpected error occurs
*/
#define rmw_serialized_message_fini rcutils_uint8_array_fini

/// Resize the internal buffer of the serialized message.
/**
* The internal buffer of the serialized message can be resized dynamically if needed.
* If the new size is smaller than the current capacity, then the memory is
* truncated.
* Be aware, that this might deallocate the memory and therefore invalidates any
* pointers to this storage.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is grammatically correct, but I would still find a modification slightly easier to read:

Suggested change
* Be aware, that this might deallocate the memory and therefore invalidates any
* pointers to this storage.
* Be aware, that this might deallocate the memory and therefore invalidate any
* pointers to this storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. At some point we should correct rcutils_uint8_array_t API documentation as well.

*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add \pre serialized_message must have been initialized or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can constrain possible states to zero-initialized or initialized, but IMO to say that using an uninitialized variable is UB goes without saying.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing here is that we are not just saying "it's undefined", but we are in a way specifying whether implementers of this function declaration need to add a check for null pointers, etc. That, I think, makes it worth being more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the added \pre and \remarks should have us covered.

* \param[inout] serialized_message pointer to the serialized message to be resized
* \param[in] new_size the new size of the internal buffer
* \return `RMW_RET_OK` if successful, or
* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero
* \return `RMW_RET_INVALID_ARGUMENT` if new_size is set to zero, or

* \return `RMW_RET_BAD_ALLOC` if memory allocation failed, or
* \return `RMW_RET_ERROR` if an unexpected error occurs
*/
#define rmw_serialized_message_resize rcutils_uint8_array_resize

#if __cplusplus
Expand Down